1814020 - Assertion failure: genObj->isSuspended(), at debugger/Debugger.cpp:708

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[lukas.bernhard]

Steps to reproduce:

The attached sample crashes the js-shell on commit cfa53fc21de3984ef9f4887235a45561666ed3a3 when invoked as obj-x86_64-pc-linux-gnu/dist/bin/js --fuzzing-safe crash.js. Older commits such as c7854bdaa6bfa104bb6e94a5b84ecd3d32551425 from Dec 29 2021 are affected as well.

async function* f0(a1) {
    f0.constructor(a1);
    this.sameZoneAs = this;
    const v11 = [].__proto__;
    const v12 = await f0; 
    const v13 = Object.setPrototypeOf(Object, this);
    const v19 = v13.newGlobal(v13).Debugger(v11).getNewestFrame().asyncPromise;
    v19.getPromiseReactions();
}
const v21 = f0();
const v22 = v21.next();
const v23 = f0(v22);
v23.next();
v23.return(v22);

#0  0x00005555581dc8e1 in js::Debugger::getFrame (this=0x7ffff4d29400, cx=0x7ffff742f100, genObj=..., result=...)
    at js/src/debugger/Debugger.cpp:708
#1  0x00005555582da808 in js::DebuggerObject::PromiseReactionRecordBuilder::pushGenerator (this=0x7fffffff99b8, 
    cx=0x7ffff742f100, unwrappedGenerator=...) at js/src/debugger/Object.cpp:1410
#2  0x00005555582da3b9 in js::DebuggerObject::PromiseReactionRecordBuilder::asyncGenerator (this=0x7fffffff99b8, 
    cx=0x7ffff742f100, unwrappedGenerator=...) at js/src/debugger/Object.cpp:1394
#3  0x0000555557ccc2c5 in js::PromiseObject::forEachReactionRecord(JSContext*, js::PromiseReactionRecordBuilder&)::$_6::operator(
)(JS::MutableHandle<JSObject*>) const (this=0x7fffffff9880, obj=...)
    at js/src/builtin/Promise.cpp:6228
#4  0x0000555557c4fac9 in ForEachReaction<js::PromiseObject::forEachReactionRecord(JSContext*, js::PromiseReactionRecordBuilder&)
::$_6>(JSContext*, JS::Handle<JS::Value>, js::PromiseObject::forEachReactionRecord(JSContext*, js::PromiseReactionRecordBuilder&)
::$_6) (cx=0x7ffff742f100, reactionsVal=..., f=...) at js/src/builtin/Promise.cpp:1976
#5  0x0000555557c4f8b4 in js::PromiseObject::forEachReactionRecord (this=0x3aa048200788, cx=0x7ffff742f100, builder=...)
    at js/src/builtin/Promise.cpp:6205
#6  0x00005555582bb0d7 in js::DebuggerObject::CallData::getPromiseReactionsMethod (this=0x7fffffff9a70)
    at js/src/debugger/Object.cpp:1443
#7  0x00005555582d0d54 in js::DebuggerObject::CallData::ToNative<&js::DebuggerObject::CallData::getPromiseReactionsMethod> (
    cx=0x7ffff742f100, argc=0, vp=0x7fffffff9f60) at js/src/debugger/Object.cpp:232
#8  0x00005555576f1f0e in CallJSNative (cx=0x7ffff742f100, 
    native=0x5555582d0ba0 <js::DebuggerObject::CallData::ToNative<&js::DebuggerObject::CallData::getPromiseReactionsMethod>(JSCon
text*, unsigned int, JS::Value*)>, reason=js::CallReason::Call, args=...)
    at js/src/vm/Interpreter.cpp:459
#9  0x00005555576f174d in js::InternalCallOrConstruct (cx=0x7ffff742f100, args=..., construct=js::NO_CONSTRUCT, 
    reason=js::CallReason::Call) at js/src/vm/Interpreter.cpp:547
#10 0x00005555576f2ae1 in InternalCall (cx=0x7ffff742f100, args=..., reason=js::CallReason::Call)
    at js/src/vm/Interpreter.cpp:614
#11 0x00005555576f2d25 in js::Call (cx=0x7ffff742f100, fval=..., thisv=..., args=..., rval=..., reason=js::CallReason::Call)
    at js/src/vm/Interpreter.cpp:646
#12 0x00005555581619b8 in js::ForwardingProxyHandler::call (this=0x555559bb8530 <js::CrossCompartmentWrapper::singleton>, 
    cx=0x7ffff742f100, proxy=..., args=...) at js/src/proxy/Wrapper.cpp:168
