76102 - leaks from cycle due to nsXPCWrappedJS as weak reference

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

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

DESCRIPTION:  I think some of the leaks I'm currently seeing when starting
mozilla are due to the use of nsXPCWrappedJS as nsIWeakReference (see bug
66950).  In particular, the leak works as follows:

 * The DocLoader's mListenerList is an nsISupportsArray of nsIWeakReference
objects that point to listeners.  One of these listeners is implemented in JS.
 * The root object (I don't completely understand what this is) for the
nsXPCWrappedJS wrapping this nsIWebProgressListener is a wrapper for an
nsIXULBrowserWindow. (Actually, I think this is wrong, since the wrapper for the
nsIXULBrowserWindow is created after the wrapper for the nsIWebProgressListener.
 So I guess I don't really understand what's going on here...)
 * The docloader maintains a weak reference to the wrapper, and the wrapper
maintains a strong reference to its root wrapper.
 * The root wrapper has a JS root that keeps the other wrapper alive, thus
   keeping the root wrapper (and the global object, etc.) alive.

Or something like that.  I cooked up a patch to test my theory, and the patch
fixes the leak, so I think there's something correct about this theory, and I
have to say I think it's a bug in XPConnect.  I don't know whether my patch is
a good way to fix the problem, but it's the only thing I could think of.

Comment 1

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

Attachment: patch that fixed leak for me

Comment 2

[John Bandhauer]

OK David, I see the problem. I failed to deal with the posibility that a wrapper 
chain would try to return a weak reference to anything other than the root 
wrapper. It is bad if it does this. There only needs to be one nsWeakReference 
object for the aggregate wrapper chain.

I think the fix is easier than what you suggest. We can just force 
GetWeakReference to delegate to the root wrapper of the wrapper chain and 
everything should work as expected.

I'll attach a patch. Can you please give it a try and let me know? Thanks.

Comment 3

[John Bandhauer]

Attachment: simpler fix that I expect will work

Comment 4

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

That fix actually didn't work.

The leak is easy to observe:

setenv XPCOM_MEM_LEAK_LOG=<filename>
./mozilla
<let mozilla start, and close the window>

If you see any nsXPCWrappedJS leaked, you're seeing the leak.  Fixing the leak
also reduces lots of other leaks (like 1 GlobalWindowImpl).

Comment 5

[John Bandhauer]

Ah yes. I missed one detail. We should only make the extra increment matter on 
the root wrapper. Doing this makes it not leak for me. I'll attach the patch.

Comment 6

[John Bandhauer]

Attachment: this patch works for me

Comment 7

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

Yeah, that makes sense to me (and works for me too).  But isn't the "this !=
mRoot" check (in Release) unnecessary since now HasWeakReferences will
always be false if a wrapper is not the root?

This does have the cost of creating and destroying a lot of nsXPCWrappedJS
objects in some cases, but that hopefully wouldn't have any major effects,
and there isn't a simple way to avoid it (certainly nothing simpler than my
patch, which may or may not cause other problems that I haven't noticed).

r=dbaron

Comment 8

[John Bandhauer]

> But isn't the "this != mRoot" check (in Release) unnecessary ...?

True enough. But this code is confusing enough. It is nice to be explicit about 
when the root matters. I'm inclined to leave a couple more bytes of code in at 
this point.

> This does have the cost of creating and destroying a lot of nsXPCWrappedJS...

Yes, that is not pretty. Let's see if this shows up as any sort of issue at all 
in Quantify. There is too much data here that matters only in the root wrapper - 
but takes up space in all wrappers. A different factoring is in order at some 
point. When that happens we can distinguish between the references held from the 
outside and those held among the wrappers in the chain. We can clearly do 
better.

Looking for sr=  brendan? shaver?

Thanks, John.

Comment 9

[Brendan Eich [:brendan]]

> > But isn't the "this != mRoot" check (in Release) unnecessary ...?
> 
> True enough. But this code is confusing enough. It is nice to be explicit
> about when the root matters. I'm inclined to leave a couple more bytes of code
> in at this point.

Isn't this a job for NS_ASSERTION?

Otherwise, sr=[e-mail].  File a bug on refactoring wrappers?

/be

Comment 10

[John Bandhauer]

fix checked in

Comment 11

[Phil Schwartau]

Based on David's comment at 2001-04-16 19:40, marking Verified -