1461448 - Assertion failure: phase != Phase::NONE (Requested child phase not found under current phase), at js/src/gc/Statistics.cpp:176 with Debugger and wasm

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision a7461494a7a0 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --cpu-count=2 --disable-oom-functions):

let lfPreamble = `
  var lfOffThreadGlobal = newGlobal();
  for (lfLocal in this)
    try {} catch(lfVare5) {}
`;
evaluate(lfPreamble);
evaluate(`
  var g = newGlobal();
  var dbg = new Debugger;
  var gw = dbg.addDebuggee(g);
  for (lfLocal in this)
    if (!(lfLocal in lfOffThreadGlobal))
      try {
        lfOffThreadGlobal[lfLocal] = this[lfLocal];
      } catch(lfVare5) {}
  var g = newGlobal();
  var gw = dbg.addDebuggee(g);
`);
lfOffThreadGlobal.offThreadCompileScript(`
  gcparam("markStackLimit", 1);
  grayRoot()[0] = "foo";
`);
lfOffThreadGlobal.runOffThreadScript();
eval(`
  var lfOffThreadGlobal = newGlobal();
  try { evaluate(\`
    gczeal(18, 1);
    grayRoot()[0] = "foo";
    let inst = new WebAssembly.Instance(new WebAssembly.Module(wasmTextToBinary(
    \\\`(module
       (memory (export "memory") 1 1)
     )\\\`
    )));
\`); } catch(exc) {}
`);


