Bug 1327725 - ensure the pagehide event we received are for the window's highlighter; r=gl
authorMatteo Ferretti <mferretti@mozilla.com>
Sun, 12 Mar 2017 13:06:00 +0100
changeset 395492 b1b91f7861ec0e8b03135192c0e0024d76ae5930
parent 395491 f152ca619a019f1b848d6f15493e84d9aca42137
child 395493 7b3260b7e95a9a3a56ea5d6f58971c9d3aeb0e95
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgl
bugs1327725
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
@@ -139,17 +139,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);
 }
@@ -327,17 +327,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);
   },
 
@@ -416,16 +418,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
@@ -473,17 +473,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();
   },