Closed Bug 1427729 Opened 2 years ago Closed 2 years ago

Crash [@ js::wasm::WasmFrameIter::popFrame] with Debugger

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: decoder, Assigned: bbouvier)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:update,bisect])

Crash Data

Attachments

(3 files, 1 obsolete file)

The following testcase crashes on mozilla-central revision ac93fdadf102 (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 --baseline-eager):

lfModule = new WebAssembly.Module(wasmTextToBinary(`
  (module
    (type $i2i (func (param i32) (result i32)))
    (import "a" "mem" (memory 1))
    (import "a" "tbl" (table 10 anyfunc))
    (import $imp "a" "imp" (result i32))
    (func $call (param $i i32) (result i32)
      (i32.add
        (call $imp)
        (i32.add
          (i32.load (i32.const 0))
          (if i32 (i32.eqz (get_local $i))
            (then (i32.const 0))
            (call_indirect $i2i (get_local $i) (get_local $i))
          )
        )
      )
    )
  )
`));
g = newGlobal();
g.parent = this;
g.eval("(" + function() {
    Debugger(parent).onExceptionUnwind = function(frame) {}
} + ")()")
function check_one(expected, each  = g => call(g, "as-br_if-cond", [])) {
    try {
        f()
    } catch (ex) {}
}
function check(expr, expected = expr) {
    var end, err
    statement = "o = {};" + expr + end;
    cases = [
        function() {
            return eval("var undef;" + statement); 
        },
    ]
    for (f of cases) 
      check_one(expected, f, err)
}
check("undef");
lfModule = new WebAssembly.Module(wasmTextToBinary(`
  (module (import $imp "" "inc") (func) (func $start (call $imp)) (start $start) (export "" $start))
`));
processModule(lfModule, ``);
processModule(lfModule, `
  o = o.p;
`);
function processModule(module, jscode) {
    imports = {}
    for (let descriptor of WebAssembly.Module.imports(module)) {
      imports[descriptor.module] = {}
        imports[descriptor.module][descriptor.name] = new Function("x", "y", "z", jscode);
    }
    instance = new WebAssembly.Instance(module, imports);
    for (let descriptor of WebAssembly.Module.exports(module)) {
      print(instance.exports[descriptor.name]())
    }
}


Backtrace:

received signal SIGSEGV, Segmentation fault.
js::wasm::WasmFrameIter::popFrame (this=this@entry=0x7fffffff9ed8) at js/src/wasm/WasmFrameIter.cpp:129
#0  js::wasm::WasmFrameIter::popFrame (this=this@entry=0x7fffffff9ed8) at js/src/wasm/WasmFrameIter.cpp:129
#1  0x0000000000dd2380 in js::wasm::WasmFrameIter::WasmFrameIter (this=0x7fffffff9ed8, activation=0x7fffffffb280, fp=<optimized out>) at js/src/wasm/WasmFrameIter.cpp:51
#2  0x0000000000c56f45 in mozilla::MaybeOneOf<js::jit::JSJitFrameIter, js::wasm::WasmFrameIter>::construct<js::wasm::WasmFrameIter, js::jit::JitActivation*&> (this=0x7fffffff9ed8) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/instrumentation/none/type/debug/dist/include/mozilla/MaybeOneOf.h:109
#3  js::JitFrameIter::JitFrameIter (this=0x7fffffff9ed0, act=<optimized out>) at js/src/vm/Stack.cpp:509
#4  0x0000000000c58b3b in js::FrameIter::settleOnActivation (this=0x7fffffffa190) at js/src/vm/Stack.cpp:644
#5  0x0000000000c58e59 in js::FrameIter::FrameIter (this=0x7fffffffa190, cx=<optimized out>, debuggerEvalOption=<optimized out>) at js/src/vm/Stack.cpp:735
#6  0x0000000000b7f2fc in js::Debugger::fireExceptionUnwind (this=this@entry=0x7ffff5f52000, cx=cx@entry=0x7ffff5f16000, vp=..., vp@entry=...) at js/src/vm/Debugger.cpp:1795
#7  0x0000000000b7fd33 in js::Debugger::<lambda(js::Debugger*)>::operator() (dbg=0x7ffff5f52000, __closure=<synthetic pointer>) at js/src/vm/Debugger.cpp:1074
#8  js::Debugger::dispatchHook<js::Debugger::slowPathOnExceptionUnwind(JSContext*, js::AbstractFramePtr)::<lambda(js::Debugger*)>, js::Debugger::slowPathOnExceptionUnwind(JSContext*, js::AbstractFramePtr)::<lambda(js::Debugger*)> > (fireHook=..., cx=0x7ffff5f16000, hookIsEnabled=...) at js/src/vm/Debugger.cpp:1914
#9  js::Debugger::slowPathOnExceptionUnwind (cx=cx@entry=0x7ffff5f16000, frame=...) at js/src/vm/Debugger.cpp:1075
#10 0x0000000000da398a in js::Debugger::onExceptionUnwind (frame=..., cx=0x7ffff5f16000) at js/src/vm/Debugger-inl.h:66
#11 js::wasm::HandleThrow (cx=cx@entry=0x7ffff5f16000, iter=...) at js/src/wasm/WasmBuiltins.cpp:203
#12 0x000000000078039f in js::jit::HandleExceptionWasm (cx=0x7ffff5f16000, iter=0x7fffffffaa88, rfe=0x7fffffffaf70) at js/src/jit/JitFrames.cpp:600
#13 0x00000000007947df in js::jit::HandleException (rfe=<optimized out>) at js/src/jit/JitFrames.cpp:639
[...]
#18 0x0000000000000000 in ?? ()
rax	0x5241c3e103495b41	5927233955786480449
rbx	0x7fffffff9ed8	140737488330456
rcx	0x30	48
rdx	0x7fffffffb020	140737488334880
rsi	0x7fffffffb280	140737488335488
rdi	0x7fffffff9ed8	140737488330456
rbp	0x7fffffff9e60	140737488330336
rsp	0x7fffffff9e40	140737488330304
r8	0x30	48
r9	0x2	2
r10	0x7ffff4dec400	140737301627904
r11	0x1	1
r12	0x7fffffffb280	140737488335488
r13	0x7ffff50b3840	140737304541248
r14	0x7fffffffa030	140737488330800
r15	0x7fffffffb280	140737488335488
rip	0xdd20d4 <js::wasm::WasmFrameIter::popFrame()+52>
=> 0xdd20d4 <js::wasm::WasmFrameIter::popFrame()+52>:	mov    0x8(%rax),%rax
   0xdd20d8 <js::wasm::WasmFrameIter::popFrame()+56>:	mov    0x18(%rax),%r12
