1620223 - Assertion failure: cx->jobQueue->empty(), at builtin/Promise.cpp:5759 with Debugger

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200305-2e1a978b09d7 (build with (buildFlags not available), run with --fuzzing-safe --no-threads):

evaluate(`
  quit();
`, { catchTermination : true });
const g9 = newGlobal({ newCompartment: true });
function setup(global, label) {
  const dbg = new Debugger;
  dbg.gDO = dbg.addDebuggee(global);
  dbg.onDebuggerStatement = function (frame) {
      Promise.resolve(42).then(v13 => {
      });
  };
}
const dbg1 = setup(g9, '1');
g9.eval(`
  debugger;
`);

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000555555b798ea in JS::AutoDebuggerJobQueueInterruption::~AutoDebuggerJobQueueInterruption() ()
#0  0x0000555555b798ea in JS::AutoDebuggerJobQueueInterruption::~AutoDebuggerJobQueueInterruption() ()
#1  0x0000555555fa13f3 in js::DebugAPI::slowPathOnDebuggerStatement(JSContext*, js::AbstractFramePtr) ()
#2  0x00005555558e1498 in Interpret(JSContext*, js::RunState&) ()
#3  0x00005555558d0863 in js::RunScript(JSContext*, js::RunState&) ()
#4  0x00005555558e8d32 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) ()
#5  0x000055555594aac1 in EvalKernel(JSContext*, JS::Handle<JS::Value>, EvalType, js::AbstractFramePtr, JS::Handle<JSObject*>, unsigned char*, JS::MutableHandle<JS::Value>) ()
#6  0x000055555594a3e0 in js::IndirectEval(JSContext*, unsigned int, JS::Value*) ()
#7  0x00005555558e66b2 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) ()
#8  0x00005555558e5fcf in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) ()
#9  0x00005555558e74e0 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) ()
#10 0x0000555555a899b5 in js::ForwardingProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const ()
#11 0x0000555555a6e044 in js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const ()
#12 0x0000555555a7696a in js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) ()
#13 0x00005555558e61d7 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) ()
#14 0x00005555558dad9c in Interpret(JSContext*, js::RunState&) ()
[..]
#23 0x000055555576675e in main ()
rax	0x555556e835e0	93825018639840
rbx	0x7fffffffa260	140737488331360
rcx	0x555557ef4850	93825035880528
rdx	0x0	0
rsi	0x7ffff6efd770	140737336301424
rdi	0x7ffff6efc540	140737336296768
rbp	0x7fffffffa200	140737488331264
rsp	0x7fffffffa1f0	140737488331248
r8	0x7ffff6efd770	140737336301424
r9	0x7ffff7f9cd00	140737353731328
r10	0x58	88
r11	0x7ffff6ba47a0	140737332791200
r12	0x7ffff5e27200	140737318646272
r13	0x7ffff5e27000	140737318645760
r14	0x7ffff5e27000	140737318645760
r15	0x7fffffffa2c8	140737488331464
rip	0x555555b798ea <JS::AutoDebuggerJobQueueInterruption::~AutoDebuggerJobQueueInterruption()+218>
=> 0x555555b798ea <_ZN2JS32AutoDebuggerJobQueueInterruptionD2Ev+218>:	movl   $0x167f,0x0
   0x555555b798f5 <_ZN2JS32AutoDebuggerJobQueueInterruptionD2Ev+229>:	callq  0x5555557ef05e <abort>

This could potentially be a shell-only problem.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jason Orendorff [:jorendorff]]

Logan, this might be an easy fix, if you're familiar with the promise stuff.

Comment 3

[Logan Smyth [:loganfsmyth]]

Here's the repro case with a bit of cleanup:

evaluate(`
  quit();
`, {
  catchTermination : true
});

const global = newGlobal({ newCompartment: true });

const dbg = new Debugger(global);
dbg.onDebuggerStatement = function (frame) {
  Promise.resolve(42).then(v13 => { });
};
global.eval(`
  debugger;
`);

Basically what's happening is that Quit explicitly sets some flags on the JSContext and then terminates. The termination is caught by catchTermination, but the flag set by js::StopDrainingJobQueue(cx) stays set: https://searchfox.org/mozilla-central/rev/13b081a62d3f3e3e3120f95564529257b0bf451c/js/src/shell/js.cpp#2834-2836 Then we get to onDebuggerStatement, which explicitly asserts that runJobs will run all current jobs, which directly conflicts with InternalJobQueue's implementation behavior in the case of Quit.

