975639 - compartments for JS components stay around forever

Status: NEW

(Core :: XPConnect, defect)

Description

[Florian Quèze [:florian]]

When looking at about:memory, I noticed a JS compartment for each command line handler, whereas I would have expected these components to only play a role during startup.

An example of these components is nsSetDefaultBrowser.js

I looked at the code using command line handlers. These components are created by nsCommandLine::EnumerateHandlers in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/commandlines/nsCommandLine.cpp :
524     nsCOMPtr<nsICommandLineHandler> clh(do_GetService(contractID.get()));

I wondered if do_GetService may cause a reference to be kept, so I replaced it with do_CreateInstance, but that didn't help (nsSetDefaultBrowser.js was still visible in about:memory after clicking "Minimize memory usage").

I reduced nsSetDefaultBrowser.js to see what's causing it to be leaked. Apparently it stays around if it contains an NSGetFactory function (even a completely empty one), but doesn't if NSGetFactory doesn't exist.

This directed me to mozJSComponentLoader::LoadModule that touches the NSGetFactory function.

I suspect that the code causing the compartment to stay around is:
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/mozJSComponentLoader.cpp#503
503     // Cache this module for later
504     mModules.Put(spec, entry);

If this analysis is correct, then the problem isn't limited to command line handlers, but applies to all JS components that are used once and are unlikely to ever be used again during the lifetime of the browser process.

Comment 1

[Bobby Holley (:bholley)]

That's a neat idea. We could make the cache entries weak and allow them to go away if the module code itself was GCed. The precise design and heuristics will obviously take some thought and measurement, though.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #1)
> That's a neat idea. We could make the cache entries weak and allow them to
> go away if the module code itself was GCed. The precise design and
> heuristics will obviously take some thought and measurement, though.

Though this would change behavior, I suppose, since the GC would cause us to throw away any static state. So in the simplest case, a global counter of "how many times have I been called?" would give the wrong answer if it was unloaded.

Also, how many of these modules end up same-compartment in b2g? Is it just JSMs, or Known Module components as well?

Comment 3

[:Gijs (he/him)]

