Closed Bug 848731 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: miker, Assigned: pbro)

References

(Blocks 1 open bug, )

Details

(Keywords: testcase)

Attachments

(1 file, 2 obsolete files)

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
The new issue is probably only present when changing the src of iframes.
Can I have STR?
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.
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
Assignee: nobody → paul
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.
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.
Assignee: paul → pbrosset
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.
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.
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.
Oh and I forgot to mention that c) also impacted the font and box model views.
Duplicate of this bug: 754612
This patch contains what I described above as solutions for a, b and c.
It also contains a new mochitest test case.
Attachment #810592 - Flags: review?(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).
Attachment #810592 - Flags: review?(paul) → review+
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!
Keywords: checkin-needed
Keywords: checkin-needed
Sorry, I forgot to attach the last revision of the patch.
Attachment #810592 - Attachment is obsolete: true
Keywords: checkin-needed
The wrong patch has been checked in!
Attachment 812621 [details] [diff] should be checked in, not 810592.
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.
Landed the last patch as: https://hg.mozilla.org/integration/fx-team/rev/2b1ba0d29007
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2b1ba0d29007
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.