Bug 1652592 - Unify the query pending checks across urlbar providers. r=adw

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

browser/components/urlbar/UrlbarProviderAutofill.jsm

@@ -273,8 +273,6 @@ function iconHelper(url) {
 class ProviderAutofill extends UrlbarProvider {
   constructor() {
     super();
-    // Maps the running queries by queryContext.
-    this.queries = new Map();
   }
 
   /**
@@ -301,6 +299,8 @@ class ProviderAutofill extends UrlbarProvider {
    * @returns {boolean} Whether this provider should be invoked for the search.
    */
   async isActive(queryContext) {
+    let instance = this.queryInstance;
+
     // First of all, check for the autoFill pref.
     if (!UrlbarPrefs.get("autoFill")) {
       return false;
@@ -359,6 +359,11 @@ class ProviderAutofill extends UrlbarProvider {
       return false;
     }
 
+    // Check the query was not canceled while this executed.
+    if (instance != this.queryInstance) {
+      return false;
+    }
+
     return true;
   }
 
@@ -387,10 +392,9 @@ class ProviderAutofill extends UrlbarProvider {
    * @returns {Promise} resolved when the query stops.
    */
   async startQuery(queryContext, addCallback) {
-    // This serves as the equivalent of checking this.queries.has to see if the
-    // query has been cancelled, since this._autofillResult is deleted in
-    // cancelQuery.
+    // Sanity check since this._autofillResult is deleted in cancelQuery.
     if (!this._autofillResult) {
+      this.logger.error("startQuery invoked without an _autofillResult");
       return;
     }
 

browser/components/urlbar/UrlbarProviderHeuristicFallback.jsm

@@ -30,8 +30,6 @@ XPCOMUtils.defineLazyModuleGetters(this, {
 class ProviderHeuristicFallback extends UrlbarProvider {
   constructor() {
     super();
-    // Maps the running queries by queryContext.
-    this.queries = new Map();
   }
 
   /**
@@ -78,8 +76,7 @@ class ProviderHeuristicFallback extends UrlbarProvider {
    * @returns {Promise} resolved when the query stops.
    */
   async startQuery(queryContext, addCallback) {
-    let instance = {};
-    this.queries.set(queryContext, instance);
+    let instance = this.queryInstance;
 
     let result = this._matchUnknownUrl(queryContext);
     if (result) {
@@ -102,7 +99,7 @@ class ProviderHeuristicFallback extends UrlbarProvider {
           let searchResult = await this._defaultEngineSearchResult(
             queryContext
           );
-          if (!this.queries.has(queryContext)) {
+          if (instance != this.queryInstance) {
             return;
           }
           addCallback(this, searchResult);
@@ -110,23 +107,19 @@ class ProviderHeuristicFallback extends UrlbarProvider {
       }
     } else {
       result = await this._defaultEngineSearchResult(queryContext);
-      if (!result || !this.queries.has(queryContext)) {
+      if (!result || instance != this.queryInstance) {
         return;
       }
       result.heuristic = true;
       addCallback(this, result);
     }
-
-    this.queries.delete(queryContext);
   }
 
   /**
    * Cancels a running query.
    * @param {object} queryContext The query context object
    */
-  cancelQuery(queryContext) {
-    this.queries.delete(queryContext);
-  }
+  cancelQuery(queryContext) {}
 
   // TODO (bug 1054814): Use visited URLs to inform which scheme to use, if the
   // scheme isn't specificed.

browser/components/urlbar/UrlbarProviderInterventions.jsm

@@ -433,8 +433,6 @@ function getL10nPropertiesForTip(tip) {
 class ProviderInterventions extends UrlbarProvider {
   constructor() {
     super();
-    // Maps the running queries by queryContext.
-    this.queries = new Map();
     // The tip we should currently show.
     this.currentTip = TIPS.NONE;
 
@@ -586,8 +584,7 @@ class ProviderInterventions extends UrlbarProvider {
    *        result. A UrlbarResult should be passed to it.
    */
   async startQuery(queryContext, addCallback) {
-    let instance = {};
-    this.queries.set(queryContext, instance);
+    let instance = this.queryInstance;
 
     // TIPS.UPDATE_CHECKING is special, and we never actually show a tip that
     // reflects a "checking" status.  Instead it's handled like this.  We call
@@ -616,7 +613,7 @@ class ProviderInterventions extends UrlbarProvider {
           };
           appUpdater.addListener(this._appUpdaterListener);
         });
-        if (!this.queries.has(queryContext)) {
+        if (instance != this.queryInstance) {
           // The query was canceled before the check finished.
           return;
         }
@@ -625,7 +622,6 @@ class ProviderInterventions extends UrlbarProvider {
         // early.
         this._setCurrentTipFromAppUpdaterStatus();
         if (this.currentTip == TIPS.UPDATE_CHECKING) {
-          this.queries.delete(queryContext);
           return;
         }
       }
@@ -645,14 +641,13 @@ class ProviderInterventions extends UrlbarProvider {
 
     Object.assign(result.payload, getL10nPropertiesForTip(this.currentTip));
 
-    if (!this.queries.has(queryContext)) {
+    if (instance != this.queryInstance) {
       return;
     }
 
     this.tipsShownInCurrentEngagement.add(this.currentTip);
 
     addCallback(this, result);
-    this.queries.delete(queryContext);
   }
 
   /**
@@ -661,8 +656,6 @@ class ProviderInterventions extends UrlbarProvider {
    *        query for.
    */
   cancelQuery(queryContext) {
-    this.queries.delete(queryContext);
-
     // If we're waiting for appUpdater to finish its update check,
     // this._appUpdaterListener will be defined.  We can stop listening now.
     if (this._appUpdaterListener) {

browser/components/urlbar/UrlbarProviderOmnibox.jsm

@@ -33,8 +33,6 @@ const MAXIMUM_ALLOWED_EXTENSION_TIME_MS = 3000;
 class ProviderOmnibox extends UrlbarProvider {
   constructor() {
     super();
-    // Maps the running queries by queryContext.
-    this.queries = new Map();
   }
 
   /**
@@ -111,8 +109,7 @@ class ProviderOmnibox extends UrlbarProvider {
    *   The callback invoked by this method to add each result.
    */
   async startQuery(queryContext, addCallback) {
-    let instance = {};
-    this.queries.set(queryContext, instance);
+    let instance = this.queryInstance;
 
     // Fetch heuristic result.
     let keyword = queryContext.tokens[0].value;
@@ -139,6 +136,9 @@ class ProviderOmnibox extends UrlbarProvider {
     this._resultsPromise = ExtensionSearchHandler.handleSearch(
       data,
       suggestions => {
+        if (instance != this.queryInstance) {
+          return;
+        }
         for (let suggestion of suggestions) {
           let content = `${queryContext.tokens[0].value} ${suggestion.content}`;
           if (content == heuristicResult.payload.content) {
@@ -172,8 +172,6 @@ class ProviderOmnibox extends UrlbarProvider {
     await Promise.race([timeoutPromise, this._resultsPromise]).catch(
       Cu.reportError
     );
-
-    this.queries.delete(queryContext);
   }
 
   /**
@@ -183,9 +181,7 @@ class ProviderOmnibox extends UrlbarProvider {
    * @param {UrlbarQueryContext} queryContext
    *   The query context object.
    */
-  cancelQuery(queryContext) {
-    this.queries.delete(queryContext);
-  }
+  cancelQuery(queryContext) {}
 }
 
 var UrlbarProviderOmnibox = new ProviderOmnibox();

browser/components/urlbar/UrlbarProviderOpenTabs.jsm

@@ -28,8 +28,6 @@ XPCOMUtils.defineLazyModuleGetters(this, {
 class UrlbarProviderOpenTabs extends UrlbarProvider {
   constructor() {
     super();
-    // Maps the running queries by queryContext.
-    this.queries = new Map();
   }
 
   /**
@@ -132,8 +130,7 @@ class UrlbarProviderOpenTabs extends UrlbarProvider {
     // temp table to return proper frecency.
     // TODO:
     //  * properly search and handle tokens, this is just a mock for now.
-    let instance = {};
-    this.queries.set(queryContext, instance);
+    let instance = this.queryInstance;
     let conn = await PlacesUtils.promiseLargeCacheDBConnection();
     await UrlbarProviderOpenTabs.promiseDBPopulated;
     await conn.executeCached(
@@ -143,7 +140,7 @@ class UrlbarProviderOpenTabs extends UrlbarProvider {
     `,
       {},
       (row, cancel) => {
-        if (!this.queries.has(queryContext)) {
+        if (instance != this.queryInstance) {
           cancel();
           return;
         }
@@ -160,17 +157,13 @@ class UrlbarProviderOpenTabs extends UrlbarProvider {
         );
       }
     );
-    // We are done.
-    this.queries.delete(queryContext);
   }
 
   /**
    * Cancels a running query.
    * @param {object} queryContext The query context object
    */
-  cancelQuery(queryContext) {
-    this.queries.delete(queryContext);
-  }
+  cancelQuery(queryContext) {}
 }
 
 /**

browser/components/urlbar/UrlbarProviderPrivateSearch.jsm

@@ -44,8 +44,6 @@ class ProviderPrivateSearch extends UrlbarProvider {
     super();
     // Maps the open tabs by userContextId.
     this.openTabs = new Map();
-    // Maps the running queries by queryContext.
-    this.queries = new Map();
   }
 
   /**
@@ -104,8 +102,7 @@ class ProviderPrivateSearch extends UrlbarProvider {
         .join(" ");
     }
 
-    let instance = {};
-    this.queries.set(queryContext, instance);
+    let instance = this.queryInstance;
 
     let engine = queryContext.engineName
       ? Services.search.getEngineByName(queryContext.engineName)
@@ -124,6 +121,10 @@ class ProviderPrivateSearch extends UrlbarProvider {
       logger: this.logger,
     }).promise;
 
+    if (instance != this.queryInstance) {
+      return;
+    }
+
     let result = new UrlbarResult(
       UrlbarUtils.RESULT_TYPE.SEARCH,
       UrlbarUtils.RESULT_SOURCE.SEARCH,
@@ -137,16 +138,13 @@ class ProviderPrivateSearch extends UrlbarProvider {
     );
     result.suggestedIndex = 1;
     addCallback(this, result);
-    this.queries.delete(queryContext);
   }
 
   /**
    * Cancels a running query.
    * @param {object} queryContext The query context object
    */
-  cancelQuery(queryContext) {
-    this.queries.delete(queryContext);
-  }
+  cancelQuery(queryContext) {}
 }
 
 var UrlbarProviderPrivateSearch = new ProviderPrivateSearch();

browser/components/urlbar/UrlbarProviderSearchSuggestions.jsm

@@ -50,8 +50,6 @@ function looksLikeUrl(str, ignoreAlphanumericHosts = false) {
 class ProviderSearchSuggestions extends UrlbarProvider {
   constructor() {
     super();
-    // Maps the running queries by queryContext.
-    this.queries = new Map();
   }
 
   /**
@@ -210,8 +208,7 @@ class ProviderSearchSuggestions extends UrlbarProvider {
    * @returns {Promise} resolved when the query stops.
    */
   async startQuery(queryContext, addCallback) {
-    let instance = {};
-    this.queries.set(queryContext, instance);
+    let instance = this.queryInstance;
 
     let trimmedOriginalSearchString = queryContext.searchString.trim();
 
@@ -296,15 +293,13 @@ class ProviderSearchSuggestions extends UrlbarProvider {
       alias
     );
 
-    if (!results || !this.queries.has(queryContext)) {
+    if (!results || instance != this.queryInstance) {
       return;
     }
 
     for (let result of results) {
       addCallback(this, result);
     }
-
-    this.queries.delete(queryContext);
   }
 
   /**
@@ -325,8 +320,6 @@ class ProviderSearchSuggestions extends UrlbarProvider {
       this._suggestionsController.stop();
       this._suggestionsController = null;
     }
-
-    this.queries.delete(queryContext);
   }
 
   /**

browser/components/urlbar/UrlbarProviderSearchTips.jsm

@@ -92,8 +92,6 @@ const LAST_UPDATE_THRESHOLD_MS = 24 * 60 * 60 * 1000;
 class ProviderSearchTips extends UrlbarProvider {
   constructor() {
     super();
-    // Maps the running queries by queryContext.
-    this.queries = new Map();
 
     // Whether we should disable tips for the current browser session, for
     // example because a tip was already shown.
@@ -169,8 +167,7 @@ class ProviderSearchTips extends UrlbarProvider {
    *       is done searching AND returning results.
    */
   async startQuery(queryContext, addCallback) {
-    let instance = {};
-    this.queries.set(queryContext, instance);
+    let instance = this.queryInstance;
 
     let tip = this.currentTip;
     this.showedTipTypeInCurrentEngagement = this.currentTip;
@@ -209,14 +206,13 @@ class ProviderSearchTips extends UrlbarProvider {
         break;
     }
 
-    if (!this.queries.has(queryContext)) {
+    if (instance != this.queryInstance) {
       return;
     }
 
     Services.telemetry.keyedScalarAdd("urlbar.tips", `${tip}-shown`, 1);
 
     addCallback(this, result);
-    this.queries.delete(queryContext);
   }
 
   /**
@@ -224,9 +220,7 @@ class ProviderSearchTips extends UrlbarProvider {
    * @param {UrlbarQueryContext} queryContext the query context object to cancel
    *        query for.
    */
-  cancelQuery(queryContext) {
-    this.queries.delete(queryContext);
-  }
+  cancelQuery(queryContext) {}
 
   /**
    * Called when the tip is selected.

browser/components/urlbar/UrlbarProviderTokenAliasEngines.jsm

@@ -27,8 +27,6 @@ XPCOMUtils.defineLazyModuleGetters(this, {
 class ProviderTokenAliasEngines extends UrlbarProvider {
   constructor() {
     super();
-    // Maps the running queries by queryContext.
-    this.queries = new Map();
     this._engines = [];
   }
 
@@ -61,6 +59,7 @@ class ProviderTokenAliasEngines extends UrlbarProvider {
    * @returns {boolean} Whether this provider should be invoked for the search.
    */
   async isActive(queryContext) {
+    let instance = this.queryInstance;
     // Once the user starts typing a search string after the token, we hand off
     // suggestions to UrlbarProviderSearchSuggestions.
     if (
@@ -75,6 +74,11 @@ class ProviderTokenAliasEngines extends UrlbarProvider {
       return false;
     }
 
+    // Check the query was not canceled while this executed.
+    if (instance != this.queryInstance) {
+      return false;
+    }
+
     if (queryContext.searchString.trim() == "@") {
       return true;
     }
@@ -98,17 +102,7 @@ class ProviderTokenAliasEngines extends UrlbarProvider {
    *        result.
    */
   async startQuery(queryContext, addCallback) {
-    let instance = {};
-    this.queries.set(queryContext, instance);
-
-    // Even though startQuery doesn't wait on any Promises, we check
-    // this.queries.has(queryContext) because isActive waits but we can't check
-    // the same condition there.
-    if (
-      !this._engines ||
-      !this._engines.length ||
-      !this.queries.has(queryContext)
-    ) {
+    if (!this._engines || !this._engines.length) {
       return;
     }
 
@@ -129,11 +123,7 @@ class ProviderTokenAliasEngines extends UrlbarProvider {
       }
     } else if (this._autofillResult) {
       addCallback(this, this._autofillResult);
-      this.queries.delete(queryContext);
-      return;
     }
-
-    this.queries.delete(queryContext);
   }
 
   /**
@@ -151,7 +141,6 @@ class ProviderTokenAliasEngines extends UrlbarProvider {
    */
   cancelQuery(queryContext) {
     delete this._autofillResult;
-    this.queries.delete(queryContext);
   }
 
   _getAutofillResult(queryContext) {

browser/components/urlbar/UrlbarProviderTopSites.jsm

@@ -34,8 +34,6 @@ XPCOMUtils.defineLazyModuleGetters(this, {
 class ProviderTopSites extends UrlbarProvider {
   constructor() {
     super();
-    // Maps the running queries by queryContext.
-    this.queries = new Map();
   }
 
   get PRIORITY() {
@@ -107,8 +105,7 @@ class ProviderTopSites extends UrlbarProvider {
 
     let sites = AboutNewTab.getTopSites();
 
-    let instance = {};
-    this.queries.set(queryContext, instance);
+    let instance = this.queryInstance;
 
     // Filter out empty values. Site is empty when there's a gap between tiles
     // on about:newtab.
@@ -182,7 +179,7 @@ class ProviderTopSites extends UrlbarProvider {
           }
 
           // Our query has been cancelled.
-          if (!this.queries.get(queryContext)) {
+          if (instance != this.queryInstance) {
             break;
           }
 
@@ -208,7 +205,7 @@ class ProviderTopSites extends UrlbarProvider {
             break;
           }
 
-          if (!this.queries.get(queryContext)) {
+          if (instance != this.queryInstance) {
             break;
           }
 
@@ -233,7 +230,6 @@ class ProviderTopSites extends UrlbarProvider {
           break;
       }
     }
-    this.queries.delete(queryContext);
   }
 
   /**
@@ -241,9 +237,7 @@ class ProviderTopSites extends UrlbarProvider {
    * @param {UrlbarQueryContext} queryContext the query context object to cancel
    *        query for.
    */
-  cancelQuery(queryContext) {
-    this.queries.delete(queryContext);
-  }
+  cancelQuery(queryContext) {}
 }
 
 var UrlbarProviderTopSites = new ProviderTopSites();

browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm

@@ -36,8 +36,6 @@ XPCOMUtils.defineLazyServiceGetter(
 class ProviderUnifiedComplete extends UrlbarProvider {
   constructor() {
     super();
-    // Maps the running queries by queryContext.
-    this.queries = new Map();
   }
 
   /**
@@ -75,10 +73,12 @@ class ProviderUnifiedComplete extends UrlbarProvider {
    * @returns {Promise} resolved when the query stops.
    */
   async startQuery(queryContext, addCallback) {
-    let instance = {};
-    this.queries.set(queryContext, instance);
+    let instance = this.queryInstance;
     let urls = new Set();
     await unifiedComplete.wrappedJSObject.startQuery(queryContext, acResult => {
+      if (instance != this.queryInstance) {
+        return;
+      }
       let results = convertLegacyAutocompleteResult(
         queryContext,
         acResult,
@@ -88,7 +88,6 @@ class ProviderUnifiedComplete extends UrlbarProvider {
         addCallback(this, result);
       }
     });
-    this.queries.delete(queryContext);
   }
 
   /**
@@ -96,8 +95,6 @@ class ProviderUnifiedComplete extends UrlbarProvider {
    * @param {object} queryContext The query context object
    */
   cancelQuery(queryContext) {
-    // This doesn't properly support being used concurrently by multiple fields.
-    this.queries.delete(queryContext);
     unifiedComplete.stopSearch();
   }
 }

browser/components/urlbar/UrlbarProvidersManager.jsm

@@ -327,6 +327,15 @@ class Query {
     let activePromises = [];
     let maxPriority = -1;
     for (let provider of this.providers) {
+      // This can be used by the provider to check the query is still running
+      // after executing async tasks:
+      //   let instance = this.queryInstance;
+      //   await ...
+      //   if (instance != this.queryInstance) {
+      //     // Query was canceled or a new one started.
+      //     return;
+      //   }
+      provider.queryInstance = this;
       activePromises.push(
         // Not all isActive implementations are async, so wrap the call in a
         // promise so we can be sure we can call `then` on it.  Note that
@@ -422,6 +431,8 @@ class Query {
       provider.logger.info(
         `Canceling query for "${this.context.searchString}"`
       );
+      // Mark the instance as no more valid, see start() for details.
+      provider.queryInstance = null;
       provider.tryMethod("cancelQuery", this.context);
     }
     if (this._chunkTimer) {

browser/components/urlbar/tests/unit/test_providerOpenTabs.js

@@ -42,5 +42,4 @@ add_task(async function test_openTabs() {
   Assert.equal(matchCount, 1, "Found the expected number of matches");
   // Sanity check that this doesn't throw.
   provider.cancelQuery(context);
-  Assert.equal(provider.queries.size, 0, "All the queries have been removed");
 });