(In reply to Bobby Holley (:bholley) from comment #2)
> (In reply to Bobby Holley (:bholley) from comment #1)
> > That's a neat idea. We could make the cache entries weak and allow them to
> > go away if the module code itself was GCed. The precise design and
> > heuristics will obviously take some thought and measurement, though.
> 
> Though this would change behavior, I suppose, since the GC would cause us to
> throw away any static state. So in the simplest case, a global counter of
> "how many times have I been called?" would give the wrong answer if it was
> unloaded.

Can we instead (shudder, twitch) introduce an API for modules to unload themselves? I'm worried about add-on/consumer impact if we really make created services non-strong-refs-by-default.

Alternatively, we could provide an alternative NSGetFactory (NSGetWeakFactory?) entrypoint that would cause the reference to be weak? Would that reduce compatibility concerns?

Comment 4

[Benjamin Smedberg]

XPCOM *services* are permanent by design and we really shouldn't mess with that. There's a *lot* of code that relies on service instantiation effects.

Reworking NSGetFactory or providing an alternate entry point with reload-on-demand behavior might work, although there's a lot of risk.

Note that command-line handlers are not only run at startup; they are run on remote also, so explicit unloading won't solve this case unless you allow for reloading.

Comment 5

[Florian Quèze [:florian]]

(In reply to :Gijs Kruitbosch (PTO april 28) from comment #3)

> I'm worried about add-on/consumer impact if we really make
> created services non-strong-refs-by-default.

As I wrote in comment 0, replacing do_GetService with do_CreateInstance still causes the compartment to stay alive. I'm not surprised that something created with GetService stays alive.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> (In reply to :Gijs Kruitbosch (PTO april 28) from comment #3)
> 
> > I'm worried about add-on/consumer impact if we really make
> > created services non-strong-refs-by-default.
> 
> As I wrote in comment 0, replacing do_GetService with do_CreateInstance
> still causes the compartment to stay alive. I'm not surprised that something
> created with GetService stays alive.

Doesn't CreateInstance do the same thing as GetService for services?

I think bsmedberg is the guy to ask about XPCOM modules. I have thoughts about unloading JSMs, which we currently don't do, and should do more of.

Comment 7

[Benjamin Smedberg]

getservice does a createinstance under the hood but stores the service object until shutdown.

Comment 8

[:Gijs (he/him)]

Excuse me while I attempt to keep this moving, even if I'm probably not the perfect person to do so:

How exactly does the dependency run between JS compartments and their content? I'm not intimately familiar with how they use refcounting/GC and how the JS, XPCOM, and JS loader bits interact in this space. The following may be wrong, but here's my understanding:

Presumably, as long as someone has a reference to some XPCOM object you createinstance'd from a js file in compartment X, and the XPCOM object has access to variables in the JS file (which it is pretty much guaranteed to, right?), the reference to the createinstance'd object keeps the js file's context and therefore its compartment alive (even if I'm not sure through which mechanism that happens).

From comment #0, it seems like even if you remove all references to the thing you createinstance'd, right now, the compartment will still be alive, because we keep it in mModules in the component loader.

From that, it would seem like we could make JS files that we load be "purgable" if we know that we will be reloading them if/when necessary, irrespective of the longevity of the associated XPCOM objects, as long as those objects being alive do keep their respective compartments alive.

Bobby, is the above accurate? If so, how hard would implementing an opt-in way for modules to work like this be?

And, separately, but no less importantly: Do you think doing so will be worth it in the effort/reward department, also considering we'll probably have to change consumers to work with this new mechanism? (e.g. through having them make createInstance calls rather than a getService call, which would lead to stuff remaining cached and therefore defeating the point of this bug)

Comment 9

[Bobby Holley (:bholley)]

(In reply to :Gijs Kruitbosch from comment #8)
> Presumably, as long as someone has a reference to some XPCOM object you
> createinstance'd from a js file in compartment X, and the XPCOM object has
> access to variables in the JS file (which it is pretty much guaranteed to,
> right?), the reference to the createinstance'd object keeps the js file's
> context and therefore its compartment alive (even if I'm not sure through
> which mechanism that happens).

Correct. If you're holding an nsCOMPtr to an nsIFoo that is implemented in JS, the nsCOMPtr secretly points to an XPCWrappedJS, which roots its underlying JS object. 

> From comment #0, it seems like even if you remove all references to the
> thing you createinstance'd, right now, the compartment will still be alive,
> because we keep it in mModules in the component loader.

Precisely.

> From that, it would seem like we could make JS files that we load be
> "purgable" if we know that we will be reloading them if/when necessary,
> irrespective of the longevity of the associated XPCOM objects, as long as
> those objects being alive do keep their respective compartments alive.

As long as this is an opt-in thing whereby the author accepts the fact that any global state in the component can go away at any time, yes.

> Bobby, is the above accurate?

Yes. \o/

> If so, how hard would implementing an opt-in
> way for modules to work like this be?

It's doable. It just needs somebody with gumption to jump in and tackle it. I'm happy to mentor.

> And, separately, but no less importantly: Do you think doing so will be
> worth it in the effort/reward department, also considering we'll probably
> have to change consumers to work with this new mechanism? (e.g. through
> having them make createInstance calls rather than a getService call, which
> would lead to stuff remaining cached and therefore defeating the point of
> this bug)

If we're detecting a sizable memory footprint of infrequently-used JS components, this is probably worth doing, yes.

Comment 10

[:Gijs (he/him)]

(In reply to Bobby Holley (:bholley) from comment #9)
> (In reply to :Gijs Kruitbosch from comment #8)
> > If so, how hard would implementing an opt-in
> > way for modules to work like this be?
> 
> It's doable. It just needs somebody with gumption to jump in and tackle it.
> I'm happy to mentor.

OK... I can tentatively sign up for this (having looked at 0 actual code so far, which makes it all sound much easier than it will be, if my previous foray into xpconnect is anything to go by!) assuming the deadline is "somewhere in the coming 6 weeks, ideally, but it's not a disaster if it slips to 33 or 34"...

First question: what would be the right "opt-in" here? Can this be something like "use strict"? Is there a better way of dealing with this? (should we make it an opt-in by the thing that loads the file, rather than the file itself?)

Second question: once we have an opt-in, is it just a question of removing the mModules.Put call, and letting GC and friends do their job, or is there more to it?

Comment 11

[Bobby Holley (:bholley)]

(In reply to :Gijs Kruitbosch from comment #10)
> OK... I can tentatively sign up for this (having looked at 0 actual code so
> far, which makes it all sound much easier than it will be, if my previous
> foray into xpconnect is anything to go by!) assuming the deadline is
> "somewhere in the coming 6 weeks, ideally, but it's not a disaster if it
> slips to 33 or 34"...

Well, there's really no deadline. It's just a question of when someone finds cycles to work on it. :-)

I assume we're mostly interested in this for b2g? Note that at the moment, we can't unload modules on b2g, due to the compartment sharing hack. See the second part of bug 980537 comment 28. So we either need to kill the compartment sharing hack (which I have hopes of doing), or sort that code out some other way.

> First question: what would be the right "opt-in" here? Can this be something
> like "use strict"? Is there a better way of dealing with this? (should we
> make it an opt-in by the thing that loads the file, rather than the file
> itself?)

I would think that this information should go in the manifest somewhere. But barring that, the module could opt in by calling something like Cu.allowUnloading().

> Second question: once we have an opt-in, is it just a question of removing
> the mModules.Put call, and letting GC and friends do their job, or is there
> more to it?

We still need to store this in mModules to avoid creating a new compartment each time somebody does createInstance, right? It seems like we'd want to trigger the Unload code somehow - possibly via some sort of reference-counting scheme on the ModuleEntry.

Comment 12

[:Gijs (he/him)]

(In reply to Bobby Holley (:bholley) from comment #11)
> (In reply to :Gijs Kruitbosch from comment #10)
> > OK... I can tentatively sign up for this (having looked at 0 actual code so
> > far, which makes it all sound much easier than it will be, if my previous
> > foray into xpconnect is anything to go by!) assuming the deadline is
> > "somewhere in the coming 6 weeks, ideally, but it's not a disaster if it
> > slips to 33 or 34"...
> 
> Well, there's really no deadline. It's just a question of when someone finds
> cycles to work on it. :-)
> 
> I assume we're mostly interested in this for b2g? Note that at the moment,
> we can't unload modules on b2g, due to the compartment sharing hack. See the
> second part of bug 980537 comment 28. So we either need to kill the
> compartment sharing hack (which I have hopes of doing), or sort that code
> out some other way.

Not sure. I'd imagine it'd be nice to have on other platforms as well.

> > First question: what would be the right "opt-in" here? Can this be something
> > like "use strict"? Is there a better way of dealing with this? (should we
> > make it an opt-in by the thing that loads the file, rather than the file
> > itself?)
> 
> I would think that this information should go in the manifest somewhere. But
> barring that, the module could opt in by calling something like
> Cu.allowUnloading().

OK. I would imagine that not all things that go through the JS component loader come from a manifest, so I'm more tempted by the second option, but I could be convinced otherwise (maybe I misunderstand?). :-)

> > Second question: once we have an opt-in, is it just a question of removing
> > the mModules.Put call, and letting GC and friends do their job, or is there
> > more to it?
> 
> We still need to store this in mModules to avoid creating a new compartment
> each time somebody does createInstance, right? It seems like we'd want to
> trigger the Unload code somehow - possibly via some sort of
> reference-counting scheme on the ModuleEntry.

Hrm. This worries me more. Presumably the reference counting is actually going to be about objects that keep the compartment alive. Is it easy to figure out if there are no such objects, or rather, to get notified when that situation occurs? And, (somewhat hypothetical as it is related to the previous question) how do we prevent Unload from being called immediately after creation, when there are no objects (yet) that are keeping a hold of the compartment?

Comment 13

[Bobby Holley (:bholley)]

(In reply to :Gijs Kruitbosch from comment #12)
> OK. I would imagine that not all things that go through the JS component
> loader come from a manifest, so I'm more tempted by the second option, but I
> could be convinced otherwise (maybe I misunderstand?). :-)

Well, it depends if we want this just for js-implemented components, or for JSMs as well.
 
> > > Second question: once we have an opt-in, is it just a question of removing
> > > the mModules.Put call, and letting GC and friends do their job, or is there
> > > more to it?
> > 
> > We still need to store this in mModules to avoid creating a new compartment
> > each time somebody does createInstance, right? It seems like we'd want to
> > trigger the Unload code somehow - possibly via some sort of
> > reference-counting scheme on the ModuleEntry.
> 
> Hrm. This worries me more. Presumably the reference counting is actually
> going to be about objects that keep the compartment alive. Is it easy to
> figure out if there are no such objects, or rather, to get notified when
> that situation occurs? And, (somewhat hypothetical as it is related to the
> previous question) how do we prevent Unload from being called immediately
> after creation, when there are no objects (yet) that are keeping a hold of
> the compartment?

Thinking about it more, I think we probably want to use weak (non-rooted) pointers from the ModuleEntry to the global, and get a notification somehow when the global and compartment are collected, at which point we can jettison the ModuleEntry. That collection won't happen until any XPCWrappedJSes are finished with the compartment anyway.

Comment 14

[Eric Rahm [:erahm]]

Gijs, do you still have any interest in pursuing this? It seems like it may be more important now that we're thinking about multiple content processes.

Comment 15

[:Gijs (he/him)]

(In reply to Eric Rahm [:erahm] from comment #14)
> Gijs, do you still have any interest in pursuing this? It seems like it may
> be more important now that we're thinking about multiple content processes.

I would not expect the components that Florian mentioned in comment #0 to be run/loaded *at all* for the (multiple) content process(es). If they are, then avoiding that altogether seems like an easier and more fruitful thing to do.

Otherwise, interested yes, but I don't have the necessary skills to fix this myself.