From 27d6a8b2b16d9bfa2dc85c80edfdca0e3f9f183e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 23 Dec 2022 18:06:05 +0100 Subject: [PATCH] src: fix creating `Isolate`s from addons MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit daae938f32f2660 broke addons which create their own `Isolate` instances, because enabling the shared-readonly-heap feature of V8 requires all snapshots used for different `Isolate`s to be identical. Usage of addons that do this has probably decreased quite a bit since Worker threads were introduced in Node.js, but it’s still a valid use case, and in any case the breakage was probably not intentional (although the referenced commit did require test changes because of this issue). This commit addresses this issue partially by caching the V8 snapshot parameters and ignoring ones passed in from users in `NewIsolate()` when this feature is enabled, and makes the `NodeMainInstance` snapshot-based isolate creation also re-use this code. PR-URL: https://github.com/nodejs/node/pull/45885 Reviewed-By: James M Snell --- node.gypi | 3 + src/api/environment.cc | 25 +++++- src/node_internals.h | 3 +- src/node_main_instance.cc | 16 +--- src/node_worker.cc | 12 +-- test/addons/new-isolate-addon/binding.cc | 76 +++++++++++++++++++ test/addons/new-isolate-addon/binding.gyp | 9 +++ .../new-isolate-addon/test-nonodesnapshot.js | 6 ++ test/addons/new-isolate-addon/test.js | 8 ++ 9 files changed, 132 insertions(+), 26 deletions(-) create mode 100644 test/addons/new-isolate-addon/binding.cc create mode 100644 test/addons/new-isolate-addon/binding.gyp create mode 100644 test/addons/new-isolate-addon/test-nonodesnapshot.js create mode 100644 test/addons/new-isolate-addon/test.js diff --git a/node.gypi b/node.gypi index 059df00bcf4e06..ecd2ea6ea87deb 100644 --- a/node.gypi +++ b/node.gypi @@ -96,6 +96,9 @@ 'NODE_USE_V8_PLATFORM=0', ], }], + [ 'v8_enable_shared_ro_heap==1', { + 'defines': ['NODE_V8_SHARED_RO_HEAP',], + }], [ 'node_tag!=""', { 'defines': [ 'NODE_TAG="<(node_tag)"' ], }], diff --git a/src/api/environment.cc b/src/api/environment.cc index 3e2e6a04ddd51e..339b9a1822f3e9 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -218,7 +218,8 @@ void SetIsolateCreateParamsForNode(Isolate::CreateParams* params) { const uint64_t total_memory = constrained_memory > 0 ? std::min(uv_get_total_memory(), constrained_memory) : uv_get_total_memory(); - if (total_memory > 0) { + if (total_memory > 0 && + params->constraints.max_old_generation_size_in_bytes() == 0) { // V8 defaults to 700MB or 1.4GB on 32 and 64 bit platforms respectively. // This default is based on browser use-cases. Tell V8 to configure the // heap based on the actual physical memory. @@ -303,9 +304,21 @@ void SetIsolateUpForNode(v8::Isolate* isolate) { // careful about what we override in the params. Isolate* NewIsolate(Isolate::CreateParams* params, uv_loop_t* event_loop, - MultiIsolatePlatform* platform) { + MultiIsolatePlatform* platform, + bool has_snapshot_data) { Isolate* isolate = Isolate::Allocate(); if (isolate == nullptr) return nullptr; +#ifdef NODE_V8_SHARED_RO_HEAP + { + // In shared-readonly-heap mode, V8 requires all snapshots used for + // creating Isolates to be identical. This isn't really memory-safe + // but also otherwise just doesn't work, and the only real alternative + // is disabling shared-readonly-heap mode altogether. + static Isolate::CreateParams first_params = *params; + params->snapshot_blob = first_params.snapshot_blob; + params->external_references = first_params.external_references; + } +#endif // Register the isolate on the platform before the isolate gets initialized, // so that the isolate can access the platform during initialization. @@ -313,7 +326,13 @@ Isolate* NewIsolate(Isolate::CreateParams* params, SetIsolateCreateParamsForNode(params); Isolate::Initialize(isolate, *params); - SetIsolateUpForNode(isolate); + if (!has_snapshot_data) { + // If in deserialize mode, delay until after the deserialization is + // complete. + SetIsolateUpForNode(isolate); + } else { + SetIsolateMiscHandlers(isolate, {}); + } return isolate; } diff --git a/src/node_internals.h b/src/node_internals.h index efbdbeabf5a6af..ece9a4cfd45bf8 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -303,7 +303,8 @@ bool SafeGetenv(const char* key, void DefineZlibConstants(v8::Local target); v8::Isolate* NewIsolate(v8::Isolate::CreateParams* params, uv_loop_t* event_loop, - MultiIsolatePlatform* platform); + MultiIsolatePlatform* platform, + bool has_snapshot_data = false); // This overload automatically picks the right 'main_script_id' if no callback // was provided by the embedder. v8::MaybeLocal StartExecution(Environment* env, diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 2ec53a1b314d2d..33f6856561df73 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -76,13 +76,9 @@ NodeMainInstance::NodeMainInstance(const SnapshotData* snapshot_data, isolate_params_.get()); } - isolate_ = Isolate::Allocate(); + isolate_ = NewIsolate( + isolate_params_.get(), event_loop, platform, snapshot_data != nullptr); CHECK_NOT_NULL(isolate_); - // Register the isolate on the platform before the isolate gets initialized, - // so that the isolate can access the platform during initialization. - platform->RegisterIsolate(isolate_, event_loop); - SetIsolateCreateParamsForNode(isolate_params_.get()); - Isolate::Initialize(isolate_, *isolate_params_); // If the indexes are not nullptr, we are not deserializing isolate_data_ = std::make_unique( @@ -91,13 +87,7 @@ NodeMainInstance::NodeMainInstance(const SnapshotData* snapshot_data, platform, array_buffer_allocator_.get(), snapshot_data == nullptr ? nullptr : &(snapshot_data->isolate_data_info)); - IsolateSettings s; - SetIsolateMiscHandlers(isolate_, s); - if (snapshot_data == nullptr) { - // If in deserialize mode, delay until after the deserialization is - // complete. - SetIsolateErrorHandlers(isolate_, s); - } + isolate_data_->max_young_gen_size = isolate_params_->constraints.max_young_generation_size_in_bytes(); } diff --git a/src/node_worker.cc b/src/node_worker.cc index 4a675fdfe7732b..63ce4625bf43f3 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -147,21 +147,15 @@ class WorkerThreadData { ArrayBufferAllocator::Create(); Isolate::CreateParams params; SetIsolateCreateParamsForNode(¶ms); - params.array_buffer_allocator_shared = allocator; - - if (w->snapshot_data() != nullptr) { - SnapshotBuilder::InitializeIsolateParams(w->snapshot_data(), ¶ms); - } w->UpdateResourceConstraints(¶ms.constraints); - - Isolate* isolate = Isolate::Allocate(); + params.array_buffer_allocator_shared = allocator; + Isolate* isolate = + NewIsolate(¶ms, &loop_, w->platform_, w->snapshot_data()); if (isolate == nullptr) { w->Exit(1, "ERR_WORKER_INIT_FAILED", "Failed to create new Isolate"); return; } - w->platform_->RegisterIsolate(isolate, &loop_); - Isolate::Initialize(isolate, params); SetIsolateUpForNode(isolate); // Be sure it's called before Environment::InitializeDiagnostics() diff --git a/test/addons/new-isolate-addon/binding.cc b/test/addons/new-isolate-addon/binding.cc new file mode 100644 index 00000000000000..ba5fbc3e9093df --- /dev/null +++ b/test/addons/new-isolate-addon/binding.cc @@ -0,0 +1,76 @@ +#include +#include + +using node::Environment; +using node::MultiIsolatePlatform; +using v8::Context; +using v8::FunctionCallbackInfo; +using v8::HandleScope; +using v8::Isolate; +using v8::Local; +using v8::Locker; +using v8::MaybeLocal; +using v8::Object; +using v8::SharedArrayBuffer; +using v8::String; +using v8::Unlocker; +using v8::Value; + +void RunInSeparateIsolate(const FunctionCallbackInfo& args) { + Isolate* parent_isolate = args.GetIsolate(); + + assert(args[0]->IsString()); + String::Utf8Value code(parent_isolate, args[0]); + assert(*code != nullptr); + assert(args[1]->IsSharedArrayBuffer()); + auto arg_bs = args[1].As()->GetBackingStore(); + + Environment* parent_env = + node::GetCurrentEnvironment(parent_isolate->GetCurrentContext()); + assert(parent_env != nullptr); + MultiIsolatePlatform* platform = node::GetMultiIsolatePlatform(parent_env); + assert(parent_env != nullptr); + + { + Unlocker unlocker(parent_isolate); + + std::vector errors; + const std::vector empty_args; + auto setup = + node::CommonEnvironmentSetup::Create(platform, + &errors, + empty_args, + empty_args, + node::EnvironmentFlags::kNoFlags); + assert(setup); + + { + Locker locker(setup->isolate()); + Isolate::Scope isolate_scope(setup->isolate()); + HandleScope handle_scope(setup->isolate()); + Context::Scope context_scope(setup->context()); + auto arg = SharedArrayBuffer::New(setup->isolate(), arg_bs); + auto result = setup->context()->Global()->Set( + setup->context(), + v8::String::NewFromUtf8Literal(setup->isolate(), "arg"), + arg); + assert(!result.IsNothing()); + + MaybeLocal loadenv_ret = + node::LoadEnvironment(setup->env(), *code); + assert(!loadenv_ret.IsEmpty()); + + (void)node::SpinEventLoop(setup->env()); + + node::Stop(setup->env()); + } + } +} + +void Initialize(Local exports, + Local module, + Local context) { + NODE_SET_METHOD(exports, "runInSeparateIsolate", RunInSeparateIsolate); +} + +NODE_MODULE_CONTEXT_AWARE(NODE_GYP_MODULE_NAME, Initialize) diff --git a/test/addons/new-isolate-addon/binding.gyp b/test/addons/new-isolate-addon/binding.gyp new file mode 100644 index 00000000000000..55fbe7050f18e4 --- /dev/null +++ b/test/addons/new-isolate-addon/binding.gyp @@ -0,0 +1,9 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'sources': [ 'binding.cc' ], + 'includes': ['../common.gypi'], + } + ] +} diff --git a/test/addons/new-isolate-addon/test-nonodesnapshot.js b/test/addons/new-isolate-addon/test-nonodesnapshot.js new file mode 100644 index 00000000000000..b487b1dae8d8ef --- /dev/null +++ b/test/addons/new-isolate-addon/test-nonodesnapshot.js @@ -0,0 +1,6 @@ +// Flags: --no-node-snapshot +'use strict'; +require('../../common'); + +// Just re-execute the main test. +require('./test'); diff --git a/test/addons/new-isolate-addon/test.js b/test/addons/new-isolate-addon/test.js new file mode 100644 index 00000000000000..bbca5451e4dedf --- /dev/null +++ b/test/addons/new-isolate-addon/test.js @@ -0,0 +1,8 @@ +'use strict'; +const common = require('../../common'); +const binding = require(`./build/${common.buildType}/binding`); +const assert = require('assert'); + +const arg = new SharedArrayBuffer(1); +binding.runInSeparateIsolate('const arr = new Uint8Array(arg); arr[0] = 0x42;', arg); +assert.deepStrictEqual(new Uint8Array(arg), new Uint8Array([0x42]));