#13 0x00005555581366e5 in js::CrossCompartmentWrapper::call (this=0x555559bb8530 <js::CrossCompartmentWrapper::singleton>, 
    cx=0x7ffff742f100, wrapper=..., args=...) at js/src/proxy/CrossCompartmentWrapper.cpp:229
#14 0x0000555558151271 in js::Proxy::call (cx=0x7ffff742f100, proxy=..., args=...)
    at js/src/proxy/Proxy.cpp:676
#15 0x00005555576f13da in js::InternalCallOrConstruct (cx=0x7ffff742f100, args=..., construct=js::NO_CONSTRUCT, 
    reason=js::CallReason::Call) at js/src/vm/Interpreter.cpp:527
#16 0x00005555576f2ae1 in InternalCall (cx=0x7ffff742f100, args=..., reason=js::CallReason::Call)
    at js/src/vm/Interpreter.cpp:614
#17 0x00005555576f28a5 in js::CallFromStack (cx=0x7ffff742f100, args=..., reason=js::CallReason::Call)
    at js/src/vm/Interpreter.cpp:619
#18 0x00005555576e35c9 in Interpret (cx=0x7ffff742f100, state=...)
    at js/src/vm/Interpreter.cpp:3362
#19 0x00005555576d56a0 in js::RunScript (cx=0x7ffff742f100, state=...)
    at js/src/vm/Interpreter.cpp:431
#20 0x00005555576f1a0c in js::InternalCallOrConstruct (cx=0x7ffff742f100, args=..., construct=js::NO_CONSTRUCT, 
    reason=js::CallReason::Call) at js/src/vm/Interpreter.cpp:579
#21 0x00005555576f2ae1 in InternalCall (cx=0x7ffff742f100, args=..., reason=js::CallReason::Call)
    at js/src/vm/Interpreter.cpp:614
#22 0x00005555576f2d25 in js::Call (cx=0x7ffff742f100, fval=..., thisv=..., args=..., rval=..., reason=js::CallReason::Call)
    at js/src/vm/Interpreter.cpp:646
#23 0x0000555557d12f2c in js::CallSelfHostedFunction (cx=0x7ffff742f100, name=..., thisv=..., args=..., rval=...)
    at js/src/vm/SelfHosting.cpp:1488
#24 0x00005555578adeaf in AsyncGeneratorResume (cx=0x7ffff742f100, generator=..., completionKind=js::CompletionKind::Normal,
    argument=...) at js/src/vm/AsyncIteration.cpp:1072
#25 0x00005555578af94c in AsyncGeneratorAwaitedFulfilled (cx=0x7ffff742f100, generator=..., value=...)
    at js/src/vm/AsyncIteration.cpp:404
#26 0x00005555578af695 in js::AsyncGeneratorPromiseReactionJob (cx=0x7ffff742f100,
    handler=js::PromiseHandler::AsyncGeneratorAwaitedFulfilled, generator=..., argument=...)
    at js/src/vm/AsyncIteration.cpp:1194
#27 0x0000555557c6b05d in PromiseReactionJob (cx=0x7ffff742f100, argc=0, vp=0x7fffffffde60)
    at js/src/builtin/Promise.cpp:2180
#28 0x00005555576f1f0e in CallJSNative (cx=0x7ffff742f100,
    native=0x555557c6aaa0 <PromiseReactionJob(JSContext*, unsigned int, JS::Value*)>, reason=js::CallReason::Call, args=...)
    at js/src/vm/Interpreter.cpp:459
#29 0x00005555576f174d in js::InternalCallOrConstruct (cx=0x7ffff742f100, args=..., construct=js::NO_CONSTRUCT,
    reason=js::CallReason::Call) at js/src/vm/Interpreter.cpp:547
#30 0x00005555576f2ae1 in InternalCall (cx=0x7ffff742f100, args=..., reason=js::CallReason::Call)
    at js/src/vm/Interpreter.cpp:614
#31 0x00005555576f2d25 in js::Call (cx=0x7ffff742f100, fval=..., thisv=..., args=..., rval=..., reason=js::CallReason::Call)
    at js/src/vm/Interpreter.cpp:646
#32 0x00005555578f0b93 in JS::Call (cx=0x7ffff742f100, thisv=..., fval=..., args=..., rval=...)
    at js/src/vm/CallAndConstruct.cpp:117
