Bug 1254204: Part 1 - Apply WebRequest header changes differentially, after all listeners have run in parallel. r=mixedpuppy
authorKris Maglione <maglione.k@gmail.com>
Wed, 09 Nov 2016 13:36:54 -0800
changeset 321883 30aa827dc7849d11478f2bcc821825c324f41de8
parent 321882 43835f5fa2b217ba7e2a59fb9f52910a2e06a28a
child 321884 07b39b68b0844b18d16747dd9a9502d47d2c487a
push id83705
push usermaglione.k@gmail.com
push dateWed, 09 Nov 2016 23:05:37 +0000
treeherdermozilla-inbound@b913f0ce1e3e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmixedpuppy
bugs1254204
milestone52.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 1254204: Part 1 - Apply WebRequest header changes differentially, after all listeners have run in parallel. r=mixedpuppy MozReview-Commit-ID: Jk1ja5Y3lMI
toolkit/components/extensions/test/mochitest/test_ext_webrequest.html
toolkit/modules/addons/WebRequest.jsm
--- a/toolkit/components/extensions/test/mochitest/test_ext_webrequest.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_webrequest.html
@@ -165,48 +165,48 @@ function backgroundScript() {
                  };
   let testHeaders = {
     request: {
       added: {
         "X-WebRequest-request": "text",
         "X-WebRequest-request-binary": "binary",
       },
       modified: {
-        "User-Agent": "WebRequest",
+        "user-agent": "WebRequest",
       },
       deleted: [
-        "Referer",
+        "referer",
       ],
     },
     response: {
       added: {
         "X-WebRequest-response": "text",
         "X-WebRequest-response-binary": "binary",
       },
       modified: {
-        "Server": "WebRequest",
-        "Content-Type": "text/html; charset=utf-8",
+        "server": "WebRequest",
+        "content-type": "text/html; charset=utf-8",
       },
       deleted: [
-        "Connection",
+        "connection",
       ],
     },
   };
 
   function checkResourceType(type) {
     let key = type.toUpperCase();
     browser.test.assertTrue(key in browser.webRequest.ResourceType, `valid resource type ${key}`);
   }
 
   function processHeaders(phase, details) {
     let headers = details[`${phase}Headers`];
     browser.test.assertTrue(Array.isArray(headers), `${phase}Headers array present`);
 
-    let processedMark = "WebRequest-processed";
-    if (headers.find(h => h.name === processedMark)) {
+    let processedMark = "webrequest-processed";
+    if (headers.find(h => h.name.toLowerCase() === processedMark)) {
       // This may happen because of redirections or cache
       browser.test.log(`${phase}Headers in ${details.requestId} already processed`);
       skippedRequests.add(details.requestId);
       return null;
     }
     headers.push({name: processedMark, value: "1"});
 
     let {added, modified, deleted} = testHeaders[phase];
@@ -218,25 +218,27 @@ function backgroundScript() {
         header.binaryValue = Array.from(added[name], c => c.charCodeAt(0));
       } else {
         header.value = added[name];
       }
       headers.push(header);
     }
 
     let modifiedAny = false;
-    for (let header of headers.filter(h => h.name in modified)) {
-      header.value = modified[header.name];
-      modifiedAny = true;
+    for (let header of headers) {
+      if (header.name.toLowerCase() in modified) {
+        header.value = modified[header.name.toLowerCase()];
+        modifiedAny = true;
+      }
     }
     browser.test.assertTrue(modifiedAny, `at least one ${phase}Headers element to modify`);
 
     let deletedAny = false;
     for (let j = headers.length; j-- > 0;) {
-      if (deleted.includes(headers[j].name)) {
+      if (deleted.includes(headers[j].name.toLowerCase())) {
         headers.splice(j, 1);
         deletedAny = true;
       }
     }
     browser.test.assertTrue(deletedAny, `at least one ${phase}Headers element to delete`);
 
     return headers;
   }
@@ -246,17 +248,17 @@ function backgroundScript() {
       return;
     }
 
     let headers = details[`${phase}Headers`];
     browser.test.assertTrue(Array.isArray(headers), `valid ${phase}Headers array`);
 
     let {added, modified, deleted} = testHeaders[phase];
     for (let name in added) {
-      browser.test.assertTrue(headers.some(h => h.name === name && h.value === added[name]), `header ${name} correctly injected in ${phase}Headers`);
+      browser.test.assertTrue(headers.some(h => h.name.toLowerCase() === name.toLowerCase() && h.value === added[name]), `header ${name} correctly injected in ${phase}Headers`);
     }
 
     let modifiedAny = false;
     browser.test.log(`HEADERS ${JSON.stringify(headers)}`);
     for (let header of headers.filter(h => h.name in modified)) {
       let {name, value} = header;
       if (name.toLowerCase() === "content-type" && skippedRequests.has(details.requestId)) {
         // Changes to Content-Type headers are not persisted in the cache.
--- a/toolkit/modules/addons/WebRequest.jsm
+++ b/toolkit/modules/addons/WebRequest.jsm
@@ -110,16 +110,125 @@ function mergeStatus(data, channel, even
     // NS_ERROR_NOT_AVAILABLE might be thrown if it's an internal redirect, happening before
     // any actual HTTP traffic. Otherwise, let's report.
     if (event !== "onRedirect" || e.result !== Cr.NS_ERROR_NOT_AVAILABLE) {
       Cu.reportError(`webRequest Error: ${e} trying to merge status in ${event}@${channel.name}`);
     }
   }
 }
 
+class HeaderChanger {
+  constructor(channel) {
+    this.channel = channel;
+
+    this.originalHeaders = new Map();
+    this.visitHeaders((name, value) => {
+      this.originalHeaders.set(name.toLowerCase(), value);
+    });
+  }
+
+  toArray() {
+    return Array.from(this.originalHeaders,
+                      ([name, value]) => ({name, value}));
+  }
+
+  validateHeaders(headers) {
+    // We should probably use schema validation for this.
+
+    if (!Array.isArray(headers)) {
+      return false;
+    }
+
+    return headers.every(header => {
+      if (typeof header !== "object" || header === null) {
+        return false;
+      }
+
+      if (typeof header.name !== "string") {
+        return false;
+      }
+
+      return (typeof header.value === "string" ||
+              Array.isArray(header.binaryValue));
+    });
+  }
+
+  applyChanges(headers) {
+    if (!this.validateHeaders(headers)) {
+      /* globals uneval */
+      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()) {
+      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);
+      }
+      if (value !== this.originalHeaders.get(name.toLowerCase())) {
+        this.setHeader(name, value);
+      }
+    }
+  }
+}
+
+class RequestHeaderChanger extends HeaderChanger {
+  setHeader(name, value) {
+    try {
+      this.channel.setRequestHeader(name, value, false);
+    } catch (e) {
+      Cu.reportError(new Error(`Error setting request header ${name}: ${e}`));
+    }
+  }
+
+  visitHeaders(visitor) {
+    this.channel.visitRequestHeaders(visitor);
+  }
+}
+
+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;
+
+        getData(this.channel).contentType = value;
+      } else {
+        this.channel.setResponseHeader(name, value, false);
+      }
+    } catch (e) {
+      Cu.reportError(new Error(`Error setting response header ${name}: ${e}`));
+    }
+  }
+
+  visitHeaders(visitor) {
+    this.channel.visitResponseHeaders((name, value) => {
+      if (name.toLowerCase() === "content-type") {
+        value = getData(this.channel).contentType || value;
+      }
+
+      visitor(name, value);
+    });
+  }
+}
+
 var HttpObserverManager;
 
 var ContentPolicyManager = {
   policyData: new Map(),
   policies: new Map(),
   idMap: new Map(),
   nextId: 0,
 
@@ -366,71 +475,16 @@ HttpObserverManager = {
                       .notificationCallbacks
                       .getInterface(Components.interfaces.nsILoadContext);
       } catch (e) {
         return null;
       }
     }
   },
 
