Fix Context PromiseHook behaviour with debugger enabled

This is a solution for https://github.com/nodejs/node/issues/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/+/3779922Reviewed-by: Victor Gomes <victorgomes@chromium.org>
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82132}

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

@@ -479,30 +479,38 @@ void MicrotaskQueueBuiltinsAssembler::RewindEnteredContext(
 void MicrotaskQueueBuiltinsAssembler::RunAllPromiseHooks(
     PromiseHookType type, TNode<Context> context,
     TNode<HeapObject> promise_or_capability) {
-  Label hook(this, Label::kDeferred), done_hook(this);
   TNode<Uint32T> promiseHookFlags = PromiseHookFlags();
+#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
+  Label hook(this, Label::kDeferred), done_hook(this);
   Branch(NeedsAnyPromiseHooks(promiseHookFlags), &hook, &done_hook);
   BIND(&hook);
   {
+#endif
     switch (type) {
       case PromiseHookType::kBefore:
+#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
         RunContextPromiseHookBefore(context, promise_or_capability,
                                     promiseHookFlags);
+#endif
         RunPromiseHook(Runtime::kPromiseHookBefore, context,
                        promise_or_capability, promiseHookFlags);
         break;
       case PromiseHookType::kAfter:
+#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
         RunContextPromiseHookAfter(context, promise_or_capability,
                                    promiseHookFlags);
+#endif
         RunPromiseHook(Runtime::kPromiseHookAfter, context,
                        promise_or_capability, promiseHookFlags);
         break;
       default:
         UNREACHABLE();
     }
+#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
     Goto(&done_hook);
   }
   BIND(&done_hook);
+#endif
 }
 
 void MicrotaskQueueBuiltinsAssembler::RunPromiseHook(

src/d8/d8.cc

@@ -2292,6 +2292,16 @@ void Shell::AsyncHooksTriggerAsyncId(
       PerIsolateData::Get(isolate)->GetAsyncHooks()->GetTriggerAsyncId()));
 }
 
+static v8::debug::DebugDelegate dummy_delegate;
+
+void Shell::EnableDebugger(const v8::FunctionCallbackInfo<v8::Value>& args) {
+  v8::debug::SetDebugDelegate(args.GetIsolate(), &dummy_delegate);
+}
+
+void Shell::DisableDebugger(const v8::FunctionCallbackInfo<v8::Value>& args) {
+  v8::debug::SetDebugDelegate(args.GetIsolate(), nullptr);
+}
+
 void Shell::SetPromiseHooks(const v8::FunctionCallbackInfo<v8::Value>& args) {
   Isolate* isolate = args.GetIsolate();
   if (i::FLAG_correctness_fuzzer_suppressions) {
@@ -3374,6 +3384,18 @@ Local<ObjectTemplate> Shell::CreateD8Template(Isolate* isolate) {
                               Local<Signature>(), 4));
     d8_template->Set(isolate, "promise", promise_template);
   }
