Don't assert GC invariants on shutdown if embedding leaks

RESOLVED FIXED in Firefox 51

Status

()

Core
JavaScript: GC
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

In general we want to assert that we free everything in a shutdown GC and that various tables end up empty, as they should be.  However if the embedding leaks JS GC things then these asserts will fail.  I think such a leak is reported later on outside the JS engine, but either way should distinguish these cases and report them separately.

We already do this for the GC things themselves and Zone::typeDescrObjects.  Other things this should apply to are the weak map list and shared script data table.
Created attachment 8780137 [details] [diff] [review]
bug1293209-shutdown-leaks

This adds state so that we can tell whether things were leaked at shutdown and avoids the assertions that will probably fail in that case.
Attachment #8780137 - Flags: review?(terrence)
What is the actual goal here? Would this go away if I landed bug 1290551? I expected to see everything from that bug in here, but it's missing some stuff. (I'm still hunting down the issue with bug 1237058, but I plan to get back to this soon).

My current blocker is that the nsWrapperCache is still live and marking things during the shutdown GC, very occasionally. Occasionally at roughly the same rate of our current shutdown hangs actually, which I think is what this is attacking? I think I'd prefer to land 1290551 with a hack to disable traversal of the nsWrapperCache and then attack the problem head-on by adding a finishGC hook that can clear the nsWrapperCache directly.

Thoughts?
(In reply to Terrence Cole [:terrence] from comment #2)
The goal is to not to crash with an assert in the SM GC if the browser leaks the world.  This gets reported later and these asserts mean that there are now lots of intermittent GC bugs to track which are really not our problem and confuse the issue (I'm assuming the leaks also get filed separately somewhere else).  So this patch doesn't fix anything, it just means leaks aren't sometimes reported as crashes.

If we don't mark anything then this will go away, yes.  But I'm not sure whether that's the best thing to do, as I commented.

> I think I'd prefer to land 1290551 with a hack to disable traversal of the nsWrapperCache and then attack the problem head-on by adding a finishGC hook that can clear the nsWrapperCache directly.

That sounds like a great idea.  Is there a way to traverse all wrappers easily?
(In reply to Jon Coppeard (:jonco) from comment #3)
> If we don't mark anything then this will go away, yes.  But I'm not sure
> whether that's the best thing to do, as I commented.

Sorry, I should have been clearer: I meant the updated patch that you haven't seen yet, that actually clears out cached roots where it's safe to do so.

> > I think I'd prefer to land 1290551 with a hack to disable traversal of the nsWrapperCache and then attack the problem head-on by adding a finishGC hook that can clear the nsWrapperCache directly.
> 
> That sounds like a great idea.  Is there a way to traverse all wrappers
> easily?

Yes, we can iterate the map with a custom JS tracer that warns about all edges into the GC: i.e. the real bug. I think if we're shutting down then doing the equivalent of DropJSObjects is probably what the CC wants regardless. We should discuss it with Andrew and Smaug. I'll try to get the first part of the patch series up today at the very least.
(In reply to Terrence Cole [:terrence] from comment #4)

> Sorry, I should have been clearer: I meant the updated patch that you
> haven't seen yet, that actually clears out cached roots where it's safe to
> do so.

That bug is about clearing tables of roots.  This bug is about checking whether tables of weak pointers end up empty after shutdown collection.

> Yes, we can iterate the map with a custom JS tracer that warns about all edges into the GC

Does that traverse the wrapper cache or all incoming grey edges?  Is there a difference? :)  Yes, let's discuss this further.


> 
> > > I think I'd prefer to land 1290551 with a hack to disable traversal of the nsWrapperCache and then attack the problem head-on by adding a finishGC hook that can clear the nsWrapperCache directly.
> > 
> > That sounds like a great idea.  Is there a way to traverse all wrappers
> > easily?
> 
> Yes, we can iterate the map with a custom JS tracer that warns about all
> edges into the GC: i.e. the real bug. I think if we're shutting down then
> doing the equivalent of DropJSObjects is probably what the CC wants
> regardless. We should discuss it with Andrew and Smaug. I'll try to get the
> first part of the patch series up today at the very least.
(In reply to Jon Coppeard (:jonco) from comment #5)
> (In reply to Terrence Cole [:terrence] from comment #4)
> 
> > Sorry, I should have been clearer: I meant the updated patch that you
> > haven't seen yet, that actually clears out cached roots where it's safe to
> > do so.
> 
> That bug is about clearing tables of roots.  This bug is about checking
> whether tables of weak pointers end up empty after shutdown collection.

Oh! Derp! I'll take another look today.

> > Yes, we can iterate the map with a custom JS tracer that warns about all edges into the GC
> 
> Does that traverse the wrapper cache or all incoming grey edges?  Is there a
> difference? :)  Yes, let's discuss this further.

There's also XPConnect stuff that gets traced there, I think? But largely no, no difference.
Comment on attachment 8780137 [details] [diff] [review]
bug1293209-shutdown-leaks

Review of attachment 8780137 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I forgot to get back to this! It looks great.
Attachment #8780137 - Flags: review?(terrence) → review+

Comment 8

a year ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/567ed465cf00
Don't assert tables are empty if the embedding leaked JS GC things r=terrence

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/567ed465cf00
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.