Bug 1327725 - ensure the pagehide event we received are for the window's highlighter; r=gl draft
authorMatteo Ferretti <mferretti@mozilla.com>
Sun, 12 Mar 2017 13:06:00 +0100
changeset 497665 2af5a430f979f3551845b9d0528c11c7dfd51125
parent 497184 55dd9efeeb00c323edaf866f549e7278ed84678f
child 548931 a04ca9eca2c3ae13333737348d379cfda96072e5
push id48946
push userbmo:zer0@mozilla.com
push dateMon, 13 Mar 2017 15:29:24 +0000
reviewersgl
bugs1327725
milestone55.0a1
Bug 1327725 - ensure the pagehide event we received are for the window's highlighter; r=gl Some tests were failing since they navigate in the history of frames; and that was triggering the `onPageHide` for the highlighter. MozReview-Commit-ID: 7lGRTg4CerU
devtools/server/actors/highlighters.js
devtools/server/actors/highlighters/box-model.js
devtools/server/actors/highlighters/css-grid.js
devtools/server/actors/highlighters/eye-dropper.js
devtools/server/actors/highlighters/geometry-editor.js
devtools/server/actors/highlighters/measuring-tool.js
devtools/server/actors/highlighters/rulers.js
devtools/server/actors/highlighters/utils/markup.js
--- a/devtools/server/actors/highlighters.js
+++ b/devtools/server/actors/highlighters.js
@@ -1,15 +1,15 @@
 /* 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";
 
-const { Ci } = require("chrome");
+const { Ci, Cu } = require("chrome");
 
 const { XPCOMUtils } = require("resource://gre/modules/XPCOMUtils.jsm");
 const EventEmitter = require("devtools/shared/event-emitter");
 const events = require("sdk/event/core");
 const protocol = require("devtools/shared/protocol");
 const Services = require("Services");
 const { isWindowIncluded } = require("devtools/shared/layout/utils");
 const { highlighterSpec, customHighlighterSpec } = require("devtools/shared/specs/highlighters");
@@ -394,16 +394,21 @@ exports.HighlighterActor = protocol.Acto
     target.addEventListener("mouseup", this._preventContentEvent, true);
     target.addEventListener("dblclick", this._preventContentEvent, true);
     target.addEventListener("keydown", this._onKey, true);
     target.addEventListener("keyup", this._preventContentEvent, true);
   },
 
   _stopPickerListeners: function () {
     let target = this._highlighterEnv.pageListenerTarget;
+
+    if (!target) {
+      return;
+    }
+
     target.removeEventListener("mousemove", this._onHovered, true);
     target.removeEventListener("click", this._onPick, true);
     target.removeEventListener("mousedown", this._preventContentEvent, true);
     target.removeEventListener("mouseup", this._preventContentEvent, true);
     target.removeEventListener("dblclick", this._preventContentEvent, true);
     target.removeEventListener("keydown", this._onKey, true);
     target.removeEventListener("keyup", this._preventContentEvent, true);
   },
@@ -609,31 +614,35 @@ HighlighterEnvironment.prototype = {
     return isXUL(this.window);
   },
 
   get window() {
     if (!this.isInitialized) {
       throw new Error("Initialize HighlighterEnvironment with a tabActor " +
         "or window first");
     }
-    return this._tabActor ? this._tabActor.window : this._win;
+    let win = this._tabActor ? this._tabActor.window : this._win;
+
+    return Cu.isDeadWrapper(win) ? null : win;
   },
 
   get document() {
-    return this.window.document;
+    return this.window && this.window.document;
   },
 
   get docShell() {
-    return this.window.QueryInterface(Ci.nsIInterfaceRequestor)
+    return this.window &&
+           this.window.QueryInterface(Ci.nsIInterfaceRequestor)
                       .getInterface(Ci.nsIWebNavigation)
                       .QueryInterface(Ci.nsIDocShell);
   },
 
   get webProgress() {
-    return this.docShell.QueryInterface(Ci.nsIInterfaceRequestor)
+    return this.docShell &&
+           this.docShell.QueryInterface(Ci.nsIInterfaceRequestor)
                         .getInterface(Ci.nsIWebProgress);
   },
 
   /**
    * Get the right target for listening to events on the page.
    * - If the environment was initialized from a TabActor *and* if we're in the
    *   Browser Toolbox (to inspect firefox desktop): the tabActor is the
    *   RootActor, in which case, the window property can be used to listen to
@@ -644,17 +653,17 @@ HighlighterEnvironment.prototype = {
    *   events, even from nested iframes.
    * - If the environment was initialized from a window, we also use the
    *   chromeEventHandler.
    */
   get pageListenerTarget() {
     if (this._tabActor && this._tabActor.isRootActor) {
       return this.window;
     }
-    return this.docShell.chromeEventHandler;
+    return this.docShell && this.docShell.chromeEventHandler;
   },
 
   relayTabActorWindowReady: function (data) {
     this.emit("window-ready", data);
   },
 
   relayTabActorNavigate: function (data) {
     this.emit("navigate", data);
--- a/devtools/server/actors/highlighters/box-model.js
+++ b/devtools/server/actors/highlighters/box-model.js
@@ -99,19 +99,23 @@ function BoxModelHighlighter(highlighter
     this._buildMarkup.bind(this));
 
   /**
    * Optionally customize each region's fill color by adding an entry to the
    * regionFill property: `highlighter.regionFill.margin = "red";
    */
   this.regionFill = {};
 
+  this.onPageHide = this.onPageHide.bind(this);
   this.onWillNavigate = this.onWillNavigate.bind(this);
 
   this.highlighterEnv.on("will-navigate", this.onWillNavigate);
+
+  let { pageListenerTarget } = highlighterEnv;
+  pageListenerTarget.addEventListener("pagehide", this.onPageHide);
 }
 
 BoxModelHighlighter.prototype = extend(AutoRefreshHighlighter.prototype, {
   typeName: "BoxModelHighlighter",
 
   ID_CLASS_PREFIX: "box-model-",
 
   _buildMarkup: function () {
@@ -255,16 +259,22 @@ BoxModelHighlighter.prototype = extend(A
     return highlighterContainer;
   },
 
   /**
    * Destroy the nodes. Remove listeners.
    */
   destroy: function () {
     this.highlighterEnv.off("will-navigate", this.onWillNavigate);
+
+    let { pageListenerTarget } = this.highlighterEnv;
+    if (pageListenerTarget) {
+      pageListenerTarget.removeEventListener("pagehide", this.onPageHide);
+    }
+
     this.markup.destroy();
     AutoRefreshHighlighter.prototype.destroy.call(this);
   },
 
   getElement: function (id) {
     return this.markup.getElement(this.ID_CLASS_PREFIX + id);
   },
 
@@ -705,15 +715,23 @@ BoxModelHighlighter.prototype = extend(A
    */
   _moveInfobar: function () {
     let bounds = this._getOuterBounds();
     let container = this.getElement("infobar-container");
 
     moveInfobar(container, bounds, this.win);
   },
 
+  onPageHide: function ({ target }) {
+    // If a pagehide event is triggered for current window's highlighter, hide the
+    // highlighter.
+    if (target.defaultView === this.win) {
+      this.hide();
+    }
+  },
+
   onWillNavigate: function ({ isTopLevel }) {
     if (isTopLevel) {
       this.hide();
     }
   }
 });
 exports.BoxModelHighlighter = BoxModelHighlighter;
--- a/devtools/server/actors/highlighters/css-grid.js
+++ b/devtools/server/actors/highlighters/css-grid.js
@@ -143,17 +143,17 @@ function CssGridHighlighter(highlighterE
     width: 0,
     height: 0
   };
 
   this.markup = new CanvasFrameAnonymousContentHelper(this.highlighterEnv,
     this._buildMarkup.bind(this));
 
   this.onNavigate = this.onNavigate.bind(this);
-  this.onPageHide = this.hide.bind(this);
+  this.onPageHide = this.onPageHide.bind(this);
   this.onWillNavigate = this.onWillNavigate.bind(this);
 
   this.highlighterEnv.on("navigate", this.onNavigate);
   this.highlighterEnv.on("will-navigate", this.onWillNavigate);
 
   let { pageListenerTarget } = highlighterEnv;
   pageListenerTarget.addEventListener("pagehide", this.onPageHide);
 }
@@ -331,17 +331,19 @@ CssGridHighlighter.prototype = extend(Au
   },
 
   destroy() {
     let { highlighterEnv } = this;
     highlighterEnv.off("navigate", this.onNavigate);
     highlighterEnv.off("will-navigate", this.onWillNavigate);
 
     let { pageListenerTarget } = highlighterEnv;
-    pageListenerTarget.removeEventListener("pagehide", this.onPageHide);
+    if (pageListenerTarget) {
+      pageListenerTarget.removeEventListener("pagehide", this.onPageHide);
+    }
 
     this.markup.destroy();
 
     // Clear the pattern cache to avoid dead object exceptions (Bug 1342051).
     this._clearCache();
     AutoRefreshHighlighter.prototype.destroy.call(this);
   },
 
@@ -406,16 +408,24 @@ CssGridHighlighter.prototype = extend(Au
   /**
    * Called when the page navigates. Used to clear the cached gap patterns and avoid
    * using DeadWrapper objects as gap patterns the next time.
    */
   onNavigate() {
     this._clearCache();
   },
 
+  onPageHide: function ({ target }) {
+    // If a page hide event is triggered for current window's highlighter, hide the
+    // highlighter.
+    if (target.defaultView === this.win) {
+      this.hide();
+    }
+  },
+
   onWillNavigate({ isTopLevel }) {
     if (isTopLevel) {
       this.hide();
     }
   },
 
   _show() {
     if (Services.prefs.getBoolPref(CSS_GRID_ENABLED_PREF) && !this.isGrid()) {
--- a/devtools/server/actors/highlighters/eye-dropper.js
+++ b/devtools/server/actors/highlighters/eye-dropper.js
@@ -173,21 +173,24 @@ EyeDropper.prototype = {
   hide() {
     if (this.highlighterEnv.isXUL) {
       return;
     }
 
     this.pageImage = null;
 
     let {pageListenerTarget} = this.highlighterEnv;
-    pageListenerTarget.removeEventListener("mousemove", this);
-    pageListenerTarget.removeEventListener("click", this, true);
-    pageListenerTarget.removeEventListener("keydown", this);
-    pageListenerTarget.removeEventListener("DOMMouseScroll", this);
-    pageListenerTarget.removeEventListener("FullZoomChange", this);
+
+    if (pageListenerTarget) {
+      pageListenerTarget.removeEventListener("mousemove", this);
+      pageListenerTarget.removeEventListener("click", this, true);
+      pageListenerTarget.removeEventListener("keydown", this);
+      pageListenerTarget.removeEventListener("DOMMouseScroll", this);
+      pageListenerTarget.removeEventListener("FullZoomChange", this);
+    }
 
     this.getElement("root").setAttribute("hidden", "true");
     this.getElement("root").removeAttribute("drawn");
 
     this.emit("hidden");
   },
 
   prepareImageCapture() {
--- a/devtools/server/actors/highlighters/geometry-editor.js
+++ b/devtools/server/actors/highlighters/geometry-editor.js
@@ -367,38 +367,45 @@ GeometryEditorHighlighter.prototype = ex
     // Avoiding exceptions if `destroy` is called multiple times; and / or the
     // highlighter environment was already destroyed.
     if (!this.highlighterEnv) {
       return;
     }
 
     let { pageListenerTarget } = this.highlighterEnv;
 
-    DOM_EVENTS.forEach(type =>
-      pageListenerTarget.removeEventListener(type, this));
+    if (pageListenerTarget) {
+      DOM_EVENTS.forEach(type =>
+        pageListenerTarget.removeEventListener(type, this));
+    }
 
     AutoRefreshHighlighter.prototype.destroy.call(this);
 
     this.markup.destroy();
     this.definedProperties.clear();
     this.definedProperties = null;
     this.offsetParent = null;
   },
 
   handleEvent: function (event, id) {
     // No event handling if the highlighter is hidden
     if (this.getElement("root").hasAttribute("hidden")) {
       return;
     }
 
-    const { type, pageX, pageY } = event;
+    const { target, type, pageX, pageY } = event;
 
     switch (type) {
       case "pagehide":
-        this.destroy();
+        // If a page hide event is triggered for current window's highlighter, hide the
+        // highlighter.
+        if (target.defaultView === this.win) {
+          this.destroy();
+        }
+
         break;
       case "mousedown":
         // The mousedown event is intended only for the handler
         if (!id) {
           return;
         }
 
         let handlerSide = this.markup.getElement(id).getAttribute("data-side");
--- a/devtools/server/actors/highlighters/measuring-tool.js
+++ b/devtools/server/actors/highlighters/measuring-tool.js
@@ -196,22 +196,24 @@ MeasuringToolHighlighter.prototype = {
 
   destroy() {
     this.hide();
 
     this._cancelUpdate();
 
     let { pageListenerTarget } = this.env;
 
-    pageListenerTarget.removeEventListener("mousedown", this);
-    pageListenerTarget.removeEventListener("mousemove", this);
-    pageListenerTarget.removeEventListener("mouseup", this);
-    pageListenerTarget.removeEventListener("scroll", this);
-    pageListenerTarget.removeEventListener("pagehide", this);
-    pageListenerTarget.removeEventListener("mouseleave", this);
+    if (pageListenerTarget) {
+      pageListenerTarget.removeEventListener("mousedown", this);
+      pageListenerTarget.removeEventListener("mousemove", this);
+      pageListenerTarget.removeEventListener("mouseup", this);
+      pageListenerTarget.removeEventListener("scroll", this);
+      pageListenerTarget.removeEventListener("pagehide", this);
+      pageListenerTarget.removeEventListener("mouseleave", this);
+    }
 
     this.markup.destroy();
 
     events.emit(this, "destroy");
   },
 
   show() {
     setIgnoreLayoutChanges(true);
@@ -530,14 +532,18 @@ MeasuringToolHighlighter.prototype = {
         if (!this._isDragging) {
           this.hideLabel("position");
         }
         break;
       case "scroll":
         this.hideLabel("position");
         break;
       case "pagehide":
-        this.destroy();
+        // If a page hide event is triggered for current window's highlighter, hide the
+        // highlighter.
+        if (event.target.defaultView === this.env.window) {
+          this.destroy();
+        }
         break;
     }
   }
 };
 exports.MeasuringToolHighlighter = MeasuringToolHighlighter;
--- a/devtools/server/actors/highlighters/rulers.js
+++ b/devtools/server/actors/highlighters/rulers.js
@@ -196,17 +196,21 @@ RulersHighlighter.prototype = {
   },
 
   handleEvent: function (event) {
     switch (event.type) {
       case "scroll":
         this._onScroll(event);
         break;
       case "pagehide":
-        this.destroy();
+        // If a page hide event is triggered for current window's highlighter, hide the
+        // highlighter.
+        if (event.target.defaultView === this.env.window) {
+          this.destroy();
+        }
         break;
     }
   },
 
   _onScroll: function (event) {
     let prefix = this.ID_CLASS_PREFIX;
     let { scrollX, scrollY } = event.view;
 
@@ -260,18 +264,21 @@ RulersHighlighter.prototype = {
     this.markup.getElement(this.ID_CLASS_PREFIX + "root").setAttribute("style",
       `stroke-width:${strokeWidth};`);
   },
 
   destroy: function () {
     this.hide();
 
     let { pageListenerTarget } = this.env;
-    pageListenerTarget.removeEventListener("scroll", this);
-    pageListenerTarget.removeEventListener("pagehide", this);
+
+    if (pageListenerTarget) {
+      pageListenerTarget.removeEventListener("scroll", this);
+      pageListenerTarget.removeEventListener("pagehide", this);
+    }
 
     this.markup.destroy();
 
     events.emit(this, "destroy");
   },
 
   show: function () {
     this.markup.removeAttributeForElement(this.ID_CLASS_PREFIX + "elements",
--- a/devtools/server/actors/highlighters/utils/markup.js
+++ b/devtools/server/actors/highlighters/utils/markup.js
@@ -461,17 +461,17 @@ CanvasFrameAnonymousContentHelper.protot
           break;
         }
       }
       node = node.parentNode;
     }
   },
 
   _removeAllListeners: function () {
-    if (this.highlighterEnv) {
+    if (this.highlighterEnv && this.highlighterEnv.pageListenerTarget) {
       let target = this.highlighterEnv.pageListenerTarget;
       for (let [type] of this.listeners) {
         target.removeEventListener(type, this, true);
       }
     }
     this.listeners.clear();
   },