61821 - Leak loading ftp://ftp.mozilla.org/

Status: RESOLVED FIXED

(Core :: XBL, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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??????????????????????

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The following 2 lines also cause the leak:

    var foo = getBrowser().markupDocumentViewer
    _content.defaultCharacterset = appCore.GetDocumentCharset();

but getBrowser() alone doesn't.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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...

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: JS workaround to avoid triggering the leak

Comment 7

[Peter "jag" Annema]

The obvious work-around :-)

r=jag

Comment 8

[Alec Flett]

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)

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK... hack checked in.

I'm going to assign this to hyatt since it seems XBL-related.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch fixing leak

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch fixing leak

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 :-(

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 17

[Peter Trudelle]

->moz0.9

Comment 18

[Brendan Eich [:brendan]]

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

/be

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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...

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: better patch

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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...

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: better fix

Comment 25

[Peter "jag" Annema]

r=jag.

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

Comment 26

[Johnny Stenback (:jst)]

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?

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 28

[Chris Waterson]

(s)r=waterson.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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