src: fix creating Isolates from addons

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