Bug 1319607: Make request resumption more resilient. r?mixedpuppy draft
authorKris Maglione <maglione.k@gmail.com>
Thu, 24 Nov 2016 13:56:22 -0800
changeset 443661 38521aafadc246d3632af86951916f4f96d93955
parent 442980 1846a78b62ff79f695c81c741cb1f15c9c2cda28
child 538106 db7d5d8bc5acb42d98ac3ce9fd441088926355b0
push id37052
push usermaglione.k@gmail.com
push dateThu, 24 Nov 2016 21:57:25 +0000
reviewersmixedpuppy
bugs1319607
milestone53.0a1
Bug 1319607: Make request resumption more resilient. r?mixedpuppy MozReview-Commit-ID: 8ObjJKDf3wb
toolkit/components/extensions/test/mochitest/test_ext_webrequest_suspend.html
toolkit/modules/addons/WebRequest.jsm
--- a/toolkit/components/extensions/test/mochitest/test_ext_webrequest_suspend.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_webrequest_suspend.html
@@ -59,11 +59,81 @@ add_task(function* test_suspend() {
 
   let headers = JSON.parse(yield result.text());
 
   is(headers.foo, "Bar", "Request header was correctly set on suspended request");
 
   yield extension.unload();
 });
 
+
+// Test that requests that were canceled while suspended for a blocking
+// listener are correctly resumed.
+add_task(function* test_error_resume() {
+  let chromeScript = SpecialPowers.loadChromeScript(() => {
+    const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
+    Cu.import("resource://gre/modules/Services.jsm");
+
+    let observer = channel => {
+      if (channel instanceof Ci.nsIHttpChannel && channel.URI.spec === "http://example.com/") {
+        Services.obs.removeObserver(observer, "http-on-modify-request");
+
+        // Wait until the next tick to make sure this runs after WebRequest observers.
+        Promise.resolve().then(() => {
+          channel.cancel(Cr.NS_BINDING_ABORTED);
+        });
+      }
+    };
+
+    Services.obs.addObserver(observer, "http-on-modify-request", false);
+  });
+
+  let extension = ExtensionTestUtils.loadExtension({
+    manifest: {
+      permissions: [
+        "webRequest",
+        "webRequestBlocking",
+      ],
+    },
+
+    background() {
+      browser.webRequest.onBeforeSendHeaders.addListener(
+        details => {
+          browser.test.log(`onBeforeSendHeaders({url: ${details.url}})`);
+
+          if (details.url === "http://example.com/") {
+            browser.test.sendMessage("got-before-send-headers");
+          }
+        },
+        {urls: ["<all_urls>"]},
+        ["blocking"]);
+
+      browser.webRequest.onErrorOccurred.addListener(
+        details => {
+          browser.test.log(`onErrorOccurred({url: ${details.url}})`);
+
+          if (details.url === "http://example.com/") {
+            browser.test.sendMessage("got-error-occurred");
+          }
+        },
+        {urls: ["<all_urls>"]});
+    },
+  });
+
+  yield extension.startup();
+
+  try {
+    yield fetch("http://example.com/");
+    ok(false, "Fetch should have failed.");
+  } catch (e) {
+    ok(true, "Got expected error.");
+  }
+
+  yield extension.awaitMessage("got-before-send-headers");
+  yield extension.awaitMessage("got-error-occurred");
+
+  yield extension.unload();
+  chromeScript.destroy();
+});
+
 </script>
 </body>
 </html>
--- a/toolkit/modules/addons/WebRequest.jsm
+++ b/toolkit/modules/addons/WebRequest.jsm
@@ -775,32 +775,28 @@ HttpObserverManager = {
         if (opts.requestHeaders && result.requestHeaders && requestHeaders) {
           requestHeaders.applyChanges(result.requestHeaders);
         }
 
         if (opts.responseHeaders && result.responseHeaders && responseHeaders) {
           responseHeaders.applyChanges(result.responseHeaders);
         }
       }
+
+      if (kind === "opening") {
+        yield this.runChannelListener(channel, loadContext, "modify");
+      } else if (kind === "modify") {
+        yield this.runChannelListener(channel, loadContext, "afterModify");
+      }
     } catch (e) {
       Cu.reportError(e);
     }
 
-    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") {
+    // Only resume the channel if either it was suspended by this call.
+    if (shouldResume) {
       this.maybeResume(channel);
     }
   }),
 
   examine(channel, topic, data) {
     let loadContext = this.getLoadContext(channel);
 
     if (this.needTracing) {