nsXPCComponents object and its wrapper leaked at shutdown

RESOLVED FIXED in Future

Status

()

P3
normal
RESOLVED FIXED
16 years ago
12 years ago

People

(Reporter: dbaron, Assigned: sayrer)

Tracking

({memory-leak, topembed+})

Trunk
Future
memory-leak, topembed+
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: edt_b3)

Attachments

(2 attachments, 7 obsolete attachments)

This problem has been bugging me for a while:

On shutdown, we leak an nsXPCComponents object and its wrapper, along with a ton
of associated JS objects that aren't GCed because these objects exist.  This is
essentially an instance of the shutdown ordering problem described at:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/src/nsXPConnect.cpp&rev=1.62&mark=109-112,130-141#107

However, I've been wondering if we need a fully constructed components object on
the JS context owned by the XPCCallContext, just in order to to call
JS_SetPrivate in XPCWrappedNative::SystemIsBeingShutDown and in
XPCWrappedNativeProto::SystemIsBeingShutDown.

I have a patch that's a bit of a hack that fixes this leak using this approach.
 It's not ready to be checked in, because I did a few evil things in it. 
However, I'd be interested to hear if people think the approach is reasonable.
Created attachment 106413 [details] [diff] [review]
patch v. 1

This certainly isn't ready to be checked in (see the XXX comments in
nsXPConnect.cpp).  I'm not sure if the DOM changes are needed anymore, but that
callsite (the call to GetSafeJSContext) used to be the point where we
constructed the components object that was leaked at shutdown.

With this patch, JS seems (based on leak logs) to be shutting down cleanly for
me on Linux (although it won't on Windows due to bug 102655).
(One possibility for a cleaner solution might be to make the
SystemIsBeingShutDown functions that now take an |XPCCallContext&| parameter
instead take a |JSContext*| parameter, since it looks like that's all they need.)
Status: NEW → ASSIGNED
Keywords: mlk
Priority: -- → P3
Whiteboard: [patch]
Target Milestone: --- → mozilla1.3alpha
Created attachment 107731 [details] [diff] [review]
patch v. 2

A possibly-better patch (see my previous comments).  Not yet tested, though.
Comment on attachment 107731 [details] [diff] [review]
patch v. 2

If these JS_Get/SetPrivate calls happen only at shutdown, when no other thread
could be accessing the objects in question, then we don't even need a context.
We'd need to include jsobj.h and use LOCKED_OBJ_SET_SLOT(obj, JSSLOT_PRIVATE,
v), e.g., instead.

I don't mind the modularity loss here -- xpconnect already knows about more
than jsapi.h, IIRC.  dbradley, jband: comments?

/be

Comment 5

16 years ago
That sounds reasonable to me Brendan. The question I don't know the answer to is
are all other threads that would touch JS shutdown at this point? I don't know
Mozilla's shutdown sequence well enough to know if that can be guarenteed or at
least relied on.
Comment on attachment 107731 [details] [diff] [review]
patch v. 2

This patch actually doesn't fix the leak because the wrapper is still reachable
from the global object of the safe JS context.
Attachment #107731 - Attachment is obsolete: true

Comment 7

16 years ago
*** Bug 78861 has been marked as a duplicate of this bug. ***

Comment 8

16 years ago
Carrying over topembed status from duped bug 78861
Keywords: topembed+
Created attachment 117847 [details] [diff] [review]
patch to reorder mozJSComponentLoader shutdown

This patch helps some shutdown leaks, although I don't remember which ones. 
I've had it in my tree for a while.  It allows the JS component loader to do
its unrooting before XPConnect shuts down.

I think bryner and brendan also had some changes to the mozJSComponentLoader so
that it wouldn't leak JSPrincipals (which can then leak nsIPrincipals)
Comment on attachment 117847 [details] [diff] [review]
patch to reorder mozJSComponentLoader shutdown

This doesn't belong on this bug either.
Attachment #117847 - Attachment is obsolete: true
dbradley, didn't you have a patch that fixed this completely?  Were there other
problems with that patch?

Comment 12

16 years ago
Created attachment 119677 [details] [diff] [review]
Updated version

Thanks Brian, yes, I have a patch on bug 200030 that deals with a similar/same
issue.

I pulled this patch again and played with it. I don't see it clearing up the
leak, though. I'm posting a revised patch. It's fresher and fixes a problem in
the nsXPConnect's destructor.

What I'm trying to figure out, is if this patch has any value to our current
efforts to reduce shutdown leaks. At least from what I'm seeing it doesn't
address any of the shutdown leaks I'm seeing. The patch in bug 200030 does
remove two of the shutdown leaks. So the leaks this patch was trying to address
may have been addressed by the component loader fixes that went in.

dbaron, you have any thoughts on this?
Attachment #106413 - Attachment is obsolete: true

Comment 13

16 years ago
*** Bug 200030 has been marked as a duplicate of this bug. ***

Updated

16 years ago
Blocks: 78861

Updated

16 years ago
Attachment #107731 - Attachment is obsolete: false

Comment 14

16 years ago
Comment on attachment 119677 [details] [diff] [review]
Updated version

I picked up the wrong patch. I retrieved V2 and it still leaks for me.
Movinging up	 XPCPerThreadData::CleanupAllThreads();
to the top of the function clears up the leak.

The issue of components being created on shutdown has been addressed, but I'm
not sure the other issues mentioned in the comment have been.
Attachment #119677 - Attachment is obsolete: true

Comment 15

16 years ago
*** Bug 201067 has been marked as a duplicate of this bug. ***

Comment 16

16 years ago
Carrying other status whiteboard monikers from duped bug.
Whiteboard: edt_b3

Comment 17

16 years ago
Created attachment 120828 [details] [diff] [review]
Same as V2 but moves the CleanupAllThreads to the top

I've been running with this for a while, and tried various shutdown situations
(pages with plugins, JS Debugger, JS Console, etc). I think now that component
creation at shutdown has been addressed, this should be ok.

So this patch addresses the shutdown leaks and removes another holder of the
component manager, leaving only the Java OJI stuff.
Attachment #107731 - Attachment is obsolete: true

Updated

16 years ago
OS: Linux → All
Hardware: PC → All

Comment 18

15 years ago
*** Bug 227187 has been marked as a duplicate of this bug. ***

Updated

15 years ago
Attachment #120828 - Flags: review?(BradleyJunk)

Updated

13 years ago
Depends on: 316414

Updated

13 years ago
Attachment #120828 - Flags: review?(dbradley) → review?(jst)
Comment on attachment 120828 [details] [diff] [review]
Same as V2 but moves the CleanupAllThreads to the top

r=jst, assmuing this still compiles, and fixes the leaks :)
Attachment #120828 - Flags: review?(jst) → review+
The patch also needs supperreview by someone.
QA Contact: pschwartau → xpconnect

