1464872 - Assertion failure: !zone->isGCMarkingBlack() || zone->isAtomsZone(), at js/src/gc/Marking.cpp:253 with Debugger

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision bf4762f10b8d (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe):

var lfOffThreadGlobal = newGlobal();
var g = newGlobal();
var dbg = Debugger(g);
dbg.onEnterFrame = function(frame) {};
for (lfLocal in this) {
    if (!(lfLocal in lfOffThreadGlobal)) {
        try {
            lfOffThreadGlobal[lfLocal] = this[lfLocal];
        } catch (lfVare5) {}
    }
}
lfOffThreadGlobal.offThreadCompileScript(`
    grayRoot().x = Object.create(null);
`);
lfOffThreadGlobal.runOffThreadScript();
var lfOffThreadGlobal = newGlobal();
var g = newGlobal();
var dbg = Debugger(g);
function basicSweeping() {
    startgc(100000, 'shrinking');
}
basicSweeping();
gczeal(0);
basicSweeping();


Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x0000000000f71bb5 in js::CheckTracedThing<JSObject> (trc=trc@entry=0x7ffff5f1a6b0, thing=thing@entry=0x7ffff4c8b1a0) at js/src/gc/Marking.cpp:252
#1  0x0000000000f7bbb9 in DoMarking<JSObject> (gcmarker=0x7ffff5f1a6b0, thing=0x7ffff4c8b1a0) at js/src/gc/Marking.cpp:823
#2  0x0000000000ae45fa in js::Debugger::markIteratively (marker=marker@entry=0x7ffff5f1a6b0) at js/src/vm/Debugger.cpp:3100
#3  0x0000000000f28843 in js::gc::GCRuntime::markWeakReferences<js::gc::SweepGroupZonesIter> (this=this@entry=0x7ffff5f19700, phase=phase@entry=js::gcstats::PhaseKind::SWEEP_MARK_GRAY_WEAK) at js/src/gc/GC.cpp:4537
#4  0x0000000000f0661f in js::gc::GCRuntime::markWeakReferencesInCurrentGroup (phase=js::gcstats::PhaseKind::SWEEP_MARK_GRAY_WEAK, this=0x7ffff5f19700) at js/src/gc/GC.cpp:4554
#5  js::gc::GCRuntime::endMarkingSweepGroup (this=0x7ffff5f19700, fop=<optimized out>, budget=...) at js/src/gc/GC.cpp:5356
#6  0x0000000000f4e940 in sweepaction::SweepActionSequence<js::gc::GCRuntime*, js::FreeOp*, js::SliceBudget&>::run (this=0x7ffff5f16380, args#0=0x7ffff5f19700, args#1=0x7fffffffb3b0, args#2=...) at js/src/gc/GC.cpp:6373
#7  0x0000000000f4ee75 in sweepaction::SweepActionRepeatFor<js::gc::SweepGroupsIter, JSRuntime*, js::gc::GCRuntime*, js::FreeOp*, js::SliceBudget&>::run (this=0x7ffff5f27190, args#0=0x7ffff5f19700, args#1=0x7fffffffb3b0, args#2=...) at js/src/gc/GC.cpp:6434
#8  0x0000000000eeef3b in js::gc::GCRuntime::performSweepActions (this=this@entry=0x7ffff5f19700, budget=...) at js/src/gc/GC.cpp:6602
#9  0x0000000000f1efb5 in js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0x7ffff5f19700, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC, session=...) at js/src/gc/GC.cpp:7154
#10 0x0000000000f20230 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff5f19700, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/gc/GC.cpp:7483
#11 0x0000000000f208c5 in js::gc::GCRuntime::collect (this=this@entry=0x7ffff5f19700, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/gc/GC.cpp:7626
#12 0x0000000000f22225 in js::gc::GCRuntime::startDebugGC (this=this@entry=0x7ffff5f19700, gckind=GC_SHRINK, budget=...) at js/src/gc/GC.cpp:7765
#13 0x00000000008b6a75 in StartGC (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/TestingFunctions.cpp:1128
#14 0x00000000005b62ee in js::CallJSNative (cx=0x7ffff5f17000, native=0x8b69d0 <StartGC(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/JSContext-inl.h:280
#15 0x00000000005ab1cf in js::InternalCallOrConstruct (cx=<optimized out>, cx@entry=0x7ffff5f17000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:471
#16 0x00000000005ab5ad in InternalCall (cx=0x7ffff5f17000, args=...) at js/src/vm/Interpreter.cpp:520
#17 0x000000000059ebb5 in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:526
#18 Interpret (cx=0x7ffff5f17000, state=...) at js/src/vm/Interpreter.cpp:3093
#19 0x00000000005aac8d in js::RunScript (cx=0x7ffff5f17000, state=...) at js/src/vm/Interpreter.cpp:421
#20 0x00000000005ab297 in js::InternalCallOrConstruct (cx=<optimized out>, cx@entry=0x7ffff5f17000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:493
#21 0x00000000005ab5ad in InternalCall (cx=0x7ffff5f17000, args=...) at js/src/vm/Interpreter.cpp:520
#22 0x00000000005ab6fa in js::CallFromStack (cx=<optimized out>, args=...) at js/src/vm/Interpreter.cpp:526
#23 0x0000000000692fd3 in js::jit::DoCallFallback (cx=<optimized out>, frame=0x7fffffffc708, stub_=<optimized out>, argc=<optimized out>, vp=0x7fffffffc6c0, res=...) at js/src/jit/BaselineIC.cpp:2372
#24 0x000015dbc88455ac in ?? ()
[...]
#33 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff4c8b1a0	140737300181408
rcx	0x7ffff6c282ad	140737333330605
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffb060	140737488334944
rsp	0x7fffffffb030	140737488334896
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4780	140737354024832
r10	0x58	88
r11	0x7ffff6b9e7a0	140737332766624
r12	0x7ffff5f1a6b0	140737319642800
r13	0x7ffff5f7e000	140737320050688
r14	0x7ffff5f19000	140737319636992
r15	0x1	1
rip	0xf71bb5 <js::CheckTracedThing<JSObject>(JSTracer*, JSObject*)+1173>
=> 0xf71bb5 <js::CheckTracedThing<JSObject>(JSTracer*, JSObject*)+1173>:	movl   $0x0,0x0
   0xf71bc0 <js::CheckTracedThing<JSObject>(JSTracer*, JSObject*)+1184>:	ud2

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/cab8d95ff4a9
user:        Jon Coppeard
date:        Tue Jun 06 11:25:57 2017 +0100
summary:     Bug 1370252 - Fix spurious jit-tests failures with incremental zeal mode r=sfink

