Bug 1448599 - fix WebRequest events for redirected channels when onBeforeRedirect is not used, r?kmag draft
authorShane Caraveo <scaraveo@mozilla.com>
Mon, 02 Apr 2018 21:12:07 -0500
changeset 776428 d3d10c7f458483286f7759c8ff1f6378074737c6
parent 776387 445255800255bb13ed096b5b7da36aa835e41dd8
push id104878
push usermixedpuppy@gmail.com
push dateTue, 03 Apr 2018 02:12:42 +0000
reviewerskmag
bugs1448599
milestone61.0a1
Bug 1448599 - fix WebRequest events for redirected channels when onBeforeRedirect is not used, r?kmag If any WebRequest listener *other* than onBeforeRedirect was used, and a channel was redirected, the ChannelWrapper was not updated with the redirected channel. This broke most listeners for redirected channels and specifically an extension would never see events for the new channel. MozReview-Commit-ID: BwXToTD5LEu
toolkit/components/extensions/test/xpcshell/test_ext_redirects.js
toolkit/modules/addons/WebRequest.jsm
--- a/toolkit/components/extensions/test/xpcshell/test_ext_redirects.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_redirects.js
@@ -7,16 +7,17 @@ Cu.importGlobalProperties(["XMLHttpReque
 
 const server = createHttpServer();
 const gServerUrl = `http://localhost:${server.identity.primaryPort}`;
 
 server.registerPathHandler("/redirect", (request, response) => {
   let params = new URLSearchParams(request.queryString);
   response.setStatusLine(request.httpVersion, 302, "Moved Temporarily");
   response.setHeader("Location", params.get("redirect_uri"));
+  response.write("redirecting");
 });
 
 server.registerPathHandler("/dummy", (request, response) => {
   response.setStatusLine(request.httpVersion, 200, "OK");
   response.write("ok");
 });
 
 function onStopListener(channel) {
@@ -51,24 +52,26 @@ async function onModifyListener(originUr
     let channel = subject.QueryInterface(Ci.nsIHttpChannel);
     if (redirectToUrl) {
       channel.redirectTo(Services.io.newURI(redirectToUrl));
     }
     return channel;
   });
 }
 
-function getExtension(accessible = false, background = undefined) {
+function getExtension(accessible = false, background = undefined, blocking = true) {
   let manifest = {
     "permissions": [
       "webRequest",
-      "webRequestBlocking",
       "<all_urls>",
     ],
   };
+  if (blocking) {
+    manifest.permissions.push("webRequestBlocking");
+  }
   if (accessible) {
     manifest.web_accessible_resources = ["finished.html"];
   }
   if (!background) {
     background = () => {
       // send the extensions public uri to the test.
       let exturi = browser.extension.getURL("finished.html");
       browser.test.sendMessage("redirectURI", exturi);
@@ -175,16 +178,159 @@ add_task(async function test_content_cha
   let url = `${gServerUrl}/dummy?r=${Math.random()}`;
   onModifyListener(url, redirectUrl);
   let contentPage = await ExtensionTestUtils.loadContentPage(url, {redirectUrl});
   equal(contentPage.browser.documentURI.spec, redirectUrl, `expected redirect`);
   await contentPage.close();
   await extension.unload();
 });
 
+// This test makes a request against a server and tests redirect to another server page.
+add_task(async function test_extension_302_redirect_web() {
+  function background(serverUrl) {
+    let expectedUrls = ["/redirect", "/dummy"];
+    let expected = ["onBeforeRequest", "onHeadersReceived", "onBeforeRedirect",
+                    "onBeforeRequest", "onHeadersReceived", "onResponseStarted", "onCompleted"];
+    browser.webRequest.onBeforeRequest.addListener(details => {
+      browser.test.assertTrue(details.url.includes(expectedUrls.shift()), "onBeforeRequest url matches");
+      browser.test.assertEq(expected.shift(), "onBeforeRequest", "onBeforeRequest matches");
+    }, {urls: [serverUrl]});
+    browser.webRequest.onHeadersReceived.addListener(details => {
+      browser.test.assertEq(expected.shift(), "onHeadersReceived", "onHeadersReceived matches");
+    }, {urls: [serverUrl]});
+    browser.webRequest.onResponseStarted.addListener(details => {
+      browser.test.assertEq(expected.shift(), "onResponseStarted", "onResponseStarted matches");
+    }, {urls: [serverUrl]});
+    browser.webRequest.onBeforeRedirect.addListener(details => {
+      browser.test.assertTrue(details.redirectUrl.includes("/dummy"), "onBeforeRedirect matches redirectUrl");
+      browser.test.assertEq(expected.shift(), "onBeforeRedirect", "onBeforeRedirect matches");
+    }, {urls: [serverUrl]});
+    browser.webRequest.onCompleted.addListener(details => {
+      browser.test.assertTrue(details.url.includes("/dummy"), "onCompleted expected url received");
+      browser.test.assertEq(expected.shift(), "onCompleted", "onCompleted matches");
+      browser.test.notifyPass("requestCompleted");
+    }, {urls: [serverUrl]});
+    browser.webRequest.onErrorOccurred.addListener(details => {
+      browser.test.log(`onErrorOccurred ${JSON.stringify(details)}`);
+      browser.test.notifyFail("requestCompleted");
+    }, {urls: [serverUrl]});
+  }
+  let extension = getExtension(false, `(${background})("*://${server.identity.primaryHost}/*")`, false);
+  await extension.startup();
+  let redirectUrl = `${gServerUrl}/dummy`;
+  let completed = extension.awaitFinish("requestCompleted");
+  let url = `${gServerUrl}/redirect?r=${Math.random()}&redirect_uri=${redirectUrl}`;
+  let contentPage = await ExtensionTestUtils.loadContentPage(url, {redirectUrl});
+  equal(contentPage.browser.documentURI.spec, redirectUrl, `expected content redirect`);
+  await completed;
+  await contentPage.close();
+  await extension.unload();
+});
+
+// This test makes a request against a server and tests redirect to another server page, without
+// onBeforeRedirect.  Bug 1448599
+add_task(async function test_extension_302_redirect_opening() {
+  let redirectUrl = `${gServerUrl}/dummy`;
+  let expectData = [
+    {
+      event: "onBeforeRequest",
+      url: `${gServerUrl}/redirect`,
+    },
+    {
+      event: "onBeforeRequest",
+      url: redirectUrl,
+    },
+  ];
+  function background(serverUrl, expected) {
+    browser.webRequest.onBeforeRequest.addListener(details => {
+      let expect = expected.shift();
+      browser.test.assertEq(expect.event, "onBeforeRequest", "onBeforeRequest event matches");
+      browser.test.assertTrue(details.url.startsWith(expect.url), "onBeforeRequest url matches");
+      if (expected.length === 0) {
+        browser.test.notifyPass("requestCompleted");
+      }
+    }, {urls: [serverUrl]});
+  }
+  let extension = getExtension(false, `(${background})("*://${server.identity.primaryHost}/*", ${JSON.stringify(expectData)})`, false);
+  await extension.startup();
+  let completed = extension.awaitFinish("requestCompleted");
+  let url = `${gServerUrl}/redirect?r=${Math.random()}&redirect_uri=${redirectUrl}`;
+  let contentPage = await ExtensionTestUtils.loadContentPage(url, {redirectUrl});
+  equal(contentPage.browser.documentURI.spec, redirectUrl, `expected content redirect`);
+  await completed;
+  await contentPage.close();
+  await extension.unload();
+});
+
+// This test makes a request against a server and tests redirect to another server page, without
+// onBeforeRedirect.  Bug 1448599
+add_task(async function test_extension_302_redirect_modify() {
+  let redirectUrl = `${gServerUrl}/dummy`;
+  let expectData = [
+    {
+      event: "onHeadersReceived",
+      url: `${gServerUrl}/redirect`,
+    },
+    {
+      event: "onHeadersReceived",
+      url: redirectUrl,
+    },
+  ];
+  function background(serverUrl, expected) {
+    browser.webRequest.onHeadersReceived.addListener(details => {
+      let expect = expected.shift();
+      browser.test.assertEq(expect.event, "onHeadersReceived", "onHeadersReceived event matches");
+      browser.test.assertTrue(details.url.startsWith(expect.url), "onHeadersReceived url matches");
+      if (expected.length === 0) {
+        browser.test.notifyPass("requestCompleted");
+      }
+    }, {urls: ["<all_urls>"]});
+  }
+  let extension = getExtension(false, `(${background})("*://${server.identity.primaryHost}/*", ${JSON.stringify(expectData)})`, false);
+  await extension.startup();
+  let completed = extension.awaitFinish("requestCompleted");
+  let url = `${gServerUrl}/redirect?r=${Math.random()}&redirect_uri=${redirectUrl}`;
+  let contentPage = await ExtensionTestUtils.loadContentPage(url, {redirectUrl});
+  equal(contentPage.browser.documentURI.spec, redirectUrl, `expected content redirect`);
+  await completed;
+  await contentPage.close();
+  await extension.unload();
+});
+
+// This test makes a request against a server and tests redirect to another server page, without
+// onBeforeRedirect.  Bug 1448599
+add_task(async function test_extension_302_redirect_tracing() {
+  let redirectUrl = `${gServerUrl}/dummy`;
+  let expectData = [
+    {
+      event: "onCompleted",
+      url: redirectUrl,
+    },
+  ];
+  function background(serverUrl, expected) {
+    browser.webRequest.onCompleted.addListener(details => {
+      let expect = expected.shift();
+      browser.test.assertEq(expect.event, "onCompleted", "onCompleted event matches");
+      browser.test.assertTrue(details.url.startsWith(expect.url), "onCompleted url matches");
+      if (expected.length === 0) {
+        browser.test.notifyPass("requestCompleted");
+      }
+    }, {urls: [serverUrl]});
+  }
+  let extension = getExtension(false, `(${background})("*://${server.identity.primaryHost}/*", ${JSON.stringify(expectData)})`, false);
+  await extension.startup();
+  let completed = extension.awaitFinish("requestCompleted");
+  let url = `${gServerUrl}/redirect?r=${Math.random()}&redirect_uri=${redirectUrl}`;
+  let contentPage = await ExtensionTestUtils.loadContentPage(url, {redirectUrl});
+  equal(contentPage.browser.documentURI.spec, redirectUrl, `expected content redirect`);
+  await completed;
+  await contentPage.close();
+  await extension.unload();
+});
+
 // This test makes a request against a server and tests webrequest.  Currently
 // disabled due to NS_BINDING_ABORTED happening.
 add_task(async function test_extension_302_redirect() {
   let extension = getExtension(true, () => {
     let myuri = browser.extension.getURL("*");
     let exturi = browser.extension.getURL("finished.html");
     browser.webRequest.onBeforeRedirect.addListener(details => {
       browser.test.assertEq(details.redirectUrl, exturi, "redirect matches");
--- a/toolkit/modules/addons/WebRequest.jsm
+++ b/toolkit/modules/addons/WebRequest.jsm
@@ -501,16 +501,17 @@ class AuthRequestor {
 
 HttpObserverManager = {
   openingInitialized: false,
   modifyInitialized: false,
   examineInitialized: false,
   redirectInitialized: false,
   activityInitialized: false,
   needTracing: false,
+  hasRedirects: false,
 
   listeners: {
     opening: new Map(),
     modify: new Map(),
     afterModify: new Map(),
     headersReceived: new Map(),
     authRequired: new Map(),
     onRedirect: new Map(),
@@ -581,17 +582,21 @@ HttpObserverManager = {
       Services.obs.addObserver(this, "http-on-examine-merged-response");
     } else if (!needExamine && this.examineInitialized) {
       this.examineInitialized = false;
       Services.obs.removeObserver(this, "http-on-examine-response");
       Services.obs.removeObserver(this, "http-on-examine-cached-response");
       Services.obs.removeObserver(this, "http-on-examine-merged-response");
     }
 
-    let needRedirect = this.listeners.onRedirect.size || haveBlocking;
+    // If we have any listeners, we need the channelsink so the channelwrapper is
+    // updated properly. Otherwise events for channels that are redirected will not
+    // happen correctly.  If we have no listeners, shut it down.
+    this.hasRedirects = this.listeners.onRedirect.size > 0;
+    let needRedirect = this.hasRedirects || needExamine || needOpening || needModify;
     if (needRedirect && !this.redirectInitialized) {
       this.redirectInitialized = true;
       ChannelEventSink.register();
     } else if (!needRedirect && this.redirectInitialized) {
       this.redirectInitialized = false;
       ChannelEventSink.unregister();
     }
 
@@ -912,17 +917,19 @@ HttpObserverManager = {
     }
   },
 
   onChannelReplaced(oldChannel, newChannel) {
     let channel = this.getWrapper(oldChannel);
 
     // We want originalURI, this will provide a moz-ext rather than jar or file
     // uri on redirects.
-    this.runChannelListener(channel, "onRedirect", {redirectUrl: newChannel.originalURI.spec});
+    if (this.hasRedirects) {
+      this.runChannelListener(channel, "onRedirect", {redirectUrl: newChannel.originalURI.spec});
+    }
     channel.channel = newChannel;
   },
 };
 
 var onBeforeRequest = {
   allowedOptions: ["blocking", "requestBody"],
 
   addListener(callback, filter = null, options = null, optionsObject = null) {