375063 - Leaking nsJSContexts and nsXBLDocGlobalObjects at shutdown

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Van der Beken [:peterv]]

We're leaking a couple of nsXBLDocGlobalObjects and the corresponding nsJSContexts at shutdown. They're in a cycle with JS objects and we're missing one edge to the JS global object. It took me a while to figure out which edge, but it turns out that the JS context has a 'weak root' to an object which ends up holding the global object through its parent chain. There are two ways to solve this:

- Clear weak roots on all JS contexts before calling the GC for cycle
  collection
- Traverse the weak root from the nsJSContext (which holds the JS context in
  quesion)

Because it's not clear to me how safe the first solution would be I've done the second. There are other objects that hold a JS context (for example mozJSComponentLoader or XPCJSContextStack in its safe context), the first solution would solve it for those contexts too, but those objects currently don't participate in cycle collection.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Let me know if you think solution 1 is safe and you'd prefer if I did that (see comment 0).

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 259400 [details] [diff] [review]
v1

It's better not to clear weak roots of cx's other than the one on which GC is being invoked, for ultimate JS API compatibility.

Asking igor for his thoughts in addition to mine.

/be

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> Asking igor for his thoughts in addition to mine.

But why the generic marking code in js_GC is not enough? It does call gcThingCallback for all the weak roots.

Comment 4

[Peter Van der Beken [:peterv]]

Because this is a different operation, gcThingCallback is used to count all the edges and the traversal code then tries to find all those edges. The gcThingCallback is getting called, but the traversal code was unaware of this edge, so it didn't find all the edges that gcThingCallback was called for.

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> the traversal code was unaware of this
> edge, so it didn't find all the edges that gcThingCallback was called for.

Here is a picture that illustrates my understanding:

nsJSContext -> JSContext -> JS weakRoot -> ... -> JS XPCOM wrapper   
   ^                                                      |
   |------------------- XPCOM  <- .......... <-  XPCOM  <--

And the problem in fact was that the traversal was unaware of the fact that it was exactly nsJSContext that points to the weakRoot indirectly via

nsJSContext -> JSContext -> JS weakRoot

That is, the edge into weakRoot was counted, but it was unknown who points to it. 

If my understanding correct, it tells that the right solution is to make JSContext a part of the traversal instead of dealing with JSContext.globalObject, JSContext.weakRoots.newborn, JSContext.weakRoots.lastAtom, JSContext.exception etc. explicitly. Is it feasible?

Comment 6

[Peter Van der Beken [:peterv]]

Making JSContext participate is harder, because currently the cycle collector only deals with JSObjects or XPCOM objects, and JSContext is neither. Or are you proposing to add a JS API that returns all JSObjects reachable from a context?

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> Making JSContext participate is harder, because currently the cycle collector
> only deals with JSObjects or XPCOM objects, and JSContext is neither. Or are
> you proposing to add a JS API that returns all JSObjects reachable from a
> context?

That is an extremely good idea! We should really add API that would allow to enumerate all GC roots stored in JSContext rather trying to find all the roots in  nsJSContext. 

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> 
> We should really add API that would allow to
> enumerate all GC roots stored in JSContext rather trying to find all the roots
> in  nsJSContext. 

I think that API does not need to be public, just accessible from nsJSContext. And if we can also use it in js_GC to mark this roots, than it would warrant that no roots would be missed.

Comment 9

[Peter Van der Beken [:peterv]]

(In reply to comment #8)
> I think that API does not need to be public, just accessible from nsJSContext.
> And if we can also use it in js_GC to mark this roots, than it would warrant
> that no roots would be missed.

Adding that API is fine by me, I guess it would call a callback for every JSObject reachable from the JSContext? If brendan is ok with it, could you help by writing the js/ patch?

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> Adding that API is fine by me, I guess it would call a callback for every
> JSObject reachable from the JSContext? 

There is one problem here. JSContext contains non-gc-things like JSAtom*, JSScopeProp* that are still controlled by GC. Moreover, both JSAtom* and JSScopeProp* can point indirectly to JSObject* that are XPCOM wrappers. Thus the enumeration callback must carefully consider all such cases to extract potential loop sources from JSAtom* and JSScopeProp*. This is a lot of new code effectively duplicating js_GC marking logic. 

So I reiterate my question: how hard would to extend the cycle collector to teach it about JSContext, JSAtom and JSScopeProp ?

Comment 11

[Peter Van der Beken [:peterv]]

(In reply to comment #10)
> So I reiterate my question: how hard would to extend the cycle collector to
> teach it about JSContext, JSAtom and JSScopeProp ?

It's doable, but not trivial. I'm not sure what you're trying to accomplish though, almost all of the JSContexts in Mozilla will have a corresponding nsJSContext, and there's a one-to-one relationship between them, the nsJSContext holds the JSContext alive as long as it's alive. So if you want to graph the ownership model, traversing the JSContext directly from the nsJSContext, or treating the JSContext as an object on its own doesn't make a difference because there's only ever one edge from nsJSContext to JSContext. The issue is how to find out what JSObjects are marked by the JSContext itself.

JSAtoms (and JSScopeProps?) are slightly different I guess. There could be multiple edges into JSAtoms, currently (after landing bug 372960) I made the GC call the gcThingCallback for the JSObject that a JSAtom holds as many times as there are edges into the JSAtom. That allows us to ignore the JSAtom.
If I understand correctly you would make the GC call gcThingCallback (or another callback) to deal with non-gc-things? We can do that, but again, the issue is mostly how to find out what JSObjects are marked by whom and that doesn't get solved with this callback.

Comment 12

[Igor Bukanov]

(In reply to comment #11)
> (In reply to comment #10)
> > So I reiterate my question: how hard would to extend the cycle collector to
> > teach it about JSContext, JSAtom and JSScopeProp ?
> 
> It's doable, but not trivial. I'm not sure what you're trying to accomplish
> though,

Well, the idea is to reuse the current code marking in js_GC so one would not forget to update the enumerator after adding new GC-controlled things.

> almost all of the JSContexts in Mozilla will have a corresponding
> nsJSContext, and there's a one-to-one relationship between them, the
> nsJSContext holds the JSContext alive as long as it's alive. So if you want to
> graph the ownership model, traversing the JSContext directly from the
> nsJSContext, or treating the JSContext as an object on its own doesn't make a
> difference because there's only ever one edge from nsJSContext to JSContext.
> The issue is how to find out what JSObjects are marked by the JSContext itself.

Then the code would almost duplicate the efforts of the patch in bug 372960 as would also need to go into scripts, SScopeProperty etc. to find JSObject*. Plus the patch is incomplete, see my comments there.

Comment 13

[Peter Van der Beken [:peterv]]

This will probably be fixed by fixing bug 377884.

Comment 14

[Peter Van der Beken [:peterv]]

Attachment: v1

Unlinking mScriptContext seems safe, we nullcheck it before use. This fixes a bunch of shutdown leaks.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 266066 [details] [diff] [review]
v1

r+sr=jst

Comment 16

[Peter Van der Beken [:peterv]]

Rlk dropped from 5.23KB to 2.91KB, Lk dropped from 1.81MB to 1.17MB.