Bug 1675926 - Do not restore search mode if pageproxystate is valid. r=adw

We were restoring search mode after tab switch even if the destination tab had a valid pageproxystate. This was causing issues in the case where the user switched away from a tab that was still loading and had not yet called setURI. If the user switched back to that tab after it was done loading, we would re-enter search mode, using the page's URL as the search string.

Making this change also requires storing pageproxystate differently. If we only made the change to setURI, we would sometimes not restore search mode when we should. If the user entered search mode with an empty string, userTypedValue would never be updated and it would still be the page URL. After the user switched from, then to, that tab, we'd call setURI. setURI would restore userTypedValue, which would be the page URL. Since that page now had a valid pageproxystate, we would not restore search mode. Now we update userTypedValue when we enter search mode, so we restore search mode properly even when there is no search string.

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

browser/components/urlbar/UrlbarInput.jsm

@@ -383,7 +383,7 @@ class UrlbarInput {
     // If we're switching tabs, restore the tab's search mode.  Otherwise, if
     // the URI is valid, exit search mode.  This must happen after setting
     // proxystate above because search mode depends on it.
-    if (dueToTabSwitch) {
+    if (dueToTabSwitch && !valid) {
       this.restoreSearchModeState();
     } else if (valid) {
       this.searchMode = null;
@@ -1510,11 +1510,17 @@ class UrlbarInput {
     // Enter search mode if the browser is selected.
     if (browser == this.window.gBrowser.selectedBrowser) {
       this._updateSearchModeUI(searchMode);
-      if (searchMode && !searchMode.isPreview && !areSearchModesSame) {
-        try {
-          BrowserUsageTelemetry.recordSearchMode(searchMode);
-        } catch (ex) {
-          Cu.reportError(ex);
+      if (searchMode) {
+        // Set userTypedValue to the query string so that it's properly restored
+        // when switching back to the current tab and across sessions.
+        this.window.gBrowser.userTypedValue = this.untrimmedValue;
+        this.valueIsTyped = true;
+        if (!searchMode.isPreview && !areSearchModesSame) {
+          try {
+            BrowserUsageTelemetry.recordSearchMode(searchMode);
+          } catch (ex) {
+            Cu.reportError(ex);
+          }
         }
       }
     }
@@ -1768,12 +1774,8 @@ class UrlbarInput {
 
     this.searchMode = searchMode;
 
-    // Set userTypedValue to the payload's query string so that it's properly
-    // restored when switching back to the current tab and across sessions.
     let value = result.payload.query?.trimStart() || "";
     this._setValue(value, false);
-    this.window.gBrowser.userTypedValue = value;
-    this.valueIsTyped = true;
 
     if (startQuery) {
       this.startQuery({ allowAutofill: false });

browser/components/urlbar/tests/browser/browser_searchMode_setURI.js

@@ -25,9 +25,9 @@ add_task(async function setURI() {
   for (let test of [
     // initialURL, searchString, url, expectSearchMode
 
-    ["about:blank", null, null, true],
-    ["about:blank", null, "about:blank", true],
-    ["about:blank", null, "http://www.example.com/", false],
+    ["about:blank", "", null, true],
+    ["about:blank", "", "about:blank", true],
+    ["about:blank", "", "http://www.example.com/", true],
 
     ["about:blank", "about:blank", null, false],
     ["about:blank", "about:blank", "about:blank", false],
@@ -41,9 +41,9 @@ add_task(async function setURI() {
     ["about:blank", "not a URL", "about:blank", true],
     ["about:blank", "not a URL", "http://www.example.com/", true],
 
-    ["http://www.example.com/", null, null, false],
-    ["http://www.example.com/", null, "about:blank", true],
-    ["http://www.example.com/", null, "http://www.example.com/", false],
+    ["http://www.example.com/", "", null, true],
+    ["http://www.example.com/", "", "about:blank", true],
+    ["http://www.example.com/", "", "http://www.example.com/", true],
 
     ["http://www.example.com/", "about:blank", null, false],
     ["http://www.example.com/", "about:blank", "about:blank", false],
@@ -104,8 +104,8 @@ async function doSetURITest(initialURL, searchString, url, expectSearchMode) {
     await UrlbarTestUtils.promisePopupClose(window);
     Assert.strictEqual(
       gBrowser.selectedBrowser.userTypedValue,
-      searchString || null,
-      `userTypedValue should be ${searchString || null}`
+      searchString,
+      `userTypedValue should be ${searchString}`
     );
 
     // Call setURI.

browser/components/urlbar/tests/browser/browser_searchMode_switchTabs.js

@@ -186,6 +186,78 @@ add_task(async function switchTabs() {
   }
 });
 
+// Start loading a SERP from search mode then immediately switch to a new tab so
+// the SERP finishes loading in the background. Switch back to the SERP tab and
+// observe that we don't re-enter search mode despite having an entry for that
+// tab in UrlbarInput._searchModesByBrowser. See bug 1675926.
+//
+// This subtest intermittently does not test bug 1675926 (NB: this does not mean
+// it is an intermittent failure). The false-positive occurs if the SERP page
+// finishes loading before we switch tabs. We include this subtest so we have
+// one covering real-world behaviour. A subtest that is guaranteed to test this
+// behaviour that does not simulate real world behaviour is included below.
+add_task(async function slow_load() {
+  await SpecialPowers.pushPrefEnv({
+    set: [["browser.urlbar.suggest.searches", false]],
+  });
+  const engineName = "Test";
+  let engine = await Services.search.addEngineWithDetails(engineName, {
+    template: "http://example.com/?search={searchTerms}",
+  });
+  const originalTab = gBrowser.selectedTab;
+  const newTab = await BrowserTestUtils.openNewForegroundTab(gBrowser);
+
+  await UrlbarTestUtils.promiseAutocompleteResultPopup({
+    window,
+    value: "test",
+    fireInputEvent: true,
+  });
+  await UrlbarTestUtils.enterSearchMode(window, { engineName });
+
+  const loadPromise = BrowserTestUtils.browserLoaded(newTab.linkedBrowser);
+  // Select the search mode heuristic to load the example.com SERP.
+  EventUtils.synthesizeKey("KEY_Enter");
+  // Switch away from the tab before we let it load.
+  await BrowserTestUtils.switchTab(gBrowser, originalTab);
+  await loadPromise;
+
+  // Switch back to the search mode tab and confirm we don't restore search
+  // mode.
+  await BrowserTestUtils.switchTab(gBrowser, newTab);
+  await UrlbarTestUtils.assertSearchMode(window, null);
+
+  BrowserTestUtils.removeTab(newTab);
+  await Services.search.removeEngine(engine);
+  await SpecialPowers.popPrefEnv();
+});
+
+// Tests the same behaviour as slow_load, but in a more reliable way using
+// non-real-world behaviour.
+add_task(async function slow_load_guaranteed() {
+  const engineName = "Test";
+  let engine = await Services.search.addEngineWithDetails(engineName, {
+    template: "http://example.com/?search={searchTerms}",
+  });
+  const backgroundTab = BrowserTestUtils.addTab(gBrowser);
+
+  // Simulate a tab that was in search mode, loaded a SERP, then was switched
+  // away from before setURI was called.
+  backgroundTab.ownerGlobal.gURLBar.searchMode = { engineName };
+  let loadPromise = BrowserTestUtils.browserLoaded(backgroundTab.linkedBrowser);
+  BrowserTestUtils.loadURI(
+    backgroundTab.linkedBrowser,
+    "http://example.com/?search=test"
+  );
+  await loadPromise;
+
+  // Switch to the background mode tab and confirm we don't restore search mode.
+  await BrowserTestUtils.switchTab(gBrowser, backgroundTab);
+  await UrlbarTestUtils.assertSearchMode(window, null);
+
+  BrowserTestUtils.removeTab(backgroundTab);
+  await Services.search.removeEngine(engine);
+});
+
 // Enters search mode by typing a restriction char with no search string.
 // Search mode and the search string should be restored after switching back to
 // the tab.