Repository: electron/electron
Pull request: fix: utility process exit code for graceful termination

#### Description of Change

With https://github.com/electron/electron/pull/42297 we rely on service process host observers for termination events and the exit code is obtained from `ServiceProcessInfo` struct which is uninitialized for `TerminatedNormally` leading to stale values in the exit event, refs https://github.com/microsoft/vscode/issues/225485

The change also removes the helper `HasObserver` from the patch since observers are meant to be added only once and if we are accidentally adding them multiple times then the callsite should be revisited. In our case, the UtilityProcess ctor is 1:1 with ServiceProcessHost so we are good. This also reduces the patch surface.

#### Release Notes

Notes: Return 0 exit code for normal termination of the utility process

Changed files:
filename: patches/chromium/feat_enable_passing_exit_code_on_service_process_crash.patch
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 f6082bada22c5f4e70af60ea6f555b0f363919c5..228f947edbe04bce242df62080052d9c383f1709 100644
+index f6082bada22c5f4e70af60ea6f555b0f363919c5..f691676a629bf82f81117599ae0bd0a4870c9f61 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 f6082bada22c5f4e70af60ea6f555b0f363919c5..228f947edbe04bce242df62080052d9c
      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 f6082bada22c5f4e70af60ea6f555b0f363919c5..228f947edbe04bce242df62080052d9c
      // 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 f6082bada22c5f4e70af60ea6f555b0f363919c5..228f947edbe04bce242df62080052d9c
    }
  
   private:
-@@ -232,6 +240,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 cdc2046f465110b60285da81fb0db7cdce064a59..8feca9f1c294b3de15d74dfc94508ee2a43e5fc3 100644
 --- a/content/browser/utility_process_host.cc
@@ -101,23 +77,8 @@ index 66cbabae31236758eef35bab211d4874f8a5c699..88515741fa08176ba9e952759c3a52e1
    };
  
    // 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 611a52e908f4cb70fbe5628e220a082e45320b70..d7fefab99f86515007aff5f523a423a421850c47 100644
---- a/content/public/browser/service_process_host.h
-+++ b/content/public/browser/service_process_host.h
-@@ -235,6 +235,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_;

filename: patches/chromium/refactor_unfilter_unresponsive_events.patch
patch: @@ -15,10 +15,10 @@ This CL removes these filters so the unresponsive event can still be
 accessed from our JS event. The filtering is moved into Electron's code.
 
 diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
-index 319800cec84a968b0e442fc760c8e1d701bda2ed..459663af86272fe1e23a6a163e01c67fc5f8a66d 100644
+index 0e742f196367bbbe8c4e147ef4db4f8dbbe4cbbe..7026b28c4f228971f74a706b6ad99777e4ca0773 100644
 --- a/content/browser/web_contents/web_contents_impl.cc
 +++ b/content/browser/web_contents/web_contents_impl.cc
-@@ -9464,25 +9464,13 @@ void WebContentsImpl::RendererUnresponsive(
+@@ -9518,25 +9518,13 @@ void WebContentsImpl::RendererUnresponsive(
      base::RepeatingClosure hang_monitor_restarter) {
    OPTIONAL_TRACE_EVENT1("content", "WebContentsImpl::RendererUnresponsive",
                          "render_widget_host", render_widget_host);

filename: shell/browser/api/electron_api_utility_process.cc
patch: @@ -145,8 +145,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();

filename: spec/api-utility-process-spec.ts
patch: @@ -215,7 +215,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);
     });
   });