Bug 1358983 - Fix zombie highlighters. r=miker.

Due to how highlighters work, it requires the inspector to be initialized.
It can happen than the user will mouseenter/mouseout on an element that
calls highlight/unhighlight very quickly.
Since the hightlight can take some time, it might happen that the unhighlight
call is handled first, before the highlight call, meaning that we now have an
highlighter displayed, even though the user isn't hovering anything that
should cause this anymore.

This patch introduces a new toolbox function called `getHighlighter` that
returns an object with a `highlight and a `unhighlight` function.
We keep a reference to any possible pending `highlight` call so we can wait
for it to be done in `unhighlight`, before destroying it.

The console makes use of the new helper function, and a test is added to ensure
we don't have zombie highlighters anymore.

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

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

devtools/client/framework/toolbox.js

@@ -3021,6 +3021,54 @@ Toolbox.prototype = {
     return this._initInspector;
   },
 
+  /**
+   * An helper function that returns an object contain a highlighter and unhighlighter
+   * function.
+   *
+   * @param {Boolean} isGrip: Set to true if the `highlight` function is going to be
+   *                          called with a Grip (and not from a NodeFront).
+   * @returns {Object} an object of the following shape:
+   *   - {AsyncFunction} highlight: A function that will initialize the highlighter front
+   *                                and call highlighter.highlight with the provided node
+   *                                front (which will be retrieved from a grip, if
+   *                                `fromGrip` is true.)
+   *   - {AsyncFunction} unhighlight: A function that will unhighlight the node that is
+   *                                  currently highlighted. If the `highlight` function
+   *                                  isn't settled yet, it will wait until it's done and
+   *                                  then unhighlight to prevent zombie highlighters.
+   *
+   */
+  getHighlighter(fromGrip = false) {
+    let pendingHighlight;
+    return {
+      highlight: async (nodeFront, options) => {
+        pendingHighlight = (async () => {
+          await this.initInspector();
+          if (!this.highlighter) {
+            return null;
+          }
+
+          if (fromGrip) {
+            nodeFront = await this.walker.gripToNodeFront(nodeFront);
+          }
+
+          return this.highlighter.highlight(nodeFront, options);
+        })();
+        return pendingHighlight;
+      },
+      unhighlight: async (forceHide) => {
+        if (pendingHighlight) {
+          await pendingHighlight;
+          pendingHighlight = null;
+        }
+
+        return this.highlighter
+          ? this.highlighter.unhighlight(forceHide)
+          : null;
+      },
+    };
+  },
+
   _onNewSelectedNodeFront: function() {
     // Emit a "selection-changed" event when the toolbox.selection has been set
     // to a new node (or cleared). Currently used in the WebExtensions APIs (to

devtools/client/webconsole/test/mochitest/browser_webconsole_nodes_highlight.js

@@ -21,10 +21,16 @@ const HTML = `
 `;
 const TEST_URI = "data:text/html;charset=utf-8," + encodeURI(HTML);
 
+// Import helpers registering the test-actor in remote targets
+Services.scriptloader.loadSubScript("chrome://mochitests/content/browser/devtools/client/shared/test/test-actor-registry.js", this);
+
 add_task(async function() {
   const hud = await openNewTabAndConsole(TEST_URI);
   const toolbox = gDevTools.getToolbox(hud.target);
 
+  await registerTestActor(toolbox.target.client);
+  const testActor = await getTestActor(toolbox);
+
   await ContentTask.spawn(gBrowser.selectedBrowser, null, () => {
     content.wrappedJSObject.logNode("h1");
   });
@@ -33,24 +39,38 @@ add_task(async function() {
   const node = msg.querySelector(".objectBox-node");
   ok(node !== null, "Node was logged as expected");
   const view = node.ownerDocument.defaultView;
+  const nonHighlightEl = toolbox.doc.getElementById("toolbox-meatball-menu-button");
 
   info("Highlight the node by moving the cursor on it");
 
   // the inspector should be initialized first and then the node should
   // highlight after the hover effect.
-  const onNodeHighlight = toolbox.target.once("inspector")
+  let onNodeHighlight = toolbox.target.once("inspector")
     .then(inspector => inspector.highlighter.once("node-highlight"));
 
   EventUtils.synthesizeMouseAtCenter(node, {type: "mousemove"}, view);
 
   const nodeFront = await onNodeHighlight;
   is(nodeFront.displayName, "h1", "The correct node was highlighted");
+  isVisible = await testActor.isHighlighting();
+  ok(isVisible, "Highlighter is displayed");
 
   info("Unhighlight the node by moving away from the node");
-  const onNodeUnhighlight = toolbox.highlighter.once("node-unhighlight");
-  const btn = toolbox.doc.getElementById("toolbox-meatball-menu-button");
-  EventUtils.synthesizeMouseAtCenter(btn, {type: "mousemove"}, view);
+  let onNodeUnhighlight = toolbox.highlighter.once("node-unhighlight");
+  EventUtils.synthesizeMouseAtCenter(nonHighlightEl, {type: "mousemove"}, view);
 
   await onNodeUnhighlight;
   ok(true, "node-unhighlight event was fired when moving away from the node");
+
+  info("Check we don't have zombie highlighters when briefly hovering a node");
+  onNodeHighlight = toolbox.highlighter.once("node-highlight");
+  onNodeUnhighlight = toolbox.highlighter.once("node-unhighlight");
+  // Move hover the node and then right after move out.
+  EventUtils.synthesizeMouseAtCenter(node, {type: "mousemove"}, view);
+  EventUtils.synthesizeMouseAtCenter(nonHighlightEl, {type: "mousemove"}, view);
+  await Promise.all([onNodeHighlight, onNodeUnhighlight]);
+  ok(true, "The highlighter was removed");
+
+  isVisible = await testActor.isHighlighting();
+  is(isVisible, false, "The highlighter is not displayed anymore");
 });

devtools/client/webconsole/webconsole-wrapper.js

@@ -278,6 +278,8 @@ class WebConsoleWrapper {
         this.toolbox.threadClient.on("paused", this.dispatchPaused);
         this.toolbox.threadClient.on("progress", this.dispatchProgress);
 
+        const {highlight, unhighlight} = this.toolbox.getHighlighter(true);
+
         Object.assign(serviceContainer, {
           onViewSourceInDebugger: frame => {
             this.toolbox.viewSourceInDebugger(
@@ -318,19 +320,8 @@ class WebConsoleWrapper {
             });
           },
           sourceMapService: this.toolbox ? this.toolbox.sourceMapURLService : null,
-          highlightDomElement: async (grip, options = {}) => {
-            await this.toolbox.initInspector();
-            if (!this.toolbox.highlighter) {
-              return null;
-            }
-            const nodeFront = await this.toolbox.walker.gripToNodeFront(grip);
-            return this.toolbox.highlighter.highlight(nodeFront, options);
-          },
-          unHighlightDomElement: (forceHide = false) => {
-            return this.toolbox.highlighter
-              ? this.toolbox.highlighter.unhighlight(forceHide)
-              : null;
-          },
+          highlightDomElement: highlight,
+          unHighlightDomElement: unhighlight,
           openNodeInInspector: async (grip) => {
             await this.toolbox.initInspector();
             const onSelectInspector = this.toolbox.selectTool("inspector", "inspect_dom");