Backtrace:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x0000000000f7f560 in js::gcstats::Statistics::lookupChildPhase (this=0x7ffb3f619780, phaseKind=<optimized out>) at js/src/gc/Statistics.cpp:175
#1  0x0000000000f82b9e in js::gcstats::Statistics::beginPhase (this=this@entry=0x7ffb3f619780, phaseKind=phaseKind@entry=js::gcstats::PhaseKind::UNMARK_GRAY) at js/src/gc/Statistics.cpp:1201
#2  0x0000000000f4d5c6 in js::gcstats::AutoPhase::AutoPhase (phaseKind=js::gcstats::PhaseKind::UNMARK_GRAY, stats=..., this=<synthetic pointer>) at js/src/gc/Statistics.h:418
#3  UnmarkGrayGCThing (rt=0x7ffb3f619000, thing=...) at js/src/gc/Marking.cpp:3642
#4  0x0000000000f4d7ba in ShouldTraceCrossCompartment (trc=0x7ffb3f61a690, src=0x7ffb3ef8b190, cell=0x7ffb3efd0060) at js/src/gc/Marking.cpp:324
#5  0x0000000000f762dc in ShouldTraceCrossCompartment (cell=<optimized out>, src=<optimized out>, trc=0x7ffb3f61a690) at js/src/gc/Marking.cpp:595
#6  js::TraceManuallyBarrieredCrossCompartmentEdge<JSObject*> (trc=0x7ffb3f61a690, src=<optimized out>, dst=0x7ffe14dbaea0, name=0x12e17a2 "Debugger.Object referent") at js/src/gc/Marking.cpp:594
#7  0x0000000000acd30d in DebuggerObject_trace (trc=0x7ffb3f61a690, obj=0x7ffb3ef8b190) at js/src/vm/Debugger.cpp:8664
#8  0x0000000000b99f0c in js::Class::doTrace (this=<optimized out>, obj=0x7ffb3ef8b190, trc=0x7ffb3f61a690) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/instrumentation/none/type/debug/dist/include/js/Class.h:869
#9  JSObject::traceChildren (this=0x7ffb3ef8b190, trc=0x7ffb3f61a690) at js/src/vm/JSObject.cpp:4005
#10 0x0000000000f8036a in js::TraceChildren (trc=<optimized out>, trc@entry=0x7ffb3f61a690, thing=<optimized out>, thing@entry=0x7ffb3ef8b190, kind=kind@entry=JS::TraceKind::Object) at js/src/gc/Tracer.cpp:137
#11 0x0000000000f4d995 in js::GCMarker::markDelayedChildren (this=this@entry=0x7ffb3f61a690, arena=arena@entry=0x7ffb3ef8b000) at js/src/gc/Marking.cpp:2598
#12 0x0000000000f4dbc4 in js::GCMarker::markDelayedChildren (this=this@entry=0x7ffb3f61a690, budget=...) at js/src/gc/Marking.cpp:2623
#13 0x0000000000f5bfef in js::GCMarker::drainMarkStack (this=this@entry=0x7ffb3f61a690, budget=...) at js/src/gc/Marking.cpp:1666
#14 0x0000000000edf61b in js::gc::GCRuntime::drainMarkStack (this=this@entry=0x7ffb3f619700, sliceBudget=..., phase=phase@entry=js::gcstats::PhaseKind::MARK) at js/src/gc/GC.cpp:5901
#15 0x0000000000f0fb86 in js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0x7ffb3f619700, budget=..., reason=reason@entry=JS::gcreason::TOO_MUCH_MALLOC, session=...) at js/src/gc/GC.cpp:7114
#16 0x0000000000f10e60 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffb3f619700, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::TOO_MUCH_MALLOC) at js/src/gc/GC.cpp:7476
#17 0x0000000000f114f5 in js::gc::GCRuntime::collect (this=this@entry=0x7ffb3f619700, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::TOO_MUCH_MALLOC) at js/src/gc/GC.cpp:7619
#18 0x0000000000f119a2 in js::gc::GCRuntime::startGC (this=0x7ffb3f619700, gckind=GC_NORMAL, reason=JS::gcreason::TOO_MUCH_MALLOC, millis=0) at js/src/gc/GC.cpp:7701
#19 0x0000000000f11bda in js::gc::GCRuntime::gcIfRequested (this=this@entry=0x7ffb3f619700) at js/src/gc/GC.cpp:7881
#20 0x0000000000f13308 in js::gc::GCRuntime::gcIfNeededAtAllocation (this=0x7ffb3f619700, cx=cx@entry=0x7ffb3f617000) at js/src/gc/Allocator.cpp:318
#21 0x0000000000f41168 in js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1> (this=<optimized out>, cx=0x7ffb3f617000, kind=<optimized out>) at js/src/gc/Allocator.cpp:273
#22 0x0000000000f41fe5 in js::Allocate<js::BaseShape, (js::AllowGC)1> (cx=cx@entry=0x7ffb3f617000) at js/src/gc/Allocator.cpp:222
#23 0x0000000000c6b589 in js::BaseShape::getUnowned (cx=cx@entry=0x7ffb3f617000, base=...) at js/src/vm/Shape.cpp:1436
#24 0x0000000000c6bc72 in js::EmptyShape::getInitialShape (cx=0x7ffb3f617000, clasp=clasp@entry=0x22cb560 <js::WasmMemoryObject::class_>, proto=..., nfixed=<optimized out>, objectFlags=<optimized out>) at js/src/vm/Shape.cpp:2112
#25 0x0000000000ba907e in NewObject (cx=<optimized out>, group=..., kind=<optimized out>, newKind=js::GenericObject, initialShapeFlags=0) at js/src/vm/JSObject.cpp:736
#26 0x0000000000ba9542 in js::NewObjectWithGivenTaggedProto (cx=<optimized out>, cx@entry=0x7ffb3f617000, clasp=clasp@entry=0x22cb560 <js::WasmMemoryObject::class_>, proto=..., allocKind=js::gc::AllocKind::OBJECT2, newKind=newKind@entry=js::GenericObject, initialShapeFlags=initialShapeFlags@entry=0) at js/src/vm/JSObject.cpp:808
#27 0x0000000000e5127b in js::NewObjectWithGivenTaggedProto (initialShapeFlags=0, newKind=js::GenericObject, proto=..., clasp=0x22cb560 <js::WasmMemoryObject::class_>, cx=0x7ffb3f617000) at js/src/vm/JSObject-inl.h:609
#28 js::NewObjectWithGivenTaggedProto<js::WasmMemoryObject> (initialShapeFlags=0, newKind=js::GenericObject, proto=..., cx=0x7ffb3f617000) at js/src/vm/JSObject-inl.h:619
#29 js::NewObjectWithGivenProto<js::WasmMemoryObject> (newKind=js::GenericObject, proto=..., cx=0x7ffb3f617000) at js/src/vm/JSObject-inl.h:662
#30 js::WasmMemoryObject::create (cx=0x7ffb3f617000, buffer=buffer@entry=..., proto=proto@entry=...) at js/src/wasm/WasmJS.cpp:1483
#31 0x0000000000e51bfe in js::wasm::Module::instantiateMemory (this=this@entry=0x7ffb3eb47ff0, cx=<optimized out>, cx@entry=0x7ffb3f617000, memory=memory@entry=...) at js/src/wasm/WasmModule.cpp:947
#32 0x0000000000e53a0b in js::wasm::Module::instantiate (this=this@entry=0x7ffb3eb47ff0, cx=0x7ffb3f617000, funcImports=funcImports@entry=..., tableImport=tableImport@entry=..., memoryImport=..., memoryImport@entry=..., globalImportValues=..., globalObjs=..., instanceProto=..., instance=...) at js/src/wasm/WasmModule.cpp:1227
#33 0x0000000000e55af0 in Instantiate (cx=<optimized out>, cx@entry=0x7ffb3f617000, module=..., importObj=..., importObj@entry=..., instanceObj=..., instanceObj@entry=...) at js/src/wasm/WasmJS.cpp:1162
#34 0x0000000000e55dc0 in js::WasmInstanceObject::construct (cx=0x7ffb3f617000, argc=<optimized out>, vp=<optimized out>) at js/src/wasm/WasmJS.cpp:1187
#35 0x00000000005b6efe in js::CallJSNative (cx=0x7ffb3f617000, native=native@entry=0xe55c00 <js::WasmInstanceObject::construct(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/JSContext-inl.h:280
[...]
#65 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:9294
rax	0x0	0
rbx	0x68	104
rcx	0x7ffb402ff2ad	140717090402989
rdx	0x0	0
rsi	0x7ffb405ce770	140717093349232
rdi	0x7ffb405cd540	140717093344576
rbp	0x7ffe14dbad70	140729248361840
rsp	0x7ffe14dbad30	140729248361776
r8	0x7ffb405ce770	140717093349232
r9	0x7ffb416bf780	140717111113600
r10	0x58	88
r11	0x7ffb402757a0	140717089839008
r12	0x69	105
r13	0x7ffb3f619780	140717076879232
r14	0x67	103
r15	0x22f0080	36634752
rip	0xf7f560 <js::gcstats::Statistics::lookupChildPhase(js::gcstats::PhaseKind) const+304>
=> 0xf7f560 <js::gcstats::Statistics::lookupChildPhase(js::gcstats::PhaseKind) const+304>:	movl   $0x0,0x0
   0xf7f56b <js::gcstats::Statistics::lookupChildPhase(js::gcstats::PhaseKind) const+315>:	ud2

