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

* fix: avoid using v8 on Isolate termination

* chore: refactor for review

---------

Co-authored-by: electron-patch-conflict-fixer[bot] <83340002+electron-patch-conflict-fixer[bot]@users.noreply.github.com>

patches/node/.patches

@@ -34,4 +34,5 @@ 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
 test_mark_cpu_prof_tests_as_flaky_in_electron.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 <[email protected]>
+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

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 <[email protected]>
-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 <[email protected]>
-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 7d47afa2c92c1da43657702d5a85251646d680fd..10c3ec8387522ce2e35b77a7e6eb04de22597349 100644
---- a/src/codegen/code-stub-assembler.cc
-+++ b/src/codegen/code-stub-assembler.cc
-@@ -6417,12 +6417,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 fdd6da601705f72bd5d1a9101171048bdf408b9d..496c73225f8641e78793ba74d80e2e8b1e3f5c02 100644
---- a/src/codegen/code-stub-assembler.h
-+++ b/src/codegen/code-stub-assembler.h
-@@ -2550,7 +2550,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

@@ -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,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, "");
                               });