src: apply IsolateSettings when using a snapshot

`NewIsolate()` deferred `SetIsolateErrorHandlers()` when snapshot data
was passed, and `CreateEnvironment()` later installed the handlers with
default `IsolateSettings` after deserializing the main context. An
embedder's `fatal_error_callback`, `oom_error_callback`,
`should_abort_on_uncaught_exception_callback` and
`prepare_stack_trace_callback` were therefore dropped whenever a
snapshot was used, and the per-isolate message listener was added even
if `MESSAGE_LISTENER_WITH_ERROR_LEVEL` had been cleared. The only way
to keep custom handlers was to call `SetIsolateUpForNode()` again after
`CreateEnvironment()`.

Install all handlers in `NewIsolate()` regardless of snapshot data, as
its documentation already describes, and stop touching isolate
handlers in `CreateEnvironment()`. The deferral dates from the initial
isolate snapshot work; every handler already copes with a missing
`Environment`, since without a snapshot they are installed before any
context exists, and workers have been calling `SetIsolateUpForNode()`
right after a snapshot `NewIsolate()` anyway.

Refs: #27321
Refs: #45888
Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
PR-URL: #65407
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

Author: codebytere

src/api/environment.cc

@@ -225,7 +225,8 @@ void SetIsolateCreateParamsForNode(Isolate::CreateParams* params) {
 #endif
 }
 
