fix: avoid using v8 on Isolate termination (#38000)

patches/node/.patches

@@ -54,3 +54,4 @@ enable_crashpad_linux_node_processes.patch
 allow_embedder_to_control_codegenerationfromstringscallback.patch
 cherry-pick-09ae62b.patch
 lib_fix_broadcastchannel_initialization_location.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 de41e5b7f6ff9f818c661484a93b74db7569e31f..819fcfa6be46328a01a612315d48e93eb025423c 100644
+--- a/src/env.cc
++++ b/src/env.cc
+@@ -920,10 +920,11 @@ void Environment::InitializeLibuv() {
+   StartProfilerIdleNotifier();
+ }
+ 
+-void Environment::ExitEnv() {
++void Environment::ExitEnv(bool terminate) {
+   set_can_call_into_js(false);
+   set_stopping(true);
+-  isolate_->TerminateExecution();
++  if (terminate)
++    isolate_->TerminateExecution();
+   SetImmediateThreadsafe([](Environment* env) { uv_stop(env->event_loop()); });
+ }
+ 
+diff --git a/src/env.h b/src/env.h
+index 34c88c1addc5f64bd46332451e5b4ba8343c8818..1d32bf5945ab814294b5b5676b228c86518a05cd 100644
+--- a/src/env.h
++++ b/src/env.h
+@@ -1100,7 +1100,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 357ca1eb55652d88d6de60618e62cd54eaff6601..e0f84c3a7ba1c1866971eacfe0b508a52446bd7e 100644
+--- a/src/node.cc
++++ b/src/node.cc
+@@ -1344,8 +1344,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 7e1be61a9cca8205666a129bafa2b2f4f4dbcc4b..00ff1d6d6560a2e97da1c675a46bcd6defe963d7 100644
+--- a/src/node.h
++++ b/src/node.h
+@@ -314,7 +314,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
 cherry-pick-f4b66ae451c2.patch

patches/v8/chore_disable_is_execution_terminating_dcheck.patch

@@ -1,35 +0,0 @@
-From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
-From: Shelley Vohr <shelley.vohr@gmail.com>
-Date: Tue, 31 May 2022 19:58:01 +0200
-Subject: chore: disable is_execution_terminating DCHECK
-
-This causes a slew of crashes in Node.js.
-
-Upstream issue opened at https://github.com/nodejs/node-v8/issues/227.
-
-diff --git a/src/api/api-macros.h b/src/api/api-macros.h
-index 149dd0555a69be576fd1eb97aa79b8aedafcac04..233e6d2ac511c4a7fa45d47bb7448beead52faf1 100644
---- a/src/api/api-macros.h
-+++ b/src/api/api-macros.h
-@@ -97,8 +97,6 @@
- 
- // Lightweight version for APIs that don't require an active context.
- #define DCHECK_NO_SCRIPT_NO_EXCEPTION(i_isolate)             \
--  /* Embedders should never enter V8 after terminating it */ \
--  DCHECK(!i_isolate->is_execution_terminating());            \
-   DCHECK_NO_SCRIPT_NO_EXCEPTION_MAYBE_TEARDOWN(i_isolate)
- 
- #define ENTER_V8_NO_SCRIPT_NO_EXCEPTION(i_isolate) \
-diff --git a/src/execution/microtask-queue.cc b/src/execution/microtask-queue.cc
-index ac48de9b499aed29a09ba918ddabfa67cd5485da..aa50aeb1d4f3943f83ded5e328b4a65bcfbc7317 100644
---- a/src/execution/microtask-queue.cc
-+++ b/src/execution/microtask-queue.cc
-@@ -180,7 +180,7 @@ int MicrotaskQueue::RunMicrotasks(Isolate* isolate) {
- 
-   if (isolate->is_execution_terminating()) {
-     DCHECK(isolate->has_scheduled_exception());
--    DCHECK(maybe_result.is_null());
-+    // DCHECK(maybe_result.is_null());
-     delete[] ring_buffer_;
-     ring_buffer_ = nullptr;
-     capacity_ = 0;

patches/v8/revert_runtime_dhceck_terminating_exception_in_microtasks.patch

@@ -1,48 +0,0 @@
-From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
-From: Jeremy Rose <japthorp@slack-corp.com>
-Date: Mon, 9 May 2022 17:09:21 -0700
-Subject: Revert "[runtime] DHCECK terminating exception in Microtasks"
-
-This reverts commit bccb536c98181e8a6e9cf0b6342311adbbf61aca.
-
-diff --git a/src/builtins/builtins-microtask-queue-gen.cc b/src/builtins/builtins-microtask-queue-gen.cc
-index f58636fee555d782e18b7521c0c4f28ed60b3a52..6b0c63b34ff09f70cb9a4fe419f3b9bb0adf6790 100644
---- a/src/builtins/builtins-microtask-queue-gen.cc
-+++ b/src/builtins/builtins-microtask-queue-gen.cc
-@@ -118,7 +118,6 @@ void MicrotaskQueueBuiltinsAssembler::PrepareForContext(
- void MicrotaskQueueBuiltinsAssembler::RunSingleMicrotask(
-     TNode<Context> current_context, TNode<Microtask> microtask) {
-   CSA_DCHECK(this, TaggedIsNotSmi(microtask));
--  CSA_DCHECK(this, Word32BinaryNot(IsExecutionTerminating()));
- 
-   StoreRoot(RootIndex::kCurrentMicrotask, microtask);
-   TNode<IntPtrT> saved_entered_context_count = GetEnteredContextCount();
-diff --git a/src/codegen/code-stub-assembler.cc b/src/codegen/code-stub-assembler.cc
-index 23111e605f4390d311a0fac2b4335926315add38..2e3a2cbcac07e0e86254d4cb11eb3fe51fadff18 100644
---- a/src/codegen/code-stub-assembler.cc
-+++ b/src/codegen/code-stub-assembler.cc
-@@ -6390,12 +6390,6 @@ void CodeStubAssembler::SetPendingMessage(TNode<HeapObject> message) {
-   StoreFullTaggedNoWriteBarrier(pending_message, message);
- }
- 
--TNode<BoolT> CodeStubAssembler::IsExecutionTerminating() {
--  TNode<HeapObject> pending_message = GetPendingMessage();
--  return TaggedEqual(pending_message,
--                     LoadRoot(RootIndex::kTerminationException));
--}
--
- TNode<BoolT> CodeStubAssembler::InstanceTypeEqual(TNode<Int32T> instance_type,
-                                                   int type) {
-   return Word32Equal(instance_type, Int32Constant(type));
-diff --git a/src/codegen/code-stub-assembler.h b/src/codegen/code-stub-assembler.h
-index f7cdced051d23a755158bb73c91057bb1d801830..183bae89819584d48fc61e06cf9cf9792b02a61a 100644
---- a/src/codegen/code-stub-assembler.h
-+++ b/src/codegen/code-stub-assembler.h
-@@ -2583,7 +2583,6 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
- 
-   TNode<HeapObject> GetPendingMessage();
-   void SetPendingMessage(TNode<HeapObject> message);
--  TNode<BoolT> IsExecutionTerminating();
- 
-   // Type checks.
-   // Check whether the map is for an object with special properties, such as a

shell/app/node_main.cc

@@ -222,7 +222,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);
 
@@ -262,7 +263,7 @@ 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);
@@ -625,7 +625,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

@@ -73,21 +73,42 @@ 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)
     : isolate_(Initialize(event_loop)),
-      isolate_holder_(base::ThreadTaskRunnerHandle::Get(),
-                      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();
 }
 
@@ -97,7 +118,7 @@ JavascriptEnvironment::~JavascriptEnvironment() {
 
   {
     v8::HandleScope scope(isolate_);
-    context_.Get(isolate_)->Exit();
+    isolate_->GetCurrentContext()->Exit();
   }
   isolate_->Exit();
   g_isolate = nullptr;

shell/browser/javascript_environment.h

@@ -34,9 +34,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();
 
@@ -47,7 +44,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,7 +59,8 @@ 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,
@@ -67,7 +68,7 @@ void NodeService::Initialize(node::mojom::NodeServiceParamsPtr params) {
                                 // 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, "");
                               });