This iteration took 259.888 seconds to run.

Comment 2

[Jon Coppeard (:jonco)]

Reduced test case:

var g = newGlobal();
var dbg = Debugger(g);
dbg.onEnterFrame = function(frame) {};
var g2 = newGlobal();
g2[g] = g;
g2.evaluate("grayRoot()")
g2 = undefined;
g = newGlobal();
dbg = Debugger(g);
gc();
startgc(100000);

Not related to bug 1370252.

Comment 3

[Jon Coppeard (:jonco)]

Thanks to decoder for fuzzing and sfink for adding grayRoots() we've found a bug in incremental sweeping that's been there for > 5 years.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1464872

We're missing an edge from debugger to debuggee global when we compute the sweep groups.

For debugger cross compartment pointers, edges are added as a consequence of their being in the CCW map.  For each edge its reverse is also added to ensure that debugger are swept in the same group as their debuggees.  This happens in Debugger::findZoneEdges().

Unfortunately the debugger to debuggee global edge is not in the cross compartment wrapper map as it's a weak pointer held in the Debugger::debuggees set.

The fix is to make Debugger::findZoneEdges() add the forward edge for the debuggee case as well as the reverse.

Comment 5

[Andrew McCreight [:mccr8]]

I'm going to mark this as sec-moderate, because it requires the debugger and would probably be difficult to trigger due to GC timing.

Comment 6

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

Comment on attachment 8981960 [details] [diff] [review]
bug1464872

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

\o/

Glad to see grayRoots() earning its keep.

::: js/src/vm/Debugger.cpp
@@ +3272,5 @@
> +             * Add edges to debuggee zones. These are weak references that are
> +             * not in the cross compartment wrapper map.
> +             */
> +            for (auto e = dbg->debuggeeZones.all(); !e.empty(); e.popFront()) {
> +                Zone* debugeeZone = e.front();

*debuggeeZone

Comment 7

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d1e3d80489de3ac0ab0fef04f20cee5b117cbac

Comment 8

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 42880a726964).

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2d1e3d80489d

Please request Beta approval on this when you get a chance. It grafts cleanly as-landed. It'll also need a rebased patch and approval request for ESR60.

Comment 10

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 11

[Jon Coppeard (:jonco)]

Comment on attachment 8981960 [details] [diff] [review]
bug1464872

Approval Request Comment
[Feature/Bug causing the regression]: Bug 790338.
[User impact if declined]: Possibly crash / security vulnerability (requires debugger).
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This is a pretty simple change to add a debugger to debuggee zone edge when computing the sweep groups.
[String changes made/needed]: None.

Comment 12

[Jon Coppeard (:jonco)]

Attachment: bug1464872-esr60

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is sec-moderate because of debugger dependency but this is a GC crash so it would still be good to fix this.
User impact if declined: Possible crash / security vulnerability with debugger.
Fix Landed on Version: 62.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8981960 [details] [diff] [review]
bug1464872

Fixes a longstanding GC crash with the debugger. Approved for 61.0b11 and ESR 60.1.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1724d1487c83
https://hg.mozilla.org/releases/mozilla-esr60/rev/4fb8d0a6ac3d

Comment 15

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx61