diff --git a/patches/chromium/.patches b/patches/chromium/.patches index 612be9f75e05..6ce2e1cd33d9 100644 --- a/patches/chromium/.patches +++ b/patches/chromium/.patches @@ -129,3 +129,4 @@ fix_use_app_launch_prefetch_namespace_for_subprocesstype.patch feat_add_support_for_missing_dialog_features_to_shell_dialogs.patch cherry-pick-22db6918bac9.patch fix_font_face_resolution_when_renderer_is_blocked.patch +feat_enable_passing_exit_code_on_service_process_crash.patch diff --git a/patches/chromium/feat_enable_passing_exit_code_on_service_process_crash.patch b/patches/chromium/feat_enable_passing_exit_code_on_service_process_crash.patch new file mode 100644 index 000000000000..a536f1c7bb89 --- /dev/null +++ b/patches/chromium/feat_enable_passing_exit_code_on_service_process_crash.patch @@ -0,0 +1,136 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Shelley Vohr +Date: Tue, 28 May 2024 10:44:06 +0200 +Subject: feat: enable passing exit code on service process crash + +This patch enables plumbing the exit code of the service process to the +browser process when the service process crashes. The process can perform cleanup +after the message pipe disconnection, which previously led to racy and incorrect +exit codes in some crashing scenarios. To mitigate this, we can rely on +ServiceProcessHost::Observer functions, but we need to pass the exit code to +the observer. + +diff --git a/content/browser/service_process_host_impl.cc b/content/browser/service_process_host_impl.cc +index 642f8b6da39615d1c68584ff18fc57ceb95b84b2..cb58d75aa172b5144998fc2e3551231523d8b555 100644 +--- a/content/browser/service_process_host_impl.cc ++++ b/content/browser/service_process_host_impl.cc +@@ -72,12 +72,15 @@ class ServiceProcessTracker { + processes_.erase(iter); + } + +- void NotifyCrashed(ServiceProcessId id) { ++ void NotifyCrashed(ServiceProcessId id, int exit_code) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); + auto iter = processes_.find(id); + DCHECK(iter != processes_.end()); +- for (auto& observer : observers_) +- observer.OnServiceProcessCrashed(iter->second.Duplicate()); ++ for (auto& observer : observers_) { ++ auto params = iter->second.Duplicate(); ++ params.set_exit_code(exit_code); ++ observer.OnServiceProcessCrashed(params); ++ } + processes_.erase(iter); + } + +@@ -86,6 +89,11 @@ class ServiceProcessTracker { + observers_.AddObserver(observer); + } + ++ bool HasObserver(ServiceProcessHost::Observer* observer) { ++ DCHECK_CURRENTLY_ON(BrowserThread::UI); ++ return observers_.HasObserver(observer); ++ } ++ + void RemoveObserver(ServiceProcessHost::Observer* observer) { + // NOTE: Some tests may remove observers after BrowserThreads are shut down. + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) || +@@ -153,7 +161,7 @@ class UtilityProcessClient : public UtilityProcessHost::Client { + process_info_->service_process_id()); + } + +- void OnProcessCrashed() override { ++ void OnProcessCrashed(int exit_code) override { + // TODO(crbug.com/40654042): It is unclear how we can observe + // |OnProcessCrashed()| without observing |OnProcessLaunched()| first, but + // it can happen on Android. Ignore the notification in this case. +@@ -161,7 +169,7 @@ class UtilityProcessClient : public UtilityProcessHost::Client { + return; + + GetServiceProcessTracker().NotifyCrashed( +- process_info_->service_process_id()); ++ process_info_->service_process_id(), exit_code); + } + + private: +@@ -230,6 +238,11 @@ void ServiceProcessHost::AddObserver(Observer* observer) { + GetServiceProcessTracker().AddObserver(observer); + } + ++// static ++bool ServiceProcessHost::HasObserver(Observer* observer) { ++ return GetServiceProcessTracker().HasObserver(observer); ++} ++ + // static + void ServiceProcessHost::RemoveObserver(Observer* observer) { + GetServiceProcessTracker().RemoveObserver(observer); +diff --git a/content/browser/utility_process_host.cc b/content/browser/utility_process_host.cc +index c564b64f25c3c2bf87ed5a93b0ce601d73aab177..f2474e89f27564a2e02ba4574dd22632f03ca2e0 100644 +--- a/content/browser/utility_process_host.cc ++++ b/content/browser/utility_process_host.cc +@@ -511,7 +511,7 @@ void UtilityProcessHost::OnProcessCrashed(int exit_code) { + // Take ownership of |client_| so the destructor doesn't notify it of + // termination. + auto client = std::move(client_); +- client->OnProcessCrashed(); ++ client->OnProcessCrashed(exit_code); + } + + std::optional UtilityProcessHost::GetServiceName() { +diff --git a/content/browser/utility_process_host.h b/content/browser/utility_process_host.h +index 1083f1683a05825f51f5b2d71f8107d910fa2474..7c92b13c9c10e1746052b79421e82cf4a6af7406 100644 +--- a/content/browser/utility_process_host.h ++++ b/content/browser/utility_process_host.h +@@ -78,7 +78,7 @@ class CONTENT_EXPORT UtilityProcessHost + + virtual void OnProcessLaunched(const base::Process& process) {} + virtual void OnProcessTerminatedNormally() {} +- virtual void OnProcessCrashed() {} ++ virtual void OnProcessCrashed(int exit_code) {} + }; + + // This class is self-owned. It must be instantiated using new, and shouldn't +diff --git a/content/public/browser/service_process_host.h b/content/public/browser/service_process_host.h +index 22e1191b57f56aa31b2c82fcc3ec0972f16752a8..15a1e41c1048197fd2373397301f9b92e9cfcb1e 100644 +--- a/content/public/browser/service_process_host.h ++++ b/content/public/browser/service_process_host.h +@@ -227,6 +227,10 @@ class CONTENT_EXPORT ServiceProcessHost { + // removed before destruction. Must be called from the UI thread only. + static void AddObserver(Observer* observer); + ++ // Returns true if the given observer is currently registered. ++ // Must be called from the UI thread only. ++ static bool HasObserver(Observer* observer); ++ + // Removes a registered observer. This must be called some time before + // |*observer| is destroyed and must be called from the UI thread only. + static void RemoveObserver(Observer* observer); +diff --git a/content/public/browser/service_process_info.h b/content/public/browser/service_process_info.h +index 1a8656aef341cd3b23af588fb00569b79d6cd100..f904af7ee6bbacf4474e0939855ecf9f2c9a5eaa 100644 +--- a/content/public/browser/service_process_info.h ++++ b/content/public/browser/service_process_info.h +@@ -64,7 +64,13 @@ class CONTENT_EXPORT ServiceProcessInfo { + const std::optional& site() const { return site_; } + const base::Process& GetProcess() const { return process_; } + ++ void set_exit_code(int exit_code) { exit_code_ = exit_code; } ++ int exit_code() const { return exit_code_; } ++ + private: ++ // The exit code of the process, if it has exited. ++ int exit_code_; ++ + // The name of the service interface for which the process was launched. + std::string service_interface_name_; + diff --git a/shell/browser/api/electron_api_app.cc b/shell/browser/api/electron_api_app.cc index 01ebe1a544d6..5b0e7b0dd0f5 100644 --- a/shell/browser/api/electron_api_app.cc +++ b/shell/browser/api/electron_api_app.cc @@ -808,12 +808,6 @@ void App::BrowserChildProcessCrashedOrKilled( if (!data.name.empty()) { details.Set("name", data.name); } - if (data.process_type == content::PROCESS_TYPE_UTILITY) { - base::ProcessId pid = data.GetProcess().Pid(); - auto utility_process_wrapper = UtilityProcessWrapper::FromProcessId(pid); - if (utility_process_wrapper) - utility_process_wrapper->Shutdown(info.exit_code); - } Emit("child-process-gone", details); } diff --git a/shell/browser/api/electron_api_utility_process.cc b/shell/browser/api/electron_api_utility_process.cc index 016485941b5f..72f34216b427 100644 --- a/shell/browser/api/electron_api_utility_process.cc +++ b/shell/browser/api/electron_api_utility_process.cc @@ -145,6 +145,9 @@ UtilityProcessWrapper::UtilityProcessWrapper( } } + if (!content::ServiceProcessHost::HasObserver(this)) + content::ServiceProcessHost::AddObserver(this); + mojo::PendingReceiver receiver = node_service_remote_.BindNewPipeAndPassReceiver(); @@ -172,9 +175,10 @@ UtilityProcessWrapper::UtilityProcessWrapper( : content::ChildProcessHost::CHILD_NORMAL) #endif .WithProcessCallback( - base::BindOnce(&UtilityProcessWrapper::OnServiceProcessLaunched, + base::BindOnce(&UtilityProcessWrapper::OnServiceProcessLaunch, weak_factory_.GetWeakPtr())) .Pass()); + node_service_remote_.set_disconnect_with_reason_handler( base::BindOnce(&UtilityProcessWrapper::OnServiceProcessDisconnected, weak_factory_.GetWeakPtr())); @@ -210,37 +214,61 @@ UtilityProcessWrapper::UtilityProcessWrapper( network_context->CreateHostResolver( {}, host_resolver.InitWithNewPipeAndPassReceiver()); params->host_resolver = std::move(host_resolver); + node_service_remote_->Initialize(std::move(params)); } -UtilityProcessWrapper::~UtilityProcessWrapper() = default; +UtilityProcessWrapper::~UtilityProcessWrapper() { + content::ServiceProcessHost::RemoveObserver(this); +} -void UtilityProcessWrapper::OnServiceProcessLaunched( +void UtilityProcessWrapper::OnServiceProcessLaunch( const base::Process& process) { DCHECK(node_service_remote_.is_connected()); pid_ = process.Pid(); GetAllUtilityProcessWrappers().AddWithID(this, pid_); - if (stdout_read_fd_ != -1) { + if (stdout_read_fd_ != -1) EmitWithoutEvent("stdout", stdout_read_fd_); - } - if (stderr_read_fd_ != -1) { + if (stderr_read_fd_ != -1) EmitWithoutEvent("stderr", stderr_read_fd_); - } - // Emit 'spawn' event EmitWithoutEvent("spawn"); } -void UtilityProcessWrapper::OnServiceProcessDisconnected( - uint32_t error_code, - const std::string& description) { +void UtilityProcessWrapper::HandleTermination(uint64_t exit_code) { if (pid_ != base::kNullProcessId) GetAllUtilityProcessWrappers().Remove(pid_); CloseConnectorPort(); - // Emit 'exit' event - EmitWithoutEvent("exit", error_code); + + EmitWithoutEvent("exit", exit_code); + Unpin(); } +void UtilityProcessWrapper::OnServiceProcessDisconnected( + uint32_t exit_code, + const std::string& description) { + if (description == "process_exit_termination") + HandleTermination(exit_code); +} + +void UtilityProcessWrapper::OnServiceProcessTerminatedNormally( + const content::ServiceProcessInfo& info) { + if (!info.IsService() || + info.GetProcess().Pid() != pid_) + return; + + HandleTermination(info.exit_code()); +} + +void UtilityProcessWrapper::OnServiceProcessCrashed( + const content::ServiceProcessInfo& info) { + if (!info.IsService() || + info.GetProcess().Pid() != pid_) + return; + + HandleTermination(info.exit_code()); +} + void UtilityProcessWrapper::CloseConnectorPort() { if (!connector_closed_ && connector_->is_valid()) { host_port_.GiveDisentangledHandle(connector_->PassMessagePipe()); @@ -250,7 +278,7 @@ void UtilityProcessWrapper::CloseConnectorPort() { } } -void UtilityProcessWrapper::Shutdown(int exit_code) { +void UtilityProcessWrapper::Shutdown(uint64_t exit_code) { if (pid_ != base::kNullProcessId) GetAllUtilityProcessWrappers().Remove(pid_); node_service_remote_.reset(); diff --git a/shell/browser/api/electron_api_utility_process.h b/shell/browser/api/electron_api_utility_process.h index cdd7a6f348ba..b57dca1677be 100644 --- a/shell/browser/api/electron_api_utility_process.h +++ b/shell/browser/api/electron_api_utility_process.h @@ -13,9 +13,11 @@ #include "base/environment.h" #include "base/memory/weak_ptr.h" #include "base/process/process_handle.h" +#include "content/public/browser/service_process_host.h" #include "gin/wrappable.h" #include "mojo/public/cpp/bindings/connector.h" #include "mojo/public/cpp/bindings/message.h" +#include "mojo/public/cpp/bindings/receiver.h" #include "mojo/public/cpp/bindings/remote.h" #include "shell/browser/event_emitter_mixin.h" #include "shell/common/gin_helper/pinnable.h" @@ -38,7 +40,8 @@ class UtilityProcessWrapper : public gin::Wrappable, public gin_helper::Pinnable, public gin_helper::EventEmitterMixin, - public mojo::MessageReceiver { + public mojo::MessageReceiver, + public content::ServiceProcessHost::Observer { public: enum class IOHandle : size_t { STDIN = 0, STDOUT = 1, STDERR = 2 }; enum class IOType { IO_PIPE, IO_INHERIT, IO_IGNORE }; @@ -47,7 +50,7 @@ class UtilityProcessWrapper static gin::Handle Create(gin::Arguments* args); static raw_ptr FromProcessId(base::ProcessId pid); - void Shutdown(int exit_code); + void Shutdown(uint64_t exit_code); // gin::Wrappable static gin::WrapperInfo kWrapperInfo; @@ -62,11 +65,11 @@ class UtilityProcessWrapper base::EnvironmentMap env_map, base::FilePath current_working_directory, bool use_plugin_helper); - void OnServiceProcessDisconnected(uint32_t error_code, - const std::string& description); - void OnServiceProcessLaunched(const base::Process& process); + void OnServiceProcessLaunch(const base::Process& process); void CloseConnectorPort(); + void HandleTermination(uint64_t exit_code); + void PostMessage(gin::Arguments* args); bool Kill() const; v8::Local GetOSProcessId(v8::Isolate* isolate) const; @@ -74,6 +77,15 @@ class UtilityProcessWrapper // mojo::MessageReceiver bool Accept(mojo::Message* mojo_message) override; + // content::ServiceProcessHost::Observer + void OnServiceProcessTerminatedNormally( + const content::ServiceProcessInfo& info) override; + void OnServiceProcessCrashed( + const content::ServiceProcessInfo& info) override; + + void OnServiceProcessDisconnected(uint32_t exit_code, + const std::string& description); + base::ProcessId pid_ = base::kNullProcessId; #if BUILDFLAG(IS_WIN) // Non-owning handles, these will be closed when the diff --git a/shell/common/gin_helper/event_emitter_caller.cc b/shell/common/gin_helper/event_emitter_caller.cc index d68c18d78186..c18c2a7895a0 100644 --- a/shell/common/gin_helper/event_emitter_caller.cc +++ b/shell/common/gin_helper/event_emitter_caller.cc @@ -13,29 +13,32 @@ v8::Local CallMethodWithArgs(v8::Isolate* isolate, v8::Local obj, const char* method, ValueVector* args) { - // Only set up the node::CallbackScope if there's a node environment. + // An active node::Environment is required for node::MakeCallback. std::unique_ptr callback_scope; if (node::Environment::GetCurrent(isolate)) { v8::HandleScope handle_scope(isolate); callback_scope = std::make_unique( isolate, v8::Object::New(isolate), node::async_context{0, 0}); + } else { + return v8::Boolean::New(isolate, false); } // Perform microtask checkpoint after running JavaScript. gin_helper::MicrotasksScope microtasks_scope( isolate, obj->GetCreationContextChecked()->GetMicrotaskQueue(), true); - // Use node::MakeCallback to call the callback, and it will also run pending - // tasks in Node.js. + + // node::MakeCallback will also run pending tasks in Node.js. v8::MaybeLocal ret = node::MakeCallback( isolate, obj, method, args->size(), args->data(), {0, 0}); + // If the JS function throws an exception (doesn't return a value) the result // of MakeCallback will be empty and therefore ToLocal will be false, in this // case we need to return "false" as that indicates that the event emitter did // not handle the event v8::Local localRet; - if (ret.ToLocal(&localRet)) { + if (ret.ToLocal(&localRet)) return localRet; - } + return v8::Boolean::New(isolate, false); } diff --git a/shell/services/node/node_service.cc b/shell/services/node/node_service.cc index e74219893747..771ce3550896 100644 --- a/shell/services/node/node_service.cc +++ b/shell/services/node/node_service.cc @@ -104,7 +104,7 @@ void NodeService::Initialize(node::mojom::NodeServiceParamsPtr params) { js_env_->DestroyMicrotasksRunner(); node::Stop(env, node::StopFlags::kDoNotTerminateIsolate); node_env_stopped_ = true; - receiver_.ResetWithReason(exit_code, ""); + receiver_.ResetWithReason(exit_code, "process_exit_termination"); }); node_env_->set_trace_sync_io(node_env_->options()->trace_sync_io); diff --git a/spec/api-utility-process-spec.ts b/spec/api-utility-process-spec.ts index c2752ac39a56..ccc551bbac95 100644 --- a/spec/api-utility-process-spec.ts +++ b/spec/api-utility-process-spec.ts @@ -59,11 +59,29 @@ describe('utilityProcess module', () => { expect(code).to.equal(0); }); + ifit(!isWindows32Bit)('emits the correct error code when child process exits nonzero', async () => { + const child = utilityProcess.fork(path.join(fixturesPath, 'empty.js')); + await once(child, 'spawn'); + const exit = once(child, 'exit'); + process.kill(child.pid!); + const [code] = await exit; + expect(code).to.not.equal(0); + }); + + ifit(!isWindows32Bit)('emits the correct error code when child process is killed', async () => { + const child = utilityProcess.fork(path.join(fixturesPath, 'empty.js')); + await once(child, 'spawn'); + const exit = once(child, 'exit'); + process.kill(child.pid!); + const [code] = await exit; + expect(code).to.not.equal(0); + }); + ifit(!isWindows32Bit)('emits \'exit\' when child process crashes', async () => { const child = utilityProcess.fork(path.join(fixturesPath, 'crash.js')); - // Do not check for exit code in this case, - // SIGSEGV code can be 139 or 11 across our different CI pipeline. - await once(child, 'exit'); + // SIGSEGV code can differ across pipelines but should never be 0. + const [code] = await once(child, 'exit'); + expect(code).to.not.equal(0); }); ifit(!isWindows32Bit)('emits \'exit\' corresponding to the child process', async () => {