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.

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

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

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

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.

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

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?

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

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

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

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

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

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

This will probably be fixed by fixing bug 377884.

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

Comment on attachment 266066 [details] [diff] [review] v1 r+sr=jst

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