Bug 1379447 - fix intermittent failure in browser_alltabslistener caused by tab hitting about:blank when waiting for page stops, r=dao
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Wed, 19 Jul 2017 23:17:01 +0100
changeset 419806 a9de37a148b77e55d4222f07e7c869ef1beee672
parent 419805 35b75b53f9d362a04950c3914cca4d344e7c4e07
child 419807 dff1bf7903c2d8e97f4f5fb8aec7524309ce55ca
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdao
bugs1379447
milestone56.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 1379447 - fix intermittent failure in browser_alltabslistener caused by tab hitting about:blank when waiting for page stops, r=dao MozReview-Commit-ID: Sn4uj4jMDc
browser/base/content/test/general/browser_alltabslistener.js
browser/base/content/test/general/browser_bug477014.js
browser/base/content/test/general/browser_e10s_switchbrowser.js
browser/base/content/test/general/browser_restore_isAppTab.js
browser/base/content/test/general/head.js
browser/base/content/test/urlbar/browser_urlbar_stop_pending.js
testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
--- a/browser/base/content/test/general/browser_alltabslistener.js
+++ b/browser/base/content/test/general/browser_alltabslistener.js
@@ -91,18 +91,18 @@ function test() {
   gForegroundTab = BrowserTestUtils.addTab(gBrowser);
   gBackgroundBrowser = gBrowser.getBrowserForTab(gBackgroundTab);
   gForegroundBrowser = gBrowser.getBrowserForTab(gForegroundTab);
   gBrowser.selectedTab = gForegroundTab;
 
   // We must wait until a page has completed loading before
   // starting tests or we get notifications from that
   let promises = [
-    waitForDocLoadComplete(gBackgroundBrowser),
-    waitForDocLoadComplete(gForegroundBrowser)
+    BrowserTestUtils.browserStopped(gBackgroundBrowser, kBasePage),
+    BrowserTestUtils.browserStopped(gForegroundBrowser, kBasePage)
   ];
   gBackgroundBrowser.loadURI(kBasePage);
   gForegroundBrowser.loadURI(kBasePage);
   Promise.all(promises).then(startTest1);
 }
 
 function runTest(browser, url, next) {
   gFrontNotificationsPos = 0;
--- a/browser/base/content/test/general/browser_bug477014.js
+++ b/browser/base/content/test/general/browser_bug477014.js
@@ -3,17 +3,17 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 // That's a gecko!
 const iconURLSpec = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAYAAAAf8/9hAAAABGdBTUEAAK/INwWK6QAAABl0RVh0U29mdHdhcmUAQWRvYmUgSW1hZ2VSZWFkeXHJZTwAAAHWSURBVHjaYvz//z8DJQAggJiQOe/fv2fv7Oz8rays/N+VkfG/iYnJfyD/1+rVq7ffu3dPFpsBAAHEAHIBCJ85c8bN2Nj4vwsDw/8zQLwKiO8CcRoQu0DxqlWrdsHUwzBAAIGJmTNnPgYa9j8UqhFElwPxf2MIDeIrKSn9FwSJoRkAEEAM0DD4DzMAyPi/G+QKY4hh5WAXGf8PDQ0FGwJ22d27CjADAAIIrLmjo+MXA9R2kAHvGBA2wwx6B8W7od6CeQcggKCmCEL8bgwxYCbUIGTDVkHDBia+CuotgACCueD3TDQN75D4xmAvCoK9ARMHBzAw0AECiBHkAlC0Mdy7x9ABNA3obAZXIAa6iKEcGlMVQHwWyjYuL2d4v2cPg8vZswx7gHyAAAK7AOif7SAbOqCmn4Ha3AHFsIDtgPq/vLz8P4MSkJ2W9h8ggBjevXvHDo4FQUQg/kdypqCg4H8lUIACnQ/SOBMYI8bAsAJFPcj1AAEEjwVQqLpAbXmH5BJjqI0gi9DTAAgDBBCcAVLkgmQ7yKCZxpCQxqUZhAECCJ4XgMl493ug21ZD+aDAXH0WLM4A9MZPXJkJIIAwTAR5pQMalaCABQUULttBGCCAGCnNzgABBgAMJ5THwGvJLAAAAABJRU5ErkJggg==";
 var testPage = "data:text/plain,test bug 477014";
 
 add_task(async function() {
   let tabToDetach = BrowserTestUtils.addTab(gBrowser, testPage);
-  await waitForDocLoadComplete(tabToDetach.linkedBrowser);
+  await BrowserTestUtils.browserStopped(tabToDetach.linkedBrowser);
 
   gBrowser.setIcon(tabToDetach, iconURLSpec,
                    Services.scriptSecurityManager.getSystemPrincipal());
   tabToDetach.setAttribute("busy", "true");
 
   // detach and set the listener on the new window
   let newWindow = gBrowser.replaceTabWithWindow(tabToDetach);
   await promiseWaitForEvent(tabToDetach.linkedBrowser, "SwapDocShells");
--- a/browser/base/content/test/general/browser_e10s_switchbrowser.js
+++ b/browser/base/content/test/general/browser_e10s_switchbrowser.js
@@ -61,30 +61,30 @@ function clear_history() {
 // Waits for a load and updates the known history
 var waitForLoad = async function(uri) {
   info("Loading " + uri);
   // Longwinded but this ensures we don't just shortcut to LoadInNewProcess
   gBrowser.selectedBrowser.webNavigation.loadURI(uri, Ci.nsIWebNavigation.LOAD_FLAGS_NONE,
                                                  null, null, null,
                                                  Services.scriptSecurityManager.getSystemPrincipal());
 
-  await waitForDocLoadComplete();
+  await BrowserTestUtils.browserStopped(gBrowser);
   gExpectedHistory.index++;
   gExpectedHistory.entries.push({
     uri: gBrowser.currentURI.spec,
     title: gBrowser.contentTitle
   });
 };
 
 // Waits for a load and updates the known history
 var waitForLoadWithFlags = async function(uri, flags = Ci.nsIWebNavigation.LOAD_FLAGS_NONE) {
   info("Loading " + uri + " flags = " + flags);
   gBrowser.selectedBrowser.loadURIWithFlags(uri, flags, null, null, null);
 
-  await waitForDocLoadComplete();
+  await BrowserTestUtils.browserStopped(gBrowser);
   if (!(flags & Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_HISTORY)) {
 
     if (flags & Ci.nsIWebNavigation.LOAD_FLAGS_REPLACE_HISTORY) {
       gExpectedHistory.entries.pop();
     } else {
       gExpectedHistory.index++;
     }
 
@@ -93,24 +93,24 @@ var waitForLoadWithFlags = async functio
       title: gBrowser.contentTitle
     });
   }
 };
 
 var back = async function() {
   info("Going back");
   gBrowser.goBack();
-  await waitForDocLoadComplete();
+  await BrowserTestUtils.browserStopped(gBrowser);
   gExpectedHistory.index--;
 };
 
 var forward = async function() {
   info("Going forward");
   gBrowser.goForward();
-  await waitForDocLoadComplete();
+  await BrowserTestUtils.browserStopped(gBrowser);
   gExpectedHistory.index++;
 };
 
 // Tests that navigating from a page that should be in the remote process and
 // a page that should be in the main process works and retains history
 add_task(async function test_navigation() {
   let expectedRemote = gMultiProcessBrowser;
 
@@ -201,28 +201,28 @@ add_task(async function test_synchronous
 
   info("2");
   // Load another page
   info("Loading about:robots");
   await BrowserTestUtils.loadURI(gBrowser.selectedBrowser, "about:robots");
   is(gBrowser.selectedBrowser.isRemoteBrowser, false, "Remote attribute should be correct");
   is(gBrowser.selectedBrowser.permanentKey, permanentKey, "browser.permanentKey is still the same");
 
-  await waitForDocLoadComplete();
+  await BrowserTestUtils.browserStopped(gBrowser);
   is(gBrowser.selectedBrowser.isRemoteBrowser, false, "Remote attribute should be correct");
   is(gBrowser.selectedBrowser.permanentKey, permanentKey, "browser.permanentKey is still the same");
 
   info("3");
   // Load the remote page again
   info("Loading http://example.org/" + DUMMY_PATH);
   await BrowserTestUtils.loadURI(gBrowser.selectedBrowser, "http://example.org/" + DUMMY_PATH);
   is(gBrowser.selectedBrowser.isRemoteBrowser, expectedRemote, "Remote attribute should be correct");
   is(gBrowser.selectedBrowser.permanentKey, permanentKey, "browser.permanentKey is still the same");
 
-  await waitForDocLoadComplete();
+  await BrowserTestUtils.browserStopped(gBrowser);
   is(gBrowser.selectedBrowser.isRemoteBrowser, expectedRemote, "Remote attribute should be correct");
   is(gBrowser.selectedBrowser.permanentKey, permanentKey, "browser.permanentKey is still the same");
 
   info("4");
   gBrowser.removeCurrentTab();
   clear_history();
 });
 
--- a/browser/base/content/test/general/browser_restore_isAppTab.js
+++ b/browser/base/content/test/general/browser_restore_isAppTab.js
@@ -48,56 +48,56 @@ var restart = async function(browser) {
 
   await promiseTabLoaded(tab);
 };
 
 add_task(async function navigate() {
   let tab = BrowserTestUtils.addTab(gBrowser, "about:robots");
   let browser = tab.linkedBrowser;
   gBrowser.selectedTab = tab;
-  await waitForDocLoadComplete();
+  await BrowserTestUtils.browserStopped(gBrowser);
   loadFrameScript(browser);
   let isAppTab = await isBrowserAppTab(browser);
   ok(!isAppTab, "Docshell shouldn't think it is an app tab");
 
   gBrowser.pinTab(tab);
   isAppTab = await isBrowserAppTab(browser);
   ok(isAppTab, "Docshell should think it is an app tab");
 
   gBrowser.loadURI(DUMMY);
-  await waitForDocLoadComplete();
+  await BrowserTestUtils.browserStopped(gBrowser);
   loadFrameScript(browser);
   isAppTab = await isBrowserAppTab(browser);
   ok(isAppTab, "Docshell should think it is an app tab");
 
   gBrowser.unpinTab(tab);
   isAppTab = await isBrowserAppTab(browser);
   ok(!isAppTab, "Docshell shouldn't think it is an app tab");
 
   gBrowser.pinTab(tab);
   isAppTab = await isBrowserAppTab(browser);
   ok(isAppTab, "Docshell should think it is an app tab");
 
   gBrowser.loadURI("about:robots");
-  await waitForDocLoadComplete();
+  await BrowserTestUtils.browserStopped(gBrowser);
   loadFrameScript(browser);
   isAppTab = await isBrowserAppTab(browser);
   ok(isAppTab, "Docshell should think it is an app tab");
 
   gBrowser.removeCurrentTab();
 });
 
 add_task(async function crash() {
   if (!gMultiProcessBrowser || !("nsICrashReporter" in Ci))
     return;
 
   let tab = BrowserTestUtils.addTab(gBrowser, DUMMY);
   let browser = tab.linkedBrowser;
   gBrowser.selectedTab = tab;
-  await waitForDocLoadComplete();
+  await BrowserTestUtils.browserStopped(gBrowser);
   loadFrameScript(browser);
   let isAppTab = await isBrowserAppTab(browser);
   ok(!isAppTab, "Docshell shouldn't think it is an app tab");
 
   gBrowser.pinTab(tab);
   isAppTab = await isBrowserAppTab(browser);
   ok(isAppTab, "Docshell should think it is an app tab");
 
--- a/browser/base/content/test/general/head.js
+++ b/browser/base/content/test/general/head.js
@@ -354,55 +354,16 @@ function promiseHistoryClearedState(aURI
            "history visit " + uri.spec + " should " + niceStr + " exist");
         callbackDone();
       });
     });
 
   });
 }
 
