fix: avoid using v8 on Isolate termination

patches/node/.patches

@@ -34,3 +34,4 @@ fix_expose_lookupandcompile_with_parameters.patch
 fix_prevent_changing_functiontemplateinfo_after_publish.patch
 enable_crashpad_linux_node_processes.patch
 allow_embedder_to_control_codegenerationfromstringscallback.patch
+src_allow_optional_isolation_termination_in_node.patch

patches/node/src_allow_optional_isolation_termination_in_node.patch

@@ -0,0 +1,75 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Shelley Vohr <shelley.vohr@gmail.com>
+Date: Tue, 7 Feb 2023 10:53:11 +0100
+Subject: src: allow optional isolation termination in node
+
+This patch allows for node::Stop() to conditionally call
+V8:Isolate::TerminateExecution().
+
+We do not want to invoke a termination exception at exit when
+we're running with only_terminate_in_safe_scope set to false. Heap and
+coverage profilers run after environment exit and if there is a pending
+exception at this stage then they will fail to generate the appropriate
+profiles. Node.js does not call node::Stop(), which previously always
+called isolate->TerminateExecution(), and therefore does not have this
+issue when also running with only_terminate_in_safe_scope set to false.
+
+diff --git a/src/env.cc b/src/env.cc
+index 837a879864c46d6f500684444ec38583c05f8be2..69a8b9ea405a400254041734b037c00aff4758f7 100644
+--- a/src/env.cc
++++ b/src/env.cc
+@@ -902,10 +902,11 @@ void Environment::InitializeLibuv() {
+   StartProfilerIdleNotifier();
+ }
+
+-void Environment::ExitEnv() {
++void Environment::ExitEnv(bool terminate) {
+   // Should not access non-thread-safe methods here.
+   set_stopping(true);
+-  isolate_->TerminateExecution();
++  if (terminate)
++    isolate_->TerminateExecution();
+   SetImmediateThreadsafe([](Environment* env) {
+     env->set_can_call_into_js(false);
+     uv_stop(env->event_loop());
+diff --git a/src/env.h b/src/env.h
+index 562610e6827d8302f146b81d599dd366ba25cd74..c358c139aafcd7c958915b036f8d176909341556 100644
+--- a/src/env.h
++++ b/src/env.h
+@@ -628,7 +628,7 @@ class Environment : public MemoryRetainer {
+   void RegisterHandleCleanups();
+   void CleanupHandles();
+   void Exit(int code);
+-  void ExitEnv();
++  void ExitEnv(bool terminate);
+
+   // Register clean-up cb to be called on environment destruction.
+   inline void RegisterHandleCleanup(uv_handle_t* handle,
+diff --git a/src/node.cc b/src/node.cc
+index 1067dee74c8877d9a3a0da6527c4c37faf9bd15f..b550fd4aa8488c6d721db3dee94cc4ce1346c708 100644
+--- a/src/node.cc
++++ b/src/node.cc
+@@ -1229,8 +1229,8 @@ int Start(int argc, char** argv) {
+   return LoadSnapshotDataAndRun(&snapshot_data, result.get());
+ }
+
+-int Stop(Environment* env) {
+-  env->ExitEnv();
++int Stop(Environment* env, bool terminate) {
++  env->ExitEnv(terminate);
+   return 0;
+ }
+
+diff --git a/src/node.h b/src/node.h
+index 5a849f047feca5d4d101c21c125e1c0500150077..db9a9c5c54f176ffdfc67e045b970729341eee7f 100644
+--- a/src/node.h
++++ b/src/node.h
+@@ -316,7 +316,7 @@ NODE_EXTERN int Start(int argc, char* argv[]);
+
+ // Tear down Node.js while it is running (there are active handles
+ // in the loop and / or actively executing JavaScript code).
+-NODE_EXTERN int Stop(Environment* env);
++NODE_EXTERN int Stop(Environment* env, bool terminate = true);
+
+ // This runs a subset of the initialization performed by
+ // InitializeOncePerProcess(), which supersedes this function.

patches/v8/.patches

@@ -6,8 +6,6 @@ workaround_an_undefined_symbol_error.patch
 do_not_export_private_v8_symbols_on_windows.patch
 fix_build_deprecated_attribute_for_older_msvc_versions.patch
 fix_disable_implies_dcheck_for_node_stream_array_buffers.patch
-revert_runtime_dhceck_terminating_exception_in_microtasks.patch
-chore_disable_is_execution_terminating_dcheck.patch
 force_cppheapcreateparams_to_be_noncopyable.patch
 chore_allow_customizing_microtask_policy_per_context.patch
 disable_the_use_of_preserve_most_on_arm64_windows.patch

shell/app/node_main.cc

@@ -226,7 +226,8 @@ int NodeMain(int argc, char* argv[]) {
       uint64_t env_flags = node::EnvironmentFlags::kDefaultFlags |
                            node::EnvironmentFlags::kHideConsoleWindows;
       env = node::CreateEnvironment(
-          isolate_data, gin_env.context(), result->args(), result->exec_args(),
+          isolate_data, isolate->GetCurrentContext(), result->args(),
+          result->exec_args(),
           static_cast<node::EnvironmentFlags::Flags>(env_flags));
       CHECK_NE(nullptr, env);
 
@@ -293,7 +294,8 @@ int NodeMain(int argc, char* argv[]) {
 
     node::ResetStdio();
 
-    node::Stop(env);
+    node::Stop(env, false);
+
     node::FreeEnvironment(env);
     node::FreeIsolateData(isolate_data);
   }

shell/browser/electron_browser_main_parts.cc

@@ -273,7 +273,7 @@ void ElectronBrowserMainParts::PostEarlyInitialization() {
   node_bindings_->Initialize();
   // Create the global environment.
   node::Environment* env = node_bindings_->CreateEnvironment(
-      js_env_->context(), js_env_->platform());
+      js_env_->isolate()->GetCurrentContext(), js_env_->platform());
   node_env_ = std::make_unique<NodeEnvironment>(env);
 
   env->set_trace_sync_io(env->options()->trace_sync_io);
@@ -626,7 +626,7 @@ void ElectronBrowserMainParts::PostMainMessageLoopRun() {
   // invoke Node/V8 APIs inside them.
   node_env_->env()->set_trace_sync_io(false);
   js_env_->DestroyMicrotasksRunner();
-  node::Stop(node_env_->env());
+  node::Stop(node_env_->env(), false);
   node_env_.reset();
 
   auto default_context_key = ElectronBrowserContext::PartitionKey("", false);

shell/browser/javascript_environment.cc

@@ -74,22 +74,45 @@ struct base::trace_event::TraceValue::Helper<
 
 namespace electron {
 
+namespace {
+
+gin::IsolateHolder CreateIsolateHolder(v8::Isolate* isolate) {
+  std::unique_ptr<v8::Isolate::CreateParams> create_params =
+      gin::IsolateHolder::getDefaultIsolateParams();
+  // Align behavior with V8 Isolate default for Node.js.
+  // This is necessary for important aspects of Node.js
+  // including heap and cpu profilers to function properly.
+  //
+  // Additional note:
+  // We do not want to invoke a termination exception at exit when
+  // we're running with only_terminate_in_safe_scope set to false. Heap and
+  // coverage profilers run after environment exit and if there is a pending
+  // exception at this stage then they will fail to generate the appropriate
+  // profiles. Node.js does not call node::Stop(), which calls
+  // isolate->TerminateExecution(), and therefore does not have this issue
+  // when also running with only_terminate_in_safe_scope set to false.
+  create_params->only_terminate_in_safe_scope = false;
+
+  return gin::IsolateHolder(
+      base::SingleThreadTaskRunner::GetCurrentDefault(),
+      gin::IsolateHolder::kSingleThread,
+      gin::IsolateHolder::IsolateType::kUtility, std::move(create_params),
+      gin::IsolateHolder::IsolateCreationMode::kNormal, isolate);
+}
+
+}  // namespace
+
 JavascriptEnvironment::JavascriptEnvironment(uv_loop_t* event_loop,
                                              bool setup_wasm_streaming)
     : isolate_(Initialize(event_loop, setup_wasm_streaming)),
-      isolate_holder_(base::SingleThreadTaskRunner::GetCurrentDefault(),
-                      gin::IsolateHolder::kSingleThread,
-                      gin::IsolateHolder::kAllowAtomicsWait,
-                      gin::IsolateHolder::IsolateType::kUtility,
-                      gin::IsolateHolder::IsolateCreationMode::kNormal,
-                      nullptr,
-                      nullptr,
-                      isolate_),
+      isolate_holder_(CreateIsolateHolder(isolate_)),
       locker_(isolate_) {
   isolate_->Enter();
+
   v8::HandleScope scope(isolate_);
   auto context = node::NewContext(isolate_);
-  context_ = v8::Global<v8::Context>(isolate_, context);
+  CHECK(!context.IsEmpty());
+
   context->Enter();
 }
 
@@ -99,7 +122,7 @@ JavascriptEnvironment::~JavascriptEnvironment() {
 
   {
     v8::HandleScope scope(isolate_);
-    context_.Get(isolate_)->Exit();
+    isolate_->GetCurrentContext()->Exit();
   }
   isolate_->Exit();
   g_isolate = nullptr;

shell/browser/javascript_environment.h

@@ -22,8 +22,8 @@ class MicrotasksRunner;
 // Manage the V8 isolate and context automatically.
 class JavascriptEnvironment {
  public:
-  explicit JavascriptEnvironment(uv_loop_t* event_loop,
-                                 bool setup_wasm_streaming = false);
+  JavascriptEnvironment(uv_loop_t* event_loop,
+                        bool setup_wasm_streaming = false);
   ~JavascriptEnvironment();
 
   // disable copy
@@ -35,9 +35,6 @@ class JavascriptEnvironment {
 
   node::MultiIsolatePlatform* platform() const { return platform_.get(); }
   v8::Isolate* isolate() const { return isolate_; }
-  v8::Local<v8::Context> context() const {
-    return v8::Local<v8::Context>::New(isolate_, context_);
-  }
 
   static v8::Isolate* GetIsolate();
 
@@ -48,7 +45,6 @@ class JavascriptEnvironment {
   v8::Isolate* isolate_;
   gin::IsolateHolder isolate_holder_;
   v8::Locker locker_;
-  v8::Global<v8::Context> context_;
 
   std::unique_ptr<MicrotasksRunner> microtasks_runner_;
 };

shell/services/node/node_service.cc

@@ -33,7 +33,7 @@ NodeService::~NodeService() {
   if (!node_env_stopped_) {
     node_env_->env()->set_trace_sync_io(false);
     js_env_->DestroyMicrotasksRunner();
-    node::Stop(node_env_->env());
+    node::Stop(node_env_->env(), false);
   }
 }
 
@@ -59,15 +59,16 @@ void NodeService::Initialize(node::mojom::NodeServiceParamsPtr params) {
 
   // Create the global environment.
   node::Environment* env = node_bindings_->CreateEnvironment(
-      js_env_->context(), js_env_->platform(), params->args, params->exec_args);
+      js_env_->isolate()->GetCurrentContext(), js_env_->platform(),
+      params->args, params->exec_args);
   node_env_ = std::make_unique<NodeEnvironment>(env);
 
   node::SetProcessExitHandler(env,
                               [this](node::Environment* env, int exit_code) {
                                 // Destroy node platform.
                                 env->set_trace_sync_io(false);
                                 js_env_->DestroyMicrotasksRunner();
-                                node::Stop(env);
+                                node::Stop(env, false);
                                 node_env_stopped_ = true;
                                 receiver_.ResetWithReason(exit_code, "");
                               });