The following testcase crashes on mozilla-central revision b043233ec04f (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager): // Adapted from randomly chosen test: js/src/jit-test/tests/debug/execution-observability-03.js p = new Proxy(this, { "deleteProperty": function () { g = newGlobal(); g.parent = this; g.eval("Debugger(parent).onEnterFrame = function() {};"); return 1; } }); delete p.f; // Adapted from randomly chosen test: js/src/jit-test/tests/wasm/timeout/1.js timeout(0.01); var code = wasmTextToBinary('(module(func(export "f1")(loop$top br$top)))'); new WebAssembly.Instance(new WebAssembly.Module(code)).exports.f1(); Backtrace: #0 js::gc::IsInsideNursery (cell=0xe4e4e4e400000000) at /home/ubuntu/shell-cache/js-dbg-64-dm-linux-b043233ec04f/objdir-js/dist/include/js/HeapAPI.h:328 #1 js::gc::Cell::isTenured (this=0xe4e4e4e400000000) at js/src/gc/Heap.h:251 #2 JSObject::readBarrier (obj=0xe4e4e4e400000000) at js/src/jsobj.h:644 #3 js::InternalBarrierMethods<js::WasmInstanceObject*>::readBarrier (v=0xe4e4e4e400000000) at js/src/gc/Barrier.h:266 #4 js::ReadBarrieredBase<js::WasmInstanceObject*>::read (this=0x7f0329759068) at js/src/gc/Barrier.h:553 /snip For detailed crash information, see attachment. Setting s-s because GC is on the stack, as a start.

autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/766ead465209 user: Yury Delendik date: Sat Jan 07 10:36:11 2017 -0600 summary: Bug 1286948 - Extends AbstractFramePtr to reference wasm::DebugFrame. r=luke,shu Yury, is bug 1286948 a likely regressor?

what does the 0xe4 memory poison value mean? calling this "sec-moderate" because it looks like Debugger calls not available to the web are involved, but if the underlying bug can be hit from a normal context we should raise this to sec-high.

