1353763 - Crash [@ js::gc::IsInsideNursery]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

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.

Comment 1

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

Attachment: Detailed Crash Information

Comment 2

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

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?

Comment 3

[Daniel Veditz [:dveditz]]

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.

Comment 4

[Eric Rahm [:erahm]]

(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

Comment 5

[Luke Wagner [:luke]]

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.

Comment 6

[Luke Wagner [:luke]]

Attachment: fix-interrupt-fp

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.

Comment 7

[Luke Wagner [:luke]]

Attachment: make-test-faster

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.

Comment 8

[Luke Wagner [:luke]]

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!

Comment 9

[Luke Wagner [:luke]]

Attachment: fix-interrupt-fp

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 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8857585 [details] [diff] [review]
make-test-faster

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

Ha, fun!

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

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.

Comment 12

[Luke Wagner [:luke]]

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

Comment 13

[Luke Wagner [:luke]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Luke Wagner [:luke]]

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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Good enough for me.

Comment 18

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.