Bug 1402944: Part 9 - Optimize request/response header handling. r=mixedpuppy,ehsan
authorKris Maglione <maglione.k@gmail.com>
Sat, 23 Sep 2017 16:25:19 -0700
changeset 383353 5a295181603e8e47192126c2a2be3253f7c9edb9
parent 383352 c24a408f1574c847123ecd8561bc6eac6364a622
child 383354 fe52d17ee61bc3023d6118cc013dc8de30d6f52e
push id95542
push usermaglione.k@gmail.com
push dateThu, 28 Sep 2017 02:22:36 +0000
treeherdermozilla-inbound@19370d245a11 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmixedpuppy, ehsan
bugs1402944
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1402944: Part 9 - Optimize request/response header handling. r=mixedpuppy,ehsan We don't use the initial Map returned by ChannelWrapper as a map, so there's no need for the overhead involved in creating it. We also don't need the header map generated by HeaderChanger unless headers are actually being modified, which for many listeners they never are, so there's no need for the map creation and string lower-casing overhead prior to modification time. MozReview-Commit-ID: K2uK93Oo542
dom/webidl/ChannelWrapper.webidl
toolkit/components/extensions/webrequest/ChannelWrapper.cpp
toolkit/components/extensions/webrequest/ChannelWrapper.h
toolkit/modules/addons/WebRequest.jsm
--- a/dom/webidl/ChannelWrapper.webidl
+++ b/dom/webidl/ChannelWrapper.webidl
@@ -295,34 +295,42 @@ interface ChannelWrapper : EventTarget {
    * originURL will provide information on what window is doing the load.  It
    * will be null if the request is not associated with a window (e.g. XHR with
    * mozBackgroundRequest = true).
    */
   [Cached, Frozen, GetterThrows, Pure]
   readonly attribute sequence<MozFrameAncestorInfo>? frameAncestors;
 
   /**
-   * For HTTP requests, returns a Map of request headers which will be, or
-   * have been, sent with this request. Each key is a non-case-normalized
-   * header name string, and each value is its string value.
+   * For HTTP requests, returns an array of request headers which will be, or
+   * have been, sent with this request.
    *
    * For non-HTTP requests, throws NS_ERROR_UNEXPECTED.
    */
   [Throws]
-  object getRequestHeaders();
+  sequence<MozHTTPHeader> getRequestHeaders();
 
   /**
-   * For HTTP requests, returns a Map of response headers which were received
-   * for this request, in the same format as returned by getRequestHeaders.
-   *
+   * For HTTP requests, returns an array of response headers which were
+   * received for this request, in the same format as returned by
+   * getRequestHeaders.
+
    * Throws NS_ERROR_NOT_AVAILABLE if a response has not yet been received, or
    * NS_ERROR_UNEXPECTED if the channel is not an HTTP channel.
+   *
+   * Note: The Content-Type header is handled specially. That header is
+   * usually not mutable after the request has been received, and the content
+   * type must instead be changed via the contentType attribute. If a caller
+   * attempts to set the Content-Type header via setRequestHeader, however,
+   * that value is assigned to the contentType attribute and its original
+   * string value is cached. That original value is returned in place of the
+   * actual Content-Type header.
    */
   [Throws]
-  object getResponseHeaders();
+  sequence<MozHTTPHeader> getResponseHeaders();
 
   /**
    * Sets the given request header to the given value, overwriting any
    * previous value. Setting a header to a null string has the effect of
    * removing it.
    *
    * For non-HTTP requests, throws NS_ERROR_UNEXPECTED.
    */
@@ -330,16 +338,19 @@ interface ChannelWrapper : EventTarget {
   void setRequestHeader(ByteString header, ByteString value);
 
   /**
    * Sets the given response header to the given value, overwriting any
    * previous value. Setting a header to a null string has the effect of
    * removing it.
    *
    * For non-HTTP requests, throws NS_ERROR_UNEXPECTED.
+   *
+   * Note: The content type header is handled specially by this function. See
+   * getResponseHeaders() for details.
    */
   [Throws]
   void setResponseHeader(ByteString header, ByteString value);
 };
 
 /**
  * Information about the proxy server handing a request. This is approximately
  * equivalent to nsIProxyInfo.
@@ -385,16 +396,30 @@ dictionary MozProxyInfo {
  * For further details see nsILoadInfo.idl and nsIDocument::AncestorPrincipals.
  */
 dictionary MozFrameAncestorInfo {
   required ByteString url;
   required unsigned long long frameId;
 };
 
 /**
+ * Represents an HTTP request or response header.
+ */
+dictionary MozHTTPHeader {
+  /**
+   * The case-insensitive, non-case-normalized header name.
+   */
+  required ByteString name;
+  /**
+   * The header value.
+   */
+  required ByteString value;
+};
+
+/**
  * An object used for filtering requests.
  */
 dictionary MozRequestFilter {
   /**
    * If present, the request only matches if its `type` attribute matches one
    * of the given types.
    */
   sequence<MozContentPolicyType>? types = null;
--- a/toolkit/components/extensions/webrequest/ChannelWrapper.cpp
+++ b/toolkit/components/extensions/webrequest/ChannelWrapper.cpp
@@ -172,41 +172,34 @@ ChannelWrapper::SetContentType(const nsA
 
 namespace {
 
 class MOZ_STACK_CLASS HeaderVisitor final : public nsIHttpHeaderVisitor
 {
 public:
   NS_DECL_NSIHTTPHEADERVISITOR
 
-  explicit HeaderVisitor(JSContext* aCx)
-    : mCx(aCx)
-    , mMap(aCx)
+  explicit HeaderVisitor(nsTArray<dom::MozHTTPHeader>& aHeaders)
+    : mHeaders(aHeaders)
   {}
 
-  JSObject* VisitRequestHeaders(nsIHttpChannel* aChannel, ErrorResult& aRv)
+  HeaderVisitor(nsTArray<dom::MozHTTPHeader>& aHeaders,
+                const nsCString& aContentTypeHdr)
+    : mHeaders(aHeaders)
+    , mContentTypeHdr(aContentTypeHdr)
+  {}
+
+  void VisitRequestHeaders(nsIHttpChannel* aChannel, ErrorResult& aRv)
   {
-    if (!Init()) {
-      return nullptr;
-    }
-    if (!CheckResult(aChannel->VisitRequestHeaders(this), aRv)) {
-      return nullptr;
-    }
-    return mMap;
+    CheckResult(aChannel->VisitRequestHeaders(this), aRv);
   }
 
-  JSObject* VisitResponseHeaders(nsIHttpChannel* aChannel, ErrorResult& aRv)
+  void VisitResponseHeaders(nsIHttpChannel* aChannel, ErrorResult& aRv)
   {
-    if (!Init()) {
-      return nullptr;
-    }
-    if (!CheckResult(aChannel->VisitResponseHeaders(this), aRv)) {
-      return nullptr;
-    }
-    return mMap;
+    CheckResult(aChannel->VisitResponseHeaders(this), aRv);
   }
 
   NS_IMETHOD QueryInterface(REFNSIID aIID, void** aInstancePtr) override;
 
   // Stub AddRef/Release since this is a stack class.
   NS_IMETHOD_(MozExternalRefCountType) AddRef(void) override
   {
     return ++mRefCnt;
@@ -218,78 +211,71 @@ public:
   }
 
   virtual ~HeaderVisitor()
   {
     MOZ_DIAGNOSTIC_ASSERT(mRefCnt == 0);
   }
 
 private:
-  bool Init()
-  {
-    mMap = NewMapObject(mCx);
-    return mMap;
-  }
-
   bool CheckResult(nsresult aNSRv, ErrorResult& aRv)
   {
-    if (JS_IsExceptionPending(mCx)) {
-      aRv.NoteJSContextException(mCx);
-      return false;
-    }
     if (NS_FAILED(aNSRv)) {
       aRv.Throw(aNSRv);
       return false;
     }
     return true;
   }
 
-  JSContext* mCx;
-  RootedObject mMap;
+  nsTArray<dom::MozHTTPHeader>& mHeaders;
+  nsCString mContentTypeHdr = VoidCString();
 
   nsrefcnt mRefCnt = 0;
 };
 
 NS_IMETHODIMP
 HeaderVisitor::VisitHeader(const nsACString& aHeader, const nsACString& aValue)
 {
-  RootedValue header(mCx);
-  RootedValue value(mCx);
+  auto dict = mHeaders.AppendElement(fallible);
+  if (!dict) {
+    return NS_ERROR_OUT_OF_MEMORY;
+  }
+  dict->mName = aHeader;
 
-  if (!xpc::NonVoidStringToJsval(mCx, NS_ConvertUTF8toUTF16(aHeader), &header) ||
-      !xpc::NonVoidStringToJsval(mCx, NS_ConvertUTF8toUTF16(aValue), &value) ||
-      !MapSet(mCx, mMap, header, value)) {
-    return NS_ERROR_OUT_OF_MEMORY;
+  if (!mContentTypeHdr.IsVoid() && aHeader.LowerCaseEqualsLiteral("content-type")) {
+    dict->mValue = mContentTypeHdr;
+  } else {
+    dict->mValue = aValue;
   }
 
   return NS_OK;
 }
 
 NS_IMPL_QUERY_INTERFACE(HeaderVisitor, nsIHttpHeaderVisitor)
 
 } // anonymous namespace
 
 
 void
-ChannelWrapper::GetRequestHeaders(JSContext* cx, JS::MutableHandle<JSObject*> aRetVal, ErrorResult& aRv) const
+ChannelWrapper::GetRequestHeaders(nsTArray<dom::MozHTTPHeader>& aRetVal, ErrorResult& aRv) const
 {
   if (nsCOMPtr<nsIHttpChannel> chan = MaybeHttpChannel()) {
-    HeaderVisitor visitor(cx);
-    aRetVal.set(visitor.VisitRequestHeaders(chan, aRv));
+    HeaderVisitor visitor(aRetVal);
+    visitor.VisitRequestHeaders(chan, aRv);
   } else {
     aRv.Throw(NS_ERROR_UNEXPECTED);
   }
 }
 
 void
-ChannelWrapper::GetResponseHeaders(JSContext* cx, JS::MutableHandle<JSObject*> aRetVal, ErrorResult& aRv) const
+ChannelWrapper::GetResponseHeaders(nsTArray<dom::MozHTTPHeader>& aRetVal, ErrorResult& aRv) const
 {
   if (nsCOMPtr<nsIHttpChannel> chan = MaybeHttpChannel()) {
-    HeaderVisitor visitor(cx);
-    aRetVal.set(visitor.VisitResponseHeaders(chan, aRv));
+    HeaderVisitor visitor(aRetVal, mContentTypeHdr);
+    visitor.VisitResponseHeaders(chan, aRv);
   } else {
     aRv.Throw(NS_ERROR_UNEXPECTED);
   }
 }
 
 void
 ChannelWrapper::SetRequestHeader(const nsCString& aHeader, const nsCString& aValue, ErrorResult& aRv)
 {
@@ -302,17 +288,24 @@ ChannelWrapper::SetRequestHeader(const n
   }
 }
 
 void
 ChannelWrapper::SetResponseHeader(const nsCString& aHeader, const nsCString& aValue, ErrorResult& aRv)
 {
   nsresult rv = NS_ERROR_UNEXPECTED;
   if (nsCOMPtr<nsIHttpChannel> chan = MaybeHttpChannel()) {
-    rv = chan->SetResponseHeader(aHeader, aValue, false);
+    if (aHeader.LowerCaseEqualsLiteral("content-type")) {
+      rv = chan->SetContentType(aValue);
+      if (NS_SUCCEEDED(rv)) {
+        mContentTypeHdr = aValue;
+      }
+    } else {
+      rv = chan->SetResponseHeader(aHeader, aValue, false);
+    }
   }
   if (NS_FAILED(rv)) {
     aRv.Throw(rv);
   }
 }
 
 /*****************************************************************************
  * LoadInfo
--- a/toolkit/components/extensions/webrequest/ChannelWrapper.h
+++ b/toolkit/components/extensions/webrequest/ChannelWrapper.h
@@ -210,19 +210,19 @@ public:
 
   bool GetCanModify(ErrorResult& aRv) const;
 
 
   void GetProxyInfo(dom::Nullable<dom::MozProxyInfo>& aRetVal, ErrorResult& aRv) const;
 
   void GetRemoteAddress(nsCString& aRetVal) const;
 
-  void GetRequestHeaders(JSContext* cx, JS::MutableHandle<JSObject*> aRetVal, ErrorResult& aRv) const;
+  void GetRequestHeaders(nsTArray<dom::MozHTTPHeader>& aRetVal, ErrorResult& aRv) const;
 
-  void GetResponseHeaders(JSContext* cx, JS::MutableHandle<JSObject*> aRetVal, ErrorResult& aRv) const;
+  void GetResponseHeaders(nsTArray<dom::MozHTTPHeader>& aRetVal, ErrorResult& aRv) const;
 
   void SetRequestHeader(const nsCString& header, const nsCString& value, ErrorResult& aRv);
 
   void SetResponseHeader(const nsCString& header, const nsCString& value, ErrorResult& aRv);
 
 
   using EventTarget::EventListenerAdded;
   using EventTarget::EventListenerRemoved;
--- a/toolkit/modules/addons/WebRequest.jsm
+++ b/toolkit/modules/addons/WebRequest.jsm
@@ -63,24 +63,31 @@ function parseExtra(extra, allowed = [],
 function isThenable(value) {
   return value && typeof value === "object" && typeof value.then === "function";
 }
 
 class HeaderChanger {
   constructor(channel) {
     this.channel = channel;
 
-    this.originalHeaders = new Map();
-    for (let [name, value] of this.iterHeaders()) {
-      this.originalHeaders.set(name.toLowerCase(), {name, value});
+    this.array = this.readHeaders();
+  }
+
+  getMap() {
+    if (!this.map) {
+      this.map = new Map();
+      for (let header of this.array) {
+        this.map.set(header.name.toLowerCase(), header);
+      }
     }
+    return this.map;
   }
 
   toArray() {
-    return Array.from(this.originalHeaders.values());
+    return this.array;
   }
 
   validateHeaders(headers) {
     // We should probably use schema validation for this.
 
     if (!Array.isArray(headers)) {
       return false;
     }
@@ -105,74 +112,61 @@ class HeaderChanger {
       Cu.reportError(`Invalid header array: ${uneval(headers)}`);
       return;
     }
 
     let newHeaders = new Set(headers.map(
       ({name}) => name.toLowerCase()));
 
     // Remove missing headers.
-    for (let name of this.originalHeaders.keys()) {
+    let origHeaders = this.getMap();
+    for (let name of origHeaders.keys()) {
       if (!newHeaders.has(name)) {
         this.setHeader(name, "");
       }
     }
 
     // Set new or changed headers.
     for (let {name, value, binaryValue} of headers) {
       if (binaryValue) {
         value = String.fromCharCode(...binaryValue);
       }
-      let original = this.originalHeaders.get(name.toLowerCase());
+      let original = origHeaders.get(name.toLowerCase());
       if (!original || value !== original.value) {
         this.setHeader(name, value);
       }
     }
   }
 }
 
 class RequestHeaderChanger extends HeaderChanger {
   setHeader(name, value) {
     try {
       this.channel.setRequestHeader(name, value);
     } catch (e) {
       Cu.reportError(new Error(`Error setting request header ${name}: ${e}`));
     }
   }
 
-  iterHeaders() {
-    return this.channel.getRequestHeaders().entries();
+  readHeaders() {
+    return this.channel.getRequestHeaders();
   }
 }
 
 class ResponseHeaderChanger extends HeaderChanger {
   setHeader(name, value) {
     try {
-      if (name.toLowerCase() === "content-type" && value) {
-        // The Content-Type header value can't be modified, so we
-        // set the channel's content type directly, instead, and
-        // record that we made the change for the sake of
-        // subsequent observers.
-        this.channel.contentType = value;
-        this.channel._contentType = value;
-      } else {
-        this.channel.setResponseHeader(name, value);
-      }
+      this.channel.setResponseHeader(name, value);
     } catch (e) {
       Cu.reportError(new Error(`Error setting response header ${name}: ${e}`));
     }
   }
 
-  * iterHeaders() {
-    for (let [name, value] of this.channel.getResponseHeaders()) {
-      if (name.toLowerCase() === "content-type") {
-        value = this.channel._contentType || value;
-      }
-      yield [name, value];
-    }
+  readHeaders() {
+    return this.channel.getResponseHeaders();
   }
 }
 
 const MAYBE_CACHED_EVENTS = new Set([
   "onResponseStarted", "onBeforeRedirect", "onCompleted", "onErrorOccurred",
 ]);
 
 const OPTIONAL_PROPERTIES = [