Bug 1305217: Part 1 - Run webRequest listeners asynchronously. r?mixedpuppy draft
authorKris Maglione <maglione.k@gmail.com>
Thu, 10 Nov 2016 17:33:13 -0800
changeset 437529 582834f34d59eaac8ed9b1ddb1f6a2dbacc83d1f
parent 437528 5a0bcc42a285ebe2951c3e39048c88d45255edc4
child 437530 5f724a9b0dda2709c869408a461f5ed2b0ad297a
push id35434
push usermaglione.k@gmail.com
push dateFri, 11 Nov 2016 01:33:56 +0000
reviewersmixedpuppy
bugs1305217
milestone52.0a1
Bug 1305217: Part 1 - Run webRequest listeners asynchronously. r?mixedpuppy MozReview-Commit-ID: 2XF6irv5WBd
toolkit/components/extensions/ExtensionChild.jsm
toolkit/components/extensions/ExtensionParent.jsm
toolkit/components/extensions/ext-webRequest.js
toolkit/components/extensions/test/mochitest/head_webrequest.js
toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html
toolkit/modules/addons/WebRequest.jsm
--- a/toolkit/components/extensions/ExtensionChild.jsm
+++ b/toolkit/components/extensions/ExtensionChild.jsm
@@ -37,16 +37,17 @@ XPCOMUtils.defineLazyGetter(this, "Paren
                             () => ExtensionParent.ParentAPIManager);
 
 const CATEGORY_EXTENSION_SCRIPTS_ADDON = "webextension-scripts-addon";
 
 Cu.import("resource://gre/modules/ExtensionCommon.jsm");
 Cu.import("resource://gre/modules/ExtensionUtils.jsm");
 
 const {
+  DefaultMap,
   EventManager,
   SingletonEventManager,
   SpreadArgs,
   defineLazyGetter,
   findPathInObject,
   getInnerWindowID,
   getMessageManager,
   injectAPI,
@@ -468,16 +469,18 @@ var apiManager = new class extends Schem
 
   registerSchemaAPI(namespace, envType, getAPI) {
     if (envType == "addon_child") {
       super.registerSchemaAPI(namespace, envType, getAPI);
     }
   }
 }();
 
