111411 - clear Inspector when target document is destroyed

Status: RESOLVED INCOMPLETE

(Other Applications :: DOM Inspector, enhancement)

Description

[Joe Hewitt (gone)]

If the document being inspected is destroyed (via the user closing its window,
browsing to another page, etc...) the Inspector should release it and go back to
inspecting nothing.  Also, I should stop holding a reference to the document in
inDOMView.

Comment 1

[Brian 'netdragon' Bober]

If the HTML document is destroyed or changed, but the remaining XUL DOM still
exists for that window, it should just go back to the parent node of the HTML
element, not clear the entire window.

Still, I don't feel this should be done automatically since people might want to
still look at DOM values even after the document is closed. What if you had a
"refresh" button that reloaded the DOM of that page and attempted to return to
the closest identical element on the DOM (for instace the parent of the HTML
element, like XUL:TABBROWSER)?

See also bug 181261.

Since search is broken on DOM inspector, it is a pain in the neck to get back to
the node you were looking at on the HTML document.

Comment 2

[Boris Zbarsky [:bzbarsky]]

*** Bug 200890 has been marked as a duplicate of this bug. ***

Comment 3

[Boris Zbarsky [:bzbarsky]]

Default owner.

Should we have an nsIDocumentObserver notification for this sort of thing?  The
DocumentWillBeDestroyed is not useful, since it will not fire if you hold a ref
to the doc (as JS might till the next GC, for example).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Maybe the right thing to do is to simply attach an "unload" event listener to
the window the inspectee lives in?

Comment 5

[Adam Hauner]

Adding signature and keyword from dupe.

Comment 6

[Boris Zbarsky [:bzbarsky]]

*** Bug 203369 has been marked as a duplicate of this bug. ***

Comment 7

[Marcel van Beurden]

This problem is bugging me a lot while developing and testing webapps. Is there
a workaround to prevent the crashes without closing the DOM inspector?

Comment 8

[Alex Vincent [:WeirdAl]]

After a quick readover of the code, and based on my work for bug 156072 and bug
193942, I can definitely state a procedure which should work.

Here's what currently happens to set a document in Inspector.

(1) inspector object (which will henceforth be "this") calls
this.setTargetDocument(aDoc).
(2) this.setTargetDocument(aDoc) sets this.mDocPanel.subject(aDoc).
(3) inspector.xml#panel binding for subject calls setSubject method
(4) this.mDocPanel.setSubject(aObject) (here still aDoc) searches
viewer-registry.rdf for viewers which are valid for that node.
(5) A default viewer is selected, from the available viewers, for the left panel.
(6) Said viewer picks a selection and tells somebody about it:

