Bug 1405286: Part 4 - Don't overwrite existing state with finishedtransferringdata. r?mixedpuppy draft
authorKris Maglione <maglione.k@gmail.com>
Sat, 14 Oct 2017 20:01:18 -0700
changeset 680560 e3811aae607d9e7a6c6183738c352424f38218bd
parent 680559 56c80b02deae61670c0e9b6a1b7ea266cf0dbe26
child 735882 6972b0c4c3682b854fe0b20f95704b5a1be28a02
push id84532
push usermaglione.k@gmail.com
push dateSun, 15 Oct 2017 03:07:18 +0000
reviewersmixedpuppy
bugs1405286
milestone58.0a1
Bug 1405286: Part 4 - Don't overwrite existing state with finishedtransferringdata. r?mixedpuppy In cases where data transfer finishes immediately after we close a request, we can sometimes wind up overwriting that state information with "finishedtransferringdata", which allows scripted callers to break certain invariants and cause crashes. MozReview-Commit-ID: Do3GttF3M9S
toolkit/components/extensions/test/mochitest/test_ext_webrequest_responseBody.html
toolkit/components/extensions/webrequest/StreamFilterChild.cpp
--- a/toolkit/components/extensions/test/mochitest/test_ext_webrequest_responseBody.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_webrequest_responseBody.html
@@ -443,16 +443,65 @@ add_task(async function test_cachedRespo
 
   await fetch("file_sample.html");
   await fetch("file_sample.html");
   await extension.awaitMessage("from-cache");
 
   await extension.unload();
 });
 
+// Test that finishing transferring data doesn't overwrite an existing closing/closed state.
+add_task(async function test_late_close() {
+  let extension = ExtensionTestUtils.loadExtension({
+    background() {
+      browser.webRequest.onBeforeRequest.addListener(data => {
+        let filter = browser.webRequest.filterResponseData(data.requestId);
+
+        filter.onstop = event => {
+          browser.test.fail("Should not receive onstop after close()");
+          browser.test.assertEq("closed", filter.status,
+                                "Fitler status should still be 'closed'");
+          browser.test.assertThrows(() => { filter.close(); });
+        };
+        filter.ondata = event => {
+          filter.write(event.data);
+          filter.close();
+
+          browser.test.sendMessage(`done-${data.url}`);
+        };
+      }, {
+        urls: ["http://mochi.test/*/file_sample.html?*"],
+      },
+      ["blocking"]);
+    },
+
+    manifest: {
+      permissions: [
+        "webRequest",
+        "webRequestBlocking",
+        "http://mochi.test/",
+      ],
+    },
+  });
+
+  await extension.startup();
+
+  // This issue involves a race, so several requests in parallel to increase
+  // the chances of triggering it.
+  let urls = [];
+  for (let i = 0; i < 32; i++) {
+    urls.push(new URL(`file_sample.html?r=${Math.random()}`, location).href);
+  }
+
+  await Promise.all(urls.map(url => fetch(url)));
+  await Promise.all(urls.map(url => extension.awaitMessage(`done-${url}`)));
+
+  await extension.unload();
+});
+
 add_task(async function test_permissions() {
   let extension = ExtensionTestUtils.loadExtension({
     background() {
       browser.test.assertEq(
           undefined, browser.webRequest.filterResponseData,
           "filterResponseData is undefined without blocking permissions");
     },
 
--- a/toolkit/components/extensions/webrequest/StreamFilterChild.cpp
+++ b/toolkit/components/extensions/webrequest/StreamFilterChild.cpp
@@ -262,16 +262,18 @@ StreamFilterChild::MaybeStopRequest()
 
   switch (mState) {
   case State::Suspending:
   case State::Resuming:
     mNextState = State::FinishedTransferringData;
     return;
 
   case State::Disconnecting:
+  case State::Closing:
+  case State::Closed:
     break;
 
   default:
     mState = State::FinishedTransferringData;
     if (mStreamFilter) {
       mStreamFilter->FireEvent(NS_LITERAL_STRING("stop"));
       // We don't need access to the stream filter after this point, so break our
       // reference cycle, so that it can be collected if we're the last reference.