#33 0x0000555557a878ce in JS::Call (cx=0x7ffff742f100, thisv=..., funObj=..., args=..., rval=...)
    at obj-x86_64-pc-linux-gnu/dist/include/js/CallAndConstruct.h:110
--Type <RET> for more, q to quit, c to continue without paging--
#34 0x0000555557a87383 in js::InternalJobQueue::runJobs (this=0x7ffff7417d00, cx=0x7ffff742f100)
    at js/src/vm/JSContext.cpp:883
#35 0x0000555557a86e1b in js::RunJobs (cx=0x7ffff742f100) at js/src/vm/JSContext.cpp:820
#36 0x00005555574f890b in RunShellJobs (cx=0x7ffff742f100) at js/src/shell/js.cpp:1156

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

Did you want to take a look at this?

Comment 2

[Bryan Thrall [:bthrall]]

I'll take a look.

Comment 3

[Bryan Thrall [:bthrall]]

:mgaudet and I explored this a bit this morning and simplified the test case to this:

k = 0;
async function* f0() {
    if (k++ > 0) {
        throw new Error()
    }
    await undefined;
    const v19 = newGlobal({ sameZoneAs: this }).Debugger(this).getNewestFrame().asyncPromise;
    var reactions = v19.getPromiseReactions();
    print("got reactions" + valueToSource(reactions))
}
const v21 = f0();
print("v21 " + v21)
// The promise p1 comes from a previous invocation of f1; it can't be a plain promise
// freshly created.
const p1 = v21.next();

// If you drainJobQueue() here, the assertion does not fail
const v23 = f0();
v23.next();

v23.return(p1);

The problem arises from having two jobs in the job queue, and when the second one throws an error, the Debugger assertion fails.

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

Bryan and I looked this over again today, and finally got to the bottom of it. There's still some aspects I'm a bit loose on, but the general thrust has clarified for us. I'll present the discussion in the form of a reduced and commented test case below; kudos to Bryan for having the insight to split the generator function into two generator functions, which makes the behaviour much more tractable to understand:

const dbg = newGlobal({ sameZoneAs: this }).Debugger(this);

async function* inspectingGenerator() {
    await undefined;

    const frame = dbg.getNewestFrame();
    const asyncPromise = frame.asyncPromise;
    // Crashes here.
    asyncPromise.getPromiseReactions();
}

async function* throwingGenerator() {
    throw new Error("hi")
}
const gen = inspectingGenerator();
const inspectingGenPromise = gen.next();

const thrownGenerator = throwingGenerator();
// Throw, and close generator.
thrownGenerator.next();

// By calling return here, we trace through the specification and end up in
// 27.6.3.9 AsyncGeneratorAwaitReturn; this creates a reaction record on the
// inspectingGenPromise which points to the thrownGenerator GeneratorObject.
thrownGenerator.return(inspectingGenPromise);

// Causes crash when job queue is run: The crash happens because getReactionRecords
// sees that we have a reaction record associated with a generator object
// (thrownGenerator), which then tries to create a Debugger.frame object for said
// object -- this hits the assertion which demands that a Debugger.frame created for
// a generator object is only created for a *suspended* generator object: but this one
// is *closed*, as it was closed when thrownGenerator.next() was called.
drainJobQueue();

We explored a variation of this test case in this pernosco trace.

Ultimately, this test case manages to violate the expected assertion that a generator's reaction records will always be async promises that are suspended -- they can also be closed.

I'm not sure if it's possible to create a Debugger.frame for a closed generator; it seems likely, as we don't actually have the problems of inspection that we'd have for a running generator. However, we may wish to instead modify the contract of getPromiseReactions to highlight somehow that one of the reactions is on a closed generator? That or perhaps simply elide this case?

Comment 5

[Bryan Thrall [:bthrall]]

Attachment: Bug 1814020 - Ignore closed generators in getPromiseReactions r=arai!

To create the DebuggerFrame for the closed generator, we need the callee
script, but the callee is set to null when the generator is closed, so we can't
create the frame.

Comment 6

[Bryan Thrall [:bthrall]]

Attachment: Bug 1814020 - Rename PromiseReactionRecordBuilder.maybePushGenerator r=arai!

It might not push the generator but still return success.

Depends on D170078

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/780aba86cd87
Ignore closed generators in getPromiseReactions r=arai
https://hg.mozilla.org/integration/autoland/rev/028a94e4d552
Rename PromiseReactionRecordBuilder.maybePushGenerator r=arai

Comment 8

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/780aba86cd87
https://hg.mozilla.org/mozilla-central/rev/028a94e4d552