From c5630ad1a79cce11e7351ebe8d6b4844d9b50f08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Thu, 1 Sep 2022 10:15:49 +0200 Subject: [PATCH] deps: V8: backport ff8d67c88449 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original commit message: Reland "Fix Context PromiseHook behaviour with debugger enabled" This is a reland of commit 872b7faa32d837f9b166d750328357f856168e72 Original change's description: > 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/+/3779922 > Reviewed-by: Victor Gomes > Commit-Queue: Camillo Bruni > Reviewed-by: Camillo Bruni > 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 Reviewed-by: Clemens Backes Commit-Queue: Camillo Bruni Reviewed-by: Victor Gomes Cr-Commit-Position: refs/heads/main@{#82575} Refs: https://github.com/v8/v8/commit/ff8d67c88449705af9ac35630cb82c250ea5ac8c Fixes: https://github.com/nodejs/node/issues/43148 Fixes: https://github.com/nodejs/node/issues/44415 PR-URL: https://github.com/nodejs/node/pull/44423 Reviewed-By: Ben Noordhuis Reviewed-By: Stephen Belanger Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Jiawen Geng Reviewed-By: Gerhard Stöbich --- common.gypi | 2 +- .../builtins/builtins-microtask-queue-gen.cc | 10 +++++- deps/v8/src/d8/d8.cc | 22 ++++++++++++ deps/v8/src/d8/d8.h | 3 ++ deps/v8/src/execution/isolate.cc | 4 ++- deps/v8/src/objects/contexts.cc | 2 ++ deps/v8/src/objects/contexts.h | 2 ++ deps/v8/src/objects/objects.cc | 12 +++++-- deps/v8/test/mjsunit/promise-hooks.js | 36 ++++++++++++++++++- 9 files changed, 87 insertions(+), 6 deletions(-) diff --git a/common.gypi b/common.gypi index 3e84350395dfe4..33cd361e67a672 100644 --- a/common.gypi +++ b/common.gypi @@ -36,7 +36,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.11', + 'v8_embedder_string': '-node.12', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/src/builtins/builtins-microtask-queue-gen.cc b/deps/v8/src/builtins/builtins-microtask-queue-gen.cc index 9edc8ce00c524a..6b0c63b34ff09f 100644 --- a/deps/v8/src/builtins/builtins-microtask-queue-gen.cc +++ b/deps/v8/src/builtins/builtins-microtask-queue-gen.cc @@ -478,30 +478,38 @@ void MicrotaskQueueBuiltinsAssembler::RewindEnteredContext( void MicrotaskQueueBuiltinsAssembler::RunAllPromiseHooks( PromiseHookType type, TNode context, TNode promise_or_capability) { - Label hook(this, Label::kDeferred), done_hook(this); TNode 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( diff --git a/deps/v8/src/d8/d8.cc b/deps/v8/src/d8/d8.cc index 6f38f7280e5b3c..42c1fbef5670a0 100644 --- a/deps/v8/src/d8/d8.cc +++ b/deps/v8/src/d8/d8.cc @@ -2218,6 +2218,16 @@ void Shell::AsyncHooksTriggerAsyncId( PerIsolateData::Get(isolate)->GetAsyncHooks()->GetTriggerAsyncId())); } +static v8::debug::DebugDelegate dummy_delegate; + +void Shell::EnableDebugger(const v8::FunctionCallbackInfo& args) { + v8::debug::SetDebugDelegate(args.GetIsolate(), &dummy_delegate); +} + +void Shell::DisableDebugger(const v8::FunctionCallbackInfo& args) { + v8::debug::SetDebugDelegate(args.GetIsolate(), nullptr); +} + void Shell::SetPromiseHooks(const v8::FunctionCallbackInfo& args) { Isolate* isolate = args.GetIsolate(); if (i::FLAG_correctness_fuzzer_suppressions) { @@ -3162,6 +3172,18 @@ Local Shell::CreateD8Template(Isolate* isolate) { Local(), 4)); d8_template->Set(isolate, "promise", promise_template); } + { + Local debugger_template = ObjectTemplate::New(isolate); + debugger_template->Set( + isolate, "enable", + FunctionTemplate::New(isolate, EnableDebugger, Local(), + Local(), 0)); + debugger_template->Set( + isolate, "disable", + FunctionTemplate::New(isolate, DisableDebugger, Local(), + Local(), 0)); + d8_template->Set(isolate, "debugger", debugger_template); + } return d8_template; } diff --git a/deps/v8/src/d8/d8.h b/deps/v8/src/d8/d8.h index d0bb1c8fdfeade..c77a8a032fd352 100644 --- a/deps/v8/src/d8/d8.h +++ b/deps/v8/src/d8/d8.h @@ -569,6 +569,9 @@ class Shell : public i::AllStatic { static void SetPromiseHooks(const v8::FunctionCallbackInfo& args); + static void EnableDebugger(const v8::FunctionCallbackInfo& args); + static void DisableDebugger(const v8::FunctionCallbackInfo& args); + static void Print(const v8::FunctionCallbackInfo& args); static void PrintErr(const v8::FunctionCallbackInfo& args); static void WriteStdout(const v8::FunctionCallbackInfo& args); diff --git a/deps/v8/src/execution/isolate.cc b/deps/v8/src/execution/isolate.cc index fb0939c03edd23..62d9f4abc33fed 100644 --- a/deps/v8/src/execution/isolate.cc +++ b/deps/v8/src/execution/isolate.cc @@ -4958,9 +4958,11 @@ void Isolate::SetPromiseHook(PromiseHook hook) { void Isolate::RunAllPromiseHooks(PromiseHookType type, Handle promise, Handle parent) { +#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS if (HasContextPromiseHooks()) { native_context()->RunPromiseHook(type, promise, parent); } +#endif if (HasIsolatePromiseHooks() || HasAsyncEventDelegate()) { RunPromiseHook(type, promise, parent); } @@ -4977,7 +4979,7 @@ void Isolate::RunPromiseHook(PromiseHookType type, Handle promise, void Isolate::OnAsyncFunctionSuspended(Handle promise, Handle 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_); diff --git a/deps/v8/src/objects/contexts.cc b/deps/v8/src/objects/contexts.cc index 2f514ac1e7b1b7..62ec8d4dcb3d01 100644 --- a/deps/v8/src/objects/contexts.cc +++ b/deps/v8/src/objects/contexts.cc @@ -551,6 +551,7 @@ STATIC_ASSERT(NativeContext::kSize == (Context::SizeFor(NativeContext::NATIVE_CONTEXT_SLOTS) + kSystemPointerSize)); +#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS void NativeContext::RunPromiseHook(PromiseHookType type, Handle promise, Handle parent) { @@ -606,6 +607,7 @@ void NativeContext::RunPromiseHook(PromiseHookType type, isolate->clear_pending_exception(); } } +#endif } // namespace internal } // namespace v8 diff --git a/deps/v8/src/objects/contexts.h b/deps/v8/src/objects/contexts.h index efdf3c383f425b..c9a0072993e885 100644 --- a/deps/v8/src/objects/contexts.h +++ b/deps/v8/src/objects/contexts.h @@ -781,8 +781,10 @@ class NativeContext : public Context { void IncrementErrorsThrown(); int GetErrorsThrown(); +#ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS void RunPromiseHook(PromiseHookType type, Handle promise, Handle parent); +#endif private: STATIC_ASSERT(OffsetOfElementAt(EMBEDDER_DATA_INDEX) == diff --git a/deps/v8/src/objects/objects.cc b/deps/v8/src/objects/objects.cc index d16146114e81cc..b87672246285ce 100644 --- a/deps/v8/src/objects/objects.cc +++ b/deps/v8/src/objects/objects.cc @@ -5398,6 +5398,14 @@ Handle JSPromise::Fulfill(Handle promise, Handle 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()); @@ -5477,8 +5485,8 @@ MaybeHandle JSPromise::Resolve(Handle promise, DCHECK( !reinterpret_cast(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)) { diff --git a/deps/v8/test/mjsunit/promise-hooks.js b/deps/v8/test/mjsunit/promise-hooks.js index 60bab26b4307b6..3b99e0e013ec62 100644 --- a/deps/v8/test/mjsunit/promise-hooks.js +++ b/deps/v8/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(); +}