Closed Bug 1756592 Opened 3 years ago Closed 3 years ago

Assertion failure: cx_->realm()->isDebuggee() || cx_->isPropagatingForcedReturn(), at js/src/jit/BaselineBailouts.cpp:867

Categories

(Core :: JavaScript Engine: JIT, defect, P3)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
100 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox98 --- wontfix
firefox99 --- wontfix
firefox100 --- fixed

People

(Reporter: decoder, Assigned: iain)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [bugmon:update,bisected,confirmed])

Attachments

(3 files)

The following testcase crashes on mozilla-central revision 20220222-9852e8d821d0 (debug build, run with --fuzzing-safe --ion-offthread-compile=off --baseline-eager --ion-warmup-threshold=0):

setJitCompilerOption("ion.forceinlineCaches", 1)
function a(b) {
    c = newGlobal({
        newCompartment: true
    })
    d = new Debugger
    setInterruptCallback(function() {
        d.addDebuggee(c)
        d.getNewestFrame().onStep = function() {
            d.removeDebuggee(c)
            return b
        }
        return true
    })
    c.eval("(" + function() {
        invokeInterruptCallback(function() {})
    } + ")()")
}
a({ return: "" })

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x00005555576d02be in BaselineStackBuilder::buildExpressionStack() ()
#1  0x00005555576d2fff in BaselineStackBuilder::buildOneFrame() ()
#2  0x00005555576cbdb9 in js::jit::BailoutIonToBaseline(JSContext*, js::jit::JitActivation*, js::jit::JSJitFrameIter const&, js::jit::BaselineBailoutInfo**, js::jit::ExceptionBailoutInfo const*) ()
#3  0x00005555576ccee7 in js::jit::ExceptionHandlerBailout(JSContext*, js::jit::InlineFrameIterator const&, js::jit::ResumeFromException*, js::jit::ExceptionBailoutInfo const&) ()
#4  0x0000555557a8bf6b in js::jit::HandleException(js::jit::ResumeFromException*) ()
#5  0x000032b6abe808e6 in ?? ()
[...]
#9  0x0000000000000000 in ?? ()
rax	0x555555781e6c	93824994516588
rbx	0x7ffff602a200	140737320755712
rcx	0x5555581a68e8	93825038706920
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffff8df0	140737488326128
rsp	0x7fffffff8da0	140737488326048
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f99840	140737353717824
r10	0x0	0
r11	0x0	0
r12	0x7fffffff8da8	140737488326056
r13	0x7fffffff8f80	140737488326528
r14	0x7fffffff8fa8	140737488326568
r15	0x0	0
rip	0x5555576d02be <BaselineStackBuilder::buildExpressionStack()+654>
=> 0x5555576d02be <_ZN20BaselineStackBuilder20buildExpressionStackEv+654>:	movl   $0x363,0x0
   0x5555576d02c9 <_ZN20BaselineStackBuilder20buildExpressionStackEv+665>:	callq  0x555556b6ae07 <abort>
Attached file Detailed Crash Information (deleted) —
Attached file Testcase (deleted) —

Bugmon Analysis
Verified bug as reproducible on mozilla-central 20220222144226-8f6979e6d30e.
The bug appears to have been introduced in the following build range:

Start: df8d93b9934294a67af0a05ea50070ce8e5fd8e6 (20210915171826)
End: ed6ca0884441cc361b1c2d01a7cb6c44ee7b8848 (20210915173940)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=df8d93b9934294a67af0a05ea50070ce8e5fd8e6&tochange=ed6ca0884441cc361b1c2d01a7cb6c44ee7b8848

Whiteboard: [bugmon:update,bisect] → [bugmon:update,bisected,confirmed]

Iain, would you be the right person to investigate while Ted is off?
Otherwise Ted would come back soon and you can forward the needinfo.

Blocks: sm-jits
Severity: -- → S4
Flags: needinfo?(iireland)
Priority: -- → P3
Regressed by: 1730426

This is just an assertion problem. The bug was introduced in this patch, which added an AutoSaveExceptionState in ExceptionHandlerBailout.

In HandleExceptionIon, if ShouldBailoutForDebugger returns true, we call ExceptionHandlerBailout with an empty exception info, which indicates that we're propagating an exception due to debug mode. This only happens if the current realm is a debuggee, or if we're propagating a forced return.

In buildExpressionStack, if the exception info is empty, we assert that one of those two cases is true. However, to check whether we're propagating a forced return, we look at cx->status. Because the new AutoSaveExceptionState temporarily resets the exception status to None, we fail the assertion.

Here's a slightly cleaner version of the testcase:

let g = newGlobal({ newCompartment: true});
let d = new Debugger;
g.eval("function foo() { invokeInterruptCallback(() => {}) }");

// Warp-compile.
setInterruptCallback(function() { return true; });
for (var i = 0; i < 20; i++) {
  g.foo();
}

setInterruptCallback(function() {
  d.addDebuggee(g)
  d.getNewestFrame().onStep = function() {
    d.removeDebuggee(g);
    return { return: 0 };
  }
  return true
});

g.foo();

I believe the bug triggers when we try to build a stack frame for foo.

One solution would be to store the ExceptionStatus in the ExceptionInfo, and look at the cached status instead of the JsContext (which has been overwritten by the AutoSaveExceptionState). There may be other solutions.

Flags: needinfo?(iireland)
Has Regression Range: --- → yes

Bugmon Analysis
Unable to reproduce bug 1756592 using build mozilla-central 20220222093709-9852e8d821d0. Without a baseline, bugmon is unable to analyze this bug.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Keywords: bugmon

Now that you're back, Ted, do you have any thoughts about the best fix here? As mentioned above, my default fix would be to cache the ExceptionStatus in the ExceptionInfo; if you can't think of anything better, I can write up that patch.

Flags: needinfo?(tcampbell)

I think you are right about just caching it. The propagatingIonExceptionForDebugMode in buildExpressionStack is relying on the excInfo instead of the cx, so caching seems more consistent too.

Flags: needinfo?(tcampbell)

My first draft of this patch cached cx->status, but that's a private member that isn't exposed by an accessor. Under the assumption that this was a deliberate choice, this version of the patch only caches whether we are doing a forced return.

Assignee: nobody → iireland
Status: NEW → ASSIGNED

:iain

Flags: needinfo?(iireland)
Flags: needinfo?(iireland)
Pushed by iireland@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/40c4a9994215 Cache forced-return in ExceptionBailoutInfo r=tcampbell
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 100 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: