Bug 1548817 - Quantum Bar controller notifications may arrive out of order. r=adw

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

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

browser/components/urlbar/UrlbarController.jsm

@@ -13,7 +13,6 @@ const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
 XPCOMUtils.defineLazyModuleGetters(this, {
   AppConstants: "resource://gre/modules/AppConstants.jsm",
   BrowserUsageTelemetry: "resource:///modules/BrowserUsageTelemetry.jsm",
-  ExtensionSearchHandler: "resource://gre/modules/ExtensionSearchHandler.jsm",
   PlacesUtils: "resource://gre/modules/PlacesUtils.jsm",
   UrlbarPrefs: "resource:///modules/UrlbarPrefs.jsm",
   UrlbarProvidersManager: "resource:///modules/UrlbarProvidersManager.jsm",
@@ -95,42 +94,53 @@ class UrlbarController {
     // Cancel any running query.
     this.cancelQuery();
 
-    this._lastQueryContext = queryContext;
+    // Wrap the external queryContext, to track a unique object, in case
+    // the external consumer reuses the same context multiple times.
+    // This also allows to add properties without polluting the context.
+    // Note this can't be null-ed or deleted once a query is done, because it's
+    // used by handleDeleteEntry and handleKeyNavigation, that can run after
+    // a query is cancelled or finished.
+    let contextWrapper = this._lastQueryContextWrapper = {queryContext};
 
     queryContext.lastResultCount = 0;
     TelemetryStopwatch.start(TELEMETRY_1ST_RESULT, queryContext);
     TelemetryStopwatch.start(TELEMETRY_6_FIRST_RESULTS, queryContext);
 
+    // For proper functionality we must ensure this notification is fired
+    // synchronously, as soon as startQuery is invoked, but after any
+    // notifications related to the previous query.
     this._notify("onQueryStarted", queryContext);
     await this.manager.startQuery(queryContext, this);
-    this._notify("onQueryFinished", queryContext);
+    // If the query has been cancelled, onQueryFinished was notified already.
+    // Note this._lastQueryContextWrapper may have changed in the meanwhile.
+    if (contextWrapper === this._lastQueryContextWrapper &&
+        !contextWrapper.done) {
+      contextWrapper.done = true;
+      // TODO (Bug 1549936) this is necessary to avoid leaks in PB tests.
+      this.manager.cancelQuery(queryContext);
+      this._notify("onQueryFinished", queryContext);
+    }
     return queryContext;
   }
 
   /**
    * Cancels an in-progress query. Note, queries may continue running if they
    * can't be cancelled.
-   *
-   * @param {UrlbarUtils.CANCEL_REASON} [reason]
-   *   The reason the query was cancelled.
    */
-  cancelQuery(reason) {
-    if (!this._lastQueryContext ||
-        this._lastQueryContext._cancelled) {
+  cancelQuery() {
+    // If the query finished already, don't handle cancel.
+    if (!this._lastQueryContextWrapper || this._lastQueryContextWrapper.done) {
       return;
     }
 
-    TelemetryStopwatch.cancel(TELEMETRY_1ST_RESULT, this._lastQueryContext);
-    TelemetryStopwatch.cancel(TELEMETRY_6_FIRST_RESULTS, this._lastQueryContext);
+    this._lastQueryContextWrapper.done = true;
 
-    this.manager.cancelQuery(this._lastQueryContext);
-    this._lastQueryContext._cancelled = true;
-    this._notify("onQueryCancelled", this._lastQueryContext);
-
-    if (reason == UrlbarUtils.CANCEL_REASON.BLUR &&
-        ExtensionSearchHandler.hasActiveInputSession()) {
-      ExtensionSearchHandler.handleInputCancelled();
-    }
+    let {queryContext} = this._lastQueryContextWrapper;
+    TelemetryStopwatch.cancel(TELEMETRY_1ST_RESULT, queryContext);
+    TelemetryStopwatch.cancel(TELEMETRY_6_FIRST_RESULTS, queryContext);
+    this.manager.cancelQuery(queryContext);
+    this._notify("onQueryCancelled", queryContext);
+    this._notify("onQueryFinished", queryContext);
   }
 
   /**
@@ -255,17 +265,15 @@ class UrlbarController {
       return;
     }
 
-    if (this.view.isOpen && executeAction) {
-      let queryContext = this._lastQueryContext;
-      if (queryContext) {
-        let handled = this.view.oneOffSearchButtons.handleKeyPress(
-          event,
-          queryContext.results.length,
-          this.view.allowEmptySelection,
-          queryContext.searchString);
-        if (handled) {
-          return;
-        }
+    if (this.view.isOpen && executeAction && this._lastQueryContextWrapper) {
+      let {queryContext} = this._lastQueryContextWrapper;
+      let handled = this.view.oneOffSearchButtons.handleKeyPress(
+        event,
+        queryContext.results.length,
+        this.view.allowEmptySelection,
+        queryContext.searchString);
+      if (handled) {
+        return;
       }
     }
 
@@ -496,7 +504,7 @@ class UrlbarController {
    * @returns {boolean} Returns true if the deletion was acted upon.
    */
   _handleDeleteEntry() {
-    if (!this._lastQueryContext) {
+    if (!this._lastQueryContextWrapper) {
       Cu.reportError("Cannot delete - the latest query is not present");
       return false;
     }
@@ -507,13 +515,14 @@ class UrlbarController {
       return false;
     }
 
-    let index = this._lastQueryContext.results.indexOf(selectedResult);
+    let {queryContext} = this._lastQueryContextWrapper;
+    let index = queryContext.results.indexOf(selectedResult);
     if (!index) {
       Cu.reportError("Failed to find the selected result in the results");
       return false;
     }
 
-    this._lastQueryContext.results.splice(index, 1);
+    queryContext.results.splice(index, 1);
     this._notify("onQueryResultRemoved", index);
 
     PlacesUtils.history.remove(selectedResult.payload.url).catch(Cu.reportError);

browser/components/urlbar/UrlbarInput.jsm

@@ -1270,10 +1270,17 @@ class UrlbarInput {
     // additional cleanup on blur.
     this._clearActionOverride();
     this.formatValue();
+
+    // The extension input sessions depends more on blur than on the fact we
+    // actually cancel a running query, so we do it here.
+    if (ExtensionSearchHandler.hasActiveInputSession()) {
+      ExtensionSearchHandler.handleInputCancelled();
+    }
+
     // Respect the autohide preference for easier inspecting/debugging via
     // the browser toolbox.
     if (!UrlbarPrefs.get("ui.popup.disable_autohide")) {
-      this.view.close(UrlbarUtils.CANCEL_REASON.BLUR);
+      this.view.close();
     }
     // We may have hidden popup notifications, show them again if necessary.
     if (this.getAttribute("pageproxystate") != "valid") {

browser/components/urlbar/UrlbarUtils.jsm

@@ -121,12 +121,6 @@ var UrlbarUtils = {
     CANCELED: 4,
   },
 
-  // This defines possible reasons for canceling a query.
-  CANCEL_REASON: {
-    // 1 is intentionally left in case we want a none/undefined/other later.
-    BLUR: 2,
-  },
-
   // Limit the length of titles and URLs we display so layout doesn't spend too
   // much time building text runs.
   MAX_TEXT_LENGTH: 255,

browser/components/urlbar/UrlbarView.jsm

@@ -180,28 +180,29 @@ class UrlbarView {
 
   /**
    * Closes the autocomplete popup, cancelling the query if necessary.
-   *
-   * @param {UrlbarUtils.CANCEL_REASON} [cancelReason]
-   *   Indicates if this close is being triggered as a result of a user action
-   *   which would cancel a query, e.g. on blur.
    */
-  close(cancelReason) {
-    this.controller.cancelQuery(cancelReason);
+  close() {
+    this.controller.cancelQuery();
     this.panel.hidePopup();
   }
 
   // UrlbarController listener methods.
   onQueryStarted(queryContext) {
+    this._queryWasCancelled = false;
     this._startRemoveStaleRowsTimer();
   }
 
   onQueryCancelled(queryContext) {
+    this._queryWasCancelled = true;
     this._cancelRemoveStaleRowsTimer();
   }
 
   onQueryFinished(queryContext) {
     this._cancelRemoveStaleRowsTimer();
-    this._removeStaleRows();
+    // If the query has not been canceled, remove stale rows immediately.
+    if (!this._queryWasCancelled) {
+      this._removeStaleRows();
+    }
   }
 
   onQueryResults(queryContext) {
@@ -753,6 +754,7 @@ class UrlbarView {
     this.window.removeEventListener("resize", this);
     this.removeAccessibleFocus();
     this.input.inputField.setAttribute("aria-expanded", "false");
+    this._rows.textContent = "";
   }
 
   _on_resize() {

browser/components/urlbar/tests/UrlbarTestUtils.jsm

@@ -456,21 +456,16 @@ class UrlbarAbstraction {
     }
     let details = {};
     if (this.quantumbar) {
-      let context = await this.urlbar.lastQueryContextPromise;
-      if (index >= context.results.length) {
-        throw new Error("Requested index not found in results");
-      }
-      let {url, postData} = UrlbarUtils.getUrlFromResult(context.results[index]);
+      let result = element.result;
+      let {url, postData} = UrlbarUtils.getUrlFromResult(result);
       details.url = url;
       details.postData = postData;
-      details.type = context.results[index].type;
-      details.heuristic = context.results[index].heuristic;
-      details.autofill = !!context.results[index].autofill;
+      details.type = result.type;
+      details.heuristic = result.heuristic;
+      details.autofill = !!result.autofill;
       details.image = element.getElementsByClassName("urlbarView-favicon")[0].src;
-      details.title = context.results[index].title;
-      details.tags = "tags" in context.results[index].payload ?
-        context.results[index].payload.tags :
-        [];
+      details.title = result.title;
+      details.tags = "tags" in result.payload ? result.payload.tags : [];
       let actions = element.getElementsByClassName("urlbarView-action");
       let urls = element.getElementsByClassName("urlbarView-url");
       let typeIcon = element.querySelector(".urlbarView-type-icon");
@@ -490,13 +485,13 @@ class UrlbarAbstraction {
       };
       if (details.type == UrlbarUtils.RESULT_TYPE.SEARCH) {
         details.searchParams = {
-          engine: context.results[index].payload.engine,
-          keyword: context.results[index].payload.keyword,
-          query: context.results[index].payload.query,
-          suggestion: context.results[index].payload.suggestion,
+          engine: result.payload.engine,
+          keyword: result.payload.keyword,
+          query: result.payload.query,
+          suggestion: result.payload.suggestion,
         };
       } else if (details.type == UrlbarUtils.RESULT_TYPE.KEYWORD) {
-        details.keyword = context.results[index].payload.keyword;
+        details.keyword = result.payload.keyword;
       }
     } else {
       details.url = this.urlbar.controller.getFinalCompleteValueAt(index);

browser/components/urlbar/tests/browser/browser_urlbarOneOffs.js

@@ -198,7 +198,7 @@ add_task(async function oneOffClick() {
   // stricter. Even if it looks like a url, we should search.
   let typedValue = "foo.bar";
   await promiseAutocompleteResultPopup(typedValue);
-  await waitForAutocompleteResultAt(1);
+  await UrlbarTestUtils.getDetailsOfResultAt(window, 0);
   assertState(0, -1, typedValue);
 
   let oneOffs = oneOffSearchButtons.getSelectableButtons(true);
@@ -219,7 +219,7 @@ add_task(async function oneOffReturn() {
   // stricter. Even if it looks like a url, we should search.
   let typedValue = "foo.bar";
   await promiseAutocompleteResultPopup(typedValue, window, true);
-  await waitForAutocompleteResultAt(1);
+  await UrlbarTestUtils.getDetailsOfResultAt(window, 0);
   assertState(0, -1, typedValue);
 
   // Alt+Down to select the first one-off.
@@ -246,7 +246,7 @@ add_task(async function collapsedOneOffs() {
 
   let typedValue = "foo";
   await promiseAutocompleteResultPopup(typedValue, window, true);
-  await waitForAutocompleteResultAt(0);
+  await UrlbarTestUtils.getDetailsOfResultAt(window, 0);
   assertState(0, -1);
   Assert.ok(oneOffSearchButtons.buttons.collapsed,
     "The one-off buttons should be collapsed");
@@ -255,26 +255,24 @@ add_task(async function collapsedOneOffs() {
   await hidePopup();
 });
 
-
 // The one-offs should be hidden when searching with an "@engine" search engine
 // alias.
 add_task(async function hiddenWhenUsingSearchAlias() {
   let typedValue = "@example";
   await promiseAutocompleteResultPopup(typedValue, window, true);
-  await waitForAutocompleteResultAt(0);
+  await UrlbarTestUtils.getDetailsOfResultAt(window, 0);
   Assert.equal(UrlbarTestUtils.getOneOffSearchButtonsVisible(window), false,
     "Should not be showing the one-off buttons");
   await hidePopup();
 
   typedValue = "not an engine alias";
   await promiseAutocompleteResultPopup(typedValue, window, true);
-  await waitForAutocompleteResultAt(0);
+  await UrlbarTestUtils.getDetailsOfResultAt(window, 0);
   Assert.equal(UrlbarTestUtils.getOneOffSearchButtonsVisible(window), true,
     "Should be showing the one-off buttons");
   await hidePopup();
 });
 
-
 function assertState(result, oneOff, textValue = undefined) {
   Assert.equal(UrlbarTestUtils.getSelectedIndex(window), result,
     "Expected result should be selected");

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

@@ -68,7 +68,7 @@ add_task(async function test_cancel_search() {
   let startedPromise = promiseControllerNotification(controller, "onQueryStarted");
   let cancelPromise = promiseControllerNotification(controller, "onQueryCancelled");
 
-  await controller.startQuery(context);
+  controller.startQuery(context);
 
   let params = await startedPromise;
 

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

@@ -39,16 +39,25 @@ class DelayedProvider extends UrlbarProvider {
   async startQuery(context, add) {
     Assert.ok(context, "context is passed-in");
     Assert.equal(typeof add, "function", "add is a callback");
-    this._context = context;
     this._add = add;
+    await new Promise(resolve => {
+      this._resultsAdded = resolve;
+    });
   }
   cancelQuery(context) {
-    Assert.ok(false, "cancelQuery should not be called");
+    // Nothing.
   }
-  addResults(matches) {
+  async addResults(matches, finish = true) {
+    // startQuery may have not been invoked yet, so wait for it
+    await TestUtils.waitForCondition(() => !!this._add,
+                                     "Waiting for the _add callback");
     for (const match of matches) {
       this._add(this, match);
     }
+    if (finish) {
+      this._add = null;
+      this._resultsAdded();
+    }
   }
 }
 
@@ -93,7 +102,7 @@ add_task(async function test_n_autocomplete_cancel() {
   Assert.ok(!TelemetryStopwatch.running(TELEMETRY_6_FIRST_RESULTS, context),
     "Should not have started first 6 results stopwatch");
 
-  await controller.startQuery(context);
+  controller.startQuery(context);
 
   Assert.ok(TelemetryStopwatch.running(TELEMETRY_1ST_RESULT, context),
     "Should have started first result stopwatch");
@@ -130,14 +139,14 @@ add_task(async function test_n_autocomplete_results() {
   Assert.ok(!TelemetryStopwatch.running(TELEMETRY_6_FIRST_RESULTS, context),
     "Should not have started first 6 results stopwatch");
 
-  await controller.startQuery(context);
+  controller.startQuery(context);
 
   Assert.ok(TelemetryStopwatch.running(TELEMETRY_1ST_RESULT, context),
     "Should have started first result stopwatch");
   Assert.ok(TelemetryStopwatch.running(TELEMETRY_6_FIRST_RESULTS, context),
     "Should have started first 6 results stopwatch");
 
-  provider.addResults([MATCH]);
+  await provider.addResults([MATCH], false);
   await resultsPromise;
 
   Assert.ok(!TelemetryStopwatch.running(TELEMETRY_1ST_RESULT, context),
@@ -155,11 +164,11 @@ add_task(async function test_n_autocomplete_results() {
   // Now add 5 more results, so that the first 6 results is triggered.
   for (let i = 0; i < 5; i++) {
     resultsPromise = promiseControllerNotification(controller, "onQueryResults");
-    provider.addResults([
+    await provider.addResults([
       new UrlbarResult(UrlbarUtils.RESULT_TYPE.TAB_SWITCH,
                        UrlbarUtils.RESULT_SOURCE.TABS,
                        { url: TEST_URL + "/i" }),
-    ]);
+    ], false);
     await resultsPromise;
   }
 
@@ -177,7 +186,7 @@ add_task(async function test_n_autocomplete_results() {
 
   // Add one more, to check neither are updated.
   resultsPromise = promiseControllerNotification(controller, "onQueryResults");
-  provider.addResults([
+  await provider.addResults([
     new UrlbarResult(UrlbarUtils.RESULT_TYPE.TAB_SWITCH,
                      UrlbarUtils.RESULT_SOURCE.TABS,
                      { url: TEST_URL + "/6" }),

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

@@ -149,11 +149,11 @@ add_task(function test_handle_query_starts_search() {
   sandbox.resetHistory();
 });
 
-add_task(function test_handle_query_starts_search_sets_allowAutofill() {
+add_task(async function test_handle_query_starts_search_sets_allowAutofill() {
   let originalValue = Services.prefs.getBoolPref("browser.urlbar.autoFill");
   Services.prefs.setBoolPref("browser.urlbar.autoFill", !originalValue);
 
-  controller.startQuery(createContext());
+  await controller.startQuery(createContext());
 
   Assert.equal(fPM.startQuery.callCount, 1,
     "Should have called startQuery once");
@@ -172,9 +172,6 @@ add_task(function test_handle_query_starts_search_sets_allowAutofill() {
 });
 
 add_task(function test_cancel_query() {
-  // Ensure the controller doesn't have any previous queries.
-  delete controller._lastQueryContext;
-
   const context = createContext();
   controller.startQuery(context);
 
@@ -205,3 +202,44 @@ add_task(function test_receiveResults() {
 
   sandbox.resetHistory();
 });
+
+add_task(async function test_notifications_order() {
+  // Clear any pending notifications.
+  const context = createContext();
+  await controller.startQuery(context);
+
+  // Check that when multiple queries are executed, the notifications arrive
+  // in the proper order.
+  let collectingListener = new Proxy({}, {
+    _notifications: [],
+    get(target, name) {
+      if (name == "notifications") {
+        return this._notifications;
+      }
+      return () => {
+        this._notifications.push(name);
+      };
+    },
+  });
+  controller.addQueryListener(collectingListener);
+  controller.startQuery(context);
+  Assert.deepEqual(["onQueryStarted"], collectingListener.notifications,
+                   "Check onQueryStarted is fired synchronously");
+  controller.startQuery(context);
+  Assert.deepEqual(["onQueryStarted", "onQueryCancelled", "onQueryFinished",
+                    "onQueryStarted"],
+                   collectingListener.notifications,
+                   "Check order of notifications");
+  controller.cancelQuery();
+  Assert.deepEqual(["onQueryStarted", "onQueryCancelled", "onQueryFinished",
+                    "onQueryStarted", "onQueryCancelled", "onQueryFinished"],
+                   collectingListener.notifications,
+                   "Check order of notifications");
+  await controller.startQuery(context);
+  controller.cancelQuery();
+  Assert.deepEqual(["onQueryStarted", "onQueryCancelled", "onQueryFinished",
+                    "onQueryStarted", "onQueryCancelled", "onQueryFinished",
+                    "onQueryStarted", "onQueryFinished"],
+                   collectingListener.notifications,
+                   "Check order of notifications");
+});

browser/docs/AddressBar.rst

@@ -154,52 +154,52 @@ implementation details may vary deeply among different providers.
 .. highlight:: JavaScript
 .. code::
 
-class UrlbarProvider {
-  /**
-   * Unique name for the provider, used by the context to filter on providers.
-   * Not using a unique name will cause the newest registration to win.
-   * @abstract
-   */
-  get name() {
-    return "UrlbarProviderBase";
+  class UrlbarProvider {
+    /**
+    * Unique name for the provider, used by the context to filter on providers.
+    * Not using a unique name will cause the newest registration to win.
+    * @abstract
+    */
+    get name() {
+      return "UrlbarProviderBase";
+    }
+    /**
+    * The type of the provider, must be one of UrlbarUtils.PROVIDER_TYPE.
+    * @abstract
+    */
+    get type() {
+      throw new Error("Trying to access the base class, must be overridden");
+    }
+    /**
+    * List of UrlbarUtils.RESULT_SOURCE, representing the data sources used by
+    * the provider.
+    * @abstract
+    */
+    get sources() {
+      throw new Error("Trying to access the base class, must be overridden");
+    }
+    /**
+    * Starts querying.
+    * @param {UrlbarQueryContext} queryContext The query context object
+    * @param {function} addCallback Callback invoked by the provider to add a new
+    *        result. A UrlbarResult should be passed to it.
+    * @note Extended classes should return a Promise resolved when the provider
+    *       is done searching AND returning results.
+    * @abstract
+    */
+    startQuery(queryContext, addCallback) {
+      throw new Error("Trying to access the base class, must be overridden");
+    }
+    /**
+    * Cancels a running query,
+    * @param {UrlbarQueryContext} queryContext The query context object to cancel
+    *        query for.
+    * @abstract
+    */
+    cancelQuery(queryContext) {
+      throw new Error("Trying to access the base class, must be overridden");
+    }
   }
-  /**
-   * The type of the provider, must be one of UrlbarUtils.PROVIDER_TYPE.
-   * @abstract
-   */
-  get type() {
-    throw new Error("Trying to access the base class, must be overridden");
-  }
-  /**
-   * List of UrlbarUtils.RESULT_SOURCE, representing the data sources used by
-   * the provider.
-   * @abstract
-   */
-  get sources() {
-    throw new Error("Trying to access the base class, must be overridden");
-  }
-  /**
-   * Starts querying.
-   * @param {UrlbarQueryContext} queryContext The query context object
-   * @param {function} addCallback Callback invoked by the provider to add a new
-   *        result. A UrlbarResult should be passed to it.
-   * @note Extended classes should return a Promise resolved when the provider
-   *       is done searching AND returning results.
-   * @abstract
-   */
-  startQuery(queryContext, addCallback) {
-    throw new Error("Trying to access the base class, must be overridden");
-  }
-  /**
-   * Cancels a running query,
-   * @param {UrlbarQueryContext} queryContext The query context object to cancel
-   *        query for.
-   * @abstract
-   */
-  cancelQuery(queryContext) {
-    throw new Error("Trying to access the base class, must be overridden");
-  }
-}
 
 UrlbarMuxer
 -----------
@@ -216,24 +216,24 @@ indicated by the UrlbarQueryContext.muxer property.
 .. highlight:: JavaScript
 .. code::
 
-class UrlbarMuxer {
-  /**
-   * Unique name for the muxer, used by the context to sort results.
-   * Not using a unique name will cause the newest registration to win.
-   * @abstract
-   */
-  get name() {
-    return "UrlbarMuxerBase";
+  class UrlbarMuxer {
+    /**
+    * Unique name for the muxer, used by the context to sort results.
+    * Not using a unique name will cause the newest registration to win.
+    * @abstract
+    */
+    get name() {
+      return "UrlbarMuxerBase";
+    }
+    /**
+    * Sorts UrlbarQueryContext results in-place.
+    * @param {UrlbarQueryContext} queryContext the context to sort results for.
+    * @abstract
+    */
+    sort(queryContext) {
+      throw new Error("Trying to access the base class, must be overridden");
+    }
   }
-  /**
-   * Sorts UrlbarQueryContext results in-place.
-   * @param {UrlbarQueryContext} queryContext the context to sort results for.
-   * @abstract
-   */
-  sort(queryContext) {
-    throw new Error("Trying to access the base class, must be overridden");
-  }
-}
 
 
 The Controller
@@ -345,7 +345,8 @@ Represents the base *View* implementation, communicates with the *Controller*.
     onQueryResults(queryContext);
     // Invoked when the query has been canceled.
     onQueryCancelled(queryContext);
-    // Invoked when the query is done.
+    // Invoked when the query is done. This is invoked in any case, even if the
+    // query was canceled earlier.
     onQueryFinished(queryContext);
     // Invoked when the view context changed, so that cached information about
     // the latest search is no more relevant and can be dropped.