Bug 1683189 - Release pending StreamFilter requests r=mattwoodrow,mixedpuppy,necko-reviewers,dragana
authorRob Wu <rob@robwu.nl>
Tue, 22 Dec 2020 15:40:15 +0000
changeset 561591 0e66f49f121db45362ee9197ef1d5cf1afa613ce
parent 561590 03a161631507910c3fe1efa0e786b8cc76554042
child 561592 eda8ec4d13cff8b075b2634c9ad343c90b010a7f
push id38052
push usercsabou@mozilla.com
push dateTue, 22 Dec 2020 21:50:26 +0000
treeherdermozilla-central@be30820869d8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmattwoodrow, mixedpuppy, necko-reviewers, dragana
bugs1683189
milestone86.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 1683189 - Release pending StreamFilter requests r=mattwoodrow,mixedpuppy,necko-reviewers,dragana mStreamFilterRequests is current rejected/resolved from nsHttpChannel::CallOnStartRequest. But that point may never be reached for various reasons, including (internal) redirects and cancellations. Ensure that they are cleaned up via nsHttpChannel::ReleaseListeners. The test in test_ext_webRequest_viewsource_StreamFilter.js was already passing before, but they were non-deterministic due to the reliance on garbage collection of the HTTP channel. Now it completes soon. The new test file, test_ext_webRequest_redirect_StreamFilter.js is a regression test that actually confirms that the filter is closed as soon as reasonably possible. Differential Revision: https://phabricator.services.mozilla.com/D100189
netwerk/protocol/http/nsHttpChannel.cpp
toolkit/components/extensions/test/xpcshell/test_ext_webRequest_redirect_StreamFilter.js
toolkit/components/extensions/test/xpcshell/test_ext_webRequest_viewsource_StreamFilter.js
toolkit/components/extensions/test/xpcshell/xpcshell-common-e10s.ini
--- a/netwerk/protocol/http/nsHttpChannel.cpp
+++ b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -1014,16 +1014,21 @@ void nsHttpChannel::DoNotifyListenerClea
   // We don't need this info anymore
   CleanRedirectCacheChainIfNecessary();
 }
 
 void nsHttpChannel::ReleaseListeners() {
   HttpBaseChannel::ReleaseListeners();
   mChannelClassifier = nullptr;
   mWarningReporter = nullptr;
+
+  for (StreamFilterRequest& request : mStreamFilterRequests) {
+    request.mPromise->Reject(false, __func__);
+  }
+  mStreamFilterRequests.Clear();
 }
 
 void nsHttpChannel::DoAsyncAbort(nsresult aStatus) {
   Unused << AsyncAbort(aStatus);
 }
 
 void nsHttpChannel::HandleAsyncRedirect() {
   MOZ_ASSERT(!mCallOnResume, "How did that happen?");
new file mode 100644
--- /dev/null
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_webRequest_redirect_StreamFilter.js
@@ -0,0 +1,129 @@
+"use strict";
+
+// StreamFilters should be closed upon a redirect.
+//
+// Some redirects are already tested in other tests:
+// - test_ext_webRequest_filterResponseData.js tests fetch requests.
+// - test_ext_webRequest_viewsource_StreamFilter.js tests view-source documents.
+//
+// Usually, redirects are caught in StreamFilterParent::OnStartRequest, but due
+// to the fact that AttachStreamFilter is deferred for document requests, OSR is
+// not called and the cleanup is triggered from nsHttpChannel::ReleaseListeners.
+
+const server = createHttpServer({ hosts: ["example.com", "example.org"] });
+
+server.registerPathHandler("/redir", (request, response) => {
+  response.setStatusLine(request.httpVersion, 302, "Found");
+  response.setHeader("Location", "/target");
+});
+server.registerPathHandler("/target", (request, response) => {
+  response.setStatusLine(request.httpVersion, 200, "OK");
+  response.write("ok");
+});
+server.registerPathHandler("/RedirectToRedir.html", (request, response) => {
+  response.setStatusLine(request.httpVersion, 200, "OK");
+  response.setHeader("Content-Type", "text/html; charset=utf-8");
+  response.write("<script>location.href='http://example.com/redir';</script>");
+});
+server.registerPathHandler("/iframeWithRedir.html", (request, response) => {
+  response.setStatusLine(request.httpVersion, 200, "OK");
+  response.setHeader("Content-Type", "text/html; charset=utf-8");
+  response.write("<iframe src='http://example.com/redir'></iframe>");
+});
+
+function loadRedirectCatcherExtension() {
+  return ExtensionTestUtils.loadExtension({
+    manifest: {
+      permissions: ["webRequest", "webRequestBlocking", "*://*/*"],
+    },
+    background() {
+      const closeCounts = {};
+      browser.webRequest.onBeforeRequest.addListener(
+        details => {
+          let expectedError = "Channel redirected";
+          if (details.type === "main_frame" || details.type === "sub_frame") {
+            // Message differs for the reason stated at the top of this file.
+            // TODO bug 1683862: Make error message more accurate.
+            expectedError = "Invalid request ID";
+          }
+
+          closeCounts[details.requestId] = 0;
+
+          let filter = browser.webRequest.filterResponseData(details.requestId);
+          filter.onstart = () => {
+            filter.disconnect();
+            browser.test.fail("Unexpected filter.onstart");
+          };
+          filter.onerror = function() {
+            closeCounts[details.requestId]++;
+            browser.test.assertEq(expectedError, filter.error, "filter.error");
+          };
+        },
+        { urls: ["*://*/redir"] },
+        ["blocking"]
+      );
+      browser.webRequest.onCompleted.addListener(
+        details => {
+          // filter.onerror from the redirect request should be called before
+          // webRequest.onCompleted of the redirection target. Regression test
+          // for bug 1683189.
+          browser.test.assertEq(
+            1,
+            closeCounts[details.requestId],
+            "filter from initial, redirected request should have been closed"
+          );
+          browser.test.log("Intentionally canceling view-source request");
+          browser.test.sendMessage("req_end", details.type);
+        },
+        { urls: ["*://*/target"] }
+      );
+    },
+  });
+}
+
+add_task(async function redirect_document() {
+  let extension = loadRedirectCatcherExtension();
+  await extension.startup();
+
+  {
+    let contentPage = await ExtensionTestUtils.loadContentPage(
+      "http://example.com/redir"
+    );
+    equal(await extension.awaitMessage("req_end"), "main_frame", "is top doc");
+    await contentPage.close();
+  }
+
+  {
+    let contentPage = await ExtensionTestUtils.loadContentPage(
+      "http://example.com/iframeWithRedir.html"
+    );
+    equal(await extension.awaitMessage("req_end"), "sub_frame", "is sub doc");
+    await contentPage.close();
+  }
+
+  await extension.unload();
+});
+
+// Cross-origin redirect = process switch.
+add_task(async function redirect_document_cross_origin() {
+  let extension = loadRedirectCatcherExtension();
+  await extension.startup();
+
+  {
+    let contentPage = await ExtensionTestUtils.loadContentPage(
+      "http://example.org/RedirectToRedir.html"
+    );
+    equal(await extension.awaitMessage("req_end"), "main_frame", "is top doc");
+    await contentPage.close();
+  }
+
+  {
+    let contentPage = await ExtensionTestUtils.loadContentPage(
+      "http://example.org/iframeWithRedir.html"
+    );
+    equal(await extension.awaitMessage("req_end"), "sub_frame", "is sub doc");
+    await contentPage.close();
+  }
+
+  await extension.unload();
+});
--- a/toolkit/components/extensions/test/xpcshell/test_ext_webRequest_viewsource_StreamFilter.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_webRequest_viewsource_StreamFilter.js
@@ -25,39 +25,43 @@ async function testViewSource(viewSource
           filter.write(new TextEncoder().encode("PREFIX_"));
         };
         filter.ondata = event => {
           filter.write(event.data);
         };
         filter.onstop = () => {
           filter.write(new TextEncoder().encode("_SUFFIX"));
           filter.disconnect();
-          browser.test.sendMessage("filter_end");
+          browser.test.notifyPass("filter_end");
         };
-        filter.onerror = function() {
+        filter.onerror = () => {
           browser.test.fail(`Unexpected error: ${filter.error}`);
-          browser.test.sendMessage("filter_end");
+          browser.test.notifyFail("filter_end");
         };
       },
       { urls: ["*://*/dummy"] },
       ["blocking"]
     );
     browser.webRequest.onBeforeRequest.addListener(
       details => {
         browser.test.assertEq(`${BASE_URL}/redir`, details.url, "Got redirect");
 
         let filter = browser.webRequest.filterResponseData(details.requestId);
         filter.onstop = () => {
           filter.disconnect();
           browser.test.fail("Unexpected onstop for redirect");
           browser.test.sendMessage("redirect_done");
         };
-        filter.onerror = function() {
+        filter.onerror = () => {
           browser.test.assertEq(
-            "Channel redirected",
+            // TODO bug 1683862: must be "Channel redirected", but it is not
+            // because document requests are handled differently compared to
+            // other requests, see the comment at the top of
+            // test_ext_webRequest_redirect_StreamFilter.js.
+            "Invalid request ID",
             filter.error,
             "Expected error in filter.onerror"
           );
           browser.test.sendMessage("redirect_done");
         };
       },
       { urls: ["*://*/redir"] },
       ["blocking"]
@@ -71,26 +75,70 @@ async function testViewSource(viewSource
   });
   await extension.startup();
   let contentPage = await ExtensionTestUtils.loadContentPage(viewSourceUrl);
   if (viewSourceUrl.includes("/redir")) {
     info("Awaiting observed completion of redirection request");
     await extension.awaitMessage("redirect_done");
   }
   info("Awaiting completion of StreamFilter on request");
-  await extension.awaitMessage("filter_end");
+  await extension.awaitFinish("filter_end");
   let contentText = await contentPage.spawn(null, () => {
     return this.content.document.body.textContent;
   });
   equal(contentText, "PREFIX_ok_SUFFIX", "view-source response body");
   await contentPage.close();
   await extension.unload();
 }
 
 add_task(async function test_StreamFilter_viewsource() {
   await testViewSource(`view-source:${BASE_URL}/dummy`);
 });
 
