848731 - JavaScript error: resource:///modules/devtools/Selection.jsm, line 165: TypeError: can't access dead object

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

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

Fixing bug 839640 has resulted in a new bug.

STR:
1. Load the testcase.
2. Using the Inspector, try to change the <iframe src> attribute from http to https.

Result:
JavaScript error: resource:///modules/devtools/Selection.jsm, line 165: TypeError: can't access dead object

Comment 1

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

The new issue is probably only present when changing the src of iframes.

Comment 2

[Paul Rouget [:paul]]

Can I have STR?

Comment 3

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

STR:
1. Apply the patch from bug 839640.
2. Load the testcase from bug 839640.
2. Using the Inspector, try to change the <iframe src> attribute from http to https.

Comment 4

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

Of course, a node inside the iframe needs to be selected before changing the iframe src in order to reproduce this error, which is raised in more than one place:

ERROR: TypeError: can't access dead object
STACK: SN_isNode@resource:///modules/devtools/Selection.jsm:165
MT__onNewSelection@resource:///modules/devtools/MarkupView.jsm:102
MarkupView@resource:///modules/devtools/MarkupView.jsm:66
InspectorPanel__onMarkupFrameLoad@resource:///modules/devtools/InspectorPanel.jsm:588
InspectorPanel_initMarkupPanel_onload@resource:///modules/devtools/InspectorPanel.jsm:572

ERROR: TypeError: can't access dead object
STACK: SN_isNode@resource:///modules/devtools/Selection.jsm:165
Highlighter_invalidateSize@resource:///modules/devtools/Highlighter.jsm:226
Highlighter_handleEvent@resource:///modules/devtools/Highlighter.jsm:685

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

This may be related to bug 849500.

Comment 6

[Patrick Brosset <:pbro>]

This may also be related to bug 917448.

According to the stacktrace shared by Mike above, it seems to point in the direction where the inspector.selection is not destroyed properly on reload, therefore retaining a node that does not exist anymore. Therefore, next time a function is called on the selection, it fails with this exception.

I haven't yet investigated in details but afaik the inspector instance survives page reloads, that is, it's newed once and not ever again while the toolbox is opened.
Because selection is stored at this level, the selection object also is never destroyed. 
However, the only place I see in the code now where the selection is reset is when the root node mutates:

  /**
   * Reset the inspector on new root mutation.
   */
  onNewRoot: function InspectorPanel_onNewRoot() {
    this._defaultNode = null;
    this.selection.setNodeFront(null);
    ...

This is not enough in case of an iframe which is not included in a root node if I'm correct.

Comment 7

[Patrick Brosset <:pbro>]

After testing this again as well as bug 754612, I don't think this problem exists anymore.
What I did was:
- load a page with an iframe
- open the inspector
- select an element inside the iframe
- change the iframe src attribute

What happens now, with the latest fx-team code base is that the iframe gets reloaded, the inspector shows the new DOM tree (although collapsed, so you have to expand it again to see).

I think this bug is a duplicate of bug 754612 in that what I think should be done now is reset the breadcrumbs and style inspector panels when the currently selected node get removed.

Comment 8

[Patrick Brosset <:pbro>]

Here are the failing cases I can think of:

a) open a page, inspect a node, right click on its container in the markup view, choose delete node
-> The rule and computed views do refresh, the breadcrumb sort of refreshes too but still shows the removed child as the last crumb. Clicking on that link leads to a "node is null" exception.

b) open a page, inspect a node, remove the node via javascript
-> Same as above

c) open a page containing an iframe, inspect a node inside that iframe, remove the iframe with javascript
-> The rule and computed view are not refreshed, nor is the breadcrumbs, and the highlighter stays on the page where the element was. Also leads to "node is null" exceptions if you click on a link in the breadcrumbs, and "cant access dead object" exceptions if you try to change a CSS rule.

Comment 9

[Patrick Brosset <:pbro>]

a) and b) can be solved by making the breadcrumb listen to the inspector "markupmutation" event, which happesns *after* mutations have been done. So far the breadcrumb only listens to new selections, and when you remove a node, it first selects its parent, that's why the breadcrumb updates, but still show the removed child.
So, listening to this "markupmutation" event allows to once more update the breadcrumbs after the tree has been updated.

Comment 10

[Patrick Brosset <:pbro>]

c) can be solved pretty easily too: so far, when the selection reacts to childList mutations, it only fires a "detached" event if the removed node is not a dead object, because it wants to try and select its parent by default.
If we remove that limitation, and if it's not dead, select its parent, otherwise select the default object, then we're fine, as in, deleting a parent iframe of the currently selected node will select back the default object (the main <body> tag) and therefore update the breadcrump, highlighter, css rule and computed views.

Comment 11

[Patrick Brosset <:pbro>]

Oh and I forgot to mention that c) also impacted the font and box model views.

Comment 13

[Patrick Brosset <:pbro>]

Attachment: bug848731-reset-breadcrumbs-styleinspector-on-node-delete.patch

This patch contains what I described above as solutions for a, b and c.
It also contains a new mochitest test case.

Comment 14

[Paul Rouget [:paul]]

Comment on attachment 810592 [details] [diff] [review]
bug848731-reset-breadcrumbs-styleinspector-on-node-delete.patch

Looks good.

Reviewing this kind of patch is not easy because you do a lot of cleaning. I'm afraid of missing important changes. I suggest that next time you do the cleaning in a different commit (or a different bug).

Comment 15

[Patrick Brosset <:pbro>]

Thanks Paul for the review. You're right, I should do the cleaning in a separate patch, so you can review the important changes easily.
Thanks!

Comment 16

[Patrick Brosset <:pbro>]

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

Comment 17

[Patrick Brosset <:pbro>]

Attachment: bug848731-reset-breadcrumbs-styleinspector-on-node-delete.patch

Sorry, I forgot to attach the last revision of the patch.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09cca9f32103

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4667fc2ca737

https://tbpl.mozilla.org/php/getParsedLog.php?id=28616605&tree=Mozilla-Inbound

Comment 20

[Patrick Brosset <:pbro>]

The wrong patch has been checked in!
Attachment 812621 [details] [diff] should be checked in, not 810592.

Comment 21

[Patrick Brosset <:pbro>]

Ok, my bad, the first patch had been r+ by Paul, but I had to do another one for fixing nits, but it didn't have r+.

I just re-tested everything just to make really sure the 2nd patch works, and it does.
I'll however have to upload a third one cause in the meantime, some testing framework things have been changed and I have to move the test declarations from makefile.in to browser.ini. So, this third patch has no functional changes whatsoever.

Comment 22

[Patrick Brosset <:pbro>]

Attachment: bug848731-reset-breadcrumbs-styleinspector-on-node-delete.patch

Comment 23

[:Gijs (he/him)]

Landed the last patch as: https://hg.mozilla.org/integration/fx-team/rev/2b1ba0d29007

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2b1ba0d29007