Skip to content

Commit ff8d67c

Browse files
Stephen BelangerV8 LUCI CQ
Stephen Belanger
authored and
V8 LUCI CQ
committed
Reland "Fix Context PromiseHook behaviour with debugger enabled"
This is a reland of commit 872b7fa Original change's description: > Fix Context PromiseHook behaviour with debugger enabled > > This is a solution for nodejs/node#43148. > > Due to differences in behaviour between code with and without the debugger enabled, some promise lifecycle events were being missed and some extra ones were being added. This change resolves this and verifies the event sequence is consistent between code with and without the debugger. > > Change-Id: I3dabf1dceb14233226b1752083d659f1c2f97966 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3779922 > Reviewed-by: Victor Gomes <[email protected]> > Commit-Queue: Camillo Bruni <[email protected]> > Reviewed-by: Camillo Bruni <[email protected]> > Cr-Commit-Position: refs/heads/main@{#82132} Change-Id: Ifdd407261c793887fbd012d5a04ba36b3744c349 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3805979 Reviewed-by: Camillo Bruni <[email protected]> Reviewed-by: Clemens Backes <[email protected]> Commit-Queue: Camillo Bruni <[email protected]> Reviewed-by: Victor Gomes <[email protected]> Cr-Commit-Position: refs/heads/main@{#82575}
1 parent 5e7c8ea commit ff8d67c

8 files changed

Lines changed: 86 additions & 5 deletions

File tree

‎src/builtins/builtins-microtask-queue-gen.cc‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -479,30 +479,38 @@ void MicrotaskQueueBuiltinsAssembler::RewindEnteredContext(
479479
void MicrotaskQueueBuiltinsAssembler::RunAllPromiseHooks(
480480
PromiseHookType type, TNode<Context> context,
481481
TNode<HeapObject> promise_or_capability) {
482-
Label hook(this, Label::kDeferred), done_hook(this);
483482
TNode<Uint32T> promiseHookFlags = PromiseHookFlags();
483+
#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
484+
Label hook(this, Label::kDeferred), done_hook(this);
484485
Branch(NeedsAnyPromiseHooks(promiseHookFlags), &hook, &done_hook);
485486
BIND(&hook);
486487
{
488+
#endif
487489
switch (type) {
488490
case PromiseHookType::kBefore:
491+
#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
489492
RunContextPromiseHookBefore(context, promise_or_capability,
490493
promiseHookFlags);
494+
#endif
491495
RunPromiseHook(Runtime::kPromiseHookBefore, context,
492496
promise_or_capability, promiseHookFlags);
493497
break;
494498
case PromiseHookType::kAfter:
499+
#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
495500
RunContextPromiseHookAfter(context, promise_or_capability,
496501
promiseHookFlags);
502+
#endif
497503
RunPromiseHook(Runtime::kPromiseHookAfter, context,
498504
promise_or_capability, promiseHookFlags);
499505
break;
500506
default:
501507
UNREACHABLE();
502508
}
509+
#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
503510
Goto(&done_hook);
504511
}
505512
BIND(&done_hook);
513+
#endif
506514
}
507515

508516
void MicrotaskQueueBuiltinsAssembler::RunPromiseHook(

‎src/d8/d8.cc‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2296,6 +2296,16 @@ void Shell::AsyncHooksTriggerAsyncId(
22962296
PerIsolateData::Get(isolate)->GetAsyncHooks()->GetTriggerAsyncId()));
22972297
}
22982298

