From 090a3f6bcc639c8d4ef2b9207e65ee1be22c75fc Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 11 Dec 2020 17:15:54 +0100 Subject: [PATCH 1/3] deps: V8: backport 4bf051d536a1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original commit message: [api] Add Context::GetMicrotaskQueue method Add a method that returns the microtask queue that is being used by the `v8::Context`. This is helpful in non-monolithic embedders like Node.js, which accept Contexts created by its own embedders like Electron, or for native Node.js addons. In particular, it enables: 1. Making sure that “nested” `Context`s use the correct microtask queue, i.e. the one from the outer Context. 2. Enqueueing microtasks into the correct microtask queue. Previously, these things only worked when the microtask queue for a given Context was the Isolate’s default queue. As an alternative, I considered adding a way to make new `Context`s inherit the queue from the `Context` that was entered at the time of their creation, but that seemed a bit more “magic”, less flexible, and didn’t take care of concern 2 listed above. Change-Id: I15ed796df90f23c97a545a8e1b30a3bf4a5c4320 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2579914 Reviewed-by: Toon Verwaest Commit-Queue: Toon Verwaest Cr-Commit-Position: refs/heads/master@{#71710} Refs: https://github.com/v8/v8/commit/4bf051d536a172e7932845d82f918ad7280c7873 --- common.gypi | 2 +- deps/v8/include/v8.h | 5 ++++- deps/v8/src/api/api.cc | 6 ++++++ deps/v8/test/cctest/test-api.cc | 10 ++++++++++ 4 files changed, 21 insertions(+), 2 deletions(-) diff --git a/common.gypi b/common.gypi index 77ffb7e4867ca6..67aa8e7200a162 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.20', + 'v8_embedder_string': '-node.21', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/include/v8.h b/deps/v8/include/v8.h index 7999f358ab5e5d..f51fad14d9e479 100644 --- a/deps/v8/include/v8.h +++ b/deps/v8/include/v8.h @@ -10417,9 +10417,12 @@ class V8_EXPORT Context { */ void Exit(); - /** Returns an isolate associated with a current context. */ + /** Returns the isolate associated with a current context. */ Isolate* GetIsolate(); + /** Returns the microtask queue associated with a current context. */ + MicrotaskQueue* GetMicrotaskQueue(); + /** * The field at kDebugIdIndex used to be reserved for the inspector. * It now serves no purpose. diff --git a/deps/v8/src/api/api.cc b/deps/v8/src/api/api.cc index c27db3e7b876e0..11a9dce9f0611e 100644 --- a/deps/v8/src/api/api.cc +++ b/deps/v8/src/api/api.cc @@ -6118,6 +6118,12 @@ v8::Isolate* Context::GetIsolate() { return reinterpret_cast(env->GetIsolate()); } +v8::MicrotaskQueue* Context::GetMicrotaskQueue() { + i::Handle env = Utils::OpenHandle(this); + CHECK(env->IsNativeContext()); + return i::Handle::cast(env)->microtask_queue(); +} + v8::Local Context::Global() { i::Handle context = Utils::OpenHandle(this); i::Isolate* isolate = context->GetIsolate(); diff --git a/deps/v8/test/cctest/test-api.cc b/deps/v8/test/cctest/test-api.cc index cd291aa524aa88..299614c122399f 100644 --- a/deps/v8/test/cctest/test-api.cc +++ b/deps/v8/test/cctest/test-api.cc @@ -28348,3 +28348,13 @@ TEST(TriggerThreadSafeMetricsEvent) { CHECK_EQ(recorder->count_, 1); // Increased. CHECK_EQ(recorder->module_count_, 42); } + +THREADED_TEST(MicrotaskQueueOfContext) { + auto microtask_queue = v8::MicrotaskQueue::New(CcTest::isolate()); + v8::HandleScope scope(CcTest::isolate()); + v8::Local context = Context::New( + CcTest::isolate(), nullptr, v8::MaybeLocal(), + v8::MaybeLocal(), v8::DeserializeInternalFieldsCallback(), + microtask_queue.get()); + CHECK_EQ(context->GetMicrotaskQueue(), microtask_queue.get()); +} From 299c63d3f215f1a7faa9bd2677fe39e4e8732ca6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 8 Dec 2020 22:48:02 +0100 Subject: [PATCH 2/3] =?UTF-8?q?src:=20use=20correct=20outer=20Context?= =?UTF-8?q?=E2=80=99s=20microtask=20queue?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fall back to using the outer context’s microtask queue, rather than the Isolate’s default one. This would otherwise result in surprising behavior if an embedder specified a custom microtask queue for the main Node.js context. --- src/async_wrap.cc | 3 +- src/node_contextify.cc | 4 ++- src/node_task_queue.cc | 3 +- test/cctest/test_environment.cc | 56 +++++++++++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 3 deletions(-) diff --git a/src/async_wrap.cc b/src/async_wrap.cc index bb15bf6adb1262..3748d542c95021 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -827,7 +827,8 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) { // interrupt to get this Microtask scheduled as fast as possible. if (env->destroy_async_id_list()->size() == 16384) { env->RequestInterrupt([](Environment* env) { - env->isolate()->EnqueueMicrotask( + env->context()->GetMicrotaskQueue()->EnqueueMicrotask( + env->isolate(), [](void* arg) { DestroyAsyncIdsCallback(static_cast(arg)); }, env); diff --git a/src/node_contextify.cc b/src/node_contextify.cc index a69570400cd897..0accd99c0c4c5e 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -199,7 +199,9 @@ MaybeLocal ContextifyContext::CreateV8Context( object_template, {}, // global object {}, // deserialization callback - microtask_queue() ? microtask_queue().get() : nullptr); + microtask_queue() ? + microtask_queue().get() : + env->isolate()->GetCurrentContext()->GetMicrotaskQueue()); if (ctx.IsEmpty()) return MaybeLocal(); // Only partially initialize the context - the primordials are left out // and only initialized when necessary. diff --git a/src/node_task_queue.cc b/src/node_task_queue.cc index 926b27fcf2a05a..dff59d8dc77b99 100644 --- a/src/node_task_queue.cc +++ b/src/node_task_queue.cc @@ -96,7 +96,8 @@ static void EnqueueMicrotask(const FunctionCallbackInfo& args) { CHECK(args[0]->IsFunction()); - isolate->EnqueueMicrotask(args[0].As()); + isolate->GetCurrentContext()->GetMicrotaskQueue() + ->EnqueueMicrotask(isolate, args[0].As()); } static void RunMicrotasks(const FunctionCallbackInfo& args) { diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index dcd59a22d3b213..efbeaee3520ae2 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -608,3 +608,59 @@ TEST_F(NodeZeroIsolateTestFixture, CtrlCWithOnlySafeTerminationTest) { isolate->Dispose(); } #endif // _WIN32 + +TEST_F(EnvironmentTest, NestedMicrotaskQueue) { + const v8::HandleScope handle_scope(isolate_); + const Argv argv; + + std::unique_ptr queue = v8::MicrotaskQueue::New(isolate_); + v8::Local context = v8::Context::New( + isolate_, nullptr, {}, {}, {}, queue.get()); + node::InitializeContext(context); + v8::Context::Scope context_scope(context); + + int callback_calls = 0; + v8::Local must_call = v8::Function::New( + context, + [](const v8::FunctionCallbackInfo& info) { + int* callback_calls = + static_cast(info.Data().As()->Value()); + *callback_calls |= info[0].As()->Value(); + }, + v8::External::New(isolate_, static_cast(&callback_calls))) + .ToLocalChecked(); + context->Global()->Set( + context, + v8::String::NewFromUtf8Literal(isolate_, "mustCall"), + must_call).Check(); + + node::IsolateData* isolate_data = node::CreateIsolateData( + isolate_, &NodeTestFixture::current_loop, platform.get()); + CHECK_NE(nullptr, isolate_data); + + node::Environment* env = node::CreateEnvironment( + isolate_data, context, {}, {}); + CHECK_NE(nullptr, env); + + node::LoadEnvironment( + env, + "Promise.resolve().then(() => mustCall(1 << 0));\n" + "require('vm').runInNewContext(" + " 'Promise.resolve().then(() => mustCall(1 << 1))'," + " { mustCall }," + " { microtaskMode: 'afterEvaluate' }" + ");" + "require('vm').runInNewContext(" + " 'Promise.resolve().then(() => mustCall(1 << 2))'," + " { mustCall }" + ");" + ).ToLocalChecked(); + EXPECT_EQ(callback_calls, 1 << 1); + isolate_->PerformMicrotaskCheckpoint(); + EXPECT_EQ(callback_calls, 1 << 1); + queue->PerformCheckpoint(isolate_); + EXPECT_EQ(callback_calls, (1 << 0) | (1 << 1) | (1 << 2)); + + node::FreeEnvironment(env); + node::FreeIsolateData(isolate_data); +} From bde7245ebcabd11402b1af564288f4ea02a5bd0d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 11 Dec 2020 17:26:00 +0100 Subject: [PATCH 3/3] =?UTF-8?q?fixup!=20src:=20use=20correct=20outer=20Con?= =?UTF-8?q?text=E2=80=99s=20microtask=20queue?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- test/cctest/test_environment.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index efbeaee3520ae2..2f1d8ad4fa3236 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -653,8 +653,7 @@ TEST_F(EnvironmentTest, NestedMicrotaskQueue) { "require('vm').runInNewContext(" " 'Promise.resolve().then(() => mustCall(1 << 2))'," " { mustCall }" - ");" - ).ToLocalChecked(); + ");").ToLocalChecked(); EXPECT_EQ(callback_calls, 1 << 1); isolate_->PerformMicrotaskCheckpoint(); EXPECT_EQ(callback_calls, 1 << 1);