From dab3005c6f6941797519cacc2adc87c2ee6c0f9a Mon Sep 17 00:00:00 2001 From: stoically Date: Mon, 12 Feb 2018 23:12:32 +0100 Subject: [PATCH] Cancel redirects for the same requestIds and urls if originating from the same tabId Fixes #940 --- package.json | 2 +- src/js/background/assignManager.js | 59 +++++++++--- test/browser.mock.js | 3 + test/helper.js | 16 ++-- test/issues/940.test.js | 141 ++++++++++++++++++++++++++++- test/setup.js | 4 +- 6 files changed, 201 insertions(+), 24 deletions(-) diff --git a/package.json b/package.json index a09f79e..988b8c3 100644 --- a/package.json +++ b/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", diff --git a/src/js/background/assignManager.js b/src/js/background/assignManager.js index bcc8dc2..a03a163 100644 --- a/src/js/background/assignManager.js +++ b/src/js/background/assignManager.js @@ -147,22 +147,44 @@ const assignManager = { || (messageHandler.lastCreatedTab && 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[options.requestId]) { - this.canceledRequests[options.requestId] = true; - // register a cleanup for handled requestIds - // all relevant requests that come in that timeframe with the same requestId will be canceled + + if (!this.canceledRequests[tab.id]) { + // we decided to cancel the request at this point, register canceled request + this.canceledRequests[tab.id] = { + requestIds: { + [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 - return { - cancel: true - }; + let cancelEarly = false; + if (this.canceledRequests[tab.id].requestIds[options.requestId] || + 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( options.url, userContextId, @@ -203,6 +225,19 @@ const assignManager = { browser.webRequest.onBeforeRequest.addListener((options) => { return this.onBeforeRequest(options); },{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: [""], types: ["main_frame"]}); + browser.webRequest.onErrorOccurred.addListener((options) => { + if (this.canceledRequests[options.tabId]) { + delete this.canceledRequests[options.tabId]; + } + },{urls: [""], types: ["main_frame"]}); + }, async _onClickedHandler(info, tab) { diff --git a/test/browser.mock.js b/test/browser.mock.js index 5ae3de5..d6d38dd 100644 --- a/test/browser.mock.js +++ b/test/browser.mock.js @@ -19,6 +19,9 @@ module.exports = () => { }, onCompleted: { addListener: sinon.stub() + }, + onErrorOccurred: { + addListener: sinon.stub() } }, windows: { diff --git a/test/helper.js b/test/helper.js index 1ce13b5..555ee7a 100644 --- a/test/helper.js +++ b/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; - } - await nextTick(); + + return promise; } }, diff --git a/test/issues/940.test.js b/test/issues/940.test.js index cf9a1ab..b889035 100644 --- a/test/issues/940.test.js +++ b/test/issues/940.test.js @@ -22,8 +22,7 @@ describe("#940", () => { active: true }; helper.browser.openNewTab(newTab, { - requestId: 1, - isAsync: false + requestId: 1 }); // 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; + }); + }); }); diff --git a/test/setup.js b/test/setup.js index 9672bfc..b6d563d 100644 --- a/test/setup.js +++ b/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; } });