-// Skipped because filter.error is incorrect and test non-deterministic, see
-// https://bugzilla.mozilla.org/show_bug.cgi?id=1683189
-// add_task(async function test_StreamFilter_viewsource_redirect_target() {
-//   await testViewSource(`view-source:${BASE_URL}/redir`);
-// });
+add_task(async function test_StreamFilter_viewsource_redirect_target() {
+  await testViewSource(`view-source:${BASE_URL}/redir`);
+});
+
+// Sanity check: nothing bad happens if the underlying response is aborted.
+add_task(async function test_StreamFilter_viewsource_cancel() {
+  let extension = ExtensionTestUtils.loadExtension({
+    manifest: {
+      permissions: ["webRequest", "webRequestBlocking", "*://*/*"],
+    },
+    background() {
+      browser.webRequest.onBeforeRequest.addListener(
+        details => {
+          let filter = browser.webRequest.filterResponseData(details.requestId);
+          filter.onstart = () => {
+            filter.disconnect();
+            browser.test.fail("Unexpected filter.onstart");
+            browser.test.notifyFail("filter_end");
+          };
+          filter.onerror = () => {
+            browser.test.assertEq("Invalid request ID", filter.error, "Error?");
+            browser.test.notifyPass("filter_end");
+          };
+        },
+        { urls: ["*://*/dummy"] },
+        ["blocking"]
+      );
+      browser.webRequest.onHeadersReceived.addListener(
+        () => {
+          browser.test.log("Intentionally canceling view-source request");
+          return { cancel: true };
+        },
+        { urls: ["*://*/dummy"] },
+        ["blocking"]
+      );
+    },
+  });
+  await extension.startup();
+  let contentPage = await ExtensionTestUtils.loadContentPage(
+    `${BASE_URL}/dummy`
+  );
+  await extension.awaitFinish("filter_end");
+  let contentText = await contentPage.spawn(null, () => {
+    return this.content.document.body.textContent;
+  });
+  equal(contentText, "", "view-source request should have been canceled");
+  await contentPage.close();
+  await extension.unload();
+});
--- a/toolkit/components/extensions/test/xpcshell/xpcshell-common-e10s.ini
+++ b/toolkit/components/extensions/test/xpcshell/xpcshell-common-e10s.ini
@@ -1,11 +1,12 @@
 # Similar to xpcshell-common.ini, except tests here only run
 # when e10s is enabled (with or without out-of-process extensions).
 
 [test_ext_webRequest_filterResponseData.js]
 # tsan failure is for test_filter_301 timing out, bug 1674773
 skip-if = tsan || os == "android" && debug
+[test_ext_webRequest_redirect_StreamFilter.js]
 [test_ext_webRequest_responseBody.js]
 skip-if = os == "android" && debug
 [test_ext_webRequest_startup_StreamFilter.js]
 skip-if = os == "android" && debug
 [test_ext_webRequest_viewsource_StreamFilter.js]