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.

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

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

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.

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

Carrying over topembed status from duped bug 78861

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.

dbradley, didn't you have a patch that fixed this completely? Were there other problems with that patch?

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?

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

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.

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

Carrying other status whiteboard monikers from duped bug.

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.

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

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

The patch also needs supperreview by someone.

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?

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

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.

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?

Does this reordering mean that if there are real leaks, we'll free the native objects anyway at shutdown, thus hiding the leaks?

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

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

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

Landed on trunk.