(In reply to Daniel Veditz [:dveditz] from comment #3) > what does the 0xe4 memory poison value mean? It's our junk fill [1]. Basically what you get when you malloc something but don't initialize it. [1] http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/memory/mozjemalloc/jemalloc.c#5107-5112

Ah hah, what's happening here is that the timeout() is asynchronously interrupting execution while wasm frames are on the stack. Because the async interrupt doesn't set WasmActivation::exitFP, FrameIter doesn't find any frames and so the soon-to-be-popped frames are left in the debugger's frame map where they later explode. I think the fix is for the interrupt callback to pass 'fp' to WasmHandleExecutionInterrupt() so that it can set exitFP. This would actually strengthen our invariants about WasmActivation::exitFP and remove some async-interrupt special cases in other places.

I'm pretty happy about this patch: it removes an entire case of "interrupted and lost the stack" with a pretty aggressive assert in FrameIterator. It also means we always get full stacks in slow-script-interrupt situations.

Unrelated, I saw this test timeout on non-JIT and eager runs. Disabling in these cases allows the test to run in 7s without loss of coverage.

Actually, the new tighter assertion shows that we now depend on the builtin thunks (b/c some of them can GC, and thus iterate the stack). Great timing on that patch Benjamin!

Testing on windows revealed that GetThreadContext() on Win64 was never returning fp; the CONTEXT_CONTROL flag had to be upgraded to CONTEXT_FULL to get fp (but only on Win64; on Win32, CONTEXT_CONtroL gives you fp...). Yay for stronger assertions!

Comment on attachment 8857585 [details] [diff] [review] make-test-faster Review of attachment 8857585 [details] [diff] [review]: ----------------------------------------------------------------- Ha, fun!

Comment on attachment 8857666 [details] [diff] [review] fix-interrupt-fp Review of attachment 8857666 [details] [diff] [review]: ----------------------------------------------------------------- So nice. That makes me wonder about the remaining builtin calls that don't get thunked, but I think that for these we're good as well, because either they have their own exit prologue/epilogue (e.g. callimport, coerce_in_place, traps and debug trap, ), or they'll inherit the FP value of the caller (e.g. throw, OOB, unaligned accesses, and now handle interrupt with this patch). Good. ::: js/src/jit/arm/Simulator-arm.cpp @@ +1555,5 @@ > +void > +Simulator::handleWasmInterrupt() > +{ > + void* pc = reinterpret_cast<void*>(get_pc()); > + uint8_t* fp = reinterpret_cast<uint8_t*>(get_register(r11)); s/r11/FramePointer/ ? @@ +1580,5 @@ > if (!act) > return false; > > void* pc = reinterpret_cast<void*>(get_pc()); > + uint8_t* fp = reinterpret_cast<uint8_t*>(get_register(r11)); (same comment as above) ::: js/src/vm/Stack.cpp @@ +1687,5 @@ > + // interrupt-during-interrupt. > + MOZ_ASSERT(!interrupted()); > + MOZ_ASSERT(compartment()->wasm.lookupCode(pc)->lookupRange(pc)->isFunction()); > + MOZ_ASSERT(pc); > + MOZ_ASSERT(fp); I'd move the last two assertions before the lookupCode assertion (since the lookupCode assertion will fail if !pc, but having the pc assertion trigger first is more explanatory). @@ +1697,5 @@ > +void > +WasmActivation::finishInterrupt() > +{ > + MOZ_ASSERT(resumePC_); > + MOZ_ASSERT(exitFP_); Or just MOZ_ASSERT(interrupted())? ::: js/src/vm/Stack.h @@ +1764,5 @@ > + // simulator) and cleared by WasmHandleExecutionInterrupt or WasmHandleThrow > + // when the interrupt is handled. > + void startInterrupt(void* pc, uint8_t* fp); > + void finishInterrupt(); > + bool interrupted() const { return !!resumePC_; } Would it make sense that in the dtor of WasmActivation, we'd assert !interrupted()? (although the interrupt stub handler doesn't reset the state, so i'm not sure it holds as is) ::: js/src/wasm/WasmFrameIterator.cpp @@ -235,4 @@ > // Only non-imported functions can have debug frames. > return code_->metadata().debugEnabled && > - fp_ && > - !missingFrameMessage_ && I like these simplifications! ::: js/src/wasm/WasmSignalHandlers.cpp @@ +843,1 @@ > // to InterruptRunningCode's use of SuspendThread. When this happens, pre-existing: InterruptRunningCode has changed name. There's a second occurrence above.

(In reply to Benjamin Bouvier [:bbouvier] from comment #11) Thanks! > s/r11/FramePointer/ ? Unfortunately, the ARM simulator has its own set of enums for registers so FramePointer isn't compatible. > Would it make sense that in the dtor of WasmActivation, we'd assert > !interrupted()? (although the interrupt stub handler doesn't reset the > state, so i'm not sure it holds as is) Technically we already do (by asserting resumePC_ == null). The interrupt state is always cleared on both the continue and stop paths (by WasmHandleExecutionInterrupt and WasmHandleThrow, resp.). I can change to !interrupted(), though.

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/f90707333e7f6daaaf364bc61890ba2b1849199d https://hg.mozilla.org/integration/mozilla-inbound/rev/20e85b5bd3429c3e7386b56fe6c6056d52219eb0 This created an intermittent win8 failure of asm.js/testBug1117255.js that I had missed in try and local runs. Reducing the shell's watchdog timer interval from .1s to 1ms makes it reproduce easily, though. With printf logging I can see a most bizarre sequence: 1. the watchdog interrupts the thread at PC x and attempts to redirect to PC y 2. an OOB fault at PC x fires (probably it had already fired internally before step 1, but hadn't bubbled up to userspace yet) 3. the OOB handler handles successfully by redirecting PC and this *clobbers* the redirect in made in step 1 4. now WasmActivation::interrupt() is left 'true' even though the interrupt handler will never be called (b/c of step 3) Fortunately, just like the other weird case in the Windows handler, this is precisely detectable and has a simple fix (once you understand the madness): https://hg.mozilla.org/integration/mozilla-inbound/rev/b411de2683e99d545e82f55113a0f0dc2920808b

https://hg.mozilla.org/mozilla-central/rev/20e85b5bd342 https://hg.mozilla.org/mozilla-central/rev/f90707333e7f https://hg.mozilla.org/mozilla-central/rev/b411de2683e9

Looks like we'll want to uplift this to 54?

IIUC, the bug can only be activated by the debugger combined with the slow-script dialog. I'm not sure what the policy is for rare devtools-only crashes, but I'd guess this didn't require uplift and letting the fix ride the trains would be fine.

Good enough for me.

JSBugMon: This bug has been automatically verified fixed.