The changes jag just checked in cause a leak caused by a bunch of leaked JS roots for treeitem nsXULElement objects. The following patch fixes the leak: Index: navigator.js =================================================================== RCS file: /cvsroot/mozilla/xpfe/browser/resources/content/navigator.js,v retrieving revision 1.259 diff -u -r1.259 navigator.js --- navigator.js 2000/12/02 14:55:42 1.259 +++ navigator.js 2000/12/02 16:21:25 @@ -1555,7 +1555,7 @@ typeof _content.HTTPIndex == "object" && !_content.HTTPIndex.constructor) { // Give directory .xul/.js access to browser instance. - _content.defaultCharacterset = getBrowser().markupDocumentViewer.defaultCharacterSet; + _content.defaultCharacterset = appCore.GetDocumentCharset(); _content.parentWindow = window; } } Why??????????????????????

The following 2 lines also cause the leak: var foo = getBrowser().markupDocumentViewer _content.defaultCharacterset = appCore.GetDocumentCharset(); but getBrowser() alone doesn't.

Considering http://lxr.mozilla.org/seamonkey/source/layout/base/src/nsDocumentViewer.cpp#406 I think the leaked roots are either a side effect of the fact that this is a document viewer or part of the obvious cycle (if they're not a side effect). But what's the rest of the cycle? What keeps the document viewer alive?

Iterestingly, the following line leaks: var foo = getBrowser().markupDocumentViewer but the following doesn't: var foo = document.getElementById("content").boxObject.QueryInterface(Components.interfaces.nsIBrowserBoxObject).docShell.contentViewer.QueryInterface(Components.interfaces.nsIMarkupDocumentViewer); jag says the long line is a concatenation of the XBL code for the short one: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/global/resources/content/xulBindings.xml&rev=1.42&mark=682#682

Hyatt, is XBL caching something that could cause this leak? (Perhaps in combination with the way XPConnect works?)

The difference can be reduced to: This line leaks: var foo = getBrowser().markupDocumentViewer This line does not leak: var foo = getBrowser().docShell.contentViewer.QueryInterface(Components.interfaces.nsIMarkupDocumentViewer); So I really think the problem is that XBL is caching properties that are looked up, or something of the sort...

The obvious work-around :-) r=jag

sr=alecf the last time we had a leak like this, we had a JS object holding a ref to an element, which held a reference to the document... the ref to the element would not be released for some reason, which meant the document got leaked (which of course leaked the world)

OK... hack checked in. I'm going to assign this to hyatt since it seems XBL-related.

Without the JS workaround, there's no DocumentViewerImpl leaked. Hmmm.

With this patch I saw the assertion in LoadComplete. Why is LoadComplete called after Stop?

Bug 61471 seems to have the same cause as this one, but the above patch doesn't fix it. Nominating for mozilla0.9 since I've spent close to two days debugging these leaks, and jag has probably spent a similar amount of time, and I don't want to have to keep doing that each time a new one pops up.

That above patch to nsDocumentViewer.cpp also causes other problems -- like pages become inactive after I hit stop. It's been bugging me for a week -- I finally realized it was a change only in my tree :-(

It seems like relevant code is in nsXBLBinding::InstallProperties.

->moz0.9

dbaron, what's the relevant code in InstallProperties? Re-clue me in, and I'll try to help. /be

I don't know if the problem exists anymore. jag says it doesn't but I can't verify for sure since loading ftp://ftp.mozilla.org/ leaks 630K either way...

I do still see this, but the previous workaround (which isn't checked in anymore either) is no longer sufficient. I still see leaked treeitem roots and the many associated leaks if I: * set up my sidebar as closed in advance (View | Sidebar) * `./mozilla ftp://ftp.mozilla.org/` * exit If I have the sidebar open in advance or open it while the browser is running I don't see the leaks.

That patch tends to cause crashes: at /builds/seamonkey/mozilla/layout/events/src/nsEventStateManager.cpp:1482 1482 docShell->GetBusyFlags(&busyFlags); (gdb) p docShell $1 = {mRawPtr = 0x0} There's also a bunch of other safety work that needs to happen...

Assigning to myself since I have a fix (I'll attach a better version soon).

r=jag. The changes make sense to me but I would appreciate someone else also giving r=.

I had a look at the patch and in general I agree with what the patch does but there's few things I'm not sure I agree with. Please be aware that I don't know document viewer internals very well, so my concerns might not be well grounded. In nsDocumentViewer.cpp: @@ -740,6 +702,7 @@ NS_IMETHODIMP DocumentViewerImpl::GetDOMDocument(nsIDOMDocument **aResult) { + NS_ENSURE_TRUE(mDocument, NS_ERROR_NOT_AVAILABLE); return CallQueryInterface(mDocument.get(), aResult); } Why flag the lack of a document as an error (i.e. an exception in JS)? IMO the fact that there's no document is not an error, it just doesn't have a document. Why not just return NS_OK and null out the output parameter? I'm not sure I agree with throwing an error in those other methods (SetBounds, Move, Show, ...) either just because there's no document in the document viewer, maybe I'm wrong, but it seems weird. In DocumentViewerImpl::LoadComlete(), why no null check for mDocument any more?

The idea of the NS_ENSURE_TRUE statements was to give an error if the document viewer is being used when it's in a state where it shouldn't be used, i.e., before |Init| is called or after |Destroy| is called. I started setting |mDocument| to null in Destroy as a marker that the document viewer is in an invalid state.

(s)r=waterson.

Fix checked in 2001-03-09 19:19 PST.