1644699 - Assertion failure: isObject(), at js/Value.h:745 or Crash [@ js::DebuggerFrame::getEnvironment] with Debugger and interrupt

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200608-63dc5e9b1b02 (debug build, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --disable-oom-functions):

const g = newGlobal({ newCompartment: true });
const dbg = new Debugger(g);
g.eval(`
async function* f() {
  var promises = {};
  await promises.top;
  await promises.block;
}
`);
dbg.onEnterFrame = f => {
  frame = f;
  dbg.onEnterFrame = undefined;
};
(async () => {
  const it = g.f();
  let promise = it.next();
  await function() { return true; };
  frame.environment.names()
})();
interruptIf(true);
foo("C");

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x00005555557bae31 in JS::Value::toObject() const ()
#1  0x0000555555ff4aa8 in js::DebuggerFrame::getEnvironment(JSContext*, JS::Handle<js::DebuggerFrame*>, JS::MutableHandle<js::DebuggerEnvironment*>) ()
#2  0x0000555555ffa9cb in js::DebuggerFrame::CallData::environmentGetter() ()
#3  0x0000555555fff6b7 in bool js::DebuggerFrame::CallData::ToNative<&js::DebuggerFrame::CallData::environmentGetter>(JSContext*, unsigned int, JS::Value*) ()
#4  0x0000555555939252 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) ()
#5  0x0000555555938b29 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) ()
#6  0x000055555593a1bc in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) ()
#7  0x000055555593b1c2 in js::CallGetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) ()
#8  0x0000555555cdb6fc in bool GetExistingProperty<(js::AllowGC)1>(JSContext*, js::MaybeRooted<JS::Value, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::Shape*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) ()
#9  0x0000555555cdc493 in bool NativeGetPropertyInline<(js::AllowGC)1>(JSContext*, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::PropertyKey, (js::AllowGC)1>::HandleType, IsNameLookup, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) ()
#10 0x0000555555811eb8 in js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, js::PropertyName*, JS::MutableHandle<JS::Value>) ()
#11 0x000055555593f1be in js::GetProperty(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) ()
#12 0x000055555592a9a9 in Interpret(JSContext*, js::RunState&) ()
#13 0x0000555555923be2 in js::RunScript(JSContext*, js::RunState&) ()
#14 0x0000555555938a3f in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) ()
#15 0x000055555593a1bc in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) ()
#16 0x000055555593a430 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) ()
#17 0x0000555555d4160c in js::CallSelfHostedFunction(JSContext*, JS::Handle<js::PropertyName*>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) ()
#18 0x0000555555ad0bba in AsyncFunctionResume(JSContext*, JS::Handle<js::AsyncFunctionGeneratorObject*>, ResumeKind, JS::Handle<JS::Value>) ()
#19 0x0000555555bd297e in PromiseReactionJob(JSContext*, unsigned int, JS::Value*) ()
#20 0x0000555555939252 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) ()
#21 0x0000555555938b29 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) ()
#22 0x000055555593a1bc in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) ()
#23 0x000055555593a430 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) ()
#24 0x0000555555a38578 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) ()
#25 0x0000555555c22faf in js::InternalJobQueue::runJobs(JSContext*) ()
#26 0x0000555555c22b63 in js::RunJobs(JSContext*) ()
#27 0x00005555557cdaa8 in RunShellJobs(JSContext*) ()
#28 0x00005555557b6987 in Shell(JSContext*, js::cli::OptionParser*, char**) ()
#29 0x00005555557afdcd in main ()
rax	0x555556f77371	93825019638641
rbx	0x7fffffffb330	140737488335664
rcx	0x555558369980	93825040554368
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffade0	140737488334304
rsp	0x7fffffffade0	140737488334304
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f9bd40	140737353727296
r10	0x0	0
r11	0x0	0
r12	0x7ffff6027000	140737320742912
r13	0x32a34df00b08	55676968635144
r14	0x11b167b9060	1215852941408
r15	0x7fffffffb2c8	140737488335560
rip	0x5555557bae31 <JS::Value::toObject() const+177>
=> 0x5555557bae31 <_ZNK2JS5Value8toObjectEv+177>:	movl   $0x2e9,0x0
   0x5555557bae3c <_ZNK2JS5Value8toObjectEv+188>:	callq  0x55555583f74e <abort>

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20200611093454-10ad7868f3ca.
The bug appears to have been introduced in the following build range:
> Start: e3354d10903988afda731b6c20a9e7afbad05119 (20191208235104)
> End: 31d14cf1975c1b23ef3db176252f8cc3b31a0772 (20191208235601)
> Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e3354d10903988afda731b6c20a9e7afbad05119&tochange=31d14cf1975c1b23ef3db176252f8cc3b31a0772

Comment 3

[Nicolas B. Pierron [:nbp]]

Logan, comment 2 seems to point at your patch series.

Comment 4

[Logan Smyth [:loganfsmyth]]

Yep, my patch set exposed this edge case.

What is happening is that the the interrupt is terminating execution in a way that causes our assumptions to be invalid. We currently assume that a generator object will only be marked closed after DebugAPI::onLeaveFrame has run for that frame, which would have taken care of cleaning up any other metadata about the frame.

In the case of this test, the termination causes the generator to be closed before the generator has resumed, this onLeaveFrame won't run, meaning that the debugger won't be informed that anything is happening at all, so it cannot clear the GENERATOR_INFO_SLOT on the Debugger.Frame object.

We'll need to validate and skip frames where the generator is closed, but still associated with the Debugger.Frame. This will mean that the Debugger.Frame and the GeneratorObject remain linked until they are both GCed, but I don't think there is any way for us to get around that, and I'm honestly not sure this bug is reachable without termination, so I imagine it is pretty unlikely in the vast majority of cases.

Comment 5

[Jason Orendorff [:jorendorff]]

Certainly the existing code for the DebuggerFrame object all assumes that the generator associated with a DebuggerFrame, if any, has not been closed. So the expectation is that DebuggerFrame::clearGenerator() is called (to disassociate it) when AbstractGeneratorObject::setClosed() is called, if not earlier.

Two opposite fixes:

• The failure path in js::AsyncGeneratorResume (where the generator is actually closed) could notify the debugger, maybe. Or setClosed itself. (And/or setClosed could assert that no remaining associated DebuggerFrame objects exist.)

• Or, Debugger.Frame could be more robust against the generator suddenly turning out to have been closed without notification.

Comment 6

[Logan Smyth [:loganfsmyth]]

Or, Debugger.Frame could be more robust against the generator suddenly turning out to have been closed without notification.

I think that's route I'd like to go with. I'm going to split hasGenerator() into hasGeneratorInfo() and isSuspended() because it's both clearer that way and satisfies every current usecase.

I have that patch done locally, the main issue I'm running into is actually writing a test for this, because the testcase relies on an interrupt triggering a termination inside of a selfhosted function, and it seems like https://searchfox.org/mozilla-central/rev/0e09b9191c02097034e46b193930f91c45b7885d/js/src/shell/js.cpp#802 automatically sets an error code, which means that https://searchfox.org/mozilla-central/rev/0e09b9191c02097034e46b193930f91c45b7885d/js/src/shell/js.cpp#10874 doesn't run, so there is no way to simultaneously

1. Use an interrupt to terminate code
2. Throw an assertion failure to trigger a test failure

Comment 7

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1644699 - Allow Debugger.Frame to have GeneratorInfo that is closed. r=arai!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d470b425ce9
Allow Debugger.Frame to have GeneratorInfo that is closed. r=arai

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/2d470b425ce9

Comment 10

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20200616154959-89a54069f124.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.