Flags: needinfo?(bbouvier)
Reduced the test case to the following:

g = newGlobal();
g.parent = this;
g.eval("(" + function() {
    Debugger(parent).onExceptionUnwind = function(frame) {}
} + ")()")

lfModule = new WebAssembly.Module(wasmTextToBinary(`
  (module (import $imp "" "inc") (func) (func $start (call $imp)) (start $start) (export "" $start))
`));

o = {};

let inst = new WebAssembly.Instance(lfModule, {
    "": {
        inc: function() { o = o.p; }
    }
});

// after instanciation, the start function has been executed and o is undefined.
// This second call will throw in the imported function:

let caught = false;
try {
    inst.exports[""]();
} catch(e) {
    caught = true;
}
assertEq(caught, true);
Flags: needinfo?(bbouvier)
Taking.

The following is happening:

- start function is called, setting o to undefined
- the second time we call the start function, it calls into the import; that's the second time and we're running with baseline-eager, so the jit exit is used.
- in js::jit::HandleException, we iterate from JS to wasm; there's a special piece of code to update activation's exitFP when unwounding (to prevent the debugger from seeing junk frames, if it creates a new frame iterator), when both the previous and next frames are JS.

But there's no updating when iterating from JS to wasm, so we try to reinterpret the last js jit frame (baseline frame) as a wasm frame, which causes the crash.
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Attached patch 1.api.patchSplinter Review
In these two functions, I think it is a better API to just claim the JitActivation instead of the cx, since that's the only thing we use in there.
Attachment #8940270 - Flags: review?(jdemooij)
Attached patch 2.fix.patch (obsolete) — Splinter Review
That's the fix to the problem mentioned in comment 2: unwind activation->packedExitFP when transitioning from JS to wasm frames.

There are only a few lines here, but finding the right spots was a bit tedious to maintain the invariants.
Attachment #8940274 - Flags: review?(jdemooij)
Attached patch 2.fix.patchSplinter Review
(same with a test)
Attachment #8940274 - Attachment is obsolete: true
Attachment #8940274 - Flags: review?(jdemooij)
Attachment #8940276 - Flags: review?(jdemooij)
Attached patch 3.dead.patchSplinter Review
Two dead fields to remove.
Attachment #8940277 - Flags: review?(jdemooij)
Attachment #8940270 - Flags: review?(jdemooij) → review+
Comment on attachment 8940276 [details] [diff] [review]
2.fix.patch

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

::: js/src/vm/Stack.h
@@ +1775,1 @@
>  // TODO(bug 1360211) In particular, this can handle the transition from wasm to

Unrelated but should we remove this comment now?
Attachment #8940276 - Flags: review?(jdemooij) → review+
Comment on attachment 8940277 [details] [diff] [review]
3.dead.patch

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

Good catch.
Attachment #8940277 - Flags: review?(jdemooij) → review+
Thanks for the reviews!

> Unrelated but should we remove this comment now?

Not yet: the ion to wasm part isn't quite yet finished (bug 1319203).
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e7604aa6ad8
Have EnsureBareExitFrame and JSJitFrameIter take only JitActivation parameters; r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/23185445f29d
Have JitFrameIter properly unwind JitActivation when transitioning from JS to wasm; r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2a67cf4639a
Remove two dead fields; r=jandem
Pretty sure this is present since bug 1360211: needs testing in Firefox 58 and maybe uplift.
Flags: needinfo?(bbouvier)
(In reply to Benjamin Bouvier [:bbouvier] from comment #11)
> Pretty sure this is present since bug 1360211: needs testing in Firefox 58
> and maybe uplift.

Would it be at high risk to uplift to 58 in this timing? If not, could you create the uplift request? Thanks.
Comment on attachment 8940270 [details] [diff] [review]
1.api.patch

I've talked with relman people and I'm currently confused with what's beta/release. This patch and the other one need to go in *Firefox 58*.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1360211
[User impact if declined]: crashes when debugging wasm
[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]: next patch, see other approval request in this bug
[Is the change risky?]: yes
[Why is the change risky/not risky?]: it touches code which is both complicated and widely used in the JS engine; fortunately it only affects wasm debugging (viz. wasm frames unwinding).
[String changes made/needed]: n/a
Flags: needinfo?(bbouvier)
Attachment #8940270 - Flags: approval-mozilla-beta?
Comment on attachment 8940276 [details] [diff] [review]
2.fix.patch

Approval Request Comment
see comment above
Attachment #8940276 - Flags: approval-mozilla-beta?
It's pretty late in 58 for a low volume crash that only affects debugging, my gut feel is this can ride the trains...
Comment on attachment 8940270 [details] [diff] [review]
1.api.patch

The volume of crash is very low. Let's let it ride the train on 59.
Attachment #8940270 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8940276 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.