JS component loader leaks

RESOLVED INCOMPLETE

Status

()

Core
XPConnect
P4
normal
RESOLVED INCOMPLETE
17 years ago
10 months ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({memory-leak})

Trunk
Future
x86
Linux
memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tind-mlk])

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 31592 [details] [diff] [review]
patch fixing shutdown leaks
Keywords: mlk
Whiteboard: [tind-mlk]

Comment 2

17 years ago
> 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.
OK.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → WONTFIX

Comment 4

17 years ago
Marking Verified - 
Status: RESOLVED → VERIFIED
Reopening so I remember to investigate why we get the NS_ERROR_FAILURE return I
mentioned in point (2).
Status: VERIFIED → REOPENED
Resolution: WONTFIX → ---
Status: REOPENED → ASSIGNED
Priority: -- → P4
Target Milestone: --- → mozilla1.1

Updated

17 years ago
Blocks: 92580

Updated

17 years ago
No longer blocks: 92580
Target Milestone: mozilla1.1 → Future
(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.
Created attachment 113912 [details] [diff] [review]
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.
Attachment #31592 - Attachment is obsolete: true
QA Contact: pschwartau → xpconnect

Updated

10 months ago
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago10 months ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.