Bug 922125 - Destroy markup, rule and computed views on navigation and unload. r=miker

browser/devtools/inspector/inspector-panel.js

@@ -34,6 +34,9 @@ function InspectorPanel(iframeWindow, toolbox) {
   this.panelWin = iframeWindow;
   this.panelWin.inspector = this;
 
+  this._onBeforeNavigate = this._onBeforeNavigate.bind(this);
+  this._target.on("will-navigate", this._onBeforeNavigate);
+
   EventEmitter.decorate(this);
 }
 
@@ -144,6 +147,13 @@ InspectorPanel.prototype = {
     return deferred.promise;
   },
 
+  _onBeforeNavigate: function() {
+    this._defaultNode = null;
+    this.selection.setNodeFront(null);
+    this._destroyMarkup();
+    this.isDirty = false;
+  },
+
   _getWalker: function() {
     let inspector = this.target.inspector;
     return inspector.getWalker().then(walker => {
@@ -473,6 +483,8 @@ InspectorPanel.prototype = {
       this.browser = null;
     }
 
+    this.target.off("will-navigate", this._onBeforeNavigate);
+
     this.target.off("thread-paused", this.updateDebuggerPausedWarning);
     this.target.off("thread-resumed", this.updateDebuggerPausedWarning);
     this._toolbox.off("select", this.updateDebuggerPausedWarning);
@@ -735,15 +747,18 @@ InspectorPanel.prototype = {
    * Schedule a low-priority change event for things like paint
    * and resize.
    */
-  scheduleLayoutChange: function Inspector_scheduleLayoutChange()
+  scheduleLayoutChange: function Inspector_scheduleLayoutChange(event)
   {
-    if (this._timer) {
-      return null;
+    // Filter out non browser window resize events (i.e. triggered by iframes)
+    if (this.browser.contentWindow === event.target) {
+      if (this._timer) {
+        return null;
+      }
+      this._timer = this.panelWin.setTimeout(function() {
+        this.emit("layout-change");
+        this._timer = null;
+      }.bind(this), LAYOUT_CHANGE_TIMER);
     }
-    this._timer = this.panelWin.setTimeout(function() {
-      this.emit("layout-change");
-      this._timer = null;
-    }.bind(this), LAYOUT_CHANGE_TIMER);
   },
 
   /**

browser/devtools/inspector/test/browser.ini

@@ -44,3 +44,4 @@ support-files = head.js
 [browser_inspector_sidebarstate.js]
 [browser_inspector_bug_848731_reset_selection_on_delete.js]
 [browser_inspector_bug_848731_reset_selection_on_delete.html]
+[browser_inspector_bug_922125_destroy_on_navigate.js]

browser/devtools/inspector/test/browser_inspector_bug_922125_destroy_on_navigate.js

@@ -0,0 +1,75 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+let Toolbox = devtools.Toolbox;
+let TargetFactory = devtools.TargetFactory;
+
+function test() {
+  waitForExplicitFinish();
+
+  const URL_1 = "data:text/html;charset=UTF-8,<div id='one' style='color:red;'>ONE</div>";
+  const URL_2 = "data:text/html;charset=UTF-8,<div id='two' style='color:green;'>TWO</div>";
+
+  let inspector;
+
+  // open tab, load URL_1, and wait for load to finish
+  let tab = gBrowser.selectedTab = gBrowser.addTab();
+  let target = TargetFactory.forTab(gBrowser.selectedTab);
+  let browser = gBrowser.getBrowserForTab(tab);
+
+  function onPageOneLoad() {
+    browser.removeEventListener("load", onPageOneLoad, true);
+
+    gDevTools.showToolbox(target).then(aToolbox => {
+      return aToolbox.selectTool("inspector");
+    }).then(i => {
+      inspector = i;
+
+      // Verify we are on page one
+      let testNode = content.document.querySelector("#one");
+      ok(testNode, "We have the test node on page 1");
+
+      assertMarkupViewIsLoaded();
+
+      // Listen to will-navigate to check if the view is empty
+      target.on("will-navigate", () => {
+        info("Navigation to page 2 has started, the inspector should be empty");
+        assertMarkupViewIsEmpty();
+      });
+      inspector.once("markuploaded", () => {
+        info("Navigation to page 2 was done, the inspector should be back up");
+
+        // Verify we are on page one
+        let testNode = content.document.querySelector("#two");
+        ok(testNode, "We have the test node on page 2");
+
+        // On page 2 load, verify we have the right content
+        assertMarkupViewIsLoaded();
+        endTests();
+      });
+
+      // Navigate to page 2
+      browser.loadURI(URL_2);
+    });
+  }
+
+  // Navigate to page 1
+  browser.addEventListener("load", onPageOneLoad, true);
+  browser.loadURI(URL_1);
+
+  function assertMarkupViewIsLoaded() {
+    let markupViewBox = inspector.panelDoc.getElementById("markup-box");
+    is(markupViewBox.childNodes.length, 1, "The markup-view is loaded");
+  }
+
+  function assertMarkupViewIsEmpty() {
+    let markupViewBox = inspector.panelDoc.getElementById("markup-box");
+    is(markupViewBox.childNodes.length, 0, "The markup-view is unloaded");
+  }
+
+  function endTests() {
+    target = browser = tab = inspector = TargetFactory = Toolbox = null;
+    gBrowser.removeCurrentTab();
+    finish();
+  }
+}

browser/devtools/inspector/test/browser_inspector_changes.js

@@ -57,6 +57,10 @@ function test() {
     inspector.once("computed-view-refreshed", stylePanelAfterChange);
 
     testDiv.style.fontSize = "15px";
+
+    // FIXME: This shouldn't be needed but as long as we don't fix the bug
+    // where the rule/computed views are not updated when the selected node's
+    // styles change, it has to stay here
     inspector.emit("layout-change");
   }
 
@@ -77,6 +81,11 @@ function test() {
       inspector.once("computed-view-refreshed", stylePanelAfterSwitch);
       testDiv.style.fontSize = "20px";
       inspector.sidebar.select("computedview");
+
+      // FIXME: This shouldn't be needed but as long as we don't fix the bug
+      // where the rule/computed views are not updated when the selected node's
+      // styles change, it has to stay here
+      inspector.emit("layout-change");
     });
   }
 

browser/devtools/markupview/markup-view.js

@@ -75,7 +75,7 @@ function MarkupView(aInspector, aFrame, aControllerWindow) {
   this.undo = new UndoStack();
   this.undo.installController(aControllerWindow);
 
-  this._containers = new WeakMap();
+  this._containers = new Map();
 
   this._boundMutationObserver = this._mutationObserver.bind(this);
   this.walker.on("mutations", this._boundMutationObserver);
@@ -383,7 +383,6 @@ MarkupView.prototype = {
     return container;
   },
 
-
   /**
    * Mutation observer used for included nodes.
    */
@@ -805,6 +804,9 @@ MarkupView.prototype = {
 
     delete this._elt;
 
+    for ([key, container] of this._containers) {
+      container.destroy();
+    }
     delete this._containers;
   },
 
@@ -945,7 +947,8 @@ function MarkupContainer(aMarkupView, aNode) {
   // Appending the editor element and attaching event listeners
   this.tagLine.appendChild(this.editor.elt);
 
-  this.elt.addEventListener("mousedown", this._onMouseDown.bind(this), false);
+  this._onMouseDown = this._onMouseDown.bind(this);
+  this.elt.addEventListener("mousedown", this._onMouseDown, false);
 }
 
 MarkupContainer.prototype = {
@@ -1164,6 +1167,29 @@ MarkupContainer.prototype = {
     if (focusable) {
       focusable.focus();
     }
+  },
+
+  /**
+   * Get rid of event listeners and references, when the container is no longer
+   * needed
+   */
+  destroy: function() {
+    // Recursively destroy children containers
+    let firstChild;
+    while (firstChild = this.children.firstChild) {
+      firstChild.container.destroy();
+      this.children.removeChild(firstChild);
+    }
+
+    // Remove event listeners
+    this.elt.removeEventListener("dblclick", this._onToggle, false);
+    this.elt.removeEventListener("mouseover", this._onMouseOver, false);
+    this.elt.removeEventListener("mouseout", this._onMouseOut, false);
+    this.elt.removeEventListener("mousedown", this._onMouseDown, false);
+    this.expander.removeEventListener("click", this._onToggle, false);
+
+    // Destroy my editor
+    this.editor.destroy();
   }
 };
 
@@ -1183,7 +1209,8 @@ function RootContainer(aMarkupView, aNode) {
 RootContainer.prototype = {
   hasChildren: true,
   expanded: true,
-  update: function() {}
+  update: function() {},
+  destroy: function() {}
 };
 
 /**
@@ -1195,6 +1222,10 @@ function GenericEditor(aContainer, aNode) {
   this.elt.textContent = aNode.nodeName;
 }
 
+GenericEditor.prototype = {
+  destroy: function() {}
+};
+
 /**
  * Creates an editor for a DOCTYPE node.
  *
@@ -1210,6 +1241,10 @@ function DoctypeEditor(aContainer, aNode) {
      '>';
 }
 
+DoctypeEditor.prototype = {
+  destroy: function() {}
+};
+
 /**
  * Creates a simple text editor node, used for TEXT and COMMENT
  * nodes.
@@ -1284,7 +1319,9 @@ TextEditor.prototype = {
         }
       }).then(null, console.error);
     }
-  }
+  },
+
+  destroy: function() {}
 };
 
 /**
@@ -1591,7 +1628,9 @@ ElementEditor.prototype = {
         }
       });
     }).then(null, console.error);
-  }
+  },
+
+  destroy: function() {}
 };
 
 function nodeDocument(node) {

browser/devtools/styleinspector/computed-view.js

@@ -270,9 +270,16 @@ CssHtmlTree.prototype = {
    */
   highlight: function(aElement) {
     if (!aElement) {
+      this.viewedElement = null;
+      this.noResults.hidden = false;
+
       if (this._refreshProcess) {
         this._refreshProcess.cancel();
       }
+      // Hiding all properties
+      for (let propView of this.propertyViews) {
+        propView.refresh();
+      }
       return promise.resolve(undefined);
     }
 
@@ -281,8 +288,8 @@ CssHtmlTree.prototype = {
     }
 
     this.viewedElement = aElement;
-
     this.refreshSourceFilter();
+
     return this.refreshPanel();
   },
 
@@ -331,6 +338,10 @@ CssHtmlTree.prototype = {
    */
   refreshPanel: function CssHtmlTree_refreshPanel()
   {
+    if (!this.viewedElement) {
+      return promise.resolve();
+    }
+
     return promise.all([
       this._createPropertyViews(),
       this.pageStyle.getComputed(this.viewedElement, {
@@ -359,8 +370,6 @@ CssHtmlTree.prototype = {
       // Reset zebra striping.
       this._darkStripe = true;
 
-      let display = this.propertyContainer.style.display;
-
       let deferred = promise.defer();
       this._refreshProcess = new UpdateProcess(this.styleWindow, this.propertyViews, {
         onItem: (aPropView) => {
@@ -647,12 +656,16 @@ CssHtmlTree.prototype = {
     // The document in which we display the results (csshtmltree.xul).
     delete this.styleDocument;
 
+    for (let propView of this.propertyViews)  {
+      propView.destroy();
+    }
+
     // The element that we're inspecting, and the document that it comes from.
     delete this.propertyViews;
     delete this.styleWindow;
     delete this.styleDocument;
     delete this.styleInspector;
-  },
+  }
 };
 
 function PropertyInfo(aTree, aName) {
@@ -761,6 +774,10 @@ PropertyView.prototype = {
    */
   get visible()
   {
+    if (!this.tree.viewedElement) {
+      return false;
+    }
+
     if (!this.tree.includeBrowserStyles && !this.hasMatchedSelectors) {
       return false;
     }
@@ -809,15 +826,16 @@ PropertyView.prototype = {
   {
     let doc = this.tree.styleDocument;
 
+    this.onMatchedToggle = this.onMatchedToggle.bind(this);
+
     // Build the container element
     this.element = doc.createElementNS(HTML_NS, "div");
     this.element.setAttribute("class", this.propertyHeaderClassName);
-    this.element.addEventListener("dblclick",
-      this.onMatchedToggle.bind(this), false);
+    this.element.addEventListener("dblclick", this.onMatchedToggle, false);
 
     // Make it keyboard navigable
     this.element.setAttribute("tabindex", "0");
-    this.element.addEventListener("keydown", (aEvent) => {
+    this.onKeyDown = (aEvent) => {
       let keyEvent = Ci.nsIDOMKeyEvent;
       if (aEvent.keyCode == keyEvent.DOM_VK_F1) {
         this.mdnLinkClick();
@@ -826,13 +844,13 @@ PropertyView.prototype = {
         aEvent.keyCode == keyEvent.DOM_VK_SPACE) {
         this.onMatchedToggle(aEvent);
       }
-    }, false);
+    };
+    this.element.addEventListener("keydown", this.onKeyDown, false);
 
     // Build the twisty expand/collapse
     this.matchedExpander = doc.createElementNS(HTML_NS, "div");
     this.matchedExpander.className = "expander theme-twisty";
-    this.matchedExpander.addEventListener("click",
-      this.onMatchedToggle.bind(this), false);
+    this.matchedExpander.addEventListener("click", this.onMatchedToggle, false);
     this.element.appendChild(this.matchedExpander);
 
     // Build the style name element
@@ -843,7 +861,8 @@ PropertyView.prototype = {
     this.nameNode.setAttribute("tabindex", "");
     this.nameNode.textContent = this.nameNode.title = this.name;
     // Make it hand over the focus to the container
-    this.nameNode.addEventListener("click", () => this.element.focus(), false);
+    this.onFocus = () => this.element.focus();
+    this.nameNode.addEventListener("click", this.onFocus, false);
     this.element.appendChild(this.nameNode);
 
     // Build the style value element
@@ -855,7 +874,7 @@ PropertyView.prototype = {
     this.valueNode.setAttribute("dir", "ltr");
     this.valueNode.textContent = this.valueNode.title = this.value;
     // Make it hand over the focus to the container
-    this.valueNode.addEventListener("click", () => this.element.focus(), false);
+    this.valueNode.addEventListener("click", this.onFocus, false);
     this.element.appendChild(this.valueNode);
 
     return this.element;
@@ -981,6 +1000,24 @@ PropertyView.prototype = {
     }
     aEvent.preventDefault();
   },
+
+  /**
+   * Destroy this property view, removing event listeners
+   */
+  destroy: function PropertyView_destroy() {
+    this.element.removeEventListener("dblclick", this.onMatchedToggle, false);
+    this.element.removeEventListener("keydown", this.onKeyDown, false);
+    this.element = null;
+
+    this.matchedExpander.removeEventListener("click", this.onMatchedToggle, false);
+    this.matchedExpander = null;
+
+    this.nameNode.removeEventListener("click", this.onFocus, false);
+    this.nameNode = null;
+
+    this.valueNode.removeEventListener("click", this.onFocus, false);
+    this.valueNode = null;
+  }
 };
 
 /**

browser/devtools/styleinspector/style-inspector.js

@@ -76,8 +76,6 @@ function RuleViewTool(aInspector, aWindow, aIFrame)
   this.refresh = this.refresh.bind(this);
   this.inspector.on("layout-change", this.refresh);
 
-  this.panelSelected = this.panelSelected.bind(this);
-  this.inspector.sidebar.on("ruleview-selected", this.panelSelected);
   this.inspector.selection.on("pseudoclass", this.refresh);
   if (this.inspector.highlighter) {
     this.inspector.highlighter.on("locked", this._onSelect);
@@ -90,10 +88,6 @@ exports.RuleViewTool = RuleViewTool;
 
 RuleViewTool.prototype = {
   onSelect: function RVT_onSelect(aEvent) {
-    if (!this.isActive()) {
-      // We'll update when the panel is selected.
-      return;
-    }
     this.view.setPageStyle(this.inspector.pageStyle);
 
     if (!this.inspector.selection.isConnected() ||
@@ -117,27 +111,12 @@ RuleViewTool.prototype = {
     }
   },
 
-  isActive: function RVT_isActive() {
-    return this.inspector.sidebar.getCurrentTabID() == "ruleview";
-  },
-
   refresh: function RVT_refresh() {
-    if (this.isActive()) {
-      this.view.nodeChanged();
-    }
-  },
-
-  panelSelected: function() {
-    if (this.inspector.selection.nodeFront === this.view.viewedElement) {
-      this.view.nodeChanged();
-    } else {
-      this.onSelect();
-    }
+    this.view.nodeChanged();
   },
 
   destroy: function RVT_destroy() {
     this.inspector.off("layout-change", this.refresh);
-    this.inspector.sidebar.off("ruleview-selected", this.refresh);
     this.inspector.selection.off("pseudoclass", this.refresh);
     this.inspector.selection.off("new-node-front", this._onSelect);
     if (this.inspector.highlighter) {
@@ -181,8 +160,6 @@ function ComputedViewTool(aInspector, aWindow, aIFrame)
   this.refresh = this.refresh.bind(this);
   this.inspector.on("layout-change", this.refresh);
   this.inspector.selection.on("pseudoclass", this.refresh);
-  this.panelSelected = this.panelSelected.bind(this);
-  this.inspector.sidebar.on("computedview-selected", this.panelSelected);
 
   this.view.highlight(null);
 
@@ -194,11 +171,6 @@ exports.ComputedViewTool = ComputedViewTool;
 ComputedViewTool.prototype = {
   onSelect: function CVT_onSelect(aEvent)
   {
-    if (!this.isActive()) {
-      // We'll try again when we're selected.
-      return;
-    }
-
     this.view.setPageStyle(this.inspector.pageStyle);
 
     if (!this.inspector.selection.isConnected() ||
@@ -226,22 +198,8 @@ ComputedViewTool.prototype = {
     }
   },
 
-  isActive: function CVT_isActive() {
-    return this.inspector.sidebar.getCurrentTabID() == "computedview";
-  },
-
   refresh: function CVT_refresh() {
-    if (this.isActive()) {
-      this.view.refreshPanel();
-    }
-  },
-
-  panelSelected: function() {
-    if (this.inspector.selection.nodeFront === this.view.viewedElement) {
-      this.view.refreshPanel();
-    } else {
-      this.onSelect();
-    }
+    this.view.refreshPanel();
   },
 
   destroy: function CVT_destroy(aContext)

browser/devtools/webconsole/test/browser_webconsole_bug_653531_highlighter_console_helper.js

@@ -103,7 +103,8 @@ function performWebConsoleTests(hud)
     is(inspector.selection.node.textContent, "bug653531",
        "node successfully updated");
 
-    executeSoon(finishTest);
+    gBrowser.removeCurrentTab();
+    finishTest();
   }
 }
 

toolkit/devtools/server/actors/inspector.js

@@ -1325,11 +1325,13 @@ var WalkerActor = protocol.ActorClass({
    * @param string selector
    */
   querySelector: method(function(baseNode, selector) {
+    if (!baseNode) {
+      return {}
+    };
     let node = baseNode.rawNode.querySelector(selector);
 
     if (!node) {
-      return {
-      }
+      return {}
     };
 
     let node = this._ref(node);

toolkit/devtools/server/actors/styles.js

@@ -136,12 +136,11 @@ var PageStyleActor = protocol.ActorClass({
    *   }
    */
   getComputed: method(function(node, options) {
-    let win = node.rawNode.ownerDocument.defaultView;
     let ret = Object.create(null);
 
     this.cssLogic.sourceFilter = options.filter || CssLogic.FILTER.UA;
     this.cssLogic.highlight(node.rawNode);
-    let computed = this.cssLogic._computedStyle;
+    let computed = this.cssLogic._computedStyle || [];
 
     Array.prototype.forEach.call(computed, name => {
       let matched = undefined;

toolkit/devtools/styleinspector/css-logic.js

@@ -381,8 +381,19 @@ CssLogic.prototype = {
    */
   forEachSheet: function CssLogic_forEachSheet(aCallback, aScope)
   {
-    for each (let sheet in this._sheets) {
-      sheet.forEach(aCallback, aScope);
+    for each (let sheets in this._sheets) {
+      for (let i = 0; i < sheets.length; i ++) {
+        // We take this as an opportunity to clean dead sheets
+        try {
+          let sheet = sheets[i];
+          sheet.domSheet; // If accessing domSheet raises an exception, then the
+          // style sheet is a dead object
+          aCallback.call(aScope, sheet, i, sheets);
+        } catch (e) {
+          sheets.splice(i, 1);
+          i --;
+        }
+      }
     }
   },
 
@@ -1008,8 +1019,8 @@ CssSheet.prototype = {
   get ruleCount()
   {
     return this._ruleCount > -1 ?
-        this._ruleCount :
-        this.domSheet.cssRules.length;
+      this._ruleCount :
+      this.domSheet.cssRules.length;
   },
 
   /**
@@ -1117,7 +1128,7 @@ CssSheet.prototype = {
   toString: function CssSheet_toString()
   {
     return "CssSheet[" + this.shortSource + "]";
-  },
+  }
 };
 
 /**