Comment 1

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision c96b4323d7b8).
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/c5561749c1c6
user:        Jon Coppeard
date:        Fri Nov 03 10:25:25 2017 +0000
summary:     Bug 1413914 - Add zeal mode to check gray marking invariants after every GC r=sfink

This iteration took 269.826 seconds to run.

Comment 2

[Steven DeTar [:sdetar]]

jonco, could you look at this bug?  Or help me find the right person that should.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1461448-delayed-gray-marking

We just need to allow the gray unmarking stats phase to occur under the delayed marking phase.

Comment 4

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

Comment on attachment 8976484 [details] [diff] [review]
bug1461448-delayed-gray-marking

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

Huh. I guess we don't hit this much? Maybe we need more zeal.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff105a4cff4e
Add gray marking phase to delayed marking phase r=sfink

Comment 6

[Andrei Ciure[:aciure]]

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

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(function() {
    q = function() {};
    wm = new WeakMap;
    global = newGlobal();
    var wrapper2 = global.eval("Object.create(null)");
    wm.set(wrapper2, global.l);
    grayRoot().r = wrapper2;
})();
gcparam('markStackLimit', 9);
relazifyFunctions('');
function f() {
    m;
}
for (let j = 0; j < 9999; j++) {
    try {
        f();
    } catch (e) {}
}

I have another testcase here (run with "--fuzzing-safe --no-threads --no-baseline --no-ion") that is fixed by the patch.

I don't think Debugger stuff is involved - :jonco do you mind seeing if this warrants a relook at landing on mozilla-beta?

Comment 8

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

As I read it, this is marked wontfix, not unaffected. I think it was just such a rare failure that we weren't bothering to backport. But if it's going to interfere with fuzzing, we can.

Comment 9

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

Comment on attachment 8976484 [details] [diff] [review]
bug1461448-delayed-gray-marking

Ok, I've changed my mind. It may be rare, but it's a MOZ_RELEASE_ASSERT, so it would crash the browser. Seems like we ought to backport it.

Approval Request Comment
[Feature/Bug causing the regression]: delayed marking + GC statistics tracking, both of which have been there for a long time.
[User impact if declined]: very rare crashes
[Is this code covered by automated tests?]: only by the regression test added in this bug.
[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?]: it's just weakening an incorrect assertion.
[String changes made/needed]: none

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Thanks! (Just wondering, what about ESR 60?)

Comment 11

[Jon Coppeard (:jonco)]

Comment on attachment 8976484 [details] [diff] [review]
bug1461448-delayed-gray-marking

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is release assert that can cause  rare browser crashes.
User impact if declined: Possible crash.
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 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8976484 [details] [diff] [review]
bug1461448-delayed-gray-marking

Makes life easier for the fuzzers. Approved for 61.0b12 and ESR 60.1.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c2c30a921af1

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/8e6e0228335c