922125 - [markup view] Implement destroy functions to clean event listeners

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

The devtools inspector's markup-view creates a bunch of objects (MarkupContainer, TextEditor, ElementEditor, ...) for each of the represented node.

When there are markup mutations, some of these objects may no longer be needed, however, they are not, as of today, cleaned at all.

This leads to un-removed event listeners, and to all sorts of exceptions further down the road ("can't access dead object" is one of them).

Comment 1

[Patrick Brosset <:pbro>]

The way things goes is:

- The InspectorPanel instantiates and destroys a MarkupView object. It instantiates it when the inspector tool is opened for the first time, and it destroys then re-instantiates it when a new root is observed (on page navigation basically).

- In turn, the MarkupView instantiates many MarkupContainer objects. This is done every time a new node needs to be displayed in the inspector (either when the tool is opened the first time, or when a parent is expanded the first time).

- On markup mutations, the MarkupView instance, needs to react and create new MarkupContainer (same as previous bullet point) as well as destroy those which are no longer needed for display ==> This step is currently missing in the code. The DOM structure of the containers is removed, but the MarkupContainer instances themselves are not.

- Each MarkupContainer instance listens to several types of events on its DOM nodes, and those listeners need to be removed on destroy ==> adding the missing step described above will allow that.
Also, each MarkupContainer instance also instantiates an Editor object, mostly responsible for the inplace-editing of tags and attributes. Since these editors can grow in complexity, the destroy method on MarkupContainer will also allow to cleanly dispose of editors.

Comment 2

[Patrick Brosset <:pbro>]

Same should go for the rule and computed views since, like the markup view, these are refreshed (objects destroyed/instantiated) everytime a new node is selected.

Comment 3

[Patrick Brosset <:pbro>]

And while we're at it, let's check all kinds of scenarios during page navigation.
Indeed, when navigation occurs, the inspector is reloaded, meaning that old instances should be destroyed and we shouldn't have exceptions during the load.

