From f8e009498131635c94e5e2f050960d40a026674c Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 20 Feb 2019 20:12:16 +0000 Subject: [PATCH] Bug 1528456. Fix browser_domainPolicy.js to not be racy. r=Gijs The old complicated code here was working around a fundamental race in the test. The test was trying to do the following: 1) Open a new tab. This sends an IPC message to do that and to start loading about:blank in the tab. 2) Add a "load" listener. This sends an IPC message to add the listener. 3) Start loading the URL we really want to load. This sends an IPC message to start the load. There are two races here: the message from step 2 can be received before or after the about:blank load is done, and the message from step 3 can be received before or after the about:blank load is done. If step 2 wins the race (we add the listener before the about:blank load is done) but step 3 loses the race (new load starts only after the about:blank load is done), then we would get a load event for the about:blank, not for the url we care about. The test worked around this by having step 2 add a listener for the "DOMDocElementInserted" event, which about:blank did not fire. Then when that happened (indicating that the load from step 3 is now in progress), it added a "load" listener. This fixed the race. Bug 1528146 fixed about:blank to also fire "DOMDocElementInserted", so the test became racy again. Now if the "DOMDocElementInserted" listener got added _after_ the about:blank load had fired that event (which is what usually happened), then the test passed. But if the IPC message from step 2 to add the "DOMDocElementInserted" listener won the race against the about:blank load firing "DOMDocElementInserted", then we added the "load" listener too early and got the "load" event for the about:blank, not the document we cared about. The fix is to just use the (now) existing openNewForegroundTab function which lets us pass in a url, then wait until that URL is loaded, instead of trying to reinvent that wheel. There is an additional ride-along change to make openNewForegroundTab always create a new process in this test, which is the original intent of the test but wasn't necessarily guaranteed by the old code. Differential Revision: https://phabricator.services.mozilla.com/D20544 --HG-- extra : moz-landing-system : lando --- dom/ipc/tests/browser_domainPolicy.js | 45 ++++++--------------------- 1 file changed, 10 insertions(+), 35 deletions(-) diff --git a/dom/ipc/tests/browser_domainPolicy.js b/dom/ipc/tests/browser_domainPolicy.js index 24c927d6b922..8196c7591b29 100644 --- a/dom/ipc/tests/browser_domainPolicy.js +++ b/dom/ipc/tests/browser_domainPolicy.js @@ -27,29 +27,10 @@ async function test_domainPolicy() { // Init test function initProcess() { - tab = BrowserTestUtils.addTab(gBrowser); - gBrowser.selectedTab = tab; - - let initPromise = ContentTask.spawn(tab.linkedBrowser, null, function() { - const {PromiseUtils} = ChromeUtils.import("resource://gre/modules/PromiseUtils.jsm"); - function loadBase() { - let deferred = PromiseUtils.defer(); - let listener = (event) => { - removeEventListener("DOMDocElementInserted", listener, true); - let listener2 = (event2) => { - content.removeEventListener("load", listener2); - deferred.resolve(); - }; - content.addEventListener("load", listener2); - }; - addEventListener("DOMDocElementInserted", listener, true); - return deferred.promise; - } - - return loadBase(); - }); - BrowserTestUtils.loadURI(tab.linkedBrowser, "http://mochi.test:8888/browser/dom/ipc/tests/file_domainPolicy_base.html"); - return initPromise; + return BrowserTestUtils.openNewForegroundTab( + {gBrowser, + opening: "http://mochi.test:8888/browser/dom/ipc/tests/file_domainPolicy_base.html", + forceNewProcess: true}); } // We use ContentTask for the tests, but we also want to pass some data and some helper functions too. @@ -113,8 +94,7 @@ async function test_domainPolicy() { info("Testing simple blocklist policy"); info("Creating child process first, activating domainPolicy after"); - currentTask = initProcess(); - await currentTask; + tab = await initProcess(); activateDomainPolicy(); var bl = policy.blocklist; bl.add(Services.io.newURI("http://example.com")); @@ -125,8 +105,7 @@ async function test_domainPolicy() { activateDomainPolicy(); bl = policy.blocklist; bl.add(Services.io.newURI("http://example.com")); - currentTask = initProcess(); - await currentTask; + tab = await initProcess(); currentTask = runTest(testDomain("http://example.com")); checkAndCleanup(await currentTask); @@ -185,8 +164,7 @@ async function test_domainPolicy() { info("Testing Blocklist-style Domain Policy"); info("Activating domainPolicy first, creating child process after"); activate(true, testPolicy.exceptions, testPolicy.superExceptions); - currentTask = initProcess(); - await currentTask; + tab = await initProcess(); let results = []; currentTask = runTest(testList(true, testPolicy.notExempt)); results = results.concat(await currentTask); @@ -195,8 +173,7 @@ async function test_domainPolicy() { checkAndCleanup(results); info("Creating child process first, activating domainPolicy after"); - currentTask = initProcess(); - await currentTask; + tab = await initProcess(); activate(true, testPolicy.exceptions, testPolicy.superExceptions); results = []; currentTask = runTest(testList(true, testPolicy.notExempt)); @@ -213,8 +190,7 @@ async function test_domainPolicy() { info("Activating domainPolicy first, creating child process after"); activate(false, testPolicy.exceptions, testPolicy.superExceptions); - currentTask = initProcess(); - await currentTask; + tab = await initProcess(); results = []; currentTask = runTest(testList(false, testPolicy.notExempt)); results = results.concat(await currentTask); @@ -223,8 +199,7 @@ async function test_domainPolicy() { checkAndCleanup(results); info("Creating child process first, activating domainPolicy after"); - currentTask = initProcess(); - await currentTask; + tab = await initProcess(); activate(false, testPolicy.exceptions, testPolicy.superExceptions); results = []; currentTask = runTest(testList(false, testPolicy.notExempt));