Closed Bug 76102 Opened 21 years ago Closed 21 years ago
leaks from cycle due to ns
XPCWrapped JS as weak reference
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.
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.
Status: NEW → ASSIGNED
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).
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.
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
> 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.
> > 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, firstname.lastname@example.org. File a bug on refactoring wrappers? /be
fix checked in
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Based on David's comment at 2001-04-16 19:40, marking Verified -
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.