Skip to content

Commit 872b7fa

Browse files
Stephen BelangerV8 LUCI CQ
Stephen Belanger
authored and
V8 LUCI CQ
committed
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}
1 parent ef9eec0 commit 872b7fa

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
@@ -2292,6 +2292,16 @@ void Shell::AsyncHooksTriggerAsyncId(
22922292
PerIsolateData::Get(isolate)->GetAsyncHooks()->GetTriggerAsyncId()));
22932293
}
22942294

2295+
static v8::debug::DebugDelegate dummy_delegate;
2296+
2297+
void Shell::EnableDebugger(const v8::FunctionCallbackInfo<v8::Value>& args) {
2298+
v8::debug::SetDebugDelegate(args.GetIsolate(), &dummy_delegate);
2299+
}
2300+
2301+
void Shell::DisableDebugger(const v8::FunctionCallbackInfo<v8::Value>& args) {
2302+
v8::debug::SetDebugDelegate(args.GetIsolate(), nullptr);
2303+
}
2304+
22952305
void Shell::SetPromiseHooks(const v8::FunctionCallbackInfo<v8::Value>& args) {
22962306
Isolate* isolate = args.GetIsolate();
22972307
if (i::FLAG_correctness_fuzzer_suppressions) {
@@ -3374,6 +3384,18 @@ Local<ObjectTemplate> Shell::CreateD8Template(Isolate* isolate) {
33743384
Local<Signature>(), 4));
33753385
d8_template->Set(isolate, "promise", promise_template);
33763386
}
3387+
{
3388+
Local<ObjectTemplate> debugger_template = ObjectTemplate::New(isolate);
3389+
debugger_template->Set(
3390+
isolate, "enable",
3391+
FunctionTemplate::New(isolate, EnableDebugger, Local<Value>(),
3392+
Local<Signature>(), 0));
3393+
debugger_template->Set(
3394+
isolate, "disable",
3395+
FunctionTemplate::New(isolate, DisableDebugger, Local<Value>(),
3396+
Local<Signature>(), 0));
3397+
d8_template->Set(isolate, "debugger", debugger_template);
3398+
}
33773399
{
33783400
Local<ObjectTemplate> serializer_template = ObjectTemplate::New(isolate);
33793401
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
@@ -5111,9 +5111,11 @@ void Isolate::SetPromiseHook(PromiseHook hook) {
51115111
void Isolate::RunAllPromiseHooks(PromiseHookType type,
51125112
Handle<JSPromise> promise,
51135113
Handle<Object> parent) {
5114+
#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
51145115
if (HasContextPromiseHooks()) {
51155116
native_context()->RunPromiseHook(type, promise, parent);
51165117
}
5118+
#endif
51175119
if (HasIsolatePromiseHooks() || HasAsyncEventDelegate()) {
51185120
RunPromiseHook(type, promise, parent);
51195121
}
@@ -5130,7 +5132,7 @@ void Isolate::RunPromiseHook(PromiseHookType type, Handle<JSPromise> promise,
51305132
void Isolate::OnAsyncFunctionSuspended(Handle<JSPromise> promise,
51315133
Handle<JSPromise> parent) {
51325134
DCHECK_EQ(0, promise->async_task_id());
5133-
RunPromiseHook(PromiseHookType::kInit, promise, parent);
5135+
RunAllPromiseHooks(PromiseHookType::kInit, promise, parent);
51345136
if (HasAsyncEventDelegate()) {
51355137
DCHECK_NE(nullptr, async_event_delegate_);
51365138
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
@@ -786,8 +786,10 @@ class NativeContext : public Context {
786786
void IncrementErrorsThrown();
787787
int GetErrorsThrown();
788788

789+
#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
789790
void RunPromiseHook(PromiseHookType type, Handle<JSPromise> promise,
790791
Handle<Object> parent);
792+
#endif
791793

792794
private:
793795
static_assert(OffsetOfElementAt(EMBEDDER_DATA_INDEX) ==

‎src/objects/objects.cc‎

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

5454+
#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
5455+
if (isolate->HasContextPromiseHooks()) {
5456+
isolate->raw_native_context().RunPromiseHook(
5457+
PromiseHookType::kResolve, promise,
5458+
isolate->factory()->undefined_value());
5459+
}
5460+
#endif
5461+
54545462
// 1. Assert: The value of promise.[[PromiseState]] is "pending".
54555463
CHECK_EQ(Promise::kPending, promise->status());
54565464

@@ -5530,8 +5538,8 @@ MaybeHandle<Object> JSPromise::Resolve(Handle<JSPromise> promise,
55305538
DCHECK(
55315539
!reinterpret_cast<v8::Isolate*>(isolate)->GetCurrentContext().IsEmpty());
55325540

5533-
isolate->RunAllPromiseHooks(PromiseHookType::kResolve, promise,
5534-
isolate->factory()->undefined_value());
5541+
isolate->RunPromiseHook(PromiseHookType::kResolve, promise,
5542+
isolate->factory()->undefined_value());
55355543

55365544
// 7. If SameValue(resolution, promise) is true, then
55375545
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)