fix: utilityProcess exit codes (#42397)

* fix: utilityProcess exit codes Co-authored-by: Shelley Vohr <[email protected]> * fix: retain disconnect_with_reason_handler Co-authored-by: Shelley Vohr <[email protected]> * chore: move node::Environment check to CallMethodWithArgs Co-authored-by: Shelley Vohr <[email protected]> * chore: address feedback from review Co-authored-by: Shelley Vohr <[email protected]> * chore: update patches --------- Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <[email protected]> Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
electron · Jun 7, 2024 · 5f78c62 · 5f78c62
1 parent 2021c2e
commit 5f78c62
Show file tree

Hide file tree

Showing 8 changed files with 226 additions and 34 deletions.
diff --git a/patches/chromium/.patches b/patches/chromium/.patches
@@ -127,3 +127,4 @@ fix_add_support_for_skipping_first_2_no-op_refreshes_in_thumb_cap.patch
 refactor_expose_file_system_access_blocklist.patch
 partially_revert_is_newly_created_to_allow_for_browser_initiated.patch
 feat_add_support_for_missing_dialog_features_to_shell_dialogs.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
@@ -0,0 +1,136 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Shelley Vohr <[email protected]>
+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 fc77cd17db0acc7ca20e7cf06fb708d196fa15e3..7f48ce424300aa704b0cfa25b7b714a6a468a7af 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_;
+
diff --git 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
@@ -145,6 +145,9 @@ UtilityProcessWrapper::UtilityProcessWrapper(
     }
   }
 
+  if (!content::ServiceProcessHost::HasObserver(this))
+    content::ServiceProcessHost::AddObserver(this);
+
   mojo::PendingReceiver<node::mojom::NodeService> 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<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() {
   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
@@ -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<UtilityProcessWrapper>,
       public gin_helper::Pinnable<UtilityProcessWrapper>,
       public gin_helper::EventEmitterMixin<UtilityProcessWrapper>,
-      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<UtilityProcessWrapper> Create(gin::Arguments* args);
   static raw_ptr<UtilityProcessWrapper> FromProcessId(base::ProcessId pid);
 
-  void Shutdown(int exit_code);
+  void Shutdown(uint64_t exit_code);
 
   // gin::Wrappable
   static gin::WrapperInfo kWrapperInfo;
@@ -62,18 +65,27 @@ 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<v8::Value> GetOSProcessId(v8::Isolate* isolate) const;
 
   // 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
@@ -13,29 +13,32 @@ v8::Local<v8::Value> CallMethodWithArgs(v8::Isolate* isolate,
                                         v8::Local<v8::Object> 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<node::CallbackScope> callback_scope;
   if (node::Environment::GetCurrent(isolate)) {
     v8::HandleScope handle_scope(isolate);
     callback_scope = std::make_unique<node::CallbackScope>(
         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<v8::Value> 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<v8::Value> 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
@@ -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);