fix: utility process exit code for graceful termination (#44749)

patches/chromium/feat_enable_passing_exit_code_on_service_process_crash.patch

@@ -11,7 +11,7 @@ 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 45cf31157c535a0cdc9236a07e2ffffd166ba412..9c89cdb3a290a7b0e68539ccd5383f2a26cc7ab3 100644
+index 45cf31157c535a0cdc9236a07e2ffffd166ba412..255f63904c7e1699dbd0d9634c7472b366abbaf5 100644
 --- a/content/browser/service_process_host_impl.cc
 +++ b/content/browser/service_process_host_impl.cc
 @@ -73,12 +73,15 @@ class ServiceProcessTracker {
@@ -33,19 +33,7 @@ index 45cf31157c535a0cdc9236a07e2ffffd166ba412..9c89cdb3a290a7b0e68539ccd5383f2a
      processes_.erase(iter);
    }
  
-@@ -87,6 +90,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) ||
-@@ -154,7 +162,7 @@ class UtilityProcessClient : public UtilityProcessHost::Client {
+@@ -154,7 +157,7 @@ class UtilityProcessClient : public UtilityProcessHost::Client {
          process_info_->service_process_id());
    }
  
@@ -54,7 +42,7 @@ index 45cf31157c535a0cdc9236a07e2ffffd166ba412..9c89cdb3a290a7b0e68539ccd5383f2a
      // 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.
-@@ -162,7 +170,7 @@ class UtilityProcessClient : public UtilityProcessHost::Client {
+@@ -162,7 +165,7 @@ class UtilityProcessClient : public UtilityProcessHost::Client {
        return;
  
      GetServiceProcessTracker().NotifyCrashed(
@@ -63,18 +51,6 @@ index 45cf31157c535a0cdc9236a07e2ffffd166ba412..9c89cdb3a290a7b0e68539ccd5383f2a
    }
  
   private:
-@@ -231,6 +239,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 1a8dd3c6950c1654c054d036acfdc83bd8b61c0b..e5297de039019285217192ade914b6f761f94480 100644
 --- a/content/browser/utility_process_host.cc
@@ -101,23 +77,8 @@ index 1083f1683a05825f51f5b2d71f8107d910fa2474..7c92b13c9c10e1746052b79421e82cf4
    };
  
    // 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
+index 1a8656aef341cd3b23af588fb00569b79d6cd100..6af523eb27a8c1e5529721c029e5b3ba0708b9fc 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 {
@@ -129,7 +90,7 @@ index 1a8656aef341cd3b23af588fb00569b79d6cd100..f904af7ee6bbacf4474e0939855ecf9f
 +
   private:
 +  // The exit code of the process, if it has exited.
-+  int exit_code_;
++  int exit_code_ = 0;
 +
    // The name of the service interface for which the process was launched.
    std::string service_interface_name_;

shell/browser/api/electron_api_utility_process.cc

@@ -146,8 +146,8 @@ UtilityProcessWrapper::UtilityProcessWrapper(
     }
   }
 
-  if (!content::ServiceProcessHost::HasObserver(this))
-    content::ServiceProcessHost::AddObserver(this);
+  // Watch for service process termination events.
+  content::ServiceProcessHost::AddObserver(this);
 
   mojo::PendingReceiver<node::mojom::NodeService> receiver =
       node_service_remote_.BindNewPipeAndPassReceiver();
@@ -257,6 +257,23 @@ void UtilityProcessWrapper::HandleTermination(uint64_t exit_code) {
 
   pid_ = base::kNullProcessId;
   CloseConnectorPort();
+  if (killed_) {
+#if BUILDFLAG(IS_POSIX)
+    // UtilityProcessWrapper::Kill relies on base::Process::Terminate
+    // to gracefully shutdown the process which is performed by sending
+    // SIGTERM signal. When listening for exit events via ServiceProcessHost
+    // observers, the exit code on posix is obtained via
+    // BrowserChildProcessHostImpl::GetTerminationInfo which inturn relies
+    // on waitpid to extract the exit signal. If the process is unavailable,
+    // then the exit_code will be set to 0, otherwise we get the signal that
+    // was sent during the base::Process::Terminate call. For a user, this is
+    // still a graceful shutdown case so lets' convert the exit code to the
+    // expected value.
+    if (exit_code == SIGTERM || exit_code == SIGKILL) {
+      exit_code = 0;
+    }
+#endif
+  }
   EmitWithoutEvent("exit", exit_code);
   Unpin();
 }
@@ -335,7 +352,7 @@ void UtilityProcessWrapper::PostMessage(gin::Arguments* args) {
   connector_->Accept(&mojo_message);
 }
 
-bool UtilityProcessWrapper::Kill() const {
+bool UtilityProcessWrapper::Kill() {
   if (pid_ == base::kNullProcessId)
     return false;
   base::Process process = base::Process::Open(pid_);
@@ -348,6 +365,7 @@ bool UtilityProcessWrapper::Kill() const {
   // process reap should be signaled through the zygote via
   // content::ZygoteCommunication::EnsureProcessTerminated.
   base::EnsureProcessTerminated(std::move(process));
+  killed_ = result;
   return result;
 }
 

shell/browser/api/electron_api_utility_process.h

@@ -73,7 +73,7 @@ class UtilityProcessWrapper final
   void HandleTermination(uint64_t exit_code);
 
   void PostMessage(gin::Arguments* args);
-  bool Kill() const;
+  bool Kill();
   v8::Local<v8::Value> GetOSProcessId(v8::Isolate* isolate) const;
 
   // mojo::MessageReceiver
@@ -99,6 +99,7 @@ class UtilityProcessWrapper final
   int stderr_read_fd_ = -1;
   bool connector_closed_ = false;
   bool terminated_ = false;
+  bool killed_ = false;
   std::unique_ptr<mojo::Connector> connector_;
   blink::MessagePortDescriptor host_port_;
   mojo::Remote<node::mojom::NodeService> node_service_remote_;

spec/api-utility-process-spec.ts

@@ -185,7 +185,8 @@ describe('utilityProcess module', () => {
       });
       await once(child, 'spawn');
       expect(child.kill()).to.be.true();
-      await once(child, 'exit');
+      const [code] = await once(child, 'exit');
+      expect(code).to.equal(0);
     });
   });