So I guess the question is, what's the actual expected behavior here in general? If job execution is flagged as interrupted, then we could explicitly discard all jobs queued during onDebuggerStatement, is explicitly excluding those alright?

There is also the case of Quit being called inside onDebuggerStatement that we'll need to consider, since I'd expect that to have similar issues.

One alternative we could consider is removing this assertion, and if we get to the end and their are jobs left, we could prepend them onto the outer job queue before returning control from onDebuggerStatement. The main downside being that if we introduce a bug that leaves jobs in the queue accidentally, then we'd risk running those jobs outside of the nested microtask queue accidentally. We might be able to avoid some of that risk by moving this assertion to only apply to the CycleCollectedJSContext's runJobs though?

Comment 4

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20200313163649-87ab9a88abce.
Failed to bisect testcase (Start build crashes!):
> Start: 5ce27c44f79e369501db52170709194a30d12f8a (20190315034243)
> End: 2e1a978b09d77c7422e972e9c3bfdeb484f6f608 (20200305095541)
> BuildFlags: BuildFlags(asan=False, tsan=False, debug=True, fuzzing=False, coverage=False, valgrind=False)

Comment 5

[Jason Kratzer [:jkratzer]]

-- Removed --

Comment 6

[Bill Parks]

I believe I have encountered another occurrence of this bug, with the following test case, compiled with --enable-debug --enable-optimize --disable-shared-js

function main() {
const v2 = {newCompartment:true};
const v3 = newGlobal(v2);
let v4 = v3;
const v6 = Debugger(v4);
let v7 = v6;
function v8(v9) {
    const v11 = Promise.resolve();
    const v12 = () => {
        const v14 = quit();
    };
    const v15 = v11.then(v12);
    const v17 = Promise.resolve();
    const v18 = v17.then();
}
v7.onDebuggerStatement = v8;
const v20 = v4.eval("debugger; debugger; debugger;");
}
main();

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 8

[Bugmon [:jkratzer for issues]]

Bugmon was unable reproduce this issue.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 9

[Jason Kratzer [:jkratzer]]

A change to the Taskcluster build definitions over the weekend caused Bugmon to fail when reproducing issues. This issue has been corrected. Re-enabling bugmon.

Comment 10

[Bugmon [:jkratzer for issues]]

Bugmon was unable reproduce this issue.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 11

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

I keep seeing this multiple times with hard-to-reproduce testcases.

Jan, do you know how we might proceed here? (It was mentioned in bug 1866385 comment 4 that :jimb might no longer be working on the debugger) Or at least a patch?

Comment 12

[Jan de Mooij [:jandem]]

arai, do you have any thoughts on comment 3? This bug also reminds me of bug 1866385.

Comment 13

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

(In reply to Jan de Mooij [:jandem] from comment #12)

This bug also reminds me of bug 1866385.

Please note that I've been seeing this assertion failure as recently as m-c rev fe38c21e5d5b (Mar 8, 2024), so it is unlikely to still be fixed, which is the current case of bug 1866385.

Comment 14

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) from comment #13)

Please note that I've been seeing this assertion failure as recently as m-c rev fe38c21e5d5b (Mar 8, 2024), so it is unlikely to still be fixed, which is the current case of bug 1866385.

I know and bug 1866385 was a browser-only issue I think. I just meant it's somewhat related.

Comment 15

[Tooru Fujisawa [:arai]]

Comment #3 and comment #6 are unrelated cases.

Comment #3 is that quit() is caught by catchTermination and the execution continues.
Here, the catchTermination should revert the quit operation and the remaining execution should work as normal.

Comment #6 is that quit() is called while draining the job queue and it leaves jobs untouched.
Since the job queue can stop draining, the status should be exposed via JS::JobQueue interface, and assertion should take it into account.

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Bug 1620223 - Part 1: Restart the job queue if quit is caught by catchTermination. r=jandem!

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Bug 1620223 - Part 2: Do not assert the job queue emptiness if the job queue itself is interrupted. r=jandem

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e59f7b24944
Part 1: Restart the job queue if quit is caught by catchTermination. r=jandem
https://hg.mozilla.org/integration/autoland/rev/bd45b4ccb129
Part 2: Do not assert the job queue emptiness if the job queue itself is interrupted. r=jandem

Comment 19

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/8e59f7b24944
https://hg.mozilla.org/mozilla-central/rev/bd45b4ccb129