2299+
static v8::debug::DebugDelegate dummy_delegate;
2300+
2301+
void Shell::EnableDebugger(const v8::FunctionCallbackInfo<v8::Value>& args) {
2302+
v8::debug::SetDebugDelegate(args.GetIsolate(), &dummy_delegate);
2303+
}
2304+
2305+
void Shell::DisableDebugger(const v8::FunctionCallbackInfo<v8::Value>& args) {
2306+
v8::debug::SetDebugDelegate(args.GetIsolate(), nullptr);
2307+
}
2308+
22992309
void Shell::SetPromiseHooks(const v8::FunctionCallbackInfo<v8::Value>& args) {
23002310
Isolate* isolate = args.GetIsolate();
23012311
if (i::FLAG_correctness_fuzzer_suppressions) {
@@ -3378,6 +3388,18 @@ Local<ObjectTemplate> Shell::CreateD8Template(Isolate* isolate) {
33783388
Local<Signature>(), 4));
33793389
d8_template->Set(isolate, "promise", promise_template);
33803390
}
3391+
{
3392+
Local<ObjectTemplate> debugger_template = ObjectTemplate::New(isolate);
3393+
debugger_template->Set(
3394+
isolate, "enable",
3395+
FunctionTemplate::New(isolate, EnableDebugger, Local<Value>(),
3396+
Local<Signature>(), 0));
3397+
debugger_template->Set(
3398+
isolate, "disable",
3399+
FunctionTemplate::New(isolate, DisableDebugger, Local<Value>(),
3400+
Local<Signature>(), 0));
3401+
d8_template->Set(isolate, "debugger", debugger_template);
3402+
}
33813403
{
33823404
Local<ObjectTemplate> serializer_template = ObjectTemplate::New(isolate);
33833405
serializer_template->Set(

‎src/d8/d8.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -583,6 +583,9 @@ class Shell : public i::AllStatic {
583583

584584
static void SetPromiseHooks(const v8::FunctionCallbackInfo<v8::Value>& args);
585585

586+
static void EnableDebugger(const v8::FunctionCallbackInfo<v8::Value>& args);
587+
static void DisableDebugger(const v8::FunctionCallbackInfo<v8::Value>& args);
588+
586589
static void SerializerSerialize(
587590
const v8::FunctionCallbackInfo<v8::Value>& args);
588591
static void SerializerDeserialize(

‎src/execution/isolate.cc‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5154,9 +5154,11 @@ void Isolate::SetPromiseHook(PromiseHook hook) {
51545154
void Isolate::RunAllPromiseHooks(PromiseHookType type,
51555155
Handle<JSPromise> promise,
51565156
Handle<Object> parent) {
5157+
#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
51575158
if (HasContextPromiseHooks()) {
51585159
native_context()->RunPromiseHook(type, promise, parent);
51595160
}
5161+
#endif
51605162
if (HasIsolatePromiseHooks() || HasAsyncEventDelegate()) {
51615163
RunPromiseHook(type, promise, parent);
51625164
}
@@ -5173,7 +5175,7 @@ void Isolate::RunPromiseHook(PromiseHookType type, Handle<JSPromise> promise,
51735175
void Isolate::OnAsyncFunctionSuspended(Handle<JSPromise> promise,
51745176
Handle<JSPromise> parent) {
51755177
DCHECK_EQ(0, promise->async_task_id());
5176-
RunPromiseHook(PromiseHookType::kInit, promise, parent);
5178+
RunAllPromiseHooks(PromiseHookType::kInit, promise, parent);
51775179
if (HasAsyncEventDelegate()) {
51785180
DCHECK_NE(nullptr, async_event_delegate_);
51795181
promise->set_async_task_id(++async_task_count_);

‎src/objects/contexts.cc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -559,6 +559,7 @@ static_assert(NativeContext::kSize ==
559559
(Context::SizeFor(NativeContext::NATIVE_CONTEXT_SLOTS) +
560560
kSystemPointerSize));
561561

562+
#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
562563
void NativeContext::RunPromiseHook(PromiseHookType type,
563564
Handle<JSPromise> promise,
564565
Handle<Object> parent) {
@@ -614,6 +615,7 @@ void NativeContext::RunPromiseHook(PromiseHookType type,
614615
isolate->clear_pending_exception();
615616
}
616617
}
618+
#endif
617619

618620
} // namespace internal
619621
} // namespace v8

‎src/objects/contexts.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -784,8 +784,10 @@ class NativeContext : public Context {
784784
void IncrementErrorsThrown();
785785
int GetErrorsThrown();
786786

787+
#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
787788
void RunPromiseHook(PromiseHookType type, Handle<JSPromise> promise,
788789
Handle<Object> parent);
790+
#endif
789791

790792
private:
791793
static_assert(OffsetOfElementAt(EMBEDDER_DATA_INDEX) ==

‎src/objects/objects.cc‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5483,6 +5483,14 @@ Handle<Object> JSPromise::Fulfill(Handle<JSPromise> promise,
54835483
Handle<Object> value) {
54845484
Isolate* const isolate = promise->GetIsolate();
54855485

5486+
#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
5487+
if (isolate->HasContextPromiseHooks()) {
5488+
isolate->raw_native_context().RunPromiseHook(
5489+
PromiseHookType::kResolve, promise,
5490+
isolate->factory()->undefined_value());
5491+
}
5492+
#endif
5493+
54865494
// 1. Assert: The value of promise.[[PromiseState]] is "pending".
54875495
CHECK_EQ(Promise::kPending, promise->status());
54885496

@@ -5562,8 +5570,8 @@ MaybeHandle<Object> JSPromise::Resolve(Handle<JSPromise> promise,
55625570
DCHECK(
55635571
!reinterpret_cast<v8::Isolate*>(isolate)->GetCurrentContext().IsEmpty());
55645572

5565-
isolate->RunAllPromiseHooks(PromiseHookType::kResolve, promise,
5566-
isolate->factory()->undefined_value());
5573+
isolate->RunPromiseHook(PromiseHookType::kResolve, promise,
5574+
isolate->factory()->undefined_value());
55675575

55685576
// 7. If SameValue(resolution, promise) is true, then
55695577
if (promise.is_identical_to(resolution)) {

‎test/mjsunit/promise-hooks.js‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ function optimizerBailout(test, verify) {
220220
d8.promise.setHooks();
221221
}
222222

223-
if (has_promise_hooks) {
223+
function doTest () {
224224
optimizerBailout(async () => {
225225
await Promise.resolve();
226226
}, () => {
@@ -234,6 +234,19 @@ if (has_promise_hooks) {
234234
assertNextEvent('after', [ 3 ]);
235235
assertEmptyLog();
236236
});
237+
optimizerBailout(async () => {
238+
await Promise.reject();
239+
}, () => {
240+
assertNextEvent('init', [ 1 ]);
241+
assertNextEvent('init', [ 2 ]);
242+
assertNextEvent('resolve', [ 2 ]);
243+
assertNextEvent('init', [ 3, 2 ]);
244+
assertNextEvent('before', [ 3 ]);
245+
assertNextEvent('resolve', [ 1 ]);
246+
assertNextEvent('resolve', [ 3 ]);
247+
assertNextEvent('after', [ 3 ]);
248+
assertEmptyLog();
249+
});
237250
optimizerBailout(async () => {
238251
await { then (cb) { cb() } };
239252
}, () => {
@@ -249,6 +262,21 @@ if (has_promise_hooks) {
249262
assertNextEvent('after', [ 3 ]);
250263
assertEmptyLog();
251264
});
265+
optimizerBailout(async () => {
266+
await { then (_, cb) { cb() } };
267+
}, () => {
268+
assertNextEvent('init', [ 1 ]);
269+
assertNextEvent('init', [ 2, 1 ]);
270+
assertNextEvent('init', [ 3, 2 ]);
271+
assertNextEvent('before', [ 2 ]);
272+
assertNextEvent('resolve', [ 2 ]);
273+
assertNextEvent('after', [ 2 ]);
274+
assertNextEvent('before', [ 3 ]);
275+
assertNextEvent('resolve', [ 1 ]);
276+
assertNextEvent('resolve', [ 3 ]);
277+
assertNextEvent('after', [ 3 ]);
278+
assertEmptyLog();
279+
});
252280
basicTest();
253281
exceptions();
254282

@@ -292,3 +320,9 @@ if (has_promise_hooks) {
292320
});
293321

294322
}
323+
324+
if (has_promise_hooks) {
325+
doTest();
326+
d8.debugger.enable();
327+
doTest();
328+
}

0 commit comments

Comments
 (0)