Merge pull request #1120 from stoically/cancel-redirects-tabid-url

Cancel redirects for the same requestIds and urls if originating from the same tabId

package.json

@@ -20,7 +20,7 @@
     "json": "^9.0.6",
     "mocha": "^5.0.0",
     "npm-run-all": "^4.0.0",
-    "sinon": "^4.2.2",
+    "sinon": "^4.4.0",
     "sinon-chai": "^2.14.0",
     "stylelint": "^7.9.0",
     "stylelint-config-standard": "^16.0.0",

src/js/background/assignManager.js

@@ -148,19 +148,41 @@ const assignManager = {
         && messageHandler.lastCreatedTab.id === tab.id);
     const openTabId = removeTab ? tab.openerTabId : tab.id;
 
-    // we decided to cancel the request at this point, register it as canceled request as early as possible
+    if (!this.canceledRequests[tab.id]) {
-    if (!this.canceledRequests[options.requestId]) {
+      // we decided to cancel the request at this point, register canceled request
-      this.canceledRequests[options.requestId] = true;
+      this.canceledRequests[tab.id] = {
-      // register a cleanup for handled requestIds
+        requestIds: {
-      // all relevant requests that come in that timeframe with the same requestId will be canceled
+          [options.requestId]: true
+        },
+        urls: {
+          [options.url]: true
+        }
+      };
+
+      // since webRequest onCompleted and onErrorOccurred are not 100% reliable (see #1120)
+      // we register a timer here to cleanup canceled requests, just to make sure we don't
+      // end up in a situation where certain urls in a tab.id stay canceled
       setTimeout(() => {
-        delete this.canceledRequests[options.requestId];
+        if (this.canceledRequests[tab.id]) {
+          delete this.canceledRequests[tab.id];
+        }
       }, 2000);
     } else {
-      // if we see a request for the same requestId at this point then this is a redirect that we have to cancel to prevent opening two tabs
+      let cancelEarly = false;
-      return {
+      if (this.canceledRequests[tab.id].requestIds[options.requestId] ||
-        cancel: true
+          this.canceledRequests[tab.id].urls[options.url]) {
-      };
+        // same requestId or url from the same tab
+        // this is a redirect that we have to cancel early to prevent opening two tabs
+        cancelEarly = true;
+      }
+      // we decided to cancel the request at this point, register canceled request
+      this.canceledRequests[tab.id].requestIds[options.requestId] = true;
+      this.canceledRequests[tab.id].urls[options.url] = true;
+      if (cancelEarly) {
+        return {
+          cancel: true
+        };
+      }
     }
 
     this.reloadPageInContainer(
@@ -203,6 +225,19 @@ const assignManager = {
     browser.webRequest.onBeforeRequest.addListener((options) => {
       return this.onBeforeRequest(options);
     },{urls: ["<all_urls>"], types: ["main_frame"]}, ["blocking"]);
+
+    // Clean up canceled requests
+    browser.webRequest.onCompleted.addListener((options) => {
+      if (this.canceledRequests[options.tabId]) {
+        delete this.canceledRequests[options.tabId];
+      }
+    },{urls: ["<all_urls>"], types: ["main_frame"]});
+    browser.webRequest.onErrorOccurred.addListener((options) => {
+      if (this.canceledRequests[options.tabId]) {
+        delete this.canceledRequests[options.tabId];
+      }
+    },{urls: ["<all_urls>"], types: ["main_frame"]});
+
   },
 
   async _onClickedHandler(info, tab) {

test/browser.mock.js

@@ -19,6 +19,9 @@ module.exports = () => {
       },
       onCompleted: {
         addListener: sinon.stub()
+      },
+      onErrorOccurred: {
+        addListener: sinon.stub()
       }
     },
     windows: {

test/helper.js

@@ -18,19 +18,21 @@ module.exports = {
       });
     },
 
-    async openNewTab(tab, options = {isAsync: true}) {
+    async openNewTab(tab, options = {}) {
+      if (options.resetHistory) {
+        background.browser.tabs.create.resetHistory();
+        background.browser.tabs.remove.resetHistory();
+      }
       background.browser.tabs.get.resolves(tab);
-      background.browser.webRequest.onBeforeRequest.addListener.yield({
+      background.browser.tabs.onCreated.addListener.yield(tab);
+      const [promise] = background.browser.webRequest.onBeforeRequest.addListener.yield({
         frameId: 0,
         tabId: tab.id,
         url: tab.url,
         requestId: options.requestId
       });
-      background.browser.tabs.onCreated.addListener.yield(tab);
+
-      if (!options.isAsync) {
+      return promise;
-        return;
-      }
-      await nextTick();
     }
   },
 

test/issues/940.test.js

@@ -22,8 +22,7 @@ describe("#940", () => {
         active: true
       };
       helper.browser.openNewTab(newTab, {
-        requestId: 1,
+        requestId: 1
-        isAsync: false
       });
 
       // other addon sees the same request
@@ -40,4 +39,142 @@ describe("#940", () => {
       background.browser.tabs.create.should.have.been.calledOnce;
     });
   });
+
+  describe("when redirects change requestId midflight", () => {
+    let promiseResults;
+    beforeEach(async () => {
+      // init
+      const activeTab = {
+        id: 1,
+        cookieStoreId: "firefox-container-1",
+        url: "https://www.youtube.com",
+        index: 0
+      };
+      await helper.browser.initializeWithTab(activeTab);
+      // assign the activeTab.url
+      await helper.popup.clickElementById("container-page-assigned");
+
+      // http://youtube.com
+      const newTab = {
+        id: 2,
+        cookieStoreId: "firefox-default",
+        url: "http://youtube.com",
+        index: 1,
+        active: true
+      };
+      const promise1 = helper.browser.openNewTab(newTab, {
+        requestId: 1
+      });
+
+      // https://youtube.com
+      const [promise2] = background.browser.webRequest.onBeforeRequest.addListener.yield({
+        frameId: 0,
+        tabId: newTab.id,
+        url: "https://youtube.com",
+        requestId: 1
+      });
+
+      // https://www.youtube.com
+      const [promise3] = background.browser.webRequest.onBeforeRequest.addListener.yield({
+        frameId: 0,
+        tabId: newTab.id,
+        url: "https://www.youtube.com",
+        requestId: 1
+      });
+
+      // https://www.youtube.com
+      const [promise4] = background.browser.webRequest.onBeforeRequest.addListener.yield({
+        frameId: 0,
+        tabId: newTab.id,
+        url: "https://www.youtube.com",
+        requestId: 2
+      });
+
+      promiseResults = await Promise.all([promise1, promise2, promise3, promise4]);
+    });
+
+    it("should not open two confirm pages", async () => {
+      // http://youtube.com is not assigned, no cancel, no reopening
+      expect(promiseResults[0]).to.deep.equal({});
+
+      // https://youtube.com is not assigned, no cancel, no reopening
+      expect(promiseResults[1]).to.deep.equal({});
+
+      // https://www.youtube.com is assigned, this triggers reopening, cancel
+      expect(promiseResults[2]).to.deep.equal({
+        cancel: true
+      });
+
+      // https://www.youtube.com is assigned, this was a redirect, cancel early, no reopening
+      expect(promiseResults[3]).to.deep.equal({
+        cancel: true
+      });
+
+      background.browser.tabs.create.should.have.been.calledOnce;
+    });
+
+    it("should uncancel after webRequest.onCompleted", async () => {
+      const [promise1] = background.browser.webRequest.onCompleted.addListener.yield({
+        tabId: 2
+      });
+      await promise1;
+
+      const [promise2] = background.browser.webRequest.onBeforeRequest.addListener.yield({
+        frameId: 0,
+        tabId: 2,
+        url: "https://www.youtube.com",
+        requestId: 123
+      });
+      await promise2;
+
+      background.browser.tabs.create.should.have.been.calledTwice;
+    });
+
+    it("should uncancel after webRequest.onErrorOccurred", async () => {
+      const [promise1] = background.browser.webRequest.onErrorOccurred.addListener.yield({
+        tabId: 2
+      });
+      await promise1;
+
+      // request to assigned url in same tab
+      const [promise2] = background.browser.webRequest.onBeforeRequest.addListener.yield({
+        frameId: 0,
+        tabId: 2,
+        url: "https://www.youtube.com",
+        requestId: 123
+      });
+      await promise2;
+
+      background.browser.tabs.create.should.have.been.calledTwice;
+    });
+
+    it("should uncancel after 2 seconds", async () => {
+      await new Promise(resolve => setTimeout(resolve, 2000));
+      // request to assigned url in same tab
+      const [promise2] = background.browser.webRequest.onBeforeRequest.addListener.yield({
+        frameId: 0,
+        tabId: 2,
+        url: "https://www.youtube.com",
+        requestId: 123
+      });
+      await promise2;
+
+      background.browser.tabs.create.should.have.been.calledTwice;
+    }).timeout(2002);
+
+    it("should not influence the canceled url in other tabs", async () => {
+      const newTab = {
+        id: 123,
+        cookieStoreId: "firefox-default",
+        url: "https://www.youtube.com",
+        index: 10,
+        active: true
+      };
+      await helper.browser.openNewTab(newTab, {
+        requestId: 321
+      });
+
+      background.browser.tabs.create.should.have.been.calledTwice;
+    });
+  });
 });

test/setup.js

@@ -91,11 +91,11 @@ global.buildPopupDom = async (options = {}) => {
 global.afterEach(() => {
   if (global.background) {
     global.background.dom.window.close();
-    delete global.background.dom;
+    delete global.background;
   }
 
   if (global.popup) {
     global.popup.dom.window.close();
-    delete global.popup.dom;
+    delete global.popup;
   }
 });