-  getHeaders(channel, method, event) {
-    let headers = [];
-    let visitor = {
-      visitHeader(name, value) {
-        try {
-          value = channel.getProperty(`webrequest-header-${name.toLowerCase()}`);
-        } catch (e) {
-          // This will throw if the property does not exist.
-        }
-        headers.push({name, value});
-      },
-
-      QueryInterface: XPCOMUtils.generateQI([Ci.nsIHttpHeaderVisitor,
-                                             Ci.nsISupports]),
-    };
-
-    try {
-      channel.QueryInterface(Ci.nsIPropertyBag);
-      channel[method](visitor);
-    } catch (e) {
-      Cu.reportError(`webRequest Error: ${e} trying to perform ${method} in ${event}@${channel.name}`);
-    }
-    return headers;
-  },
-
-  replaceHeaders(headers, originalNames, setHeader) {
-    let failures = new Set();
-    // Start by clearing everything.
-    for (let name of originalNames) {
-      try {
-        setHeader(name, "");
-      } catch (e) {
-        // Let's collect physiological failures in order
-        // to know what is worth reporting.
-        failures.add(name);
-      }
-    }
-    try {
-      for (let {name, value, binaryValue} of headers) {
-        try {
-          if (Array.isArray(binaryValue)) {
-            value = String.fromCharCode.apply(String, binaryValue);
-          }
-          setHeader(name, value);
-        } catch (e) {
-          if (!failures.has(name)) {
-            Cu.reportError(e);
-          }
-        }
-      }
-    } catch (e) {
-      Cu.reportError(e);
-    }
-  },
-
   observe(subject, topic, data) {
     let channel = subject.QueryInterface(Ci.nsIHttpChannel);
     switch (topic) {
       case "http-on-modify-request":
         this.modify(channel, topic, data);
         break;
       case "http-on-examine-cached-response":
       case "http-on-examine-merged-response":
@@ -513,30 +567,37 @@ HttpObserverManager = {
     }
     let listeners = this.listeners[kind];
     let browser = loadContext ? loadContext.topFrameElement : null;
     let loadInfo = channel.loadInfo;
     let policyType = loadInfo ?
                      loadInfo.externalContentPolicyType :
                      Ci.nsIContentPolicy.TYPE_OTHER;
 
-    let requestHeaderNames;
-    let responseHeaderNames;
-
     let requestBody;
 
     let includeStatus = (
                           kind === "headersReceived" ||
                           kind === "onRedirect" ||
                           kind === "onStart" ||
                           kind === "onStop"
                         ) && channel instanceof Ci.nsIHttpChannel;
 
+    let requestHeaders = new RequestHeaderChanger(channel);
+    let responseHeaders;
+    try {
+      responseHeaders = new ResponseHeaderChanger(channel);
+    } catch (e) {
+      // Just ignore this for the request phases where response headers
+      // aren't yet available.
+    }
+
     let commonData = null;
     let uri = channel.URI;
+    let handlerResults = [];
     for (let [callback, opts] of listeners.entries()) {
       if (!this.shouldRunListener(policyType, uri, opts.filter)) {
         continue;
       }
 
       if (!commonData) {
         commonData = {
           requestId: RequestId.get(channel),
@@ -576,80 +637,73 @@ HttpObserverManager = {
             // The remoteAddress getter throws if the address is unavailable,
             // but ip is an optional property so just ignore the exception.
           }
         }
         if (extraData) {
           Object.assign(commonData, extraData);
         }
       }
+
       let data = Object.assign({}, commonData);
+
       if (opts.requestHeaders) {
-        data.requestHeaders = this.getHeaders(channel, "visitRequestHeaders", kind);
-        requestHeaderNames = data.requestHeaders.map(h => h.name);
+        data.requestHeaders = requestHeaders.toArray();
       }
-      if (opts.responseHeaders) {
-        data.responseHeaders = this.getHeaders(channel, "visitResponseHeaders", kind);
-        responseHeaderNames = data.responseHeaders.map(h => h.name);
+
+      if (opts.responseHeaders && responseHeaders) {
+        data.responseHeaders = responseHeaders.toArray();
       }
+
       if (opts.requestBody) {
         if (requestBody === undefined) {
           requestBody = WebRequestUpload.createRequestBody(channel);
         }
         if (requestBody) {
           data.requestBody = requestBody;
         }
       }
+
       if (includeStatus) {
         mergeStatus(data, channel, kind);
       }
 
-      let result = null;
       try {
-        result = callback(data);
+        let result = callback(data);
+
+        if (result && typeof result === "object" && opts.blocking) {
+          handlerResults.push({opts, result});
+        }
       } catch (e) {
         Cu.reportError(e);
       }
+    }
 
-      if (!result || !opts.blocking) {
-        continue;
-      }
+    for (let {opts, result} of handlerResults) {
       if (result.cancel) {
         channel.cancel(Cr.NS_ERROR_ABORT);
         this.errorCheck(channel, loadContext);
         return false;
       }
+
       if (result.redirectUrl) {
-        channel.redirectTo(BrowserUtils.makeURI(result.redirectUrl));
-        return false;
+        try {
+          channel.redirectTo(BrowserUtils.makeURI(result.redirectUrl));
+          return false;
+        } catch (e) {
+          Cu.reportError(e);
+        }
       }
+
       if (opts.requestHeaders && result.requestHeaders) {
-        this.replaceHeaders(
-          result.requestHeaders, requestHeaderNames,
-          (name, value) => channel.setRequestHeader(name, value, false)
-        );
+        requestHeaders.applyChanges(result.requestHeaders);
       }
+
       if (opts.responseHeaders && result.responseHeaders) {
-        this.replaceHeaders(
-          result.responseHeaders, responseHeaderNames,
-          (name, value) => {
-            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.
-              channel.contentType = value;
-
-              channel.QueryInterface(Ci.nsIWritablePropertyBag);
-              channel.setProperty("webrequest-header-content-type", value);
-            } else {
-              channel.setResponseHeader(name, value, false);
-            }
-          }
-        );
+        responseHeaders.applyChanges(result.responseHeaders);
       }
     }
 
     return true;
   },
 
   modify(channel, topic, data) {
     let loadContext = this.getLoadContext(channel);