Bug 1448599 - fix WebRequest events for redirected channels when onBeforeRedirect is not used, r=kmag
authorShane Caraveo <scaraveo@mozilla.com>
Mon, 02 Apr 2018 21:12:07 -0500
changeset 411436 5c9c6f5a4ca35835b9614a8cdb3eefda44cb585d
parent 411435 102872860c7ab710800251aac7814bb5bb1168f9
child 411437 c3b12adc84b141d795fc1c86d9c0754c3b4e4241
push id101651
push useraiakab@mozilla.com
push dateTue, 03 Apr 2018 09:42:02 +0000
treeherdermozilla-inbound@99a953f1823f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag
bugs1448599
milestone61.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 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) {