-void SetIsolateErrorHandlers(v8::Isolate* isolate, const IsolateSettings& s) {
+static void SetIsolateErrorHandlers(v8::Isolate* isolate,
+                                    const IsolateSettings& s) {
   if (s.flags & MESSAGE_LISTENER_WITH_ERROR_LEVEL)
     isolate->AddMessageListenerWithErrorLevel(
             errors::PerIsolateMessageListener,
@@ -350,16 +351,7 @@ Isolate* NewIsolate(Isolate::CreateParams* params,
 
   SetIsolateCreateParamsForNode(params);
   Isolate::Initialize(isolate, *params);
-
-  Isolate::Scope isolate_scope(isolate);
-
-  if (snapshot_data == nullptr) {
-    // If in deserialize mode, delay until after the deserialization is
-    // complete.
-    SetIsolateUpForNode(isolate, settings);
-  } else {
-    SetIsolateMiscHandlers(isolate, settings);
-  }
+  SetIsolateUpForNode(isolate, settings);
 
   return isolate;
 }
@@ -469,7 +461,6 @@ Environment* CreateEnvironment(
       FreeEnvironment(env);
       return nullptr;
     }
-    SetIsolateErrorHandlers(isolate, {});
   }
 
   Context::Scope context_scope(context);

src/node_internals.h

@@ -380,7 +380,6 @@ class InitializationResultImpl final : public InitializationResult {
   MultiIsolatePlatform* platform_ = nullptr;
 };
 
-void SetIsolateErrorHandlers(v8::Isolate* isolate, const IsolateSettings& s);
 void SetIsolateMiscHandlers(v8::Isolate* isolate, const IsolateSettings& s);
 void SetIsolateCreateParamsForNode(v8::Isolate::CreateParams* params);
 

src/node_worker.cc

@@ -190,8 +190,6 @@ class WorkerThreadData {
       return;
     }
 
-    SetIsolateUpForNode(isolate);
-
     // Be sure it's called before Environment::InitializeDiagnostics()
     // so that this callback stays when the callback of
     // --heapsnapshot-near-heap-limit gets is popped.

test/embedding/embedtest.cc

@@ -19,6 +19,7 @@ using node::MultiIsolatePlatform;
 using v8::Context;
 using v8::HandleScope;
 using v8::Isolate;
+using v8::Local;
 using v8::Locker;
 using v8::MaybeLocal;
 using v8::V8;
@@ -27,6 +28,11 @@ using v8::Value;
 static int RunNodeInstance(MultiIsolatePlatform* platform,
                            const std::vector<std::string>& args,
                            const std::vector<std::string>& exec_args);
+static int RunSnapshotWithIsolateSettings(
+    MultiIsolatePlatform* platform,
+    const node::EmbedderSnapshotData* snapshot,
+    const std::vector<std::string>& args,
+    const std::vector<std::string>& exec_args);
 
 // --create-v8-startup-blob <file>: a plain V8 startup blob (what V8's
 // mksnapshot produces), to test building the Node.js snapshot on top of one.
@@ -130,6 +136,7 @@ int RunNodeInstance(MultiIsolatePlatform* platform,
   // Running snapshot:
   // embedtest --embedder-snapshot-blob blob-path
   //           [--embedder-snapshot-as-file]
+  //           [--embedder-isolate-settings]
   //           arg1 arg2...
   // No snapshot:
   // embedtest arg1 arg2...
@@ -139,6 +146,7 @@ int RunNodeInstance(MultiIsolatePlatform* platform,
   std::vector<std::string> filtered_args;
   bool is_building_snapshot = false;
   bool snapshot_as_file = false;
+  bool with_isolate_settings = false;
   std::optional<node::SnapshotConfig> snapshot_config;
   std::string snapshot_blob_path;
   for (size_t i = 0; i < args.size(); ++i) {
@@ -147,6 +155,8 @@ int RunNodeInstance(MultiIsolatePlatform* platform,
       is_building_snapshot = true;
     } else if (arg == "--embedder-snapshot-as-file") {
       snapshot_as_file = true;
+    } else if (arg == "--embedder-isolate-settings") {
+      with_isolate_settings = true;
     } else if (arg == "--without-code-cache") {
       if (!snapshot_config.has_value()) {
         snapshot_config = node::SnapshotConfig{};
@@ -217,6 +227,11 @@ int RunNodeInstance(MultiIsolatePlatform* platform,
                          node::GetAnonymousMainPath());
   }
 
+  if (snapshot && with_isolate_settings) {
+    return RunSnapshotWithIsolateSettings(
+        platform, snapshot.get(), filtered_args, exec_args);
+  }
+
   std::vector<std::string> errors;
   std::unique_ptr<CommonEnvironmentSetup> setup;
 
@@ -300,3 +315,67 @@ int RunNodeInstance(MultiIsolatePlatform* platform,
 
   return exit_code;
 }
+
+// CommonEnvironmentSetup does not take IsolateSettings, so this goes through
+// NewIsolate()/CreateIsolateData()/CreateEnvironment() directly.
+static int RunSnapshotWithIsolateSettings(
+    MultiIsolatePlatform* platform,
+    const node::EmbedderSnapshotData* snapshot,
+    const std::vector<std::string>& args,
+    const std::vector<std::string>& exec_args) {
+  uv_loop_t loop;
+  int ret = uv_loop_init(&loop);
+  assert(ret == 0);
+
+  std::shared_ptr<node::ArrayBufferAllocator> allocator =
+      node::ArrayBufferAllocator::Create();
+  node::IsolateSettings settings;
+  settings.prepare_stack_trace_callback = [](Local<Context> context,
+                                             Local<Value> exception,
+                                             Local<v8::Array> trace) {
+    return MaybeLocal<Value>(v8::String::NewFromUtf8Literal(
+        v8::Isolate::GetCurrent(), "stack trace prepared by the embedder"));
+  };
+  Isolate* isolate =
+      node::NewIsolate(allocator, &loop, platform, snapshot, settings);
+  assert(isolate != nullptr);
+
+  int exit_code = 1;
+  {
+    Locker locker(isolate);
+    Isolate::Scope isolate_scope(isolate);
+    HandleScope handle_scope(isolate);
+
+    std::unique_ptr<node::IsolateData, decltype(&node::FreeIsolateData)>
+        isolate_data(node::CreateIsolateData(
+                         isolate, &loop, platform, allocator.get(), snapshot),
+                     node::FreeIsolateData);
+    std::unique_ptr<Environment, decltype(&node::FreeEnvironment)> env(
+        node::CreateEnvironment(
+            isolate_data.get(), Local<Context>(), args, exec_args),
+        node::FreeEnvironment);
+    assert(env);
+
+    Context::Scope context_scope(node::GetMainContext(env.get()));
+    if (!node::LoadEnvironment(env.get(), node::StartExecutionCallback{})
+             .IsEmpty()) {
+      exit_code = node::SpinEventLoop(env.get()).FromMaybe(1);
+    }
+    node::Stop(env.get());
+  }
+
+  bool platform_finished = false;
+  platform->AddIsolateFinishedCallback(
+      isolate,
+      [](void* data) {
+        bool* finished = static_cast<bool*>(data);
+        *finished = true;
+      },
+      &platform_finished);
+  platform->DisposeIsolate(isolate);
+  while (!platform_finished) uv_run(&loop, UV_RUN_ONCE);
+  ret = uv_loop_close(&loop);
+  assert(ret == 0);
+
+  return exit_code;
+}

test/embedding/test-embedding-snapshot-isolate-settings.js

@@ -0,0 +1,38 @@
+'use strict';
+
+// IsolateSettings passed to NewIsolate() with a snapshot must survive
+// CreateEnvironment(); see RunSnapshotWithIsolateSettings() in embedtest.cc.
+
+const common = require('../common');
+const tmpdir = require('../common/tmpdir');
+
+const {
+  spawnSyncAndAssert,
+  spawnSyncAndExitWithoutError,
+} = require('../common/child_process');
+
+const embedtest = common.resolveBuiltBinary('embedtest');
+const snapshotBlobArgs = [
+  '--embedder-snapshot-blob', tmpdir.resolve('embedder-snapshot.blob'),
+];
+const buildSnapshotScript = `
+  require('v8').startupSnapshot.setDeserializeMainFunction(() => {
+    console.log(new Error('from the snapshot main function').stack);
+  });
+`;
+
+tmpdir.refresh();
+
+spawnSyncAndExitWithoutError(
+  embedtest,
+  ['--', buildSnapshotScript, ...snapshotBlobArgs, '--embedder-snapshot-create'],
+  { cwd: tmpdir.path });
+
+spawnSyncAndAssert(
+  embedtest,
+  ['--', ...snapshotBlobArgs, '--embedder-isolate-settings'],
+  { cwd: tmpdir.path },
+  {
+    trim: true,
+    stdout: 'stack trace prepared by the embedder',
+  });