Updated

13 years ago
Attachment #120828 - Flags: superreview?(dbaron)

Updated

12 years ago
Attachment #120828 - Flags: review?(bzbarsky)
Comment on attachment 120828 [details] [diff] [review]
Same as V2 but moves the CleanupAllThreads to the top

Looks reasonable.  Let's get this checked in?
Attachment #120828 - Flags: superreview?(dbaron)
Attachment #120828 - Flags: superreview+
Attachment #120828 - Flags: review?(bzbarsky)
Attachment #120828 - Flags: review+
Flags: blocking1.9?
I've actually been tempted to superreview- this while it's been sitting in my review queue, so I'm not so sure.

jband had some good arguments on why we shouldn't need to propagate those releases, and I really don't see why we should need to if there weren't other leaks that shouldn't have happened.  Could somebody give a good reason why we really should need to propagate them?
What is it that we're releasing that we wouldn't be releasing otherwise?

(FWIW, I have an updated version of the patch without the reordering lying around, if anyone wants it...)
One of the previous discussions of these issues was in bug 76869, but I can't find the earlier one back when I argued for roughly the same reordering.

Maybe we should just do this, though.  But I'd like to know why the reordering part is really needed -- especially now that the JS component loader shuts down earlier, on xpcom-shutdown-loaders.  (Or is it still after XPConnect shutdown in FreeServices?)
Flags: blocking1.9? → blocking1.9-
(Assignee)

Comment 25

12 years ago
Created attachment 270043 [details] [diff] [review]
update to trunk
Attachment #120828 - Attachment is obsolete: true
(Assignee)

Comment 26

12 years ago
Created attachment 270046 [details]
bloat test with patch applied

I tried the patch without reordering ~XPConnect(), and it had no impact on leaking components objects. I'm not sure if the comment about FreeServices() is still relevant.
Blocks: 385237
(Assignee)

Comment 27

