Bug 1591183 - fix tabbrowser's expectations about state_start with documentchannel and add a test, r=dao

Differential Revision: https://phabricator.services.mozilla.com/D51081

--HG--
extra : moz-landing-system : lando

browser/base/content/tabbrowser.js

@@ -5625,37 +5625,28 @@
         location
       );
 
+      const {
+        STATE_START,
+        STATE_STOP,
+        STATE_IS_NETWORK,
+      } = Ci.nsIWebProgressListener;
+
       // If we were ignoring some messages about the initial about:blank, and we
       // got the STATE_STOP for it, we'll want to pay attention to those messages
       // from here forward. Similarly, if we conclude that this state change
       // is one that we shouldn't be ignoring, then stop ignoring.
       if (
         (ignoreBlank &&
-          aStateFlags & Ci.nsIWebProgressListener.STATE_STOP &&
-          aStateFlags & Ci.nsIWebProgressListener.STATE_IS_NETWORK) ||
+          aStateFlags & STATE_STOP &&
+          aStateFlags & STATE_IS_NETWORK) ||
         (!ignoreBlank && this.mBlank)
       ) {
         this.mBlank = false;
       }
 
-      if (aStateFlags & Ci.nsIWebProgressListener.STATE_START) {
+      if (aStateFlags & STATE_START && aStateFlags & STATE_IS_NETWORK) {
         this.mRequestCount++;
-      } else if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
-        const NS_ERROR_UNKNOWN_HOST = 2152398878;
-        if (--this.mRequestCount > 0 && aStatus == NS_ERROR_UNKNOWN_HOST) {
-          // to prevent bug 235825: wait for the request handled
-          // by the automatic keyword resolver
-          return;
-        }
-        // since we (try to) only handle STATE_STOP of the last request,
-        // the count of open requests should now be 0
-        this.mRequestCount = 0;
-      }
 
-      if (
-        aStateFlags & Ci.nsIWebProgressListener.STATE_START &&
-        aStateFlags & Ci.nsIWebProgressListener.STATE_IS_NETWORK
-      ) {
         if (aWebProgress.isTopLevel) {
           // Need to use originalLocation rather than location because things
           // like about:home and about:privatebrowsing arrive with nsIRequest
@@ -5705,10 +5696,16 @@
             gBrowser._isBusy = true;
           }
         }
