Bug 1863797 - Frequent failures in browser_schemeless.js due to UrlbarView.selectedElement removed from DOM. r=adw

#selectedElement may end up pointing to disconnected nodes. And so the public
.selectedElement getter.
This is how it was happening: a first call to onQueryResults adds and selects a
heuristic result. Then a second call to onQueryResults brings a new heuristic
result that requires new content (not compatible with the previous one), so the
old heuristic is emptied out, and new DOM is generated.
Because the code in onQueryResults relies on .selectedElement, at the second
invokation it thinks the selection is still valid, and doesn't select the new
heuristic. In reality .selectedElement at that time is pointing to a removed
DOM node.
The patch introduces a #rawSelectedElement and converts #selectedElement
into a getter.

Plus some minor logging improvements, and removing unused #mainContainer property.

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

browser/components/urlbar/UrlbarController.sys.mjs

@@ -340,6 +340,7 @@ export class UrlbarController {
         }
       // Fall through, we want the SPACE key to activate this element.
       case KeyEvent.DOM_VK_RETURN:
+        this.logger.debug(`Enter pressed${executeAction ? "" : " delayed"}`);
         if (executeAction) {
           this.input.handleCommand(event);
         }

browser/components/urlbar/UrlbarInput.sys.mjs

@@ -284,6 +284,10 @@ export class UrlbarInput {
 
     this.editor.newlineHandling =
       Ci.nsIEditor.eNewlinesStripSurroundingWhitespace;
+
+    ChromeUtils.defineLazyGetter(this, "logger", () =>
+      lazy.UrlbarUtils.getLogger({ prefix: "Input" })
+    );
   }
 
   /**
@@ -881,6 +885,9 @@ export class UrlbarInput {
    */
   pickElement(element, event) {
     let result = this.view.getResultFromElement(element);
+    this.logger.debug(
+      `pickElement ${element} with event ${event?.type}, result: ${result}`
+    );
     if (!result) {
       return;
     }
@@ -3284,6 +3291,7 @@ export class UrlbarInput {
   }
 
   _on_blur(event) {
+    this.logger.debug("Blur Event");
     // We cannot count every blur events after a missed engagement as abandoment
     // because the user may have clicked on some view element that executes
     // a command causing a focus change. For example opening preferences from
@@ -3399,6 +3407,7 @@ export class UrlbarInput {
   }
 
   _on_focus(event) {
+    this.logger.debug("Focus Event");
     if (!this._hideFocus) {
       this.setAttribute("focused", "true");
     }

browser/components/urlbar/UrlbarView.sys.mjs

@@ -77,7 +77,6 @@ export class UrlbarView {
     this.document = this.panel.ownerDocument;
     this.window = this.document.defaultView;
 
-    this.#mainContainer = this.panel.querySelector(".urlbarView-body-inner");
     this.#rows = this.panel.querySelector(".urlbarView-results");
     this.resultMenu = this.panel.querySelector(".urlbarView-result-menu");
     this.#resultMenuCommands = new WeakMap();
@@ -770,7 +769,7 @@ export class UrlbarView {
       );
     }
 
-    if (!this.selectedElement && !this.oneOffSearchButtons.selectedButton) {
+    if (!this.#selectedElement && !this.oneOffSearchButtons.selectedButton) {
       if (firstResult.heuristic) {
         // Select the heuristic result.  The heuristic may not be the first
         // result added, which is why we do this check here when each result is
@@ -820,10 +819,10 @@ export class UrlbarView {
 
     // If we update the selected element, a new unique ID is generated for it.
     // We need to ensure that aria-activedescendant reflects this new ID.
-    if (this.selectedElement && !this.oneOffSearchButtons.selectedButton) {
+    if (this.#selectedElement && !this.oneOffSearchButtons.selectedButton) {
       let aadID = this.input.inputField.getAttribute("aria-activedescendant");
       if (aadID && !this.document.getElementById(aadID)) {
-        this.#setAccessibleFocus(this.selectedElement);
+        this.#setAccessibleFocus(this.#selectedElement);
       }
     }
 
@@ -866,13 +865,13 @@ export class UrlbarView {
       return;
     }
 
+    let updateSelection = rowToRemove == this.#getSelectedRow();
     rowToRemove.remove();
     this.#updateIndices();
 
-    if (rowToRemove != this.#getSelectedRow()) {
+    if (!updateSelection) {
       return;
     }
-
     // Select the row at the same index, if possible.
     let newSelectionIndex = index;
     if (index >= this.#queryContext.results.length) {
@@ -1040,7 +1039,6 @@ export class UrlbarView {
   #blobUrlsByResultUrl = null;
   #inputWidthOnLastClose = 0;
   #l10nCache;
-  #mainContainer;
   #mousedownSelectedElement;
   #openPanelInstance;
   #oneOffSearchButtons;
@@ -1052,9 +1050,22 @@ export class UrlbarView {
   #resultMenuResult;
   #resultMenuCommands;
   #rows;
-  #selectedElement;
+  #rawSelectedElement;
   #zeroPrefixStopwatchInstance = null;
 
+  /**
+   * #rawSelectedElement may be disconnected from the DOM (e.g. it was remove()d)
+   * but we want a connected #selectedElement usually. We don't use a WeakRef
+   * because it would depend too much on GC timing.
+   *
+   * @returns {DOMElement} the selected element.
+   */
+  get #selectedElement() {
+    return this.#rawSelectedElement?.isConnected
+      ? this.#rawSelectedElement
+      : null;
+  }
+
   #createElement(name) {
     return this.document.createElementNS("http://www.w3.org/1999/xhtml", name);
   }
@@ -1622,7 +1633,8 @@ export class UrlbarView {
       !lazy.ObjectUtils.deepEqual(
         oldResult.payload.buttons,
         result.payload.buttons
-      );
+      ) ||
+      result.testForceNewContent;
 
     if (needsNewContent) {
       while (item.lastChild) {
@@ -2364,7 +2376,7 @@ export class UrlbarView {
     }
 
     this.#setAccessibleFocus(setAccessibleFocus && element);
-    this.#selectedElement = element;
+    this.#rawSelectedElement = element;
 
     if (updateInput) {
       let urlOverride = null;

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

@@ -1478,6 +1478,8 @@ class TestProvider extends UrlbarProvider {
    *   {@link UrlbarView.#selectElement} method is called.
    * @param {Function} [options.onEngagement]
    *   If given, a function that will be called when engagement.
+   * @param {Function} [options.delayResultsPromise]
+   *   If given, we'll await on this before returning results.
    */
   constructor({
     results,
@@ -1488,7 +1490,13 @@ class TestProvider extends UrlbarProvider {
     onCancel = null,
     onSelection = null,
     onEngagement = null,
+    delayResultsPromise = null,
   } = {}) {
+    if (delayResultsPromise && addTimeout) {
+      throw new Error(
+        "Can't provide both `addTimeout` and `delayResultsPromise`"
+      );
+    }
     super();
     this._results = results;
     this._name = name;
@@ -1498,6 +1506,12 @@ class TestProvider extends UrlbarProvider {
     this._onCancel = onCancel;
     this._onSelection = onSelection;
     this._onEngagement = onEngagement;
+    this._delayResultsPromise = delayResultsPromise;
+    // As this has been a common source of mistakes, auto-upgrade the provider
+    // type to heuristic if any result is heuristic.
+    if (!type && this._results?.some(r => r.heuristic)) {
+      this._type = UrlbarUtils.PROVIDER_TYPE.HEURISTIC;
+    }
   }
   get name() {
     return this._name;
@@ -1515,6 +1529,9 @@ class TestProvider extends UrlbarProvider {
     if (!this._results.length && this._addTimeout) {
       await new Promise(resolve => lazy.setTimeout(resolve, this._addTimeout));
     }
+    if (this._delayResultsPromise) {
+      await this._delayResultsPromise;
+    }
     for (let result of this._results) {
       if (!this._addTimeout) {
         addCallback(this, result);

browser/components/urlbar/tests/browser/browser.toml

@@ -666,6 +666,8 @@ support-files = ["file_userTypedValue.html"]
 
 ["browser_view_emptyResultSet.js"]
 
+["browser_view_removedSelectedElement.js"]
+
 ["browser_view_resultDisplay.js"]
 
 ["browser_view_resultTypes_display.js"]

browser/components/urlbar/tests/browser/browser_view_removedSelectedElement.js

@@ -0,0 +1,87 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Tests that if the selectedElement is removed from the DOM, the view still
+// sets a selection on the next received results.
+
+add_task(async function () {
+  let view = gURLBar.view;
+  // We need a heuristic provider that the Muxer will prefer over other
+  // heuristics and that will return results after the first onQueryResults.
+  // Luckily TEST providers come first in the heuristic group!
+  let result = new UrlbarResult(
+    UrlbarUtils.RESULT_TYPE.URL,
+    UrlbarUtils.RESULT_SOURCE.OTHER_LOCAL,
+    { url: "https://example.com/1", title: "example" }
+  );
+  result.heuristic = true;
+  // To ensure the selectedElement is removed, we use this special property that
+  // asks the view to generate new content for the row.
+  result.testForceNewContent = true;
+
+  let receivedResults = false;
+  let firstSelectedElement;
+  let delayResultsPromise = new Promise(resolve => {
+    gURLBar.controller.addQueryListener({
+      async onQueryResults(queryContext) {
+        Assert.ok(!receivedResults, "Should execute only once");
+        gURLBar.controller.removeQueryListener(this);
+        receivedResults = true;
+        // Store the corrent selection.
+        firstSelectedElement = view.selectedElement;
+        Assert.ok(firstSelectedElement, "There should be a selected element");
+        Assert.ok(
+          view.selectedResult.heuristic,
+          "Selected result should be a heuristic"
+        );
+        Assert.notEqual(
+          result,
+          view.selectedResult,
+          "Should not immediately select our result"
+        );
+        resolve();
+      },
+    });
+  });
+
+  let delayedHeuristicProvider = new UrlbarTestUtils.TestProvider({
+    delayResultsPromise,
+    results: [result],
+    type: UrlbarUtils.PROVIDER_TYPE.HEURISTIC,
+  });
+  UrlbarProvidersManager.registerProvider(delayedHeuristicProvider);
+  registerCleanupFunction(async function () {
+    UrlbarProvidersManager.unregisterProvider(delayedHeuristicProvider);
+    await UrlbarTestUtils.promisePopupClose(window);
+    gURLBar.handleRevert();
+  });
+
+  await UrlbarTestUtils.promiseAutocompleteResultPopup({
+    window,
+    value: "exa",
+  });
+  Assert.ok(receivedResults, "Results observer was invoked");
+  Assert.ok(
+    UrlbarTestUtils.getResultCount(window) > 0,
+    `There should be some results in the view.`
+  );
+  Assert.ok(view.isOpen, `The view should be open.`);
+  Assert.ok(view.selectedElement.isConnected, "selectedElement is connected");
+  Assert.equal(view.selectedElementIndex, 0, "selectedElementIndex is correct");
+  Assert.deepEqual(
+    view.getResultFromElement(view.selectedElement),
+    result,
+    "result is the expected one"
+  );
+  Assert.notEqual(
+    view.selectedElement,
+    firstSelectedElement,
+    "Selected element should have changed"
+  );
+  Assert.ok(
+    !firstSelectedElement.isConnected,
+    "Previous selected element should be disconnected"
+  );
+});

dom/security/test/https-first/browser.toml

@@ -42,9 +42,6 @@ support-files = [
 support-files = ["file_navigation.html"]
 
 ["browser_schemeless.js"]
-skip-if = [
-  "os == 'linux'", # Bug 1863797
-]
 
 ["browser_slow_download.js"]
 support-files = [

dom/security/test/https-first/browser_schemeless.js

@@ -29,7 +29,6 @@ async function runMainTest(aInput, aDesc, aExpectedScheme) {
       window,
       value: aInput,
     });
-    await TestUtils.waitForTick();
     EventUtils.synthesizeKey("KEY_Enter");
     await loaded;
 
@@ -48,7 +47,6 @@ async function runCanonizedTest(aInput, aDesc, aExpectedScheme) {
       window,
       value: aInput,
     });
-    await TestUtils.waitForTick();
     EventUtils.synthesizeKey("KEY_Enter", { ctrlKey: true });
     await loaded;
 
@@ -73,7 +71,6 @@ async function runNewTabTest(aInput, aDesc, aExpectedScheme) {
         window,
         value: aInput,
       });
-      await TestUtils.waitForTick();
       EventUtils.synthesizeKey("KEY_Enter", { altKey: true });
       const newTab = await newTabPromise;
 
@@ -101,7 +98,6 @@ async function runNewWindowTest(aInput, aDesc, aExpectedScheme) {
       window,
       value: aInput,
     });
-    await TestUtils.waitForTick();
     EventUtils.synthesizeKey("KEY_Enter", { shiftKey: true });
     const newWindow = await newWindowPromise;