+let nextId = 1;
+
 /**
  * An object that runs an remote implementation of an API.
  */
 class ProxyAPIImplementation extends SchemaAPIInterface {
   /**
    * @param {string} namespace The full path to the namespace that contains the
    *     `name` member. This may contain dots, e.g. "storage.local".
    * @param {string} name The name of the method or property.
@@ -493,96 +496,105 @@ class ProxyAPIImplementation extends Sch
     this.childApiManager.callParentFunctionNoReturn(this.path, args);
   }
 
   callAsyncFunction(args, callback) {
     return this.childApiManager.callParentAsyncFunction(this.path, args, callback);
   }
 
   addListener(listener, args) {
-    let set = this.childApiManager.listeners.get(this.path);
-    if (!set) {
-      set = new Set();
-      this.childApiManager.listeners.set(this.path, set);
+    let map = this.childApiManager.listeners.get(this.path);
+
+    if (map.listeners.has(listener)) {
+      // TODO: Called with different args?
+      return;
     }
 
-    set.add(listener);
+    let id = nextId++;
 
-    if (set.size == 1) {
-      args = args.slice(1);
+    map.ids.set(id, listener);
+    map.listeners.set(listener, id);
 
-      this.childApiManager.messageManager.sendAsyncMessage("API:AddListener", {
-        childId: this.childApiManager.id,
-        path: this.path,
-        args,
-      });
-    }
+    this.childApiManager.messageManager.sendAsyncMessage("API:AddListener", {
+      childId: this.childApiManager.id,
+      listenerId: id,
+      path: this.path,
+      args,
+    });
   }
 
   removeListener(listener) {
-    let set = this.childApiManager.listeners.get(this.path);
-    if (!set) {
+    let map = this.childApiManager.listeners.get(this.path);
+
+    if (!map.listeners.has(listener)) {
       return;
     }
-    set.delete(listener);
+
+    let id = map.listeners.get(listener);
+    map.listeners.delete(listener);
+    map.ids.delete(id);
 
-    if (set.size == 0) {
-      this.childApiManager.messageManager.sendAsyncMessage("API:RemoveListener", {
-        childId: this.childApiManager.id,
-        path: this.path,
-      });
-    }
+    this.childApiManager.messageManager.sendAsyncMessage("API:RemoveListener", {
+      childId: this.childApiManager.id,
+      listenerId: id,
+      path: this.path,
+    });
   }
 
   hasListener(listener) {
-    let set = this.childApiManager.listeners.get(this.path);
-    return set ? set.has(listener) : false;
+    let map = this.childApiManager.listeners.get(this.path);
+    return map.listeners.has(listener);
   }
 }
 
-let nextId = 1;
-
 // We create one instance of this class for every extension context that
 // needs to use remote APIs. It uses the message manager to communicate
 // with the ParentAPIManager singleton in ExtensionParent.jsm. It
 // handles asynchronous function calls as well as event listeners.
 class ChildAPIManagerBase {
   constructor(context, messageManager, localApis, contextData) {
     this.context = context;
     this.messageManager = messageManager;
 
     // The root namespace of all locally implemented APIs. If an extension calls
     // an API that does not exist in this object, then the implementation is
     // delegated to the ParentAPIManager.
     this.localApis = localApis;
 
     this.id = `${context.extension.id}.${context.contextId}`;
 
-    messageManager.addMessageListener("API:RunListener", this);
+    MessageChannel.addListener(messageManager, "API:RunListener", this);
     messageManager.addMessageListener("API:CallResult", this);
 
-    // Map[path -> Set[listener]]
-    // path is, e.g., "runtime.onMessage".
-    this.listeners = new Map();
+    this.messageFilterStrict = {childId: this.id};
+
+    this.listeners = new DefaultMap(() => ({
+      ids: new Map(),
+      listeners: new Map(),
+    }));
 
     // Map[callId -> Deferred]
     this.callPromises = new Map();
   }
 
-  receiveMessage({name, data}) {
+  receiveMessage({name, messageName, data}) {
     if (data.childId != this.id) {
       return;
     }
 
-    switch (name) {
+    switch (name || messageName) {
       case "API:RunListener":
-        let listeners = this.listeners.get(data.path);
-        for (let callback of listeners) {
-          this.context.runSafe(callback, ...data.args);
+        let map = this.listeners.get(data.path);
+        let listener = map.ids.get(data.listenerId);
+
+        if (listener) {
+          return this.context.runSafe(listener, ...data.args);
         }
+
+        Cu.reportError(`Unknown listener at childId=${data.childId} path=${data.path} listenerId=${data.listenerId}\n`);
         break;
 
       case "API:CallResult":
         let deferred = this.callPromises.get(data.callId);
         if ("error" in data) {
           deferred.reject(data.error);
         } else {
           deferred.resolve(new SpreadArgs(data.result));
@@ -754,18 +766,17 @@ class PseudoChildAPIManager extends Chil
     // Synchronously unload the ProxyContext because we synchronously create it.
     this.context.callOnClose(this.parentContext);
   }
 
   getFallbackImplementation(namespace, name) {
     // This is gross and should be removed ASAP.
     let useDirectParentAPI = (
       // Incompatible APIs are listed here.
-      namespace == "webNavigation" || // ChildAPIManager is oblivious to filters.
-      namespace == "webRequest" // Incompatible by design (synchronous).
+      namespace == "webNavigation" // ChildAPIManager is oblivious to filters.
     );
 
     if (useDirectParentAPI) {
       let apiObj = findPathInObject(this.parentContext.apiObj, namespace, false);
 
       if (apiObj && name in apiObj) {
         return new LocalAPIImplementation(apiObj, name, this.context);
       }
--- a/toolkit/components/extensions/ExtensionParent.jsm
+++ b/toolkit/components/extensions/ExtensionParent.jsm
@@ -502,22 +502,31 @@ ParentAPIManager = {
   },
 
   addListener(data, target) {
     let context = this.getContextById(data.childId);
     if (context.parentMessageManager !== target.messageManager) {
       Cu.reportError("WebExtension warning: Message manager unexpectedly changed");
     }
 
+    let {childId} = data;
+
     function listener(...listenerArgs) {
-      context.parentMessageManager.sendAsyncMessage("API:RunListener", {
-        childId: data.childId,
-        path: data.path,
-        args: listenerArgs,
-      });
+      return context.sendMessage(
+        context.parentMessageManager,
+        "API:RunListener",
+        {
+          childId,
+          listenerId: data.listenerId,
+          path: data.path,
+          args: listenerArgs,
+        },
+        {
+          recipient: {childId},
+        });
     }
 
     context.listenerProxies.set(data.path, listener);
 
     let args = Cu.cloneInto(data.args, context.sandbox);
     findPathInObject(context.apiObj, data.path).addListener(listener, ...args);
   },
 
--- a/toolkit/components/extensions/ext-webRequest.js
+++ b/toolkit/components/extensions/ext-webRequest.js
@@ -8,17 +8,16 @@ XPCOMUtils.defineLazyModuleGetter(this, 
                                   "resource://gre/modules/MatchPattern.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "WebRequest",
                                   "resource://gre/modules/WebRequest.jsm");
 
 Cu.import("resource://gre/modules/ExtensionManagement.jsm");
 Cu.import("resource://gre/modules/ExtensionUtils.jsm");
 var {
   SingletonEventManager,
-  runSafeSync,
 } = ExtensionUtils;
 
 // EventManager-like class specifically for WebRequest. Inherits from
 // SingletonEventManager. Takes care of converting |details| parameter
 // when invoking listeners.
 function WebRequestEventManager(context, eventName) {
   let name = `webRequest.${eventName}`;
   let register = (callback, filter, info) => {
@@ -54,17 +53,17 @@ function WebRequestEventManager(context,
       let optional = ["requestHeaders", "responseHeaders", "statusCode", "statusLine", "error", "redirectUrl",
                       "requestBody"];
       for (let opt of optional) {
         if (opt in data) {
           data2[opt] = data[opt];
         }
       }
 
-      return runSafeSync(context, callback, data2);
+      return context.runSafe(callback, data2);
     };
 
     let filter2 = {};
     filter2.urls = new MatchPattern(filter.urls);
     if (filter.types) {
       filter2.types = filter.types;
     }
     if (filter.tabId) {
--- a/toolkit/components/extensions/test/mochitest/head_webrequest.js
+++ b/toolkit/components/extensions/test/mochitest/head_webrequest.js
@@ -30,16 +30,19 @@ function background(events) {
       // last event for that entry.  This will either be onCompleted, or the
       // last entry if an events list was provided.
       promises.push(new Promise(resolve => { entry.test.resolve = resolve; }));
       // If events was left undefined, we're expecting all normal events we're
       // listening for, exclude onBeforeRedirect and onErrorOccurred
       if (entry.events === undefined) {
         entry.events = Object.keys(events).filter(name => name != "onErrorOccurred" && name != "onBeforeRedirect");
       }
+      if (entry.optional_events === undefined) {
+        entry.optional_events = [];
+      }
     }
     // When every expected entry has finished our test is done.
     Promise.all(promises).then(() => {
       browser.test.sendMessage("done");
     });
     browser.test.sendMessage("continue");
   });
 
@@ -148,20 +151,25 @@ function background(events) {
     return details => {
       let result = {};
       browser.test.log(`${name} ${details.requestId} ${details.url}`);
       let expected = getExpected(details);
       if (!expected) {
         return result;
       }
       let expectedEvent = expected.events[0] == name;
-      browser.test.assertTrue(expectedEvent, `recieved ${name}`);
       if (expectedEvent) {
         expected.events.shift();
+      } else {
+        expectedEvent = expected.optional_events[0] == name;
+        if (expectedEvent) {
+          expected.optional_events.shift();
+        }
       }
+      browser.test.assertTrue(expectedEvent, `received ${name}`);
       browser.test.assertEq(expected.type, details.type, "resource type is correct");
       browser.test.assertEq(expected.origin || defaultOrigin, details.originUrl, "origin is correct");
 
       if (name == "onBeforeRequest") {
         // Save some values to test request consistency in later events.
         browser.test.assertTrue(details.tabId !== undefined, `tabId ${details.tabId}`);
         browser.test.assertTrue(details.requestId !== undefined, `requestId ${details.requestId}`);
         // Validate requestId if it's already set, this happens with redirects.
@@ -194,17 +202,17 @@ function background(events) {
       }
       if (name == "onSendHeaders") {
         if (expected.headers && expected.headers.request) {
           checkHeaders("request", expected, details);
         }
       }
       if (name == "onHeadersReceived") {
         browser.test.assertEq(expected.status || 200, details.statusCode,
-                              `expected HTTP status recieved for ${details.url}`);
+                              `expected HTTP status received for ${details.url}`);
         if (expected.headers && expected.headers.response) {
           result.responseHeaders = processHeaders("response", expected, details);
         }
       }
       if (name == "onCompleted") {
         // If we have already completed a GET request for this url,
         // and it was found, we expect for the response to come fromCache.
         // expected.cached may be undefined, force boolean.
--- a/toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html
@@ -38,19 +38,19 @@ add_task(function* setup() {
 add_task(function* test_webRequest_links() {
   let expect = {
     "file_style_bad.css": {
       type: "stylesheet",
       events: ["onBeforeRequest", "onErrorOccurred"],
       cancel: "onBeforeRequest",
     },
     "file_style_redirect.css": {
-      status: 302,
       type: "stylesheet",
       events: ["onBeforeRequest", "onBeforeSendHeaders", "onBeforeRedirect"],
+      optional_events: ["onHeadersReceived"],
       redirect: "file_style_good.css",
     },
     "file_style_good.css": {
       type: "stylesheet",
     },
   };
   extension.sendMessage("set-expected", {expect, origin: location.href});
   yield extension.awaitMessage("continue");
@@ -67,19 +67,19 @@ add_task(function* test_webRequest_links
 add_task(function* test_webRequest_images() {
   let expect = {
     "file_image_bad.png": {
       type: "image",
       events: ["onBeforeRequest", "onErrorOccurred"],
       cancel: "onBeforeRequest",
     },
     "file_image_redirect.png": {
-      status: 302,
       type: "image",
       events: ["onBeforeRequest", "onBeforeSendHeaders", "onBeforeRedirect"],
+      optional_events: ["onHeadersReceived"],
       redirect: "file_image_good.png",
     },
     "file_image_good.png": {
       type: "image",
     },
   };
   extension.sendMessage("set-expected", {expect, origin: location.href});
   yield extension.awaitMessage("continue");
@@ -93,19 +93,19 @@ add_task(function* test_webRequest_image
 add_task(function* test_webRequest_scripts() {
   let expect = {
     "file_script_bad.js": {
       type: "script",
       events: ["onBeforeRequest", "onErrorOccurred"],
       cancel: "onBeforeRequest",
     },
     "file_script_redirect.js": {
-      status: 302,
       type: "script",
       events: ["onBeforeRequest", "onBeforeSendHeaders", "onBeforeRedirect"],
+      optional_events: ["onHeadersReceived"],
       redirect: "file_script_good.js",
     },
     "file_script_good.js": {
       type: "script",
     },
   };
   extension.sendMessage("set-expected", {expect, origin: location.href});
   yield extension.awaitMessage("continue");
@@ -237,17 +237,17 @@ add_task(function* test_webRequest_tabId
 add_task(function* test_webRequest_frames() {
   let expect = {
     "text/plain,webRequestTest": {
       type: "sub_frame",
       events: ["onBeforeRequest", "onCompleted"],
     },
     "text/plain,webRequestTest_bad": {
       type: "sub_frame",
-      events: ["onBeforeRequest", "onErrorOccurred"],
+      events: ["onBeforeRequest", "onCompleted"],
       cancel: "onBeforeRequest",
     },
     "redirection.sjs": {
       status: 302,
       type: "sub_frame",
       events: ["onBeforeRequest", "onBeforeSendHeaders", "onSendHeaders", "onHeadersReceived", "onBeforeRedirect"],
     },
     "dummy_page.html": {
--- a/toolkit/modules/addons/WebRequest.jsm
+++ b/toolkit/modules/addons/WebRequest.jsm
@@ -326,28 +326,30 @@ ContentPolicyManager.init();
 function StartStopListener(manager, loadContext) {
   this.manager = manager;
   this.loadContext = loadContext;
   this.orig = null;
 }
 
 StartStopListener.prototype = {
   QueryInterface: XPCOMUtils.generateQI([Ci.nsIRequestObserver,
-                                         Ci.nsIStreamListener,
-                                         Ci.nsISupports]),
+                                         Ci.nsIStreamListener]),
 
   onStartRequest: function(request, context) {
     this.manager.onStartRequest(request, this.loadContext);
-    return this.orig.onStartRequest(request, context);
+    this.orig.onStartRequest(request, context);
   },
 
   onStopRequest(request, context, statusCode) {
-    let result = this.orig.onStopRequest(request, context, statusCode);
+    try {
+      this.orig.onStopRequest(request, context, statusCode);
+    } catch (e) {
+      Cu.reportError(e);
+    }
     this.manager.onStopRequest(request, this.loadContext);
-    return result;
   },
 
   onDataAvailable(...args) {
     return this.orig.onDataAvailable(...args);
   },
 };
 
 var ChannelEventSink = {
@@ -559,209 +561,263 @@ HttpObserverManager = {
   errorCheck(channel, loadContext, channelData = null) {
     let errorData = this.maybeError(channel, null, channelData);
     if (errorData) {
       this.runChannelListener(channel, loadContext, "onError", errorData);
     }
     return errorData;
   },
 
+  /**
+   * Resumes the channel if it is currently suspended due to this
+   * listener.
+   *
+   * @param {nsIChannel} channel
+   *        The channel to possibly suspend.
+   */
+  maybeResume(channel) {
+    let data = getData(channel);
+    if (data.suspended) {
+      channel.resume();
+      data.suspended = false;
+    }
+  },
+
+  /**
+   * Suspends the channel if it is not currently suspended due to this
+   * listener. Returns true if the channel was suspended as a result of
+   * this call.
+   *
+   * @param {nsIChannel} channel
+   *        The channel to possibly suspend.
+   * @returns {boolean}
+   *        True if this call resulted in the channel being suspended.
+   */
+  maybeSuspend(channel) {
+    let data = getData(channel);
+    if (!data.suspended) {
+      channel.suspend();
+      data.suspended = true;
+      return true;
+    }
+  },
+
   runChannelListener(channel, loadContext = null, kind, extraData = null) {
-    if (this.activityInitialized) {
-      let channelData = getData(channel);
-      if (kind === "onError") {
-        if (channelData.errorNotified) {
+    let handlerResults = [];
+    let requestHeaders;
+    let responseHeaders;
+
+    try {
+      if (this.activityInitialized) {
+        let channelData = getData(channel);
+        if (kind === "onError") {
+          if (channelData.errorNotified) {
+            return;
+          }
+          channelData.errorNotified = true;
+        } else if (this.errorCheck(channel, loadContext, channelData)) {
           return;
         }
-        channelData.errorNotified = true;
-      } else if (this.errorCheck(channel, loadContext, channelData)) {
-        return;
       }
-    }
-    let listeners = this.listeners[kind];
-    let browser = loadContext && loadContext.topFrameElement;
-    let loadInfo = channel.loadInfo;
-    let policyType = (loadInfo ? loadInfo.externalContentPolicyType
-                               : Ci.nsIContentPolicy.TYPE_OTHER);
+      let listeners = this.listeners[kind];
+      let browser = loadContext && loadContext.topFrameElement;
+      let loadInfo = channel.loadInfo;
+      let policyType = (loadInfo ? loadInfo.externalContentPolicyType
+                                 : Ci.nsIContentPolicy.TYPE_OTHER);
 
-    let includeStatus = (["headersReceived", "onRedirect", "onStart", "onStop"].includes(kind) &&
-                         channel instanceof Ci.nsIHttpChannel);
+      let includeStatus = (["headersReceived", "onRedirect", "onStart", "onStop"].includes(kind) &&
+                           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 blockable = ["headersReceived", "opening", "modify"].includes(kind);
 
-    let commonData = null;
-    let uri = channel.URI;
-    let handlerResults = [];
-    let requestBody;
-    for (let [callback, opts] of listeners.entries()) {
-      if (!this.shouldRunListener(policyType, uri, opts.filter)) {
-        continue;
+      if (channel instanceof Ci.nsIHttpChannel) {
+        requestHeaders = new RequestHeaderChanger(channel);
+        try {
+          responseHeaders = new ResponseHeaderChanger(channel);
+        } catch (e) {
+          // Just ignore this for the request phases where response headers
+          // aren't yet available.
+        }
       }
 
-      if (!commonData) {
-        commonData = {
-          requestId: RequestId.get(channel),
-          url: uri.spec,
-          method: channel.requestMethod,
-          browser: browser,
-          type: WebRequestCommon.typeForPolicyType(policyType),
-          fromCache: getData(channel).fromCache,
-          windowId: 0,
-          parentWindowId: 0,
-        };
+      let commonData = null;
+      let uri = channel.URI;
+      let requestBody;
+      for (let [callback, opts] of listeners.entries()) {
+        if (!this.shouldRunListener(policyType, uri, opts.filter)) {
+          continue;
+        }
+
+        if (!commonData) {
+          commonData = {
+            requestId: RequestId.get(channel),
+            url: uri.spec,
+            method: channel.requestMethod,
+            browser: browser,
+            type: WebRequestCommon.typeForPolicyType(policyType),
+            fromCache: getData(channel).fromCache,
+            windowId: 0,
+            parentWindowId: 0,
+          };
 
-        if (loadInfo) {
-          let originPrincipal = loadInfo.triggeringPrincipal;
-          if (originPrincipal.URI) {
-            commonData.originUrl = originPrincipal.URI.spec;
+          if (loadInfo) {
+            let originPrincipal = loadInfo.triggeringPrincipal;
+            if (originPrincipal.URI) {
+              commonData.originUrl = originPrincipal.URI.spec;
+            }
+
+            let {isSystemPrincipal} = Services.scriptSecurityManager;
+
+            commonData.isSystemPrincipal = (isSystemPrincipal(loadInfo.triggeringPrincipal) ||
+                                            isSystemPrincipal(loadInfo.loadingPrincipal));
+
+            if (loadInfo.frameOuterWindowID) {
+              Object.assign(commonData, {
+                windowId: loadInfo.frameOuterWindowID,
+                parentWindowId: loadInfo.outerWindowID,
+              });
+            } else {
+              Object.assign(commonData, {
+                windowId: loadInfo.outerWindowID,
+                parentWindowId: loadInfo.parentOuterWindowID,
+              });
+            }
           }
 
-          let {isSystemPrincipal} = Services.scriptSecurityManager;
-
-          commonData.isSystemPrincipal = (isSystemPrincipal(loadInfo.triggeringPrincipal) ||
-                                          isSystemPrincipal(loadInfo.loadingPrincipal));
+          if (channel instanceof Ci.nsIHttpChannelInternal) {
+            try {
+              commonData.ip = channel.remoteAddress;
+            } catch (e) {
+              // The remoteAddress getter throws if the address is unavailable,
+              // but ip is an optional property so just ignore the exception.
+            }
+          }
 
-          if (loadInfo.frameOuterWindowID) {
-            Object.assign(commonData, {
-              windowId: loadInfo.frameOuterWindowID,
-              parentWindowId: loadInfo.outerWindowID,
-            });
-          } else {
-            Object.assign(commonData, {
-              windowId: loadInfo.outerWindowID,
-              parentWindowId: loadInfo.parentOuterWindowID,
-            });
-          }
+          Object.assign(commonData, extraData);
         }
 
-        if (channel instanceof Ci.nsIHttpChannelInternal) {
-          try {
-            commonData.ip = channel.remoteAddress;
-          } catch (e) {
-            // The remoteAddress getter throws if the address is unavailable,
-            // but ip is an optional property so just ignore the exception.
-          }
+        let data = Object.assign({}, commonData);
+
+        if (opts.requestHeaders) {
+          data.requestHeaders = requestHeaders.toArray();
+        }
+
+        if (opts.responseHeaders && responseHeaders) {
+          data.responseHeaders = responseHeaders.toArray();
         }
 
-        Object.assign(commonData, extraData);
-      }
-
-      let data = Object.assign({}, commonData);
+        if (opts.requestBody) {
+          requestBody = requestBody || WebRequestUpload.createRequestBody(channel);
+          data.requestBody = requestBody;
+        }
 
-      if (opts.requestHeaders) {
-        data.requestHeaders = requestHeaders.toArray();
-      }
-
-      if (opts.responseHeaders && responseHeaders) {
-        data.responseHeaders = responseHeaders.toArray();
-      }
+        if (includeStatus) {
+          mergeStatus(data, channel, kind);
+        }
 
-      if (opts.requestBody) {
-        requestBody = requestBody || WebRequestUpload.createRequestBody(channel);
-        data.requestBody = requestBody;
-      }
-
-      if (includeStatus) {
-        mergeStatus(data, channel, kind);
-      }
+        try {
+          let result = callback(data);
 
-      try {
-        let result = callback(data);
-
-        if (result && typeof result === "object" && opts.blocking) {
-          handlerResults.push({opts, result});
+          if (result && typeof result === "object" && blockable && opts.blocking) {
+            handlerResults.push({opts, result});
+          }
+        } catch (e) {
+          Cu.reportError(e);
         }
-      } catch (e) {
-        Cu.reportError(e);
       }
+    } catch (e) {
+      Cu.reportError(e);
     }
 
-    this.applyChanges(kind, channel, loadContext, handlerResults, requestHeaders, responseHeaders);
+    return this.applyChanges(kind, channel, loadContext, handlerResults, requestHeaders, responseHeaders);
   },
 
   applyChanges: Task.async(function* (kind, channel, loadContext, handlerResults, requestHeaders, responseHeaders) {
     let asyncHandlers = handlerResults.filter(({result}) => isThenable(result));
     let isAsync = asyncHandlers.length > 0;
+    let shouldResume = false;
 
     try {
       if (isAsync) {
-        channel.suspend();
+        shouldResume = this.maybeSuspend(channel);
 
         for (let value of asyncHandlers) {
           try {
             value.result = yield value.result;
           } catch (e) {
             Cu.reportError(e);
             value.result = {};
           }
         }
       }
 
       for (let {opts, result} of handlerResults) {
         if (result.cancel) {
+          this.maybeResume(channel);
           channel.cancel(Cr.NS_ERROR_ABORT);
 
           this.errorCheck(channel, loadContext);
           return;
         }
 
         if (result.redirectUrl) {
           try {
-            if (isAsync) {
-              channel.resume();
-            }
+            this.maybeResume(channel);
 
             channel.redirectTo(BrowserUtils.makeURI(result.redirectUrl));
             return;
           } catch (e) {
             Cu.reportError(e);
           }
         }
 
-        if (opts.requestHeaders && result.requestHeaders) {
+        if (opts.requestHeaders && result.requestHeaders && requestHeaders) {
           requestHeaders.applyChanges(result.requestHeaders);
         }
 
-        if (opts.responseHeaders && result.responseHeaders) {
+        if (opts.responseHeaders && result.responseHeaders && responseHeaders) {
           responseHeaders.applyChanges(result.responseHeaders);
         }
       }
     } catch (e) {
       Cu.reportError(e);
     }
 
-    if (isAsync) {
-      channel.resume();
-    }
 
     if (kind === "opening") {
       return this.runChannelListener(channel, loadContext, "modify");
     } else if (kind === "modify") {
       return this.runChannelListener(channel, loadContext, "afterModify");
     }
+
+    // Only resume the channel if either it was suspended by this call,
+    // and this callback is not part of the opening-modify-afterModify
+    // chain, or if this is an afterModify callback. This allows us to
+    // chain the aforementioned handlers without repeatedly suspending
+    // and resuming the request.
+    if (shouldResume || kind === "afterModify") {
+      this.maybeResume(channel);
+    }
   }),
 
   examine(channel, topic, data) {
     let loadContext = this.getLoadContext(channel);
 
     if (this.needTracing) {
-      if (channel instanceof Ci.nsITraceableChannel) {
+      // Check whether we've already added a listener to this channel,
+      // so we don't wind up chaining multiple listeners.
+      let channelData = getData(channel);
+      if (!channelData.listener && channel instanceof Ci.nsITraceableChannel) {
         let responseStatus = channel.responseStatus;
         // skip redirections, https://bugzilla.mozilla.org/show_bug.cgi?id=728901#c8
         if (responseStatus < 300 || responseStatus >= 400) {
           let listener = new StartStopListener(this, loadContext);
           let orig = channel.setNewListener(listener);
           listener.orig = orig;
+          channelData.listener = listener;
         }
       }
     }
 
     this.runChannelListener(channel, loadContext, "headersReceived");
   },
 
   onChannelReplaced(oldChannel, newChannel) {