Leaking nsJSContexts and nsXBLDocGlobalObjects at shutdown

RESOLVED FIXED in mozilla1.9alpha5

Status

()

RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

({memory-leak})

Trunk
mozilla1.9alpha5
memory-leak
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
Created attachment 259400 [details] [diff] [review]
v1

Let me know if you think solution 1 is safe and you'd prefer if I did that (see comment 0).
Attachment #259400 - Flags: review?(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
Attachment #259400 - Flags: review?(igor)
Attachment #259400 - Flags: review?(brendan)
Attachment #259400 - Flags: review+

Comment 3

12 years ago
(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.
(Assignee)

Comment 4

12 years ago
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

12 years ago
(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?
(Assignee)

Comment 6

12 years ago
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

12 years ago
(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

12 years ago
(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.
(Assignee)

Comment 9

12 years ago
(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

12 years ago
(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 ?
(Assignee)

Comment 11

12 years ago
(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

12 years ago
(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.
(Assignee)

Updated

12 years ago
Depends on: 377884

Updated

12 years ago
Blocks: 381239
(Assignee)

Comment 13

12 years ago
This will probably be fixed by fixing bug 377884.
(Assignee)

Comment 14

12 years ago
Created attachment 266066 [details] [diff] [review]
v1

Unlinking mScriptContext seems safe, we nullcheck it before use. This fixes a bunch of shutdown leaks.
Attachment #259400 - Attachment is obsolete: true
Attachment #266066 - Flags: superreview?(jst)
Attachment #266066 - Flags: review?(jst)
Attachment #259400 - Flags: review?(igor)
Comment on attachment 266066 [details] [diff] [review]
v1

r+sr=jst
Attachment #266066 - Flags: superreview?(jst)
Attachment #266066 - Flags: superreview+
Attachment #266066 - Flags: review?(jst)
Attachment #266066 - Flags: review+
(Assignee)

Comment 16

12 years ago
Rlk dropped from 5.23KB to 2.91KB, Lk dropped from 1.81MB to 1.17MB.
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha5

Updated

12 years ago
Flags: in-testsuite-

Updated

11 years ago
Keywords: mlk
You need to log in before you can comment on or make changes to this bug.