-/**
- * Waits for the next load to complete in any browser or the given browser.
- * If a <tabbrowser> is given it waits for a load in any of its browsers.
- *
- * @return promise
- */
-function waitForDocLoadComplete(aBrowser = gBrowser) {
-  return new Promise(resolve => {
-    let listener = {
-      onStateChange(webProgress, req, flags, status) {
-        let docStop = Ci.nsIWebProgressListener.STATE_IS_NETWORK |
-                      Ci.nsIWebProgressListener.STATE_STOP;
-        info("Saw state " + flags.toString(16) + " and status " + status.toString(16));
-
-        // When a load needs to be retargetted to a new process it is cancelled
-        // with NS_BINDING_ABORTED so ignore that case
-        if ((flags & docStop) == docStop && status != Cr.NS_BINDING_ABORTED) {
-          aBrowser.removeProgressListener(this);
-          waitForDocLoadComplete.listeners.delete(this);
-
-          let chan = req.QueryInterface(Ci.nsIChannel);
-          info("Browser loaded " + chan.originalURI.spec);
-          resolve();
-        }
-      },
-      QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener,
-                                             Ci.nsISupportsWeakReference])
-    };
-    aBrowser.addProgressListener(listener);
-    waitForDocLoadComplete.listeners.add(listener);
-    info("Waiting for browser load");
-  });
-}
-
-// Keep a set of progress listeners for waitForDocLoadComplete() to make sure
-// they're not GC'ed before we saw the page load.
-waitForDocLoadComplete.listeners = new Set();
-registerCleanupFunction(() => waitForDocLoadComplete.listeners.clear());
-
 var FullZoomHelper = {
 
   selectTabAndWaitForLocationChange: function selectTabAndWaitForLocationChange(tab) {
     if (!tab)
       throw new Error("tab must be given.");
     if (gBrowser.selectedTab == tab)
       return Promise.resolve();
 
--- a/browser/base/content/test/urlbar/browser_urlbar_stop_pending.js
+++ b/browser/base/content/test/urlbar/browser_urlbar_stop_pending.js
@@ -69,17 +69,17 @@ add_task(async function() {
   info("added link");
   let newTabPromise = BrowserTestUtils.waitForEvent(gBrowser.tabContainer, "TabOpen");
   // Middle click the link:
   await BrowserTestUtils.synthesizeMouseAtCenter("#clickme", { button: 1 }, tab.linkedBrowser);
   // get new tab, switch to it
   let newTab = (await newTabPromise).target;
   await BrowserTestUtils.switchTab(gBrowser, newTab);
   is(gURLBar.value, SLOW_HOST, "Should have slow page in URL bar");
-  let browserStoppedPromise = BrowserTestUtils.browserStopped(newTab.linkedBrowser);
+  let browserStoppedPromise = BrowserTestUtils.browserStopped(newTab.linkedBrowser, null, true);
   BrowserStop();
   await browserStoppedPromise;
 
   is(gURLBar.value, SLOW_HOST, "Should still have slow page in URL bar after stop");
   await BrowserTestUtils.removeTab(newTab);
   await BrowserTestUtils.removeTab(tab);
 });
 /**
@@ -116,23 +116,23 @@ add_task(async function() {
   info("added link");
   let newTabPromise = BrowserTestUtils.waitForEvent(gBrowser.tabContainer, "TabOpen");
   // Middle click the link:
   await BrowserTestUtils.synthesizeMouseAtCenter("#clickme", { button: 1 }, tab.linkedBrowser);
   // get new tab, switch to it
   let newTab = (await newTabPromise).target;
   await BrowserTestUtils.switchTab(gBrowser, newTab);
   is(gURLBar.value, SLOW_HOST1, "Should have slow page in URL bar");
-  let browserStoppedPromise = BrowserTestUtils.browserStopped(newTab.linkedBrowser);
+  let browserStoppedPromise = BrowserTestUtils.browserStopped(newTab.linkedBrowser, null, true);
   gURLBar.value = SLOW_HOST2;
   gURLBar.handleCommand();
   await browserStoppedPromise;
 
   is(gURLBar.value, SLOW_HOST2, "Should have second slow page in URL bar");
-  browserStoppedPromise = BrowserTestUtils.browserStopped(newTab.linkedBrowser);
+  browserStoppedPromise = BrowserTestUtils.browserStopped(newTab.linkedBrowser, null, true);
   BrowserStop();
   await browserStoppedPromise;
 
   is(gURLBar.value, SLOW_HOST2, "Should still have second slow page in URL bar after stop");
   await BrowserTestUtils.removeTab(newTab);
   await BrowserTestUtils.removeTab(tab);
 });
 
--- a/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
+++ b/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ -313,49 +313,65 @@ this.BrowserTestUtils = {
     let mm = win.messageManager;
     return this.waitForMessage(mm, "browser-test-utils:loadEvent", (msg) => {
       let selectedBrowser = win.gBrowser.selectedBrowser;
       return msg.target == selectedBrowser &&
              (aboutBlank || selectedBrowser.currentURI.spec != "about:blank")
     });
   },
 
+  _webProgressListeners: new Set(),
+
   /**
    * Waits for the web progress listener associated with this tab to fire a
    * STATE_STOP for the toplevel document.
    *
    * @param {xul:browser} browser
    *        A xul:browser.
+   * @param {String} expectedURI (optional)
+   *        A specific URL to check the channel load against
+   * @param {Boolean} checkAborts (optional, defaults to false)
+   *        Whether NS_BINDING_ABORTED stops 'count' as 'real' stops
+   *        (e.g. caused by the stop button or equivalent APIs)
    *
    * @return {Promise}
    * @resolves When STATE_STOP reaches the tab's progress listener
    */
-  browserStopped(browser) {
+  browserStopped(browser, expectedURI, checkAborts=false) {
     return new Promise(resolve => {
+      const kDocStopFlags = Ci.nsIWebProgressListener.STATE_IS_NETWORK |
+                            Ci.nsIWebProgressListener.STATE_STOP;
       let wpl = {
         onStateChange(aWebProgress, aRequest, aStateFlags, aStatus) {
-          if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP &&
+          dump("Saw state " + aStateFlags.toString(16) + " and status " + aStatus.toString(16) + "\n");
+          if (aStateFlags & Ci.nsIWebProgressListener.STATE_IS_NETWORK &&
+              aStateFlags & Ci.nsIWebProgressListener.STATE_STOP &&
+              (checkAborts || aStatus != Cr.NS_BINDING_ABORTED) &&
               aWebProgress.isTopLevel) {
-            browser.webProgress.removeProgressListener(filter);
-            filter.removeProgressListener(wpl);
-            resolve();
+            let chan = aRequest.QueryInterface(Ci.nsIChannel);
+            dump("Browser loaded " + chan.originalURI.spec + "\n");
+            if (!expectedURI || chan.originalURI.spec == expectedURI) {
+              browser.removeProgressListener(wpl);
+              BrowserTestUtils._webProgressListeners.delete(wpl);
+              resolve();
+            }
           };
         },
         onSecurityChange() {},
         onStatusChange() {},
         onLocationChange() {},
         QueryInterface: XPCOMUtils.generateQI([
           Ci.nsIWebProgressListener,
           Ci.nsIWebProgressListener2,
+          Ci.nsISupportsWeakReference,
         ]),
       };
-      const filter = Cc["@mozilla.org/appshell/component/browser-status-filter;1"]
-                       .createInstance(Ci.nsIWebProgress);
-      filter.addProgressListener(wpl, Ci.nsIWebProgress.NOTIFY_ALL);
-      browser.webProgress.addProgressListener(filter, Ci.nsIWebProgress.NOTIFY_ALL);
+      browser.addProgressListener(wpl);
+      this._webProgressListeners.add(wpl);
+      dump("Waiting for browser load" + (expectedURI ? (" of " + expectedURI) : "") + "\n");
     });
   },
 
   /**
    * Waits for the next tab to open and load a given URL.
    *
    * The method doesn't wait for the tab contents to load.
    *