Remove jsg::ByteString

The type is largely obsolete and was being used only to
warn about invalid header names/values in Headers.
However, it incurred an additional overhead when
being created. This was especially non-sensical for
header names since we already validate those to be
proper HTTP token strings. Removing the type simplifies
code and helps boost overall performance.

Author: jasnell

src/workerd/api/cache.c++

@@ -532,7 +532,7 @@ kj::Own<kj::HttpClient> Cache::getHttpClient(IoContext& context,
     kj::Maybe<kj::String> cfBlobJson,
     kj::LiteralStringConst operationName,
     kj::StringPtr url,
-    kj::Maybe<jsg::ByteString> cacheControl,
+    kj::Maybe<kj::String> cacheControl,
     bool enableCompatFlags) {
   auto span = context.makeTraceSpan(operationName);
   auto userSpan = context.makeUserTraceSpan(operationName);

src/workerd/api/cache.h

@@ -93,7 +93,7 @@ class Cache: public jsg::Object {
       kj::Maybe<kj::String> cfBlobJson,
       kj::LiteralStringConst operationName,
       kj::StringPtr url,
-      kj::Maybe<jsg::ByteString> cacheControl,
+      kj::Maybe<kj::String> cacheControl,
       bool enableCompatFlags);
 };
 

src/workerd/api/eventsource.c++

@@ -362,8 +362,6 @@ void EventSource::start(jsg::Lock& js) {
             js, self, kj::str("The response status code was ", response->getStatus(), "."));
         }
 
