Closed
Bug 1239813
Opened 8 years ago
Closed 8 years ago
Assertion failure: zone->runtimeFromAnyThread()->gc.nursery.isEmpty()
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: jandem, Assigned: jimb)
Details
Attachments
(2 files)
1.01 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
2.56 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
Noticed this assertion failure on inbound on a devtools test: https://treeherder.mozilla.org/logviewer.html#?job_id=19826756&repo=mozilla-inbound In IterateScripts we do: rt->gc.evictNursery(); AutoPrepareForTracing prep(rt, SkipAtoms); if (compartment) { for (ZoneCellIterUnderGC i(compartment->zone(), gc::AllocKind::SCRIPT); ... ZoneCellIterUnderGC asserts because the nursery is not empty, but that's weird because we just called evictNursery. I don't see how but maybe GC is suppressed: evictNursery is a no-op in that case. Or maybe AutoPrepareForTracing ends up allocating stuff in the nursery. Maybe can add some asserts to guard against both of these.
Reporter | ||
Comment 1•8 years ago
|
||
This adds 2 asserts. Maybe one of them will fail some day.
Attachment #8708049 -
Flags: review?(terrence)
Comment 2•8 years ago
|
||
Comment on attachment 8708049 [details] [diff] [review] Add some asserts Review of attachment 8708049 [details] [diff] [review]: ----------------------------------------------------------------- I like having those asserts there, just to narrow down the insanity if it happens again.
Attachment #8708049 -
Flags: review?(terrence) → review+
Reporter | ||
Comment 3•8 years ago
|
||
Bah, I forgot about this bug, but just noticed that this is still happening: https://treeherder.mozilla.org/logviewer.html#?job_id=21681950&repo=mozilla-inbound I'll get this patch landed. There's definitely something weird going on here.
Reporter | ||
Comment 4•8 years ago
|
||
Hm thinking about this more, it occurred to me Debugger.findScripts is buggy. Consider this: (1) We call Debugger.findScripts(..) (2) We add all debuggee compartments to ScriptQuery. (3) Then in ScriptQuery::parseQuery, we call GetProperty multiple times and this might trigger a GC (while allocating wrappers for instance). The GC can destroy one of the compartments we added to ScriptQuery. (4) Now accessing the ScriptQuery's compartments can crash or behave incorrectly. Below is a shell testcase that crashes in the ZoneCellIterUnderGC constructor: var g = newGlobal(); var dbg = new Debugger(); dbg.addDebuggee(g); g = newGlobal({sameZoneAs: g.Math}); dbg.findScripts({get source() { gc(); return undefined; }}); ObjectQuery and others may have the same issue.
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(jimb)
Assignee | ||
Comment 5•8 years ago
|
||
EXCELLENT. Is there a reason that the static analysis didn't catch this? Doesn't it track JSCompartment pointers like everything else?
Flags: needinfo?(jimb)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jimb
Comment 6•8 years ago
|
||
(In reply to Jim Blandy :jimb from comment #5) > EXCELLENT. > > Is there a reason that the static analysis didn't catch this? Doesn't it > track JSCompartment pointers like everything else? No. JSCompartment is not a GC type. I tried making the analysis treat JSCompartment* as a GCPointer once, because I found a place where we were collecting compartments while iterating over them, with predictable results. But there turned out to be some users that were difficult to reliably fix, so I took the cowardly way out in bug 1120655 and suppressed collection *of zones/compartments only* while iterating. Sadly, this code looks like it is keeping compartment iteration state externally, so the mechanism from bug 1120655 does not apply. Perhaps ScriptQuery could contain an AutoEnterIteration field? I suppose the nice way to extend the analysis to cover this sort of error would be to split GC into "regular GC" and "compartment-safe GC", which would be identical in all ways except AutoEnterIteration would suppress "regular GC" (only) within its scope. Then we could label JSCompartment* as a GC pointer for the purpose of regular GC but not for compartment-safe GCs. (I do not want to simply treat AutoEnterIteration as equivalent to AutoSuppressGC, as that would remove analysis coverage from anything dominated by an AutoEnterIteration's scope and we have caught multiple hazards in such things.) But that would take some doing, since nothing like that is currently implemented in the analysis. So I'm disinclined to do it until we find more errors of this flavor.
Comment 7•8 years ago
|
||
Alternatively, we could do all the things that could GC up front, then add the compartments, iterate under an AutoCheckCannotGC while accumulating results in a GC-safe vector or something, and then convert that to the array results object after iteration.
Flags: needinfo?(nfitzgerald)
Assignee | ||
Comment 8•8 years ago
|
||
Try push for patch I'm about to attach: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffcfffe3623b
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #8721571 -
Flags: review?(sphink)
Assignee | ||
Comment 10•8 years ago
|
||
Try push looks excellent.
Comment 11•8 years ago
|
||
Comment on attachment 8721571 [details] [diff] [review] Ensure compartments don't get GC'd while Debugger.prototype.findScripts' ScriptQuery is holding them in its HashSet. Review of attachment 8721571 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +3850,5 @@ > > /* The debugger for which we conduct queries. */ > Debugger* debugger; > > + gc::AutoEnterIteration iterMarker; Please add a comment, such as /* Require the set of compartments to stay fixed while the ScriptQuery is alive. */
Attachment #8721571 -
Flags: review?(sphink) → review+
Assignee | ||
Updated•8 years ago
|
Flags: in-testsuite+
Target Milestone: --- → mozilla47
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/db9aec3d777c
Assignee | ||
Comment 14•8 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #3) > Bah, I forgot about this bug, but just noticed that this is still happening: > > https://treeherder.mozilla.org/logviewer.html#?job_id=21681950&repo=mozilla- > inbound > > I'll get this patch landed. There's definitely something weird going on here. Hi, Jandem. Could you land this? I think it might help with bug 1240231.
Flags: needinfo?(jdemooij)
Reporter | ||
Comment 16•8 years ago
|
||
(In reply to Jim Blandy :jimb from comment #14) > Hi, Jandem. Could you land this? I think it might help with bug 1240231. Just pushed it, sorry for the delay! I was hoping your patch here would fix those oranges, but apparently not. Let's hope this will help.
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(jdemooij)
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b80d52309b26
You need to log in
before you can comment on or make changes to this bug.
Description
•