src: fix creating Isolates from addons

daae938 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: #45885
Reviewed-By: James M Snell <[email protected]>

Author: addaleax

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)"' ],
     }],

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,17 +306,35 @@ 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.
   platform->RegisterIsolate(isolate, event_loop);
 
   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;
 }

src/node_internals.h

@@ -303,7 +303,8 @@ bool SafeGetenv(const char* key,
 void DefineZlibConstants(v8::Local<v8::Object> 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<v8::Value> StartExecution(Environment* env,

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<IsolateData>(
@@ -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();
 }

src/node_worker.cc

@@ -148,14 +148,10 @@ class WorkerThreadData {
         ArrayBufferAllocator::Create();
     Isolate::CreateParams params;
     SetIsolateCreateParamsForNode(&params);
-    params.array_buffer_allocator_shared = allocator;
-
-    if (w->snapshot_data() != nullptr) {
-      SnapshotBuilder::InitializeIsolateParams(w->snapshot_data(), &params);
-    }
     w->UpdateResourceConstraints(&params.constraints);
-
-    Isolate* isolate = Isolate::Allocate();
+    params.array_buffer_allocator_shared = allocator;
+    Isolate* isolate =
+        NewIsolate(&params, &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()

test/addons/new-isolate-addon/binding.cc

@@ -0,0 +1,76 @@
+#include <assert.h>
+#include <node.h>
+
+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<Value>& 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<SharedArrayBuffer>()->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<std::string> errors;
+    const std::vector<std::string> 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<Value> loadenv_ret =
+          node::LoadEnvironment(setup->env(), *code);
+      assert(!loadenv_ret.IsEmpty());
+
+      (void)node::SpinEventLoop(setup->env());
+
+      node::Stop(setup->env());
+    }
+  }
+}
+
+void Initialize(Local<Object> exports,
+                Local<Value> module,
+                Local<Context> context) {
+  NODE_SET_METHOD(exports, "runInSeparateIsolate", RunInSeparateIsolate);
+}
+
+NODE_MODULE_CONTEXT_AWARE(NODE_GYP_MODULE_NAME, Initialize)

test/addons/new-isolate-addon/binding.gyp

@@ -0,0 +1,9 @@
+{
+  'targets': [
+    {
+      'target_name': 'binding',
+      'sources': [ 'binding.cc' ],
+      'includes': ['../common.gypi'],
+    }
+  ]
+}

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');

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]));