12 years ago
Comment on attachment 270043 [details] [diff] [review]
update to trunk

I can't find anything concrete describing the problems that might be caused by this reordering. Just vague references to "bad things", going all the way back to 1999 or so. I've been running with this patch and haven't noticed any problems. Are there specific questions that need answering before we land this?
Attachment #270043 - Flags: superreview?(dbaron)
Does this reordering mean that if there are real leaks, we'll free the native objects anyway at shutdown, thus hiding the leaks?
(Assignee)

Comment 29

12 years ago
(In reply to comment #28)
> Does this reordering mean that if there are real leaks, we'll free the native
> objects anyway at shutdown, thus hiding the leaks?

It depends what you mean by "leak". If you mean dangling references, as often seen in GCed languages, then yes, it will mask them from shutdown measurements.

For instance, a JS XPCOM service that maintained a global array of expensive objects and never deleted any members wouldn't be detected by Rlk, because all of those array members would be released at shutdown. I don't think we should depend on leaking at shutdown to detect these--I would rather get it to zero and increase the number of pages on the bloaturls list, while adding heap profiling.
I actually don't see how this patch would increase such behavior.  I think the thing it does that fixes reported leaks is clean up safe JS Contexts earlier (and safe JS contexts can have a Components property).  How does this mean that any of the behavior you cite in comment 29 would happen more as a result of this patch?
(Assignee)

Comment 31

12 years ago
(In reply to comment #30)
> I actually don't see how this patch would increase such behavior.  I think the
> thing it does that fixes reported leaks is clean up safe JS Contexts earlier
> (and safe JS contexts can have a Components property).  How does this mean that
> any of the behavior you cite in comment 29 would happen more as a result of
> this patch?

If creating a new safe context creates a new Components object that won't be GC'd, that would draw in all of the subproperties as well, like Components.classes and Components.interfaces. The behavior I have observed seems to indicate that this will also draw in things closed over by the nsIModule definition, which can include global objects declared above the COM stuff. Is this incorrect? See 

<http://lxr.mozilla.org/seamonkey/source/browser/fuel/src/fuelApplication.js#1317>

for an example.
I'll have a look at that tomorrow.  But two other comments on the patch:

 * we should call JS_NewContext before calling XPCPerThreadData::CleanupAllThreads to fix the leak of xpcPerThreadData that this causes

 * the xpcjsid.cpp changes seem unrelated and look like some debugging code; I don't see why they should be in the patch
I wonder if this fix actually masks the nsIFactory leak.  After all, isn't a service's nsIFactory unnecessary once the singleton has been created?  And even for a component that isn't a service, it's not clear to me why XPConnect should continue to hold a reference to its factory after using the factory to create an instance of the component.

But then I don't know much about the XPConnect code; perhaps I misunderstand how it operates.
The reason reordering ~nsXPConnect fixes the leaks of the wrappers for the global objects of the JS components is that the reordering means that we do a GC before calling the stuff inside the JS_BeginRequest/JS_EndRequest pair (which I'm told suppresses GC).  I think the reason it fixes the nsXPCComponents leak is that it's the nsXPCComponents attached to the safe JS Context, which is destroyed in CleanupAllThreads (triggering the GC).

So I think this patch is fine given the suggestions in comment 32.

I suppose it's probably fine that we clean up global variables in JS components before shutdown.  We already did, really, but some interned atoms were preventing things from being GCed at JS Component loader shutdown.  mrbkap said something about them going away when a script is destroyed, but I'm not sure how to find the right script.
(Assignee)

Comment 35

12 years ago
Created attachment 270435 [details]
bloat test with dbaron's comments addressed - leaking 296 bytes
Attachment #270046 - Attachment is obsolete: true
(Assignee)

Comment 36

12 years ago
Created attachment 270437 [details] [diff] [review]
address dbaron's comments
Assignee: dbaron → sayrer
Attachment #270043 - Attachment is obsolete: true
Attachment #270437 - Flags: superreview?(dbaron)
Attachment #270043 - Flags: superreview?(dbaron)
Comment on attachment 270437 [details] [diff] [review]
address dbaron's comments

>+    if (mRuntime && cx) {

This should just check |cx|, since cx can only be non-null if mRuntime is non-null.

sr=dbaron with that
Attachment #270437 - Flags: superreview?(dbaron) → superreview+
(Assignee)

Comment 38

12 years ago
Landed on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.