fix: utilityProcess exit codes (#42297)

This commit is contained in:
Shelley Vohr 2024-06-07 10:06:00 +02:00 committed by GitHub
parent 1c6d7d7ece
commit 7f3dc7d4ce
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 226 additions and 34 deletions

View file

@ -129,3 +129,4 @@ fix_use_app_launch_prefetch_namespace_for_subprocesstype.patch
feat_add_support_for_missing_dialog_features_to_shell_dialogs.patch feat_add_support_for_missing_dialog_features_to_shell_dialogs.patch
cherry-pick-22db6918bac9.patch cherry-pick-22db6918bac9.patch
fix_font_face_resolution_when_renderer_is_blocked.patch fix_font_face_resolution_when_renderer_is_blocked.patch
feat_enable_passing_exit_code_on_service_process_crash.patch

View file

@ -0,0 +1,136 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Shelley Vohr <shelley.vohr@gmail.com>
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<std::string> 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<GURL>& 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_;

View file

@ -808,12 +808,6 @@ void App::BrowserChildProcessCrashedOrKilled(
if (!data.name.empty()) { if (!data.name.empty()) {
details.Set("name", data.name); 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); Emit("child-process-gone", details);
} }

View file

@ -145,6 +145,9 @@ UtilityProcessWrapper::UtilityProcessWrapper(
} }
} }
if (!content::ServiceProcessHost::HasObserver(this))
content::ServiceProcessHost::AddObserver(this);
mojo::PendingReceiver<node::mojom::NodeService> receiver = mojo::PendingReceiver<node::mojom::NodeService> receiver =
node_service_remote_.BindNewPipeAndPassReceiver(); node_service_remote_.BindNewPipeAndPassReceiver();
@ -172,9 +175,10 @@ UtilityProcessWrapper::UtilityProcessWrapper(
: content::ChildProcessHost::CHILD_NORMAL) : content::ChildProcessHost::CHILD_NORMAL)
#endif #endif
.WithProcessCallback( .WithProcessCallback(
base::BindOnce(&UtilityProcessWrapper::OnServiceProcessLaunched, base::BindOnce(&UtilityProcessWrapper::OnServiceProcessLaunch,
weak_factory_.GetWeakPtr())) weak_factory_.GetWeakPtr()))
.Pass()); .Pass());
node_service_remote_.set_disconnect_with_reason_handler( node_service_remote_.set_disconnect_with_reason_handler(
base::BindOnce(&UtilityProcessWrapper::OnServiceProcessDisconnected, base::BindOnce(&UtilityProcessWrapper::OnServiceProcessDisconnected,
weak_factory_.GetWeakPtr())); weak_factory_.GetWeakPtr()));
@ -210,37 +214,61 @@ UtilityProcessWrapper::UtilityProcessWrapper(
network_context->CreateHostResolver( network_context->CreateHostResolver(
{}, host_resolver.InitWithNewPipeAndPassReceiver()); {}, host_resolver.InitWithNewPipeAndPassReceiver());
params->host_resolver = std::move(host_resolver); params->host_resolver = std::move(host_resolver);
node_service_remote_->Initialize(std::move(params)); 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) { const base::Process& process) {
DCHECK(node_service_remote_.is_connected()); DCHECK(node_service_remote_.is_connected());
pid_ = process.Pid(); pid_ = process.Pid();
GetAllUtilityProcessWrappers().AddWithID(this, pid_); GetAllUtilityProcessWrappers().AddWithID(this, pid_);
if (stdout_read_fd_ != -1) { if (stdout_read_fd_ != -1)
EmitWithoutEvent("stdout", stdout_read_fd_); EmitWithoutEvent("stdout", stdout_read_fd_);
} if (stderr_read_fd_ != -1)
if (stderr_read_fd_ != -1) {
EmitWithoutEvent("stderr", stderr_read_fd_); EmitWithoutEvent("stderr", stderr_read_fd_);
}
// Emit 'spawn' event
EmitWithoutEvent("spawn"); EmitWithoutEvent("spawn");
} }
void UtilityProcessWrapper::OnServiceProcessDisconnected( void UtilityProcessWrapper::HandleTermination(uint64_t exit_code) {
uint32_t error_code,
const std::string& description) {
if (pid_ != base::kNullProcessId) if (pid_ != base::kNullProcessId)
GetAllUtilityProcessWrappers().Remove(pid_); GetAllUtilityProcessWrappers().Remove(pid_);
CloseConnectorPort(); CloseConnectorPort();
// Emit 'exit' event
EmitWithoutEvent("exit", error_code); EmitWithoutEvent("exit", exit_code);
Unpin(); 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<node::mojom::NodeService>() ||
info.GetProcess().Pid() != pid_)
return;
HandleTermination(info.exit_code());
}
void UtilityProcessWrapper::OnServiceProcessCrashed(
const content::ServiceProcessInfo& info) {
if (!info.IsService<node::mojom::NodeService>() ||
info.GetProcess().Pid() != pid_)
return;
HandleTermination(info.exit_code());
}
void UtilityProcessWrapper::CloseConnectorPort() { void UtilityProcessWrapper::CloseConnectorPort() {
if (!connector_closed_ && connector_->is_valid()) { if (!connector_closed_ && connector_->is_valid()) {
host_port_.GiveDisentangledHandle(connector_->PassMessagePipe()); 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) if (pid_ != base::kNullProcessId)
GetAllUtilityProcessWrappers().Remove(pid_); GetAllUtilityProcessWrappers().Remove(pid_);
node_service_remote_.reset(); node_service_remote_.reset();

View file

@ -13,9 +13,11 @@
#include "base/environment.h" #include "base/environment.h"
#include "base/memory/weak_ptr.h" #include "base/memory/weak_ptr.h"
#include "base/process/process_handle.h" #include "base/process/process_handle.h"
#include "content/public/browser/service_process_host.h"
#include "gin/wrappable.h" #include "gin/wrappable.h"
#include "mojo/public/cpp/bindings/connector.h" #include "mojo/public/cpp/bindings/connector.h"
#include "mojo/public/cpp/bindings/message.h" #include "mojo/public/cpp/bindings/message.h"
#include "mojo/public/cpp/bindings/receiver.h"
#include "mojo/public/cpp/bindings/remote.h" #include "mojo/public/cpp/bindings/remote.h"
#include "shell/browser/event_emitter_mixin.h" #include "shell/browser/event_emitter_mixin.h"
#include "shell/common/gin_helper/pinnable.h" #include "shell/common/gin_helper/pinnable.h"
@ -38,7 +40,8 @@ class UtilityProcessWrapper
: public gin::Wrappable<UtilityProcessWrapper>, : public gin::Wrappable<UtilityProcessWrapper>,
public gin_helper::Pinnable<UtilityProcessWrapper>, public gin_helper::Pinnable<UtilityProcessWrapper>,
public gin_helper::EventEmitterMixin<UtilityProcessWrapper>, public gin_helper::EventEmitterMixin<UtilityProcessWrapper>,
public mojo::MessageReceiver { public mojo::MessageReceiver,
public content::ServiceProcessHost::Observer {
public: public:
enum class IOHandle : size_t { STDIN = 0, STDOUT = 1, STDERR = 2 }; enum class IOHandle : size_t { STDIN = 0, STDOUT = 1, STDERR = 2 };
enum class IOType { IO_PIPE, IO_INHERIT, IO_IGNORE }; enum class IOType { IO_PIPE, IO_INHERIT, IO_IGNORE };
@ -47,7 +50,7 @@ class UtilityProcessWrapper
static gin::Handle<UtilityProcessWrapper> Create(gin::Arguments* args); static gin::Handle<UtilityProcessWrapper> Create(gin::Arguments* args);
static raw_ptr<UtilityProcessWrapper> FromProcessId(base::ProcessId pid); static raw_ptr<UtilityProcessWrapper> FromProcessId(base::ProcessId pid);
void Shutdown(int exit_code); void Shutdown(uint64_t exit_code);
// gin::Wrappable // gin::Wrappable
static gin::WrapperInfo kWrapperInfo; static gin::WrapperInfo kWrapperInfo;
@ -62,11 +65,11 @@ class UtilityProcessWrapper
base::EnvironmentMap env_map, base::EnvironmentMap env_map,
base::FilePath current_working_directory, base::FilePath current_working_directory,
bool use_plugin_helper); bool use_plugin_helper);
void OnServiceProcessDisconnected(uint32_t error_code, void OnServiceProcessLaunch(const base::Process& process);
const std::string& description);
void OnServiceProcessLaunched(const base::Process& process);
void CloseConnectorPort(); void CloseConnectorPort();
void HandleTermination(uint64_t exit_code);
void PostMessage(gin::Arguments* args); void PostMessage(gin::Arguments* args);
bool Kill() const; bool Kill() const;
v8::Local<v8::Value> GetOSProcessId(v8::Isolate* isolate) const; v8::Local<v8::Value> GetOSProcessId(v8::Isolate* isolate) const;
@ -74,6 +77,15 @@ class UtilityProcessWrapper
// mojo::MessageReceiver // mojo::MessageReceiver
bool Accept(mojo::Message* mojo_message) override; 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; base::ProcessId pid_ = base::kNullProcessId;
#if BUILDFLAG(IS_WIN) #if BUILDFLAG(IS_WIN)
// Non-owning handles, these will be closed when the // Non-owning handles, these will be closed when the

View file

@ -13,29 +13,32 @@ v8::Local<v8::Value> CallMethodWithArgs(v8::Isolate* isolate,
v8::Local<v8::Object> obj, v8::Local<v8::Object> obj,
const char* method, const char* method,
ValueVector* args) { 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<node::CallbackScope> callback_scope; std::unique_ptr<node::CallbackScope> callback_scope;
if (node::Environment::GetCurrent(isolate)) { if (node::Environment::GetCurrent(isolate)) {
v8::HandleScope handle_scope(isolate); v8::HandleScope handle_scope(isolate);
callback_scope = std::make_unique<node::CallbackScope>( callback_scope = std::make_unique<node::CallbackScope>(
isolate, v8::Object::New(isolate), node::async_context{0, 0}); isolate, v8::Object::New(isolate), node::async_context{0, 0});
} else {
return v8::Boolean::New(isolate, false);
} }
// Perform microtask checkpoint after running JavaScript. // Perform microtask checkpoint after running JavaScript.
gin_helper::MicrotasksScope microtasks_scope( gin_helper::MicrotasksScope microtasks_scope(
isolate, obj->GetCreationContextChecked()->GetMicrotaskQueue(), true); 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<v8::Value> ret = node::MakeCallback( v8::MaybeLocal<v8::Value> ret = node::MakeCallback(
isolate, obj, method, args->size(), args->data(), {0, 0}); isolate, obj, method, args->size(), args->data(), {0, 0});
// If the JS function throws an exception (doesn't return a value) the result // 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 // 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 // case we need to return "false" as that indicates that the event emitter did
// not handle the event // not handle the event
v8::Local<v8::Value> localRet; v8::Local<v8::Value> localRet;
if (ret.ToLocal(&localRet)) { if (ret.ToLocal(&localRet))
return localRet; return localRet;
}
return v8::Boolean::New(isolate, false); return v8::Boolean::New(isolate, false);
} }

View file

@ -104,7 +104,7 @@ void NodeService::Initialize(node::mojom::NodeServiceParamsPtr params) {
js_env_->DestroyMicrotasksRunner(); js_env_->DestroyMicrotasksRunner();
node::Stop(env, node::StopFlags::kDoNotTerminateIsolate); node::Stop(env, node::StopFlags::kDoNotTerminateIsolate);
node_env_stopped_ = true; 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); node_env_->set_trace_sync_io(node_env_->options()->trace_sync_io);

View file

@ -59,11 +59,29 @@ describe('utilityProcess module', () => {
expect(code).to.equal(0); 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 () => { ifit(!isWindows32Bit)('emits \'exit\' when child process crashes', async () => {
const child = utilityProcess.fork(path.join(fixturesPath, 'crash.js')); const child = utilityProcess.fork(path.join(fixturesPath, 'crash.js'));
// Do not check for exit code in this case, // SIGSEGV code can differ across pipelines but should never be 0.
// SIGSEGV code can be 139 or 11 across our different CI pipeline. const [code] = await once(child, 'exit');
await once(child, 'exit'); expect(code).to.not.equal(0);
}); });
ifit(!isWindows32Bit)('emits \'exit\' corresponding to the child process', async () => { ifit(!isWindows32Bit)('emits \'exit\' corresponding to the child process', async () => {