+  {
+    Local<ObjectTemplate> debugger_template = ObjectTemplate::New(isolate);
+    debugger_template->Set(
+        isolate, "enable",
+        FunctionTemplate::New(isolate, EnableDebugger, Local<Value>(),
+                              Local<Signature>(), 0));
+    debugger_template->Set(
+        isolate, "disable",
+        FunctionTemplate::New(isolate, DisableDebugger, Local<Value>(),
+                              Local<Signature>(), 0));
+    d8_template->Set(isolate, "debugger", debugger_template);
+  }
   {
     Local<ObjectTemplate> serializer_template = ObjectTemplate::New(isolate);
     serializer_template->Set(

src/d8/d8.h

@@ -583,6 +583,9 @@ class Shell : public i::AllStatic {
 
   static void SetPromiseHooks(const v8::FunctionCallbackInfo<v8::Value>& args);
 
+  static void EnableDebugger(const v8::FunctionCallbackInfo<v8::Value>& args);
+  static void DisableDebugger(const v8::FunctionCallbackInfo<v8::Value>& args);
+
   static void SerializerSerialize(
       const v8::FunctionCallbackInfo<v8::Value>& args);
   static void SerializerDeserialize(

src/execution/isolate.cc

@@ -5111,9 +5111,11 @@ void Isolate::SetPromiseHook(PromiseHook hook) {
 void Isolate::RunAllPromiseHooks(PromiseHookType type,
                                  Handle<JSPromise> promise,
                                  Handle<Object> parent) {
+#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
   if (HasContextPromiseHooks()) {
     native_context()->RunPromiseHook(type, promise, parent);
   }
+#endif
   if (HasIsolatePromiseHooks() || HasAsyncEventDelegate()) {
     RunPromiseHook(type, promise, parent);
   }
@@ -5130,7 +5132,7 @@ void Isolate::RunPromiseHook(PromiseHookType type, Handle<JSPromise> promise,
 void Isolate::OnAsyncFunctionSuspended(Handle<JSPromise> promise,
                                        Handle<JSPromise> parent) {
   DCHECK_EQ(0, promise->async_task_id());
-  RunPromiseHook(PromiseHookType::kInit, promise, parent);
+  RunAllPromiseHooks(PromiseHookType::kInit, promise, parent);
   if (HasAsyncEventDelegate()) {
     DCHECK_NE(nullptr, async_event_delegate_);
     promise->set_async_task_id(++async_task_count_);

src/objects/contexts.cc

@@ -559,6 +559,7 @@ static_assert(NativeContext::kSize ==
               (Context::SizeFor(NativeContext::NATIVE_CONTEXT_SLOTS) +
                kSystemPointerSize));
 
+#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
 void NativeContext::RunPromiseHook(PromiseHookType type,
                                    Handle<JSPromise> promise,
                                    Handle<Object> parent) {
@@ -614,6 +615,7 @@ void NativeContext::RunPromiseHook(PromiseHookType type,
     isolate->clear_pending_exception();
   }
 }
+#endif
 
 }  // namespace internal
 }  // namespace v8

src/objects/contexts.h

@@ -786,8 +786,10 @@ class NativeContext : public Context {
   void IncrementErrorsThrown();
   int GetErrorsThrown();
 
+#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
   void RunPromiseHook(PromiseHookType type, Handle<JSPromise> promise,
                       Handle<Object> parent);
+#endif
 
  private:
   static_assert(OffsetOfElementAt(EMBEDDER_DATA_INDEX) ==

src/objects/objects.cc

@@ -5451,6 +5451,14 @@ Handle<Object> JSPromise::Fulfill(Handle<JSPromise> promise,
                                   Handle<Object> value) {
   Isolate* const isolate = promise->GetIsolate();
 
+#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
+  if (isolate->HasContextPromiseHooks()) {
+    isolate->raw_native_context().RunPromiseHook(
+        PromiseHookType::kResolve, promise,
+        isolate->factory()->undefined_value());
+  }
+#endif
+
   // 1. Assert: The value of promise.[[PromiseState]] is "pending".
   CHECK_EQ(Promise::kPending, promise->status());
 
@@ -5530,8 +5538,8 @@ MaybeHandle<Object> JSPromise::Resolve(Handle<JSPromise> promise,
   DCHECK(
       !reinterpret_cast<v8::Isolate*>(isolate)->GetCurrentContext().IsEmpty());
 
-  isolate->RunAllPromiseHooks(PromiseHookType::kResolve, promise,
-                              isolate->factory()->undefined_value());
+  isolate->RunPromiseHook(PromiseHookType::kResolve, promise,
+                          isolate->factory()->undefined_value());
 
   // 7. If SameValue(resolution, promise) is true, then
   if (promise.is_identical_to(resolution)) {

test/mjsunit/promise-hooks.js

@@ -220,7 +220,7 @@ function optimizerBailout(test, verify) {
   d8.promise.setHooks();
 }
 
-if (has_promise_hooks) {
+function doTest () {
   optimizerBailout(async () => {
     await Promise.resolve();
   }, () => {
@@ -234,6 +234,19 @@ if (has_promise_hooks) {
     assertNextEvent('after', [ 3 ]);
     assertEmptyLog();
   });
+  optimizerBailout(async () => {
+    await Promise.reject();
+  }, () => {
+    assertNextEvent('init', [ 1 ]);
+    assertNextEvent('init', [ 2 ]);
+    assertNextEvent('resolve', [ 2 ]);
+    assertNextEvent('init', [ 3, 2 ]);
+    assertNextEvent('before', [ 3 ]);
+    assertNextEvent('resolve', [ 1 ]);
+    assertNextEvent('resolve', [ 3 ]);
+    assertNextEvent('after', [ 3 ]);
+    assertEmptyLog();
+  });
   optimizerBailout(async () => {
     await { then (cb) { cb() } };
   }, () => {
@@ -249,6 +262,21 @@ if (has_promise_hooks) {
     assertNextEvent('after', [ 3 ]);
     assertEmptyLog();
   });
+  optimizerBailout(async () => {
+    await { then (_, cb) { cb() } };
+  }, () => {
+    assertNextEvent('init', [ 1 ]);
+    assertNextEvent('init', [ 2, 1 ]);
+    assertNextEvent('init', [ 3, 2 ]);
+    assertNextEvent('before', [ 2 ]);
+    assertNextEvent('resolve', [ 2 ]);
+    assertNextEvent('after', [ 2 ]);
+    assertNextEvent('before', [ 3 ]);
+    assertNextEvent('resolve', [ 1 ]);
+    assertNextEvent('resolve', [ 3 ]);
+    assertNextEvent('after', [ 3 ]);
+    assertEmptyLog();
+  });
   basicTest();
   exceptions();
 
@@ -292,3 +320,9 @@ if (has_promise_hooks) {
   });
 
 }
+
+if (has_promise_hooks) {
+  doTest();
+  d8.debugger.enable();
+  doTest();
+}