Closed Bug 1353763 Opened 4 years ago Closed 4 years ago

Crash [@ js::gc::IsInsideNursery]


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox-esr45 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- verified


(Reporter: gkw, Assigned: luke)



(4 keywords, Whiteboard: [jsbugmon:update][adv-main55+])

Crash Data


(3 files, 1 obsolete file)

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
var code = wasmTextToBinary('(module(func(export "f1")(loop$top br$top)))');
new WebAssembly.Instance(new WebAssembly.Module(code)).exports.f1();


#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

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:
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?
Blocks: 1286948
Flags: needinfo?(ydelendik)
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.
Keywords: sec-moderate
(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.

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.
Attached patch fix-interrupt-fp (obsolete) — Splinter Review
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.
Assignee: nobody → luke
Attachment #8857584 - Flags: review?(bbouvier)
Attached patch make-test-fasterSplinter Review
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.
Attachment #8857585 - Flags: review?(bbouvier)
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!
Blocks: 1340219
Attached patch fix-interrupt-fpSplinter Review
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!
Attachment #8857584 - Attachment is obsolete: true
Attachment #8857584 - Flags: review?(bbouvier)
Attachment #8857666 - Flags: review?(bbouvier)
Comment on attachment 8857585 [details] [diff] [review]

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

Ha, fun!
Attachment #8857585 - Flags: review?(bbouvier) → review+
Comment on attachment 8857666 [details] [diff] [review]

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.
Attachment #8857666 - Flags: review?(bbouvier) → review+
Flags: needinfo?(ydelendik)
(In reply to Benjamin Bouvier [:bbouvier] from comment #11)

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

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):
Looks like we'll want to uplift this to 54?
Flags: needinfo?(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.
Flags: needinfo?(luke)
Group: javascript-core-security → core-security-release
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main55+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.