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

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jan de Mooij [:jandem]]

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.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Add some asserts

This adds 2 asserts.

Maybe one of them will fail some day.

Comment 2

[Steve Fink [:sfink] [:s:]]

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.

Comment 3

[Jan de Mooij [:jandem]]

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.

Comment 4

[Jan de Mooij [:jandem]]

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.

Comment 5

[Jim Blandy :jimb]

EXCELLENT.

Is there a reason that the static analysis didn't catch this? Doesn't it track JSCompartment pointers like everything else?

Comment 6

[Steve Fink [:sfink] [:s:]]

(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

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 8

[Jim Blandy :jimb]

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

Comment 9

[Jim Blandy :jimb]

Attachment: Ensure compartments don't get GC'd while Debugger.prototype.findScripts' ScriptQuery is holding them in its HashSet.

Comment 10

[Jim Blandy :jimb]

Try push looks excellent.

Comment 11

[Steve Fink [:sfink] [:s:]]

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. */

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db9aec3d777c

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/db9aec3d777c

Comment 14

[Jim Blandy :jimb]

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

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b80d52309b26

Comment 16

[Jan de Mooij [:jandem]]

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b80d52309b26