Bug 1880069 - Switch-to-tab result sometimes appears twice with wrong container. r=jteow

The InputHistory (adaptive) provider is not properly reporting the open tab
userContextId, thus the Muxer is unable to dedupe results properly.
This cleans up the query a little bit avoiding numeric indices and fields that
are unused later.

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

browser/components/urlbar/UrlbarInput.sys.mjs

@@ -3834,8 +3834,9 @@ export class UrlbarInput {
       isPrivate: this.isPrivate,
       maxResults: lazy.UrlbarPrefs.get("maxRichResults"),
       searchString,
-      userContextId:
-        this.window.gBrowser.selectedBrowser.getAttribute("usercontextid"),
+      userContextId: parseInt(
+        this.window.gBrowser.selectedBrowser.getAttribute("usercontextid") || 0
+      ),
       currentPage: this.window.gBrowser.currentURI.spec,
       formHistoryName: this.formHistoryName,
       prohibitRemoteResults:

browser/components/urlbar/UrlbarProviderInputHistory.sys.mjs

@@ -23,16 +23,6 @@ ChromeUtils.defineESModuleGetters(lazy, {
   UrlbarResult: "resource:///modules/UrlbarResult.sys.mjs",
 });
 
-// Sqlite result row index constants.
-const QUERYINDEX = {
-  URL: 0,
-  TITLE: 1,
-  BOOKMARKED: 2,
-  BOOKMARKTITLE: 3,
-  TAGS: 4,
-  SWITCHTAB: 8,
-};
-
 // Constants to support an alternative frecency algorithm.
 const PAGES_USE_ALT_FRECENCY = Services.prefs.getBoolPref(
   "places.frecency.pages.alternative.featureGate",
@@ -42,23 +32,20 @@ const PAGES_FRECENCY_FIELD = PAGES_USE_ALT_FRECENCY
   ? "alt_frecency"
   : "frecency";
 
-// This SQL query fragment provides the following:
-//   - whether the entry is bookmarked (QUERYINDEX_BOOKMARKED)
-//   - the bookmark title, if it is a bookmark (QUERYINDEX_BOOKMARKTITLE)
-//   - the tags associated with a bookmarked entry (QUERYINDEX_TAGS)
-const SQL_BOOKMARK_TAGS_FRAGMENT = `EXISTS(SELECT 1 FROM moz_bookmarks WHERE fk = h.id) AS bookmarked,
-   ( SELECT title FROM moz_bookmarks WHERE fk = h.id AND title NOTNULL
-     ORDER BY lastModified DESC LIMIT 1
-   ) AS btitle,
-   ( SELECT GROUP_CONCAT(t.title ORDER BY t.title)
-     FROM moz_bookmarks b
-     JOIN moz_bookmarks t ON t.id = +b.parent AND t.parent = :parent
-     WHERE b.fk = h.id
-   ) AS tags`;
-
 const SQL_ADAPTIVE_QUERY = `/* do not warn (bug 487789) */
-   SELECT h.url, h.title, ${SQL_BOOKMARK_TAGS_FRAGMENT}, h.visit_count,
-          h.typed, h.id, t.open_count, ${PAGES_FRECENCY_FIELD}
+   SELECT h.url,
+          h.title,
+          EXISTS(SELECT 1 FROM moz_bookmarks WHERE fk = h.id) AS bookmarked,
+          ( SELECT title FROM moz_bookmarks WHERE fk = h.id AND title NOTNULL
+            ORDER BY lastModified DESC LIMIT 1
+          ) AS bookmark_title,
+          ( SELECT GROUP_CONCAT(t.title ORDER BY t.title)
+            FROM moz_bookmarks b
+            JOIN moz_bookmarks t ON t.id = +b.parent AND t.parent = :parent
+            WHERE b.fk = h.id
+          ) AS tags,
+          t.open_count,
+          t.userContextId
    FROM (
      SELECT ROUND(MAX(use_count) * (1 + (input = :search_string)), 1) AS rank,
             place_id
@@ -71,7 +58,7 @@ const SQL_ADAPTIVE_QUERY = `/* do not warn (bug 487789) */
           ON t.url = h.url
           AND (t.userContextId = :userContextId OR (t.userContextId <> -1 AND :userContextId IS NULL))
    WHERE AUTOCOMPLETE_MATCH(NULL, h.url,
-                            IFNULL(btitle, h.title), tags,
+                            IFNULL(bookmark_title, h.title), tags,
                             h.visit_count, h.typed, bookmarked,
                             t.open_count,
                             :matchBehavior, :searchBehavior,
@@ -142,14 +129,14 @@ class ProviderInputHistory extends UrlbarProvider {
     }
 
     for (let row of rows) {
-      const url = row.getResultByIndex(QUERYINDEX.URL);
-      const openPageCount = row.getResultByIndex(QUERYINDEX.SWITCHTAB) || 0;
-      const historyTitle = row.getResultByIndex(QUERYINDEX.TITLE) || "";
-      const bookmarked = row.getResultByIndex(QUERYINDEX.BOOKMARKED);
+      const url = row.getResultByName("url");
+      const openPageCount = row.getResultByName("open_count") || 0;
+      const historyTitle = row.getResultByName("title") || "";
+      const bookmarked = row.getResultByName("bookmarked");
       const bookmarkTitle = bookmarked
-        ? row.getResultByIndex(QUERYINDEX.BOOKMARKTITLE)
+        ? row.getResultByName("bookmark_title")
         : null;
-      const tags = row.getResultByIndex(QUERYINDEX.TAGS) || "";
+      const tags = row.getResultByName("tags") || "";
 
       let resultTitle = historyTitle;
       if (openPageCount > 0 && lazy.UrlbarPrefs.get("suggest.openpage")) {
@@ -164,7 +151,7 @@ class ProviderInputHistory extends UrlbarProvider {
             url: [url, UrlbarUtils.HIGHLIGHT.TYPED],
             title: [resultTitle, UrlbarUtils.HIGHLIGHT.TYPED],
             icon: UrlbarUtils.getIconForUrl(url),
-            userContextId: queryContext.userContextId || 0,
+            userContextId: row.getResultByName("userContextId") || 0,
           })
         );
         addCallback(this, result);

browser/components/urlbar/UrlbarProviderOpenTabs.sys.mjs

@@ -238,6 +238,8 @@ export class UrlbarProviderOpenTabs extends UrlbarProvider {
       }
       if (oldCount == 1) {
         entries.delete(url);
+        // Note: `entries` might be an empty Map now, though we don't remove it
+        // from `_openTabs` as it's likely to be reused later.
       } else {
         entries.set(url, oldCount - 1);
       }

browser/components/urlbar/UrlbarUtils.sys.mjs

@@ -1029,8 +1029,9 @@ export var UrlbarUtils = {
       isPrivate: lazy.PrivateBrowsingUtils.isWindowPrivate(window),
       maxResults: 1,
       searchString,
-      userContextId:
-        window.gBrowser.selectedBrowser.getAttribute("usercontextid"),
+      userContextId: parseInt(
+        window.gBrowser.selectedBrowser.getAttribute("usercontextid") || 0
+      ),
       prohibitRemoteResults: true,
       providers: ["AliasEngines", "BookmarkKeywords", "HeuristicFallback"],
     };

browser/components/urlbar/tests/browser/browser_switchTab_inputHistory.js

@@ -89,3 +89,54 @@ add_task(async function test_adaptive_with_search_term_and_switch_tab() {
     BrowserTestUtils.removeTab(tab);
   }
 });
+
+add_task(
+  async function test_adaptive_nonadaptive_container_dedupe_switch_tab() {
+    await SpecialPowers.pushPrefEnv({
+      set: [["privacy.userContext.enabled", true]],
+    });
+    // Add a url both to history and input history, ensure that the Muxer will
+    // properly dedupe the 2 entries, also with containers involved.
+    await PlacesUtils.history.clear();
+    const url = "https://example.com/";
+
+    let promiseVisited = PlacesTestUtils.waitForNotification(
+      "page-visited",
+      events => events.some(e => e.url === url)
+    );
+    let tab = BrowserTestUtils.addTab(gBrowser, url, { userContextId: 1 });
+    await promiseVisited;
+
+    async function queryAndCheckOneSwitchTabResult() {
+      await UrlbarTestUtils.promiseAutocompleteResultPopup({
+        window,
+        value: "xampl",
+      });
+      Assert.equal(
+        2,
+        UrlbarTestUtils.getResultCount(window),
+        "Check number of results"
+      );
+      let result = await UrlbarTestUtils.getDetailsOfResultAt(window, 1);
+      Assert.equal(url, result.url, `Url is the first non-heuristic result`);
+      Assert.equal(
+        UrlbarUtils.RESULT_TYPE.TAB_SWITCH,
+        result.type,
+        "Should be a switch tab result"
+      );
+      Assert.equal(
+        1,
+        result.result.payload.userContextId,
+        "Should use the expected container"
+      );
+    }
+    info("Check the tab is returned as history by a search.");
+    await queryAndCheckOneSwitchTabResult();
+    info("Add the same url to input history.");
+    await UrlbarUtils.addToInputHistory(url, "xampl");
+    info("Repeat the query.");
+    await queryAndCheckOneSwitchTabResult();
+    BrowserTestUtils.removeTab(tab);
+    await SpecialPowers.popPrefEnv();
+  }
+);