From 1b0740623c6d8f7b4a44ba3210c4c710e9fba83f Mon Sep 17 00:00:00 2001 From: Marco Bonardo Date: Wed, 15 Jul 2020 10:16:50 +0000 Subject: [PATCH] Bug 1652592 - Unify the query pending checks across urlbar providers. r=adw Differential Revision: https://phabricator.services.mozilla.com/D83473 --- .../urlbar/UrlbarProviderAutofill.jsm | 14 +++++++---- .../UrlbarProviderHeuristicFallback.jsm | 15 +++-------- .../urlbar/UrlbarProviderInterventions.jsm | 13 +++------- .../urlbar/UrlbarProviderOmnibox.jsm | 14 ++++------- .../urlbar/UrlbarProviderOpenTabs.jsm | 13 +++------- .../urlbar/UrlbarProviderPrivateSearch.jsm | 14 +++++------ .../UrlbarProviderSearchSuggestions.jsm | 11 ++------ .../urlbar/UrlbarProviderSearchTips.jsm | 12 +++------ .../UrlbarProviderTokenAliasEngines.jsm | 25 ++++++------------- .../urlbar/UrlbarProviderTopSites.jsm | 14 +++-------- .../urlbar/UrlbarProviderUnifiedComplete.jsm | 11 +++----- .../urlbar/UrlbarProvidersManager.jsm | 11 ++++++++ .../tests/unit/test_providerOpenTabs.js | 1 - 13 files changed, 61 insertions(+), 107 deletions(-) diff --git a/browser/components/urlbar/UrlbarProviderAutofill.jsm b/browser/components/urlbar/UrlbarProviderAutofill.jsm index 8d71535a2bc2..2f93dc1714d7 100644 --- a/browser/components/urlbar/UrlbarProviderAutofill.jsm +++ b/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; } diff --git a/browser/components/urlbar/UrlbarProviderHeuristicFallback.jsm b/browser/components/urlbar/UrlbarProviderHeuristicFallback.jsm index a24f71ee4871..80216c7c5b16 100644 --- a/browser/components/urlbar/UrlbarProviderHeuristicFallback.jsm +++ b/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. diff --git a/browser/components/urlbar/UrlbarProviderInterventions.jsm b/browser/components/urlbar/UrlbarProviderInterventions.jsm index b49125edc785..d303da0f491b 100644 --- a/browser/components/urlbar/UrlbarProviderInterventions.jsm +++ b/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) { diff --git a/browser/components/urlbar/UrlbarProviderOmnibox.jsm b/browser/components/urlbar/UrlbarProviderOmnibox.jsm index d0f6dce6bcab..40af554feee2 100644 --- a/browser/components/urlbar/UrlbarProviderOmnibox.jsm +++ b/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(); diff --git a/browser/components/urlbar/UrlbarProviderOpenTabs.jsm b/browser/components/urlbar/UrlbarProviderOpenTabs.jsm index 8a5185f6882c..08b24ea10714 100644 --- a/browser/components/urlbar/UrlbarProviderOpenTabs.jsm +++ b/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) {} } /** diff --git a/browser/components/urlbar/UrlbarProviderPrivateSearch.jsm b/browser/components/urlbar/UrlbarProviderPrivateSearch.jsm index 8aef7313dfb2..752f518495e5 100644 --- a/browser/components/urlbar/UrlbarProviderPrivateSearch.jsm +++ b/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(); diff --git a/browser/components/urlbar/UrlbarProviderSearchSuggestions.jsm b/browser/components/urlbar/UrlbarProviderSearchSuggestions.jsm index 4819e0b4bdc5..380f1df66d53 100644 --- a/browser/components/urlbar/UrlbarProviderSearchSuggestions.jsm +++ b/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); } /** diff --git a/browser/components/urlbar/UrlbarProviderSearchTips.jsm b/browser/components/urlbar/UrlbarProviderSearchTips.jsm index 06aa9b9c0a8c..bfaf36d7f78c 100644 --- a/browser/components/urlbar/UrlbarProviderSearchTips.jsm +++ b/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. diff --git a/browser/components/urlbar/UrlbarProviderTokenAliasEngines.jsm b/browser/components/urlbar/UrlbarProviderTokenAliasEngines.jsm index 24ccfb0fb93d..6fe33dc2d982 100644 --- a/browser/components/urlbar/UrlbarProviderTokenAliasEngines.jsm +++ b/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) { diff --git a/browser/components/urlbar/UrlbarProviderTopSites.jsm b/browser/components/urlbar/UrlbarProviderTopSites.jsm index 31b513b90572..f850c8f987ad 100644 --- a/browser/components/urlbar/UrlbarProviderTopSites.jsm +++ b/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(); diff --git a/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm b/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm index ea0420964081..bd3de65fc11e 100644 --- a/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm +++ b/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(); } } diff --git a/browser/components/urlbar/UrlbarProvidersManager.jsm b/browser/components/urlbar/UrlbarProvidersManager.jsm index ca149c67bf4a..cef43ab91332 100644 --- a/browser/components/urlbar/UrlbarProvidersManager.jsm +++ b/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) { diff --git a/browser/components/urlbar/tests/unit/test_providerOpenTabs.js b/browser/components/urlbar/tests/unit/test_providerOpenTabs.js index 51f76b932005..898bb6885ec4 100644 --- a/browser/components/urlbar/tests/unit/test_providerOpenTabs.js +++ b/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"); });