Bug 1680931 - Fix inspector highlighter race conditions. r=rcaliman

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

devtools/client/inspector/shared/highlighters-overlay.js

@@ -114,6 +114,10 @@ class HighlightersOverlay {
     // It will fully replace this.highlighters when all highlighter consumers are updated
     // to use it as the single source of truth for which highlighters are visible.
     this._activeHighlighters = new Map();
+    // Map of highlighter types to symbols. Showing highlighters is an async operation,
+    // until it doesn't complete, this map will be populated with the requested type and
+    // a unique symbol identifying that request. Once completed, the entry is removed.
+    this._pendingHighlighters = new Map();
     // Collection of instantiated highlighter actors like FlexboxHighlighter,
     // ShapesHighlighter and GeometryEditorHighlighter.
     this.highlighters = {};
@@ -369,17 +373,35 @@ class HighlightersOverlay {
    * @return {Promise}
    */
   async showHighlighterTypeForNode(type, nodeFront, options) {
-    const skipShow = await this._beforeShowHighlighterTypeForNode(
+    const promise = this._beforeShowHighlighterTypeForNode(
       type,
       nodeFront,
       options
     );
 
-    if (skipShow) {
+    // Set a pending highlighter in order to detect if, while we were awaiting, there was
+    // a more recent request to highlight a node with the same type, or a request to hide
+    // the highlighter. Then we will abort this one in favor of the newer one.
+    // This needs to be done before the 'await' in order to be synchronous, but after
+    // calling _beforeShowHighlighterTypeForNode, since it can call hideHighlighterType.
+    const id = Symbol();
+    this._pendingHighlighters.set(type, id);
+    const skipShow = await promise;
+
+    if (this._pendingHighlighters.get(type) !== id) {
+      return;
+    } else if (skipShow) {
+      this._pendingHighlighters.delete(type);
       return;
     }
 
     const highlighter = await this._getHighlighterTypeForNode(type, nodeFront);
+
+    if (this._pendingHighlighters.get(type) !== id) {
+      return;
+    }
+    this._pendingHighlighters.delete(type);
+
     // Set a timer to automatically hide the highlighter if a duration is provided.
     const timer = this.scheduleAutoHideHighlighterType(type, options?.duration);
     // TODO: support case for multiple highlighter instances (ex: multiple grids)
@@ -431,6 +453,10 @@ class HighlightersOverlay {
    * @return {Promise}
    */
   async hideHighlighterType(type) {
+    if (this._pendingHighlighters.has(type)) {
+      // Abort pending highlighters for the given type.
+      this._pendingHighlighters.delete(type);
+    }
     if (!this._activeHighlighters.has(type)) {
       return;
     }
@@ -1696,6 +1722,7 @@ class HighlightersOverlay {
     }
 
     this._activeHighlighters.clear();
+    this._pendingHighlighters.clear();
     this.gridHighlighters.clear();
     this.parentGridHighlighters.clear();
     this.subgridToParentMap.clear();
@@ -1754,6 +1781,7 @@ class HighlightersOverlay {
     }
 
     this._activeHighlighters.clear();
+    this._pendingHighlighters.clear();
 
     for (const type in this.highlighters) {
       if (this.highlighters[type]) {

devtools/client/inspector/test/browser.ini

@@ -85,6 +85,7 @@ skip-if = fission && (bits == 64 && (os == "win" || os == "linux")) # Bug 166548
 [browser_inspector_highlighter-05.js]
 [browser_inspector_highlighter-06.js]
 [browser_inspector_highlighter-07.js]
+[browser_inspector_highlighter-08.js]
 [browser_inspector_highlighter-autohide-config_01.js]
 [browser_inspector_highlighter-autohide-config_02.js]
 [browser_inspector_highlighter-autohide-config_03.js]

devtools/client/inspector/test/browser_inspector_highlighter-08.js

@@ -0,0 +1,65 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+// Test that performing multiple requests to highlight nodes or to hide the highlighter,
+// without waiting for the former ones to complete, still works well.
+add_task(async function() {
+  info("Loading the test document and opening the inspector");
+  const { inspector, testActor } = await openInspectorForURL("data:text/html,");
+  const html = await getNodeFront("html", inspector);
+  const body = await getNodeFront("body", inspector);
+  const type = inspector.highlighters.TYPES.BOXMODEL;
+  const getActiveHighlighter = () => {
+    return inspector.highlighters.getActiveHighlighter(type);
+  };
+  is(getActiveHighlighter(), null, "The highlighter is hidden by default");
+
+  info("Highlight <body>, and hide the highlighter immediately after");
+  await Promise.all([
+    inspector.highlighters.showHighlighterTypeForNode(type, body),
+    inspector.highlighters.hideHighlighterType(type),
+  ]);
+  is(getActiveHighlighter(), null, "The highlighter is hidden");
+
+  info("Highlight <body>, then <html>, then <body> again, synchronously");
+  await Promise.all([
+    inspector.highlighters.showHighlighterTypeForNode(type, body),
+    inspector.highlighters.showHighlighterTypeForNode(type, html),
+    inspector.highlighters.showHighlighterTypeForNode(type, body),
+  ]);
+  ok(
+    await testActor.assertHighlightedNode("body"),
+    "The highlighter highlights <body>"
+  );
+
+  info("Highlight <html>, then <body>, then <html> again, synchronously");
+  await Promise.all([
+    inspector.highlighters.showHighlighterTypeForNode(type, html),
+    inspector.highlighters.showHighlighterTypeForNode(type, body),
+    inspector.highlighters.showHighlighterTypeForNode(type, html),
+  ]);
+  ok(
+    await testActor.assertHighlightedNode("html"),
+    "The highlighter highlights <html>"
+  );
+
+  info("Hide the highlighter, and highlight <html> immediately after");
+  await Promise.all([
+    inspector.highlighters.hideHighlighterType(type),
+    inspector.highlighters.showHighlighterTypeForNode(type, body),
+  ]);
+  ok(
+    await testActor.assertHighlightedNode("body"),
+    "The highlighter highlights <body>"
+  );
+
+  info("Highlight <html>, and hide the highlighter immediately after");
+  await Promise.all([
+    inspector.highlighters.showHighlighterTypeForNode(type, html),
+    inspector.highlighters.hideHighlighterType(type),
+  ]);
+  is(getActiveHighlighter(), null, "The highlighter is hidden");
+});

devtools/client/inspector/test/browser_inspector_highlighter-autohide-config_03.js

@@ -28,14 +28,14 @@ add_task(async function() {
   );
 
   info("Show Box Model Highlighter, then hide after half a second");
-  inspector.highlighters.showHighlighterTypeForNode(
+  await inspector.highlighters.showHighlighterTypeForNode(
     inspector.highlighters.TYPES.BOXMODEL,
     nodeFront,
     { duration: HALF_SECOND }
   );
 
   info("Show Box Model Highlighter again, then hide after one second");
-  inspector.highlighters.showHighlighterTypeForNode(
+  await inspector.highlighters.showHighlighterTypeForNode(
     inspector.highlighters.TYPES.BOXMODEL,
     nodeFront,
     { duration: ONE_SECOND }
@@ -47,7 +47,9 @@ add_task(async function() {
   /*
   Since the second duration passed is longer than the first and is supposed to overwrite
   the first, it is reasonable to expect that the "highlighter-hidden" event was emitted
-  after the second (longer) duration expired. As an added check, we naively wait for an additional time amounting to the sum of both durations to check if the first timer was somehow not overwritten and fires another "highlighter-hidden" event.
+  after the second (longer) duration expired. As an added check, we naively wait for an
+  additional time amounting to the sum of both durations to check if the first timer was
+  somehow not overwritten and fires another "highlighter-hidden" event.
    */
   let wasEmitted = false;
   const waitForExtraEvent = new Promise((resolve, reject) => {