One such scenario is bug 914744.
Another one is navigating to a page that contains an iframe that gets deleted during the load (http://jsbin.com)

Comment 4

[Patrick Brosset <:pbro>]

Computed view destroy is definitely needed. Indeed, the following scenario fails:
- open the inspector
- switch to the computed view
- navigate to a few different sites
=> "Can't access dead object" exceptions are triggered.

Comment 5

[Patrick Brosset <:pbro>]

For the above case, we already set the current node selection to null when navigating away, so it's just a matter of making sure the computed view knows it and hides styles, etc.

I found another navigation case: navigating to a site that has iframes or popups will trigger inspector refreshes via the browser.onresize event. Indeed, even if the browser isn't really resized, that event triggers when iframes are detected.
We listen to it with:
this.browser.addEventListener("resize", this.scheduleLayoutChange, true);

So it's necessary to filter out any non real resize before trying to refresh the inspector's tools, therefore avoiding them from trying to get nodes or styles for things that either are not loaded anymore (previous page) or not loaded yet (page being navigated to).

Comment 6

[Patrick Brosset <:pbro>]

I think I have now got most failing scenarios fixed.
Here is a summary of the changes. I will be attaching a patch soon:

- The inspector now listens to navigation (will-navigate) events to destroy the markup view and make the current node selection to `null`.
- This has the effect of emptying the markup, rule and computed views during navigation.
- The inspector listens for window resize events to refresh the views, however, this event is also triggered by iframes in the content page, so now these cases are filtered out.
- The markup view used to store each of the represented node in a WeakMap. I've made this into a Map now so that I can iterate over it during the destroy phase and therefore destroy the MarkupContainers too.
- destroy methods were added to the markup, rule and computed views classes.
- The rule and computed views used to only refresh when they were activated (when one would click on the rule or computed tab). I have removed and instead those views get refreshed immediately when a node gets selected. This also means that when navigating away, because we select `null`, these views immediately become empty, whether they're shown or not.
- Finally, on the server-side, in css-logic.js, some style sheet objects may be still referenced after a page navigation, and therefore they become DeadObjects. So I've made a change to clean these up to make sure further calls to the css-logic API won't fail.

Comment 7

[Patrick Brosset <:pbro>]

Green try build https://tbpl.mozilla.org/?tree=Try&rev=dd98be76f7d1

Comment 8

[Patrick Brosset <:pbro>]

Attachment: bug922125-add-markup-destroy-methods.patch

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 815240 [details] [diff] [review]
bug922125-add-markup-destroy-methods.patch

Review of attachment 815240 [details] [diff] [review]:
-----------------------------------------------------------------

I have not r+ssed this simply because I don't like the executeSoon. We need to either force GC or add a comment explaining why we need to use executeSoon.

::: browser/devtools/inspector/test/browser_inspector_bug_922125_destroy_on_navigate.js
@@ +67,5 @@
> +    is(markupViewBox.childNodes.length, 0, "The markup-view is unloaded");
> +  }
> +
> +  function endTests() {
> +    target = browser = tab = inspector = null;

Please also set Toolbox and TargetFactory to null. Just because they are globals.

::: browser/devtools/markupview/markup-view.js
@@ +1174,5 @@
> +   * needed
> +   */
> +  destroy: function() {
> +    // Recursively destroy children containers
> +    while (this.children.firstChild) {

Maybe:
while (let firstChild = this.children.firstChild) {
  firstChild.container.destroy();
  this.children.removeChild(firstChild);
}

::: browser/devtools/webconsole/test/browser_webconsole_bug_653531_highlighter_console_helper.js
@@ +102,5 @@
>      let inspector = gDevTools.getToolbox(target).getPanel("inspector");
>      is(inspector.selection.node.textContent, "bug653531",
>         "node successfully updated");
>  
> +    executeSoon(() => {

Do we really need executeSoon here? Please add a comment or if you are waiting for GC then maybe forceGC instead.

Comment 10

[Patrick Brosset <:pbro>]

Thanks Mike for the review.
- Good point about nullifying toolbox and targetfactory, I forgot about those.
- Good point too for the while loop in the destroy function.
- About the executeSoon, this was here before I touched the test, so I didn't dare look into this. I'll try to remove it see what happens. But if it doesn't work, I'll have to ask whoever wrote the test.

Comment 11

[Patrick Brosset <:pbro>]

Attachment: bug922125-add-markup-destroy-methods.patch

Comment 12

[Patrick Brosset <:pbro>]

Try build https://tbpl.mozilla.org/?tree=Try&rev=f0932c62e336

Comment 13

[Patrick Brosset <:pbro>]

Thanks Mike.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/39b5782426c5

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/39b5782426c5

Comment 17

[Luke]

Still seeing that node-is-null error on a regular basis, in the latest Firefox 27.

Should I report a new bug?

The exception stack is:
TypeError: node is null: LocalHighlighter_updateInfobar@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/highlighter.js:569
EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/event-emitter.js:110
Selection.prototype.setNodeFront@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/selection.js:166
InspectorPanel_onDetached@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/inspector-panel.js:496
EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/event-emitter.js:110
Selection.prototype._onMutations@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/selection.js:108
lazy@resource://gre/modules/XPIProvider.jsm -> jar:file:///(profiledir)/extensions/[e-mail]!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:110
emit@resource://gre/modules/XPIProvider.jsm -> jar:file:///(profiledir)/extensions/[e-mail]!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:83
exports.WalkerFront<.getMutations</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:2205
resolve@resource://gre/modules/devtools/server/main.js -> resource://gre/modules/commonjs/sdk/core/promise.js:118
then@resource://gre/modules/devtools/server/main.js -> resource://gre/modules/commonjs/sdk/core/promise.js:43
resolve@resource://gre/modules/devtools/server/main.js -> resource://gre/modules/commonjs/sdk/core/promise.js:185
resolve@resource://gre/modules/devtools/server/main.js -> resource://gre/modules/commonjs/sdk/core/promise.js:118
then@resource://gre/modules/devtools/server/main.js -> resource://gre/modules/commonjs/sdk/core/promise.js:43
resolve@resource://gre/modules/devtools/server/main.js -> resource://gre/modules/commonjs/sdk/core/promise.js:185
resolve@resource://gre/modules/devtools/server/main.js -> resource://gre/modules/commonjs/sdk/core/promise.js:118
then@resource://gre/modules/devtools/server/main.js -> resource://gre/modules/commonjs/sdk/core/promise.js:43
resolve@resource://gre/modules/devtools/server/main.js -> resource://gre/modules/commonjs/sdk/core/promise.js:185
Front<.onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:1071
@resource://gre/modules/devtools/dbg-client.jsm:664
resolve@resource://gre/modules/commonjs/sdk/core/promise.js:118
then@resource://gre/modules/commonjs/sdk/core/promise.js:43
then@resource://gre/modules/commonjs/sdk/core/promise.js:153
DC_onPacket@resource://gre/modules/devtools/dbg-client.jsm:705
@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/transport.js:258
@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/DevToolsUtils.js:75

Comment 18

[Patrick Brosset <:pbro>]

(In reply to Luke from comment #17)
> Still seeing that node-is-null error on a regular basis, in the latest
> Firefox 27.
> 
> Should I report a new bug?
> 
> The exception stack is:
> TypeError: node is null:
> LocalHighlighter_updateInfobar@resource://gre/modules/commonjs/toolkit/
> loader.js -> resource:///modules/devtools/inspector/highlighter.js:569
FWIW that file is gone in Firefox 29 (current Aurora) since the highlighter has been entirely reworked in this release.
I haven't seen these errors anymore since then. Would you mind trying your particular usecase on Aurora and reporting if that error still occurs there?