Navigation Menu

Skip to content

Commit

Permalink
deps: V8: backport ff8d67c88449
Browse files Browse the repository at this point in the history
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 #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 <victorgomes@chromium.org>
    > Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    > Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    > 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 <cbruni@chromium.org>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Reviewed-by: Victor Gomes <victorgomes@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#82575}

Refs: v8/v8@ff8d67c
Fixes: #43148
Fixes: #44415
PR-URL: #44423
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
  • Loading branch information
targos authored and RafaelGSS committed Sep 7, 2022
1 parent 4c33e5d commit c5630ad
Show file tree
Hide file tree
Showing 9 changed files with 87 additions and 6 deletions.
2 changes: 1 addition & 1 deletion common.gypi
Expand Up @@ -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 #####

Expand Down
10 changes: 9 additions & 1 deletion deps/v8/src/builtins/builtins-microtask-queue-gen.cc
Expand Up @@ -478,30 +478,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(
Expand Down
22 changes: 22 additions & 0 deletions deps/v8/src/d8/d8.cc
Expand Up @@ -2218,6 +2218,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) {
Expand Down Expand Up @@ -3162,6 +3172,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);
}
return d8_template;
}

Expand Down
3 changes: 3 additions & 0 deletions deps/v8/src/d8/d8.h
Expand Up @@ -569,6 +569,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 Print(const v8::FunctionCallbackInfo<v8::Value>& args);
static void PrintErr(const v8::FunctionCallbackInfo<v8::Value>& args);
static void WriteStdout(const v8::FunctionCallbackInfo<v8::Value>& args);
Expand Down
4 changes: 3 additions & 1 deletion deps/v8/src/execution/isolate.cc
Expand Up @@ -4958,9 +4958,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);
}
Expand All @@ -4977,7 +4979,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_);
Expand Down
2 changes: 2 additions & 0 deletions deps/v8/src/objects/contexts.cc
Expand Up @@ -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<JSPromise> promise,
Handle<Object> parent) {
Expand Down Expand Up @@ -606,6 +607,7 @@ void NativeContext::RunPromiseHook(PromiseHookType type,
isolate->clear_pending_exception();
}
}
#endif

} // namespace internal
} // namespace v8
2 changes: 2 additions & 0 deletions deps/v8/src/objects/contexts.h
Expand Up @@ -781,8 +781,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) ==
Expand Down
12 changes: 10 additions & 2 deletions deps/v8/src/objects/objects.cc
Expand Up @@ -5398,6 +5398,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());

Expand Down Expand Up @@ -5477,8 +5485,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)) {
Expand Down
36 changes: 35 additions & 1 deletion deps/v8/test/mjsunit/promise-hooks.js
Expand Up @@ -220,7 +220,7 @@ function optimizerBailout(test, verify) {
d8.promise.setHooks();
}

if (has_promise_hooks) {
function doTest () {
optimizerBailout(async () => {
await Promise.resolve();
}, () => {
Expand All @@ -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() } };
}, () => {
Expand All @@ -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();

Expand Down Expand Up @@ -292,3 +320,9 @@ if (has_promise_hooks) {
});

}

if (has_promise_hooks) {
doTest();
d8.debugger.enable();
doTest();
}

0 comments on commit c5630ad

Please sign in to comment.