-      } else if (
-        aStateFlags & Ci.nsIWebProgressListener.STATE_STOP &&
-        aStateFlags & Ci.nsIWebProgressListener.STATE_IS_NETWORK
-      ) {
+      } else if (aStateFlags & STATE_STOP && aStateFlags & STATE_IS_NETWORK) {
+        if (--this.mRequestCount > 0 && aStatus == Cr.NS_ERROR_UNKNOWN_HOST) {
+          // to prevent bug 235825: wait for the request handled
+          // by the automatic keyword resolver
+          return;
+        }
+        // since we (try to) only handle STATE_STOP of the last request,
+        // the count of open requests should now be 0
+        this.mRequestCount = 0;
+
         let modifiedAttrs = [];
         if (this.mTab.hasAttribute("busy")) {
           this.mTab.removeAttribute("busy");
@@ -5805,11 +5802,7 @@
         false
       );
 
-      if (
-        aStateFlags &
-        (Ci.nsIWebProgressListener.STATE_START |
-          Ci.nsIWebProgressListener.STATE_STOP)
-      ) {
+      if (aStateFlags & (STATE_START | STATE_STOP)) {
         // reset cached temporary values at beginning and end
         this.mMessage = "";
         this.mTotalProgress = 0;

browser/base/content/test/tabs/browser.ini

@@ -83,6 +83,7 @@ support-files = file_anchor_elements.html
 [browser_positional_attributes.js]
 skip-if = (verify && (os == 'win' || os == 'mac'))
 [browser_preloadedBrowser_zoom.js]
+[browser_progress_keyword_search_handling.js]
 [browser_reload_deleted_file.js]
 skip-if = (debug && os == 'mac') || (debug && os == 'linux' && bits == 64) #Bug 1421183, disabled on Linux/OSX for leaked windows
 [browser_tabCloseSpacer.js]

browser/base/content/test/tabs/browser_progress_keyword_search_handling.js

@@ -0,0 +1,80 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+const kButton = document.getElementById("reload-button");
+const kDocChanPref = "browser.tabs.documentchannel";
+
+add_task(async function setup() {
+  await SpecialPowers.pushPrefEnv({
+    set: [["browser.fixup.dns_first_for_single_words", true]],
+  });
+  registerCleanupFunction(() => {
+    Services.prefs.clearUserPref(kDocChanPref);
+  });
+});
+
+/*
+ * When loading a keyword search as a result of an unknown host error,
+ * check that we can stop the load.
+ * See https://bugzilla.mozilla.org/show_bug.cgi?id=235825
+ */
+add_task(async function test_unknown_host() {
+  for (let docChan of [true, false]) {
+    Services.prefs.setBoolPref(kDocChanPref, docChan);
+    await BrowserTestUtils.withNewTab("about:blank", async browser => {
+      const kNonExistingHost = "idontreallyexistonthisnetwork";
+      let searchPromise = BrowserTestUtils.waitForDocLoadAndStopIt(
+        Services.uriFixup.keywordToURI(kNonExistingHost).preferredURI.spec,
+        browser,
+        () => {
+          ok(kButton.hasAttribute("displaystop"), "Should be showing stop");
+          return true;
+        }
+      );
+      gURLBar.value = kNonExistingHost;
+      gURLBar.select();
+      EventUtils.synthesizeKey("KEY_Enter");
+      await searchPromise;
+      await BrowserTestUtils.waitForCondition(
+        () => !kButton.hasAttribute("displaystop")
+      );
+      ok(
+        !kButton.hasAttribute("displaystop"),
+        "Should no longer be showing stop after search"
+      );
+    });
+  }
+});
+
+/*
+ * When NOT loading a keyword search as a result of an unknown host error,
+ * check that the stop button goes back to being a reload button.
+ * See https://bugzilla.mozilla.org/show_bug.cgi?id=1591183
+ */
+add_task(async function test_unknown_host_without_search() {
+  for (let docChan of [true, false]) {
+    Services.prefs.setBoolPref(kDocChanPref, docChan);
+    await BrowserTestUtils.withNewTab("about:blank", async browser => {
+      const kNonExistingHost = "idontreallyexistonthisnetwork.example.com";
+      let searchPromise = BrowserTestUtils.browserLoaded(
+        browser,
+        false,
+        "http://" + kNonExistingHost + "/",
+        true /* want an error page */
+      );
+      gURLBar.value = kNonExistingHost;
+      gURLBar.select();
+      EventUtils.synthesizeKey("KEY_Enter");
+      await searchPromise;
+      await BrowserTestUtils.waitForCondition(
+        () => !kButton.hasAttribute("displaystop")
+      );
+      ok(
+        !kButton.hasAttribute("displaystop"),
+        "Should not be showing stop on error page"
+      );
+    });
+  }
+});

testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm

@@ -1422,9 +1422,11 @@ var BrowserTestUtils = {
    *        The URL of the document that is expected to load.
    * @param {object} browser
    *        The browser to wait for.
+   * @param {function} checkFn (optional)
+   *        Function to run on the channel before stopping it.
    * @returns {Promise}
    */
-  waitForDocLoadAndStopIt(expectedURL, browser) {
+  waitForDocLoadAndStopIt(expectedURL, browser, checkFn) {
     let isHttp = url => /^https?:/.test(url);
 
     let stoppedDocLoadPromise = () => {
@@ -1464,6 +1466,9 @@ var BrowserTestUtils = {
           if (!chan.originalURI || chan.originalURI.spec !== expectedURL) {
             return;
           }
+          if (checkFn && !checkFn(chan)) {
+            return;
+          }
 
           // TODO: We should check that the channel's BrowsingContext matches
           // the browser's. See bug 1587114.