From 19630c9ecf3fa2f7d5a92699554d8ac4eb6d3fb6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 12 Dec 2022 17:31:46 +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. --- 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 0cbbe3fb048517..1e3fcd645b13a7 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -220,7 +220,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. @@ -305,9 +306,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. @@ -315,7 +328,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 e47b180b192c9a..23b8c37c614c40 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 3e0167fdddea12..23e817515dddb2 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 3cbede5f3cf7ca..5f7d807b1dac8f 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -148,14 +148,10 @@ 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) { // TODO(joyeecheung): maybe this should be kBootstrapFailure instead? w->Exit(ExitCode::kGenericUserError, @@ -164,8 +160,6 @@ class WorkerThreadData { 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]));