diff --git a/patches/node/.patches b/patches/node/.patches index a222f4a950d37..5aa0d6dc9340e 100644 --- a/patches/node/.patches +++ b/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 diff --git a/patches/node/src_allow_optional_isolation_termination_in_node.patch b/patches/node/src_allow_optional_isolation_termination_in_node.patch new file mode 100644 index 0000000000000..f9ff03d5c1f99 --- /dev/null +++ b/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 +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. diff --git a/patches/v8/.patches b/patches/v8/.patches index 55e51988893ba..bd2409a468f17 100644 --- a/patches/v8/.patches +++ b/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 diff --git a/patches/v8/chore_disable_is_execution_terminating_dcheck.patch b/patches/v8/chore_disable_is_execution_terminating_dcheck.patch deleted file mode 100644 index 9fe3917362b89..0000000000000 --- a/patches/v8/chore_disable_is_execution_terminating_dcheck.patch +++ /dev/null @@ -1,35 +0,0 @@ -From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 -From: Shelley Vohr -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; diff --git a/patches/v8/revert_runtime_dhceck_terminating_exception_in_microtasks.patch b/patches/v8/revert_runtime_dhceck_terminating_exception_in_microtasks.patch deleted file mode 100644 index f48f9e576e734..0000000000000 --- a/patches/v8/revert_runtime_dhceck_terminating_exception_in_microtasks.patch +++ /dev/null @@ -1,48 +0,0 @@ -From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 -From: Jeremy Rose -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 current_context, TNode microtask) { - CSA_DCHECK(this, TaggedIsNotSmi(microtask)); -- CSA_DCHECK(this, Word32BinaryNot(IsExecutionTerminating())); - - StoreRoot(RootIndex::kCurrentMicrotask, microtask); - TNode 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 message) { - StoreFullTaggedNoWriteBarrier(pending_message, message); - } - --TNode CodeStubAssembler::IsExecutionTerminating() { -- TNode pending_message = GetPendingMessage(); -- return TaggedEqual(pending_message, -- LoadRoot(RootIndex::kTerminationException)); --} -- - TNode CodeStubAssembler::InstanceTypeEqual(TNode 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 GetPendingMessage(); - void SetPendingMessage(TNode message); -- TNode IsExecutionTerminating(); - - // Type checks. - // Check whether the map is for an object with special properties, such as a diff --git a/shell/app/node_main.cc b/shell/app/node_main.cc index 182e0427536d8..ded995d0dca6e 100644 --- a/shell/app/node_main.cc +++ b/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(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); } diff --git a/shell/browser/electron_browser_main_parts.cc b/shell/browser/electron_browser_main_parts.cc index 0cd475694ee72..ef72790a57e3f 100644 --- a/shell/browser/electron_browser_main_parts.cc +++ b/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(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); diff --git a/shell/browser/javascript_environment.cc b/shell/browser/javascript_environment.cc index e9312131baf3d..5172a8e7d2ee8 100644 --- a/shell/browser/javascript_environment.cc +++ b/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 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(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; diff --git a/shell/browser/javascript_environment.h b/shell/browser/javascript_environment.h index c40a939127a40..49cc2175f907c 100644 --- a/shell/browser/javascript_environment.h +++ b/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 context() const { - return v8::Local::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 context_; std::unique_ptr microtasks_runner_; }; diff --git a/shell/services/node/node_service.cc b/shell/services/node/node_service.cc index ffd2f6966f23e..7ce0fcb552d19 100644 --- a/shell/services/node/node_service.cc +++ b/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(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, ""); });