Backed out changeset 986f4f6398a4 (bug 1816572) for causing failures at browser_search_bookmarks.js. CLOSED TREE

browser/components/search/SearchOneOffs.sys.mjs

@@ -398,7 +398,6 @@ export class SearchOneOffs {
       console.error("Search-one-offs::_rebuild() error:", ex);
     } finally {
       this._rebuilding = false;
-      this.dispatchEvent(new Event("rebuild"));
     }
   }
 
@@ -471,6 +470,8 @@ export class SearchOneOffs {
 
     let engines = (await this.getEngineInfo()).engines;
     this._rebuildEngineList(engines, addEngines);
+
+    this.dispatchEvent(new Event("rebuild"));
   }
 
   /**

browser/components/urlbar/UrlbarEventBufferer.sys.mjs

@@ -18,6 +18,10 @@ XPCOMUtils.defineLazyGetter(lazy, "logger", () =>
   lazy.UrlbarUtils.getLogger({ prefix: "EventBufferer" })
 );
 
+// Maximum time events can be deferred for. In automation providers can be quite
+// slow, thus we need a longer timeout to avoid intermittent failures.
+const DEFERRING_TIMEOUT_MS = Cu.isInAutomation ? 1000 : 300;
+
 // Array of keyCodes to defer.
 const DEFERRED_KEY_CODES = new Set([
   KeyboardEvent.DOM_VK_RETURN,
@@ -48,12 +52,6 @@ const QUERY_STATUS = {
  * until more results arrive, at which time they're replayed.
  */
 export class UrlbarEventBufferer {
-  // Maximum time events can be deferred for. In automation providers can be
-  // quite slow, thus we need a longer timeout to avoid intermittent failures.
-  // Note: to avoid handling events too early, this timer should be larger than
-  // UrlbarProvidersManager.CHUNK_HEURISTIC_RESULTS_DELAY_MS.
-  static DEFERRING_TIMEOUT_MS = Cu.isInAutomation ? 1000 : 300;
-
   /**
    * Initialises the class.
    *
@@ -185,7 +183,7 @@ export class UrlbarEventBufferer {
 
     if (!this._deferringTimeout) {
       let elapsed = Cu.now() - this._lastQuery.startDate;
-      let remaining = UrlbarEventBufferer.DEFERRING_TIMEOUT_MS - elapsed;
+      let remaining = DEFERRING_TIMEOUT_MS - elapsed;
       this._deferringTimeout = lazy.setTimeout(() => {
         this.replayDeferredEvents(false);
         this._deferringTimeout = null;
@@ -262,10 +260,7 @@ export class UrlbarEventBufferer {
 
     // This is an event that we'd defer, but if enough time has passed since the
     // start of the search, we don't want to block the user's workflow anymore.
-    if (
-      this._lastQuery.startDate + UrlbarEventBufferer.DEFERRING_TIMEOUT_MS <=
-      Cu.now()
-    ) {
+    if (this._lastQuery.startDate + DEFERRING_TIMEOUT_MS <= Cu.now()) {
       return false;
     }
 

browser/components/urlbar/UrlbarProvidersManager.sys.mjs

@@ -83,6 +83,11 @@ var localMuxerModules = {
     "resource:///modules/UrlbarMuxerUnifiedComplete.sys.mjs",
 };
 
+// To improve dataflow and reduce UI work, when a result is added by a
+// non-heuristic provider, we notify it to the controller after a delay, so
+// that we can chunk results coming in that timeframe into a single call.
+const CHUNK_RESULTS_DELAY_MS = 16;
+
 const DEFAULT_MUXER = "UnifiedComplete";
 
 /**
@@ -114,15 +119,8 @@ class ProvidersManager {
       this.registerMuxer(muxer);
     }
 
-    // These can be set by tests to increase or reduce the chunk delays.
-    // See _notifyResultsFromProvider for additional details.
-    // To improve dataflow and reduce UI work, when a result is added we may notify
-    // it to the controller after a delay, so that we can chunk results in that
-    // timeframe into a single call. See _notifyResultsFromProvider for details.
-    // Note: to avoid handling events too early, the heuristic timer should be
-    // smaller than UrlbarEventBufferer.DEFERRING_TIMEOUT_MS.
-    this.CHUNK_HEURISTIC_RESULTS_DELAY_MS = 200;
-    this.CHUNK_OTHER_RESULTS_DELAY_MS = 16;
+    // This is defined as a property so tests can override it.
+    this._chunkResultsDelayMs = CHUNK_RESULTS_DELAY_MS;
   }
 
   /**
@@ -510,7 +508,12 @@ class Query {
 
     // All the providers are done returning results, so we can stop chunking.
     if (!this.canceled) {
-      await this._chunkTimer?.fire();
+      if (this._heuristicProviderTimer) {
+        await this._heuristicProviderTimer.fire();
+      }
+      if (this._chunkTimer) {
+        await this._chunkTimer.fire();
+      }
     }
 
     // Break cycles with the controller to avoid leaks.
@@ -534,8 +537,15 @@ class Query {
       provider.queryInstance = null;
       provider.tryMethod("cancelQuery", this.context);
     }
-    this._chunkTimer?.cancel().catch(ex => lazy.logger.error(ex));
-    this._sleepTimer?.fire().catch(ex => lazy.logger.error(ex));
+    if (this._heuristicProviderTimer) {
+      this._heuristicProviderTimer.cancel().catch(ex => lazy.logger.error(ex));
+    }
+    if (this._chunkTimer) {
+      this._chunkTimer.cancel().catch(ex => lazy.logger.error(ex));
+    }
+    if (this._sleepTimer) {
+      this._sleepTimer.fire().catch(ex => lazy.logger.error(ex));
+    }
   }
 
   /**
@@ -607,46 +617,53 @@ class Query {
   }
 
   _notifyResultsFromProvider(provider) {
-    // We use a timer to reduce UI flicker, by adding results in chunks.
-    if (!this._chunkTimer) {
-      // This is the first time we see a result, so we start a longer
-      // "heuristic" timeout. This timeout is longer, because we don't want to
-      // surprise the user with an unexpected default action.
-      // Since heuristic providers return results pretty quickly, this timer
-      // will often be skipped early.
-      // Note that if the heuristic timer elapses, we may still cause an
-      // imperfect default action, but that's still better than looking stale.
+    // We create two chunking timers: one for heuristic results, and one for
+    // other results. We expect heuristic providers to return their heuristic
+    // results before other results/providers in most cases. When all heuristic
+    // providers have returned some results, we fire the heuristic timer early.
+    // If the timer fires first, we stop waiting on the remaining heuristic
+    // providers.
+    // Both timers are used to reduce UI flicker.
+    if (provider.type == lazy.UrlbarUtils.PROVIDER_TYPE.HEURISTIC) {
+      if (!this._heuristicProviderTimer) {
+        this._heuristicProviderTimer = new lazy.SkippableTimer({
+          name: "Heuristic provider timer",
+          callback: () => this._notifyResults(),
+          time: UrlbarProvidersManager._chunkResultsDelayMs,
+          logger: provider.logger,
+        });
+      }
+    } else if (!this._chunkTimer) {
       this._chunkTimer = new lazy.SkippableTimer({
-        name: "heuristic",
+        name: "Query chunk timer",
         callback: () => this._notifyResults(),
-        time: UrlbarProvidersManager.CHUNK_HEURISTIC_RESULTS_DELAY_MS,
+        time: UrlbarProvidersManager._chunkResultsDelayMs,
         logger: provider.logger,
       });
-    } else if (this._chunkTimer.done) {
-      // The previous timer is done, but we're still getting results. Start a
-      // new shorter timer to chunk remaining results.
-      this._chunkTimer = new lazy.SkippableTimer({
-        name: "chunking",
-        callback: () => this._notifyResults(),
-        time: UrlbarProvidersManager.CHUNK_OTHER_RESULTS_DELAY_MS,
-        logger: provider.logger,
-      });
-    } else if (
-      this._chunkTimer.name == "heuristic" &&
-      !this._chunkTimer.done &&
+    }
+    // If all active heuristic providers have returned results, we can skip the
+    // heuristic results timer and start showing results immediately.
+    if (
+      this._heuristicProviderTimer &&
       !this.context.pendingHeuristicProviders.size
     ) {
-      // All the active heuristic providers have returned results, we can skip
-      // the heuristic chunk timer and start showing results immediately.
-      this._chunkTimer.fire().catch(ex => lazy.logger.error(ex));
+      this._heuristicProviderTimer.fire().catch(ex => lazy.logger.error(ex));
     }
-
-    // Otherwise some timer is still ongoing and we'll wait for it.
   }
 
   _notifyResults() {
     this.muxer.sort(this.context, this.unsortedResults);
 
+    if (this._heuristicProviderTimer) {
+      this._heuristicProviderTimer.cancel().catch(ex => lazy.logger.error(ex));
+      this._heuristicProviderTimer = null;
+    }
+
+    if (this._chunkTimer) {
+      this._chunkTimer.cancel().catch(ex => lazy.logger.error(ex));
+      this._chunkTimer = null;
+    }
+
     // We don't want to notify consumers if there are no results since they
     // generally expect at least one result when notified, so bail, but only
     // after nulling out the chunk timer above so that it will be restarted

browser/components/urlbar/UrlbarUtils.sys.mjs

@@ -2413,11 +2413,6 @@ export class UrlbarProvider {
  *   await timer.promise;
  */
 export class SkippableTimer {
-  /**
-   * This can be used to track whether the timer completed.
-   */
-  done = false;
-
   /**
    * Creates a skippable timer for the given callback and time.
    *
@@ -2468,7 +2463,6 @@ export class SkippableTimer {
     });
 
     this.promise = Promise.race([timerPromise, firePromise]).then(() => {
-      this.done = true;
       // If we've been canceled, don't call back.
       if (callback && !this._canceled) {
         callback();

browser/components/urlbar/tests/UrlbarTestUtils.sys.mjs

@@ -101,17 +101,6 @@ export var UrlbarTestUtils = {
       // Search mode may start a second query.
       context = await waitForQuery();
     }
-    if (win.gURLBar.view.oneOffSearchButtons._rebuilding) {
-      await new Promise(resolve =>
-        win.gURLBar.view.oneOffSearchButtons.addEventListener(
-          "rebuild",
-          resolve,
-          {
-            once: true,
-          }
-        )
-      );
-    }
     return context;
   },
 
@@ -1418,7 +1407,6 @@ class TestProvider extends UrlbarProvider {
    * @param {number} [options.addTimeout]
    *   If non-zero, each result will be added on this timeout.  If zero, all
    *   results will be added immediately and synchronously.
-   *   If there's no results, the query will be completed after this timeout.
    * @param {Function} [options.onCancel]
    *   If given, a function that will be called when the provider's cancelQuery
    *   method is called.
@@ -1461,9 +1449,6 @@ class TestProvider extends UrlbarProvider {
     return true;
   }
   async startQuery(context, addCallback) {
-    if (!this._results.length && this._addTimeout) {
-      await new Promise(resolve => lazy.setTimeout(resolve, this._addTimeout));
-    }
     for (let result of this._results) {
       if (!this._addTimeout) {
         addCallback(this, result);

browser/components/urlbar/tests/browser/browser_selectStaleResults.js

@@ -16,8 +16,10 @@ add_setup(async function () {
     set: [["browser.urlbar.suggest.quickactions", false]],
   });
 
-  // We'll later replace this, so ensure it's restored.
+  // Increase the timeout of the remove-stale-rows timer so that it doesn't
+  // interfere with the tests.
   let originalRemoveStaleRowsTimeout = UrlbarView.removeStaleRowsTimeout;
+  UrlbarView.removeStaleRowsTimeout = 1000;
   registerCleanupFunction(() => {
     UrlbarView.removeStaleRowsTimeout = originalRemoveStaleRowsTimeout;
   });
@@ -26,22 +28,6 @@ add_setup(async function () {
 // This tests the case where queryContext.results.length < the number of rows in
 // the view, i.e., the view contains stale rows.
 add_task(async function viewContainsStaleRows() {
-  // Set the remove-stale-rows timer to a very large value, so there's no
-  // possibility it interferes with this test.
-  UrlbarView.removeStaleRowsTimeout = 10000;
-
-  // For the test stability we need a slow provider that ensures the search
-  // doesn't complete too fast.
-  let slowProvider = new UrlbarTestUtils.TestProvider({
-    results: [],
-    name: "emptySlowProvider",
-    addTimeout: 1000,
-  });
-  UrlbarProvidersManager.registerProvider(slowProvider);
-  registerCleanupFunction(() => {
-    UrlbarProvidersManager.unregisterProvider(slowProvider);
-  });
-
   await PlacesUtils.history.clear();
   await PlacesUtils.bookmarks.eraseEverything();
 
@@ -77,22 +63,29 @@ add_task(async function viewContainsStaleRows() {
   });
 
   // Below we'll do a search for "xx".  Get the row that will show the last
-  // result in that search, and await for it to be updated.
-  Assert.ok(
-    !UrlbarTestUtils.getRowAt(window, halfResults).hasAttribute("stale"),
-    "Should not be stale"
-  );
+  // result in that search.
+  let row = UrlbarTestUtils.getRowAt(window, halfResults);
 
-  let lastMatchingResultUpdatedPromise = TestUtils.waitForCondition(() => {
-    let row = UrlbarTestUtils.getRowAt(window, halfResults);
-    console.log(row.result.title);
-    return row.result.title.startsWith("xx");
-  }, "Wait for the result to be updated");
+  // Add a mutation listener on that row.  Wait for its "stale" attribute to be
+  // removed.
+  let mutationPromise = new Promise(resolve => {
+    let observer = new MutationObserver(mutations => {
+      for (let mut of mutations) {
+        if (mut.attributeName == "stale" && !row.hasAttribute("stale")) {
+          observer.disconnect();
+          resolve();
+          break;
+        }
+      }
+    });
+    observer.observe(row, { attributes: true });
+  });
 
   // Type another "x" so that we search for "xx", but don't wait for the search
-  // to finish.  Instead, wait for the row to be updated.
+  // to finish.  Instead, wait for the row's stale attribute to be removed.
   EventUtils.synthesizeKey("x");
-  await lastMatchingResultUpdatedPromise;
+  info("Waiting for 'stale' attribute to be removed... ");
+  await mutationPromise;
 
   // Now arrow down.  The search, which is still ongoing, will now stop and the
   // view won't be updated anymore.
@@ -102,16 +95,6 @@ add_task(async function viewContainsStaleRows() {
   info("Waiting for the search to stop... ");
   await gURLBar.lastQueryContextPromise;
 
-  // Check stale status of results.
-  Assert.ok(
-    !UrlbarTestUtils.getRowAt(window, halfResults).hasAttribute("stale"),
-    "Should not be stale"
-  );
-  Assert.ok(
-    UrlbarTestUtils.getRowAt(window, halfResults + 1).hasAttribute("stale"),
-    "Should be stale"
-  );
-
   // The query context for the last search ("xx") should contain only
   // halfResults + 1 results (+ 1 for the heuristic).
   Assert.ok(gURLBar.controller._lastQueryContextWrapper);
@@ -148,7 +131,6 @@ add_task(async function viewContainsStaleRows() {
   await UrlbarTestUtils.promisePopupClose(window, () =>
     EventUtils.synthesizeKey("KEY_Escape")
   );
-  UrlbarProvidersManager.unregisterProvider(slowProvider);
 });
 
 // This tests the case where, before the search finishes, stale results have
@@ -172,8 +154,8 @@ add_task(async function staleReplacedWithFresh() {
   //
   // NB: If this test ends up failing, it may be because the remove-stale-rows
   // timer fires before the history results are added.  i.e., steps 2 and 3
-  // above happen out of order.  If that happens, try increasing it.
-  UrlbarView.removeStaleRowsTimeout = 1000;
+  // above happen out of order.  If that happens, try increasing
+  // UrlbarView.removeStaleRowsTimeout above.
 
   await PlacesUtils.history.clear();
   await PlacesUtils.bookmarks.eraseEverything();

browser/components/urlbar/tests/engagementTelemetry/browser/browser_glean_telemetry_engagement_edge_cases.js

@@ -85,13 +85,10 @@ add_task(async function engagement_before_showing_results() {
     set: [["browser.urlbar.tipShownCount.searchTip_onboard", 999]],
   });
 
-  // Increase chunk delays to delay the call to notifyResults.
-  let originalHeuristicTimeout =
-    UrlbarProvidersManager.CHUNK_HEURISTIC_RESULTS_DELAY_MS;
-  UrlbarProvidersManager.CHUNK_HEURISTIC_RESULTS_DELAY_MS = 1000000;
-  let originalOtherTimeout =
-    UrlbarProvidersManager.CHUNK_OTHER_RESULTS_DELAY_MS;
-  UrlbarProvidersManager.CHUNK_OTHER_RESULTS_DELAY_MS = 1000000;
+  // Update chunkResultsDelayMs to delay the call to notifyResults.
+  const originalChuldResultDelayMs =
+    UrlbarProvidersManager._chunkResultsDelayMs;
+  UrlbarProvidersManager._chunkResultsDelayMs = 1000000;
 
   // Add a provider that waits forever in startQuery() to avoid fireing
   // heuristicProviderTimer.
@@ -103,9 +100,7 @@ add_task(async function engagement_before_showing_results() {
   const cleanup = () => {
     UrlbarProvidersManager.unregisterProvider(noResponseProvider);
     UrlbarProvidersManager.unregisterProvider(anotherHeuristicProvider);
-    UrlbarProvidersManager.CHUNK_HEURISTIC_RESULTS_DELAY_MS =
-      originalHeuristicTimeout;
-    UrlbarProvidersManager.CHUNK_OTHER_RESULTS_DELAY_MS = originalOtherTimeout;
+    UrlbarProvidersManager._chunkResultsDelayMs = originalChuldResultDelayMs;
   };
   registerCleanupFunction(cleanup);
 

browser/components/urlbar/tests/quicksuggest/browser/browser_telemetry_impressionEdgeCases.js

@@ -385,18 +385,11 @@ async function doEngagementWithoutAddingResultToView(
   // Set the timeout of the chunk timer to a really high value so that it will
   // not fire. The view updates when the timer fires, which we specifically want
   // to avoid here.
-  let originalHeuristicTimeout =
-    UrlbarProvidersManager.CHUNK_HEURISTIC_RESULTS_DELAY_MS;
-  UrlbarProvidersManager.CHUNK_HEURISTIC_RESULTS_DELAY_MS = 30000;
-  let originalOtherTimeout =
-    UrlbarProvidersManager.CHUNK_OTHER_RESULTS_DELAY_MS;
-  UrlbarProvidersManager.CHUNK_OTHER_RESULTS_DELAY_MS = 30000;
-  const cleanup = () => {
-    UrlbarProvidersManager.CHUNK_HEURISTIC_RESULTS_DELAY_MS =
-      originalHeuristicTimeout;
-    UrlbarProvidersManager.CHUNK_OTHER_RESULTS_DELAY_MS = originalOtherTimeout;
-  };
-  registerCleanupFunction(cleanup);
+  let originalChunkDelayMs = UrlbarProvidersManager._chunkResultsDelayMs;
+  UrlbarProvidersManager._chunkResultsDelayMs = 30000;
+  registerCleanupFunction(() => {
+    UrlbarProvidersManager._chunkResultsDelayMs = originalChunkDelayMs;
+  });
 
   // Stub `UrlbarProviderQuickSuggest.getPriority()` to return Infinity.
   let sandbox = sinon.createSandbox();
@@ -490,7 +483,7 @@ async function doEngagementWithoutAddingResultToView(
   // Clean up.
   resolveQuery();
   UrlbarProvidersManager.unregisterProvider(provider);
-  cleanup();
+  UrlbarProvidersManager._chunkResultsDelayMs = originalChunkDelayMs;
   sandboxCleanup();
 }
 

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

@@ -1,77 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-/**
- * Tests that a slow heuristic provider returns results normally if it reacts
- * before the longer heuristic chunk timer elapses.
- */
-
-add_task(async function test_chunking_delays() {
-  let { UrlbarEventBufferer } = ChromeUtils.importESModule(
-    "resource:///modules/UrlbarEventBufferer.sys.mjs"
-  );
-  Assert.greater(
-    UrlbarEventBufferer.DEFERRING_TIMEOUT_MS,
-    UrlbarProvidersManager.CHUNK_HEURISTIC_RESULTS_DELAY_MS,
-    "The deferring timeout must be larger than the heuristic chunk timer"
-  );
-});
-
-add_task(async function test() {
-  // Must be between CHUNK_RESULTS_DELAY_MS and CHUNK_HEURISTIC_RESULTS_DELAY_MS.
-  let timeout = 150;
-  Assert.greater(timeout, UrlbarProvidersManager.CHUNK_OTHER_RESULTS_DELAY_MS);
-  Assert.greater(
-    UrlbarProvidersManager.CHUNK_HEURISTIC_RESULTS_DELAY_MS,
-    timeout
-  );
-  let provider1 = new UrlbarTestUtils.TestProvider({
-    results: [
-      makeVisitResult(createContext("test"), {
-        uri: "https://www.example.com/test",
-        title: "test visit for https://www.example.com/test",
-        heuristic: true,
-      }),
-    ],
-    name: "TestHeuristic",
-    type: UrlbarUtils.PROVIDER_TYPE.HEURISTIC,
-    addTimeout: timeout,
-  });
-  UrlbarProvidersManager.registerProvider(provider1);
-  let provider2 = new UrlbarTestUtils.TestProvider({
-    results: [
-      makeVisitResult(createContext("test"), {
-        uri: "https://www.example.com/test2",
-        title: "test visit for https://www.example.com/test2",
-      }),
-    ],
-    name: "TestNormal",
-    type: UrlbarUtils.PROVIDER_TYPE.PROFILE,
-  });
-  UrlbarProvidersManager.registerProvider(provider2);
-  registerCleanupFunction(() => {
-    UrlbarProvidersManager.unregisterProvider(provider1);
-    UrlbarProvidersManager.unregisterProvider(provider2);
-  });
-  let controller = UrlbarTestUtils.newMockController();
-  let promiseResultsNotified = new Promise(resolve => {
-    let listener = {
-      onQueryResults(queryContext) {
-        controller.removeQueryListener(listener);
-        resolve(queryContext.results);
-      },
-    };
-    controller.addQueryListener(listener);
-  });
-  let context = createContext("test", {
-    providers: [provider1.name, provider2.name],
-  });
-  await controller.startQuery(context);
-  let results = await promiseResultsNotified;
-  info(
-    "Results:\n" + results.map(m => `${m.title} - ${m.payload.url}`).join("\n")
-  );
-  Assert.equal(results.length, 2, "Found the expected number of results");
-  Assert.ok(context.results[0].heuristic);
-  Assert.equal(context.results[0].payload.url, "https://www.example.com/test");
-});

browser/components/urlbar/tests/unit/xpcshell.ini

@@ -74,7 +74,6 @@ skip-if = !sync
 [test_search_suggestions.js]
 [test_search_suggestions_aliases.js]
 [test_search_suggestions_tail.js]
-[test_slow_heuristic.js]
 [test_special_search.js]
 [test_suggestedIndex.js]
 [test_suggestedIndexRelativeToGroup.js]