this.mDocPanel.viewer.mObsMan.dispatchEvent("selectionChange", { selection:
this.mDocPanel.viewer.mSelection } );
(7) Somewhere along the line (I haven't figured out where), the right panel gets
its subject from the left panel's selection.

bz suggested an unload event listener on the doc.  Such should follow this sequence:

function(evt) {
  this.mDocPanel.viewer.mObsMan.dispatchEvent("selectionChange", { selection:
null } );
  // killing the right-side viewers
  this.setTargetDocument(null);
  // killing the left side viewers
}

We also need to change
mozilla/extensions/inspector/resources/content/res/viewer-registry.rdf on the
last viewer's ins:filter attribute.  Currently it reads "return true;".  It
should read:  "return object != null".  That'll make sure the JSObject viewer
doesn't show up for null, and save on some processor time.

Only question I have is which window object's scope would the evt object fall
under... as the inspected document won't have an inspector object to call all
this on.

Comment 9

[Alex Vincent [:WeirdAl]]

If the unload event listener does turn out to be on the inspected document's
defaultView scope, could we get away with adding an "inspectorWindow" property
to the inspected document?

Comment 10

[Alex Vincent [:WeirdAl]]

Attachment: work-in-progress

This patch works, mostly, but causes a couple unacceptable regressions.

(1) Open DOM Inspector via Tools, Web Development, DOM Inspector, then tell it
to inspect itself.  

Error: [Exception... "Component returned failure code: 0x80070057
(NS_ERROR_ILLEGAL_VALUE) [nsIRDFContainerUtils.IndexToOrdinalResource]" 
nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame ::
chrome://inspector/content/jsutil/rdf/RDFU.js :: anonymous :: line 62"	data:
no]
Source File: chrome://inspector/content/jsutil/rdf/RDFU.js
Line: 62

(2) Inspect a window (specifically a browser window), then navigate to a new
page in that window.  Inspector, which was looking at navigator.xul, now fires
the unload event handler.

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

Mass re-assigning bugs to [e-mail]

Comment 12

[Alex Vincent [:WeirdAl]]

Okay, I've done a little research, and this bug may (or may not) be fixable
based on the DOM 2 Events model.

Every single document which DOM Inspector looks at goes through inspector.js @
setTargetDocument as an argument.  So, in theory, we should be able to detect an
unload event dispatched to either the document (which would be ideal and very
simple) or to a simple, common node or window related to the document.

DOM Inspector can look at a document in three ways:  by directly loading a URL
into itself, by inspecting the document as loaded from the browser via
CTRL+SHIFT+I, or by inspecting any registered application such as the browser
itself.  A document can be unloaded either by closing the application which
contains it (this may be Inspector under the first scenario) or by loading a new
URL into the container application.

So there are six possibilities to consider.  Here are some brief results, going
from about: to about:mozilla, then closing the application.

(1) DOM-I as container, navigates to new URL:  evt.target = unloaded document
(2) DOM-I as container, closes:  evt.target = unloaded document
(3) DOM-I looking at document via CTRL+SHIFT+I, navigates to new URL: evt.target
= unloaded document
(4) DOM-I looking at document via CTRL+SHIFT+I, tab closes:  JavaScript
Exception:  the target of the event has no properties (probably means the
document is null before the event reaches its listener, not a good thing.  We
also get assertions, at nsWebShell.cpp line 289 and sometimes at
nsGenericHTMLElement.cpp line 4483.)
(5) DOM-I looking at Mozilla Navigator, navigates to new URL: evt.target =
<xul:tabbrowser/>.
(6) DOM-I looking at Mozilla Navigator, application closes: multiple unload
events, 1 for each tabbrowser, and one for the Navigator XUL document.

In the first three cases, the document being unloaded is the target, and this is
very conveinient for DOM Inspector.  In the fourth case, we have an exception
and a bunch of assertions.  In the last two cases, the event stops just shy of
the document being unloaded... which is inconsistent with the first three cases.

Note bug 99820 for some related information.

  setTargetDocument: function(aDoc)
  {
    try {
      if (this.mDocPanel.subject) {
        this.mDocPanel.subject.defaultView.removeEventListener("unload",
DocumentUnloadListener, true);
        dump("Event listener removed for " + this.mDocPanel.subject.location +
"\n");
      }
    }
    catch(e) {
      dump("setTargetDocument removeEventListener error: " + e.msg + "\n");
    }
  
    this.mDocPanel.subject = aDoc;
    
    try {
      if (aDoc) {
        aDoc.defaultView.addEventListener("unload", DocumentUnloadListener, true);
        dump("Event listener added for " + aDoc.location + "\n");
      }
    }
    catch(e) {
      dump("setTargetDocument addEventListener error: " + e.msg + "\n");
    }
  },

// then, at the end of the script:

function DocumentUnloadListener(aEvent) {
  dump(aEvent.target);
  if (aEvent.target.nodeType == 1) {
    dump(" (" + aEvent.target.nodeName + ")");
  } else if (aEvent.target.nodeType == 9) {
    dump(" (" + aEvent.target.location + ")");
  }
  dump(" unload event at " + aEvent.currentTarget + "\n");
}

Comment 13

[[e-mail]]

> (5) DOM-I looking at Mozilla Navigator, navigates to new URL: evt.target =
> <xul:tabbrowser/>.

Because of XBL. Try using evt.originalTarget instead.

Comment 14

[Alex Vincent [:WeirdAl]]

Heh, oops.  I should've thought of that.

Okay, that solves five of six scenarios.  The only one left, number 4, still
throws assertions, and evt.originalTarget === null.

caillon: module owner's question.  I now have enough information to fix this
bug, and by doing so, we'll almost certainly close two other critical bugs in
DOM Inspector.  My question is, should I wait for the bug I'm seeing in scenario
4 to be fixed?  I can rig the code to accept evt.originalTarget === null and
dump() something to warn people this is happening.

Comment 15

[Brian 'netdragon' Bober]

Alex: Is there any way that when the document is reloaded and you are within
that document on DOM inspector (such as within an HTML document) that we can
return to the node before the part of the DOM that was reloaded? Sorry if this
question is redundant to something you already explained, I just want to make
sure its not reset to the very beginning of the DOM as I mentioned in comment
#1. For instance, if the HTML page is reloaded, we could change the selected
tree member in DOM Inspector to the tab panel, not the very beginning of the
DOM. That would be easier for people that are inspecting a document's DOM and
constantly reloading the document within the tab, etc.

Comment 16

[Alex Vincent [:WeirdAl]]

I don't see how that's possible, or even safe.

When the document is reloaded, that means the current document is destroyed and
a new document (pulled from the same URL) is created.  With the destruction of
the current document comes the destruction of every single node beneath it.  Any
attempts to reference those nodes is a clear invitation to crash (bug 128110,
bug 219514).

Your comment 1 in this bug is nice, but not immediately doable  John Keiser's In
bug 181261 comment 2, jkeiser offers a possible solution, but it's not nearly as
important as stopping the crashes.

Believe me, I sympathize a great deal; I know exactly what you are talking about
and I'm constantly reloading too.  But this is not the place to raise that issue.

Comment 17

[Alex Vincent [:WeirdAl]]

Note bug 224741 for what I saw in comment 12, scenario 4.  Hopefully, we'll get
some traction there.

Comment 18

[Alex Vincent [:WeirdAl]]

Attachment: patch, draft 1 (needs testing, feedback)

I don't quite consider this ready for reviews yet, for two reasons.

(1) I'm getting a lot of XPConnect errors.  Although they don't appear to
affect the operation of DOM Inspector, I would like to figure out how to shut
them down.

(2) This patch depends partly on a maintenance patch for bug 193942.  The
reason is the left-hand panel with this patch gets its subject set to null. 
This means the JavaScript Object viewer takes over on the left side, and the
right-side panel's subject never gets updated to reflect this.

I'd really appreciate it if a few volunteers would do some serious testing of
this patch, and offer suggestions for nailing down the XPConnect exceptions.

Comment 19

[[e-mail]]

>(2) This patch depends partly on a maintenance patch for bug 193942.  The
>reason is the left-hand panel with this patch gets its subject set to null. 
>This means the JavaScript Object viewer takes over on the left side, and the
>right-side panel's subject never gets updated to reflect this.
I changed line 89 of viewer-registry.rdf to "return object instanceof Object;"

Comment 20

[[e-mail]]

Heh, that doesn't unload the left panel at all :-)

Comment 21

[James Napolitano]

*** Bug 244285 has been marked as a duplicate of this bug. ***

Comment 22

[Alex Vincent [:WeirdAl]]

*** Bug 266732 has been marked as a duplicate of this bug. ***

Comment 23

[Robert Kaiser]

Alex (or anyone else here), does this still happen? Do we need to track this, maybe get the patch in, or should we close the bug?

Comment 24

[Sergio Del Franco]

I don't remember the application this bug applies to, so I made a little test in both browsers.

Firefox: I don't know if the new(?) inspector is the same this bug relates to, but it is closed when a link is clicked, so the problem is worked around.

Seamonkey: It uses the "classic" DOM inspector. The DOM being inspected is not destroyed / updated / resetted but I can't detect any crash when working with nodes (cut, paste and delete works ok; blink element does nothing -naturally-). The DOM is updated when it is changed using javascript, no crashes.

I think this bug should be closed, but it would be nice if DOM inspector notifies the user [*] when the rendered document is not visible anymore (tab or window closed or navigated out)

[*] May it use the yellow top bar like popup blocker do?

Comment 25

[Boris Zbarsky [:bzbarsky]]

> I don't remember the application this bug applies to

See the "product" and "component" fields on the bug?

Comment 26

[Sergio Del Franco]

"Product: Other Applications" did not help.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Well, the Firefox inspector is part of the "Firefox" product; it's not a separate application....

Comment 28

[Sergio Del Franco]

I am sorry I think it was a separate application in 2001 when this bug was reported, but I am not sure if I can remember correctly.
Just ignore my references to Firefox, please.

Comment 29

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to Robert Kaiser (:[e-mail]) from comment #23)
> Alex (or anyone else here), does this still happen? Do we need to track
> this, maybe get the patch in, or should we close the bug?

This is a crash bug, but the suggestions about what to do when documents are unloaded are pertinent enough for an enhancement bug.

FWIW, I inspect unloaded documents all the time (with XBL even), and it hasn't been prone to cause crashes, at least not over the last few years using release builds.  If you're comfortable with that, I'd say remove the crash bits from this bug or close it, and the enhancement suggestions can live in a new bug.

Comment 30

[Robert Kaiser]

(In reply to Colby Russell :crussell from comment #29)
> FWIW, I inspect unloaded documents all the time (with XBL even), and it
> hasn't been prone to cause crashes, at least not over the last few years
> using release builds.  If you're comfortable with that, I'd say remove the
> crash bits from this bug or close it, and the enhancement suggestions can
> live in a new bug.

I'll leave it up to you if you want out handle the enhancements in this or in a new bug, I'm removing the crash stuff from here at least, everything else is up to you as the DOMi maintainer. :)

Comment 31

[Firefox Bug Husbandry Bot]

Bulk close. This component is no longer supported or maintained.

https://bugzilla.mozilla.org/show_bug.cgi?id=1499023

Comment 32

[Firefox Bug Husbandry Bot]

Bulk close. This component is no longer supported or maintained.

https://bugzilla.mozilla.org/show_bug.cgi?id=1499023