Assertion failure: zone->runtimeFromAnyThread()->gc.nursery.isEmpty()

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jandem, Assigned: jimb)

Tracking

(Blocks: 1 bug)

unspecified
mozilla47
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

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

3 years ago
Created attachment 8708049 [details] [diff] [review]
Add some asserts

This adds 2 asserts.

Maybe one of them will fail some day.
Attachment #8708049 - Flags: review?(terrence)
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

3 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

3 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

3 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

3 years ago
Assignee: nobody → jimb
(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.
Blocks: 1248467
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

3 years ago
Try push for patch I'm about to attach:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffcfffe3623b
(Assignee)

Comment 9

3 years ago
Created attachment 8721571 [details] [diff] [review]
Ensure compartments don't get GC'd while Debugger.prototype.findScripts' ScriptQuery is holding them in its HashSet.
Attachment #8721571 - Flags: review?(sphink)
(Assignee)

Comment 10

3 years ago
Try push looks excellent.
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

3 years ago
Flags: in-testsuite+
Target Milestone: --- → mozilla47

Comment 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/db9aec3d777c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 14

3 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

3 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

3 years ago
Flags: needinfo?(jdemooij)
You need to log in before you can comment on or make changes to this bug.