76869 - JS component loader leaks

Status: RESOLVED INCOMPLETE

(Core :: XPConnect, defect, P4)

Description

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

The JS component loader leaks when JS components are loaded.  There are 2
causes for the leak, one of which I don't fully understand yet:

 1) The XPConnect singleton gets shutdown before the JS component loader.  This
can be fixed by having the JS component loader own a reference to the XPConnect
singleton.

 2) The call to CanUnload in UnloadAndReleaseModules returns NS_ERROR_FAILURE,
so the module doesn't get released.  The leak can be fixed by releasing anyway
if CanUnload returns a failure status, but I'm still looking for a way to fix
the failure return.

Comment 1

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

Attachment: patch fixing shutdown leaks

Comment 2

[John Bandhauer]

> 1) The XPConnect singleton gets shutdown before the JS component loader.  
> This can be fixed by having the JS component loader own a reference to the 
> XPConnect singleton.

No. We've been over this ground before. Delaying xpconnect shutdown causes too 
many bad things to happen throughout the application during shutdown of various 
services.

When xpconnect shuts down it sets up walls so that JS code and native code can't 
call either way through xpconnect. We have found many times that by delaying 
this we can fix some shutdown leaks, but the cost turns out to be calls into 
code *all* over the place* that is in an unstable state because the service 
manager is by then not vending services and the various poormans's 
weakreferences that so much code relies on have become dangling pointers.

We trade shutdown leaks for shutdown crashes. I'll go with the leaks.

Maybe I'm being a luddite here, but I don't think we can safely assume that we 
can fix *all* these kinds of things. there is too *stuff* out there and too much 
uncertainly of shutdown order etc.

It may be that we can get some relief by adding a system to control from 
xpcom the shutdown order of component loaders and well known services.

Comment 3

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

OK.

Comment 4

[Phil Schwartau]

Marking Verified - 

Comment 5

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

Reopening so I remember to investigate why we get the NS_ERROR_FAILURE return I
mentioned in point (2).

Comment 6

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

(This leak has become more of an issue again in regular leak detection, and is
adding noise, thanks to nsCloseAllWindows.js.)

One other possibility here is that mozJSComponentLoader::UnloadAll do unrooting
in addition to just calling CanUnload on all the modules.  It seems to me that
the code in the mozJSComponentLoader destructor will never do anything useful
since it is guaranteed to be run after XPConnect shutdown (at the earliest, from
the loop to release the loaders in nsComponentManagerImpl::Shutdown, which is
after module destructors are called).

I don't think this violates either of the rules that (1) XPConnect (as is any
module (now, anyway) implementing features allowing modules to be implemented in
non-native code) is just a normal native code module and (2) once we've started
to unload modules, we shouldn't propagate any calls across modules, since the
non-native component loaders' UnloadAll methods are called (see
nsComponentManagerImpl::UnloadLibraries) before any of the native modules are
unloaded.

But actually, thinking through this makes me notice a few problems:
 * The loop in nsComponentManagerImpl::Shutdown to release the loaders is
happening too late, since it's propagating releases into native code modules
after module shutdown (which is when we would be unloading libraries if we
unloaded libraries).  (This, I think, could be fixed by folding the releases of
the loaders into UnloadAll itself, rather than doing them in a separate loop in
Shutdown.)
 * nsComponentManagerImpl::UnloadAll doesn't consider mStaticComponentLoader
specially, but it probably needs to.

Comment 7

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

Attachment: patch fixing shutdown leaks

This is sufficient to fix the shutdown leaks, but it doesn't fix the problems I
mentioned in my previous comment.