-        // TODO(cleanup): Using jsg::ByteString here is really annoying. It would be nice to have
-        // an internal alternative that doesn't require an allocation.
         KJ_IF_SOME(contentType,
             response->getHeaders(js)->getCommon(js, capnp::CommonHeaderName::CONTENT_TYPE)) {
         bool invalid = false;

src/workerd/api/headers.c++

@@ -164,15 +164,39 @@ static_assert(HEADER_HASH_TABLE.find("AcCePt-ChArSeT"_kj) == 1);
 
 static_assert(std::size(COMMON_HEADER_NAMES) == (MAX_COMMON_HEADER_ID + 1));
 
-inline constexpr void requireValidHeaderName(const jsg::ByteString& name) {
+inline constexpr void requireValidHeaderName(kj::StringPtr name) {
+  if (HEADER_HASH_TABLE.find(name) != 0) {
+    // Known common header, always valid
+    return;
+  }
   for (char c: name) {
     JSG_REQUIRE(util::isHttpTokenChar(c), TypeError, "Invalid header name.");
   }
 }
 
+void maybeWarnIfBadHeaderString(kj::StringPtr str) {
+  if (IoContext::hasCurrent()) {
+    auto& context = IoContext::current();
+    if (context.isInspectorEnabled()) {
+      if (!simdutf::validate_ascii(str.begin(), str.size())) {
+        // The string contains non-ASCII characters. While any 8-bit value is technically valid
+        // in HTTP headers, we encode header strings as UTF-8, so we want to warn the user that
+        // their header name/value may not be what they may expect based on what browsers do.
+        auto utf8Hex =
+            kj::strArray(KJ_MAP(b, str) { return kj::str("\\x", kj::hex(kj::byte(b))); }, "");
+        context.logWarning(kj::str("A header value contains non-ASCII characters: \"", str,
+            "\" (raw bytes: \"", utf8Hex,
+            "\"). As a quirk to support Unicode, we are encoding "
+            "values as UTF-8 in the header, but in a browser this would likely result in a "
+            "TypeError exception. Consider encoding this string in ASCII for compatibility with "
+            "browser implementations of the Fetch specification."));
+      }
+    }
+  }
+}
+
 // Left- and right-trim HTTP whitespace from `value`.
-kj::String normalizeHeaderValue(jsg::Lock& js, jsg::ByteString value) {
-  JSG_REQUIRE(workerd::util::isValidHeaderValue(value), TypeError, "Invalid header value.");
+kj::String normalizeHeaderValue(kj::String value) {
   // Fast path: if empty, return as-is
   if (value.size() == 0) return kj::mv(value);
 
@@ -183,9 +207,18 @@ kj::String normalizeHeaderValue(jsg::Lock& js, jsg::ByteString value) {
   while (begin < end && util::isHttpWhitespace(*(end - 1))) --end;
 
   size_t newSize = end - begin;
-  if (newSize == value.size()) return kj::mv(value);
+  if (newSize == value.size()) {
+    JSG_REQUIRE(workerd::util::isValidHeaderValue(value), TypeError, "Invalid header value.");
+    maybeWarnIfBadHeaderString(value);
+    return kj::mv(value);
+  }
 
-  return kj::str(kj::ArrayPtr(begin, newSize));
+  auto trimmed = kj::ArrayPtr(begin, newSize);
+  JSG_REQUIRE(workerd::util::isValidHeaderValue(trimmed), TypeError, "Invalid header value.");
+  maybeWarnIfBadHeaderString(value);
+  // By attaching the original array to the trimmed view, we keep the original allocation alive
+  // and prevent an unnecessary copy.
+  return kj::str(trimmed.attach(value.releaseArray()));
 }
 
 constexpr bool isSetCookie(const Headers::HeaderKey& key) {
@@ -285,8 +318,7 @@ kj::uint Headers::HeaderCallbacks::hashCode(capnp::CommonHeaderName commondId) {
   return kj::hashCode(commondId);
 }
 
-Headers::Headers(jsg::Lock& js, jsg::Dict<jsg::ByteString, jsg::ByteString> dict)
-    : guard(Guard::NONE) {
+Headers::Headers(jsg::Lock& js, jsg::Dict<kj::String, kj::String> dict): guard(Guard::NONE) {
   headers.reserve(dict.fields.size());
   for (auto& field: dict.fields) {
     append(js, kj::mv(field.name), kj::mv(field.value));
@@ -386,7 +418,7 @@ kj::Array<Headers::DisplayedHeader> Headers::getDisplayedHeaders(jsg::Lock& js)
 }
 
 jsg::Ref<Headers> Headers::constructor(jsg::Lock& js, jsg::Optional<Initializer> init) {
-  using StringDict = jsg::Dict<jsg::ByteString, jsg::ByteString>;
+  using StringDict = jsg::Dict<kj::String, kj::String>;
 
   KJ_IF_SOME(i, init) {
     KJ_SWITCH_ONEOF(kj::mv(i)) {
@@ -398,7 +430,7 @@ jsg::Ref<Headers> Headers::constructor(jsg::Lock& js, jsg::Optional<Initializer>
         // It's important to note here that we are treating the Headers object
         // as a special case here. Per the fetch spec, we *should* be grabbing
         // the Symbol.iterator off the Headers object and interpreting it as
-        // a Sequence<Sequence<ByteString>> (as in the ByteStringPairs case
+        // a Sequence<Sequence<kj::String>> (as in the StringPairs case
         // below). However, special casing Headers like we do here is more
         // performant and has other side effects such as preserving the casing
         // of header names that have been received.
@@ -415,7 +447,7 @@ jsg::Ref<Headers> Headers::constructor(jsg::Lock& js, jsg::Optional<Initializer>
         // implementation here, however, we are ignoring the Symbol.iterator so
         // the test fails.
       }
-      KJ_CASE_ONEOF(pairs, ByteStringPairs) {
+      KJ_CASE_ONEOF(pairs, StringPairs) {
         auto dict = KJ_MAP(entry, pairs) {
           JSG_REQUIRE(entry.size() == 2, TypeError,
               "To initialize a Headers object from a sequence, each inner sequence "
@@ -430,7 +462,7 @@ jsg::Ref<Headers> Headers::constructor(jsg::Lock& js, jsg::Optional<Initializer>
   return js.alloc<Headers>();
 }
 
-kj::Maybe<kj::String> Headers::get(jsg::Lock& js, jsg::ByteString name) {
+kj::Maybe<kj::String> Headers::get(jsg::Lock& js, kj::String name) {
   requireValidHeaderName(name);
   return getUnguarded(js, name.asPtr());
 }
@@ -457,7 +489,7 @@ kj::Array<kj::StringPtr> Headers::getSetCookie() {
   return nullptr;
 }
 
-kj::Array<kj::StringPtr> Headers::getAll(jsg::ByteString name) {
+kj::Array<kj::StringPtr> Headers::getAll(kj::String name) {
   requireValidHeaderName(name);
 
   if (!strcaseeq(name, "set-cookie"_kj)) {
@@ -470,7 +502,7 @@ kj::Array<kj::StringPtr> Headers::getAll(jsg::ByteString name) {
   return getSetCookie();
 }
 
-bool Headers::has(jsg::ByteString name) {
+bool Headers::has(kj::String name) {
   requireValidHeaderName(name);
   return headers.find(getHeaderKeyFor(name)) != kj::none;
 }
@@ -480,23 +512,24 @@ bool Headers::hasCommon(capnp::CommonHeaderName idx) {
   return headers.find(idx) != kj::none;
 }
 
-void Headers::set(jsg::Lock& js, jsg::ByteString name, jsg::ByteString value) {
+void Headers::set(jsg::Lock& js, kj::String name, kj::String value) {
   checkGuard();
   requireValidHeaderName(name);
-  setUnguarded(js, kj::mv(name), normalizeHeaderValue(js, kj::mv(value)));
+  setUnguarded(js, kj::mv(name), normalizeHeaderValue(kj::mv(value)));
 }
 
 void Headers::setUnguarded(jsg::Lock& js, kj::String name, kj::String value) {
   auto key = getHeaderKeyFor(name);
   auto& header = headers.findOrCreate(key, [&]() {
     Header header(kj::mv(key));
-    if (header.getHeaderName() != name) {
+    auto keyName = header.getKeyName();
+    if (keyName.size() != name.size() || keyName != name) {
       header.name = kj::mv(name);
     }
     return kj::mv(header);
   });
-  header.values.clear();
-  header.values.add(kj::mv(value));
+  header.values.resize(1);
+  header.values[0] = kj::mv(value);
 }
 
 void Headers::setCommon(capnp::CommonHeaderName idx, kj::String value) {
@@ -507,25 +540,26 @@ void Headers::setCommon(capnp::CommonHeaderName idx, kj::String value) {
   header.values.add(kj::mv(value));
 }
 
-void Headers::append(jsg::Lock& js, jsg::ByteString name, jsg::ByteString value) {
+void Headers::append(jsg::Lock& js, kj::String name, kj::String value) {
   checkGuard();
   requireValidHeaderName(name);
-  appendUnguarded(js, kj::mv(name), normalizeHeaderValue(js, kj::mv(value)));
+  appendUnguarded(js, kj::mv(name), normalizeHeaderValue(kj::mv(value)));
 }
 
 void Headers::appendUnguarded(jsg::Lock& js, kj::String name, kj::String value) {
   auto key = getHeaderKeyFor(name);
   auto& header = headers.findOrCreate(key, [&]() {
     Header header(kj::mv(key));
-    if (header.getHeaderName() != name) {
+    auto keyName = header.getKeyName();
+    if (keyName.size() != name.size() || keyName != name) {
       header.name = kj::mv(name);
     }
     return kj::mv(header);
   });
   header.values.add(kj::mv(value));
 }
 
-void Headers::delete_(jsg::ByteString name) {
+void Headers::delete_(kj::String name) {
   checkGuard();
   requireValidHeaderName(name);
   headers.eraseMatch(getHeaderKeyFor(name));

src/workerd/api/headers.h

@@ -32,7 +32,7 @@ class Headers final: public jsg::Object {
   };
 
   Headers(): guard(Guard::NONE) {}
-  explicit Headers(jsg::Lock& js, jsg::Dict<jsg::ByteString, jsg::ByteString> dict);
+  explicit Headers(jsg::Lock& js, jsg::Dict<kj::String, kj::String> dict);
   explicit Headers(jsg::Lock& js, const Headers& other);
   explicit Headers(jsg::Lock& js, const kj::HttpHeaders& other, Guard guard);
   KJ_DISALLOW_COPY_AND_MOVE(Headers);
@@ -51,42 +51,42 @@ class Headers final: public jsg::Object {
   // Returns headers with lower-case name and comma-concatenated duplicates.
   kj::Array<DisplayedHeader> getDisplayedHeaders(jsg::Lock& js);
 
-  using ByteStringPair = jsg::Sequence<jsg::ByteString>;
-  using ByteStringPairs = jsg::Sequence<ByteStringPair>;
+  using StringPair = jsg::Sequence<kj::String>;
+  using StringPairs = jsg::Sequence<StringPair>;
 
   // Per the fetch specification, it is possible to initialize a Headers object
   // from any other object that has a Symbol.iterator implementation. Those are
-  // handled in this Initializer definition using the ByteStringPairs definition
-  // that aliases jsg::Sequence<jsg::Sequence<jsg::ByteString>>. Technically,
+  // handled in this Initializer definition using the StringPairs definition
+  // that aliases jsg::Sequence<jsg::Sequence<kj::String>>. Technically,
   // the Headers object itself falls under that definition as well. However, treating
   // a Headers object as a jsg::Sequence<jsg::Sequence<T>> is nowhere near as
   // performant and has the side effect of forcing all header names to be lower-cased
   // rather than case-preserved. Instead of following the spec exactly here, we
   // choose to special case creating a Header object from another Header object.
   // This is an intentional departure from the spec.
   using Initializer = kj::OneOf<jsg::Ref<Headers>,
-                                ByteStringPairs,
-                                jsg::Dict<jsg::ByteString, jsg::ByteString>>;
+                                StringPairs,
+                                jsg::Dict<kj::String, kj::String>>;
 
   static jsg::Ref<Headers> constructor(jsg::Lock& js, jsg::Optional<Initializer> init);
-  kj::Maybe<kj::String> get(jsg::Lock& js, jsg::ByteString name);
+  kj::Maybe<kj::String> get(jsg::Lock& js, kj::String name);
 
   kj::Maybe<kj::String> getUnguarded(jsg::Lock& js, kj::StringPtr name);
 
   // getAll is a legacy non-standard extension API that we introduced before
   // getSetCookie() was defined. We continue to support it for backwards
   // compatibility but users really ought to be using getSetCookie() now.
-  kj::Array<kj::StringPtr> getAll(jsg::ByteString name);
+  kj::Array<kj::StringPtr> getAll(kj::String name);
 
   // The Set-Cookie header is special in that it is the only HTTP header that
   // is not permitted to be combined into a single instance.
   kj::Array<kj::StringPtr> getSetCookie();
 
-  bool has(jsg::ByteString name);
+  bool has(kj::String name);
 
-  void set(jsg::Lock& js, jsg::ByteString name, jsg::ByteString value);
-  void append(jsg::Lock& js, jsg::ByteString name, jsg::ByteString value);
-  void delete_(jsg::ByteString name);
+  void set(jsg::Lock& js, kj::String name, kj::String value);
+  void append(jsg::Lock& js, kj::String name, kj::String value);
+  void delete_(kj::String name);
 
   void setUnguarded(jsg::Lock& js, kj::String name, kj::String value);
   void appendUnguarded(jsg::Lock& js, kj::String name, kj::String value);

src/workerd/api/http.c++

@@ -215,7 +215,7 @@ Body::Body(jsg::Lock& js, kj::Maybe<ExtractedBody> init, Headers& headers)
           if (!headers.hasCommon(capnp::CommonHeaderName::CONTENT_TYPE)) {
             // The spec allows the user to override the Content-Type, if they wish, so we only set
             // the Content-Type if it doesn't already exist.
-            headers.setCommon(capnp::CommonHeaderName::CONTENT_TYPE, jsg::ByteString(kj::mv(ct)));
+            headers.setCommon(capnp::CommonHeaderName::CONTENT_TYPE, kj::mv(ct));
           } else if (MimeType::FORM_DATA == ct) {
             // Custom content-type request/responses with FormData are broken since they require a
             // boundary parameter only the FormData serializer can provide. Let's warn if a dev does this.

src/workerd/api/r2-bucket.c++

@@ -50,11 +50,11 @@ static kj::Date parseDate(jsg::Lock& js, kj::StringPtr value) {
   return js.date(value);
 }
 
-static jsg::ByteString toUTCString(jsg::Lock& js, kj::Date date) {
+static kj::String toUTCString(jsg::Lock& js, kj::Date date) {
   return js.date(date).toUTCString(js);
 }
 
-static jsg::ByteString toISOString(jsg::Lock& js, kj::Date date) {
+static kj::String toISOString(jsg::Lock& js, kj::Date date) {
   return js.date(date).toISOString(js);
 }
 

src/workerd/api/trace.c++

@@ -341,8 +341,7 @@ jsg::Optional<jsg::V8Ref<v8::Object>> TraceItem::FetchEventInfo::Request::getCf(
   return detail->cf.map([&](jsg::V8Ref<v8::Object>& obj) { return obj.addRef(js); });
 }
 
-jsg::Dict<jsg::ByteString, jsg::ByteString> TraceItem::FetchEventInfo::Request::getHeaders(
-    jsg::Lock& js) {
+jsg::Dict<kj::String, kj::String> TraceItem::FetchEventInfo::Request::getHeaders(jsg::Lock& js) {
   auto shouldRedact = [](kj::StringPtr name) {
     return (
         //(name == "authorization"_kj) || // covered below
@@ -351,12 +350,11 @@ jsg::Dict<jsg::ByteString, jsg::ByteString> TraceItem::FetchEventInfo::Request::
         name.contains("token"_kjc));
   };
 
-  using HeaderDict = jsg::Dict<jsg::ByteString, jsg::ByteString>;
+  using HeaderDict = jsg::Dict<kj::String, kj::String>;
   auto builder = kj::heapArrayBuilder<HeaderDict::Field>(detail->headers.size());
   for (const auto& header: detail->headers) {
     auto v = (redacted && shouldRedact(header.name)) ? "REDACTED"_kj : header.value;
-    builder.add(
-        HeaderDict::Field{jsg::ByteString(kj::str(header.name)), jsg::ByteString(kj::str(v))});
+    builder.add(HeaderDict::Field{kj::str(header.name), kj::str(v)});
   }
 
   // TODO(conform): Better to return a frozen JS Object?

src/workerd/api/trace.h

@@ -220,7 +220,7 @@ class TraceItem::FetchEventInfo::Request final: public jsg::Object {
   explicit Request(Detail& detail, bool redacted = true);
 
   jsg::Optional<jsg::V8Ref<v8::Object>> getCf(jsg::Lock& js);
-  jsg::Dict<jsg::ByteString, jsg::ByteString> getHeaders(jsg::Lock& js);
+  jsg::Dict<kj::String, kj::String> getHeaders(jsg::Lock& js);
   kj::StringPtr getMethod();
   kj::String getUrl();
 

src/workerd/jsg/README.md

@@ -105,16 +105,15 @@ At the time of writing this, the primitive value types currently supported by th
 | uint32_t             |  v8::Uint32    |     number      |                         |
 | uint64_t             |  v8::BigInt    |     bigint      |                         |
 | kj::String(Ptr)      |  v8::String    |     string      |                         |
-| jsg::ByteString      |  v8::String    |     string      | See [ByteString][] spec |
 | kj::Date             |  v8::Date      |     Date        |                         |
 | nullptr              |  v8::Null      |     null        | See kj::Maybe<T>     |
 | nullptr              |  v8::Undefined |     undefined   | See jsg::Optional<T> |
 
 Specifically, for example, when mapping from JavaScript into C++, when JSG encounters a
-string value, it can convert that into either a `kj::String`, or `jsg::ByteString`,
+string value, it can convert that into either a `kj::String`, or `jsg::USVString`,
 depending on what is needed by the C++ layer. Likewise, when translating from C++ to
 JavaScript, JSG will generate a JavaScript `string` whenever it encounters a `kj::String`,
-`kj::StringPtr`, or `jsg::ByteString`.
+`kj::StringPtr`, or `jsg::USVString`.
 
 JSG will *not* translate JavaScript `string` to `kj::StringPtr`.
 
@@ -1414,7 +1413,6 @@ TODO(soon): TBD
 
 ["KJ Style Guide"]: https://github.com/capnproto/capnproto/blob/master/style-guide.md
 ["KJ Tour"]: https://github.com/capnproto/capnproto/blob/master/kjdoc/tour.md
-[ByteString]: https://webidl.spec.whatwg.org/#idl-ByteString
 [Record]: https://webidl.spec.whatwg.org/#idl-record
 [Sequence]: https://webidl.spec.whatwg.org/#idl-sequence