More consistent order for service process shutdown

Today, service processes self-terminate as soon as they detect peer
closure on their main service pipe. This is detected on the IO thread
and results in the immediate scheduling of a main-thread task to
terminate the process.

Meanwhile, service instances themselves also watch for peer closure on
the same pipe and are notified on whichever thread runs the service (IO
or main thread). This triggers immediate destruction of the service
instance.

Because there is no ordering guarantee between the two independent
signal handlers, the net result is that during clean shutdown of a
service process, the service instance's destructor may not run, or
may run after shutdown has already started. This can be problematic
if the service continues to operate and perform tasks that depend
on a now-partially-shut-down process environment like the task
scheduler.

As a separate but related issue, the pipe-watching logic has been
watching for peer closure when it should really be watching for an
unreadable state (i.e., peer closure AND empty inbound queue). This
means that service termination could race with messages still on the
pipe unless developers are careful to synchronize their browser-side
Remote's teardown against some kind of ack message from the service.

This change does a bunch of stuff all at once to solve these problems:

- Modifies ContentClient ServiceFactory APIs so that they populate a
  shared instance (two shared instances really, one for IO, one for
  main thread) rather than having embedders provide their own instance.
- Gives UtilityThreadImpl and its internal (IO-thread-bound)
  ServiceBinderImpl their own ServiceFactory instances with
  clearly-defined ownership and lifetime.
- Removes independent pipe watching logic from ServiceBinderImpl,
  instead having it track service instance lifetimes from both
  ServiceFactory instances.
- Modifies ServiceFactory's pipe watching logic to watch for an
  unreadable pipe rather than for peer closure.

The net result is that service processes which are cleanly shut down,
meaning they neither crashed nor were reaped during full browser
process shutdown, will always run their service's destructor before
initiating shutdown.

Bug: 1135957
Change-Id: I16adbd7c98b4eb4333a92cd338643d4d5a9f2d6f
Tbr: [email protected]
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2503346
Commit-Queue: Ken Rockot <[email protected]>
Reviewed-by: Kinuko Yasuda <[email protected]>
Cr-Commit-Position: refs/heads/master@{#821847}

chrome/utility/chrome_content_utility_client.cc

@@ -95,11 +95,11 @@ void ChromeContentUtilityClient::UtilityThreadStarted() {
   }
 }
 
-mojo::ServiceFactory*
-ChromeContentUtilityClient::GetMainThreadServiceFactory() {
+void ChromeContentUtilityClient::RegisterMainThreadServices(
+    mojo::ServiceFactory& services) {
   if (utility_process_running_elevated_)
-    return ::GetElevatedMainThreadServiceFactory();
-  return ::GetMainThreadServiceFactory();
+    return ::RegisterElevatedMainThreadServices(services);
+  return ::RegisterMainThreadServices(services);
 }
 
 void ChromeContentUtilityClient::PostIOThreadCreated(
@@ -109,8 +109,9 @@ void ChromeContentUtilityClient::PostIOThreadCreated(
                                 metrics::CallStackProfileParams::IO_THREAD));
 }
 
-mojo::ServiceFactory* ChromeContentUtilityClient::GetIOThreadServiceFactory() {
-  return ::GetIOThreadServiceFactory();
+void ChromeContentUtilityClient::RegisterIOThreadServices(
+    mojo::ServiceFactory& services) {
+  return ::RegisterIOThreadServices(services);
 }
 
 // static

chrome/utility/chrome_content_utility_client.h

@@ -33,8 +33,8 @@ class ChromeContentUtilityClient : public content::ContentUtilityClient {
   void RegisterNetworkBinders(
       service_manager::BinderRegistry* registry) override;
   void UtilityThreadStarted() override;
-  mojo::ServiceFactory* GetMainThreadServiceFactory() override;
-  mojo::ServiceFactory* GetIOThreadServiceFactory() override;
+  void RegisterMainThreadServices(mojo::ServiceFactory& services) override;
+  void RegisterIOThreadServices(mojo::ServiceFactory& services) override;
 
   // See NetworkBinderProvider above.
   static void SetNetworkBinderCreationCallback(

chrome/utility/services.cc

@@ -252,91 +252,76 @@ auto RunAssistantAudioDecoder(
 
 }  // namespace
 
-mojo::ServiceFactory* GetElevatedMainThreadServiceFactory() {
+void RegisterElevatedMainThreadServices(mojo::ServiceFactory& services) {
   // NOTE: This ServiceFactory is only used in utility processes which are run
   // with elevated system privileges.
-  // clang-format off
-  static base::NoDestructor<mojo::ServiceFactory> factory {
 #if BUILDFLAG(ENABLE_EXTENSIONS) && defined(OS_WIN)
-    // On non-Windows, this service runs in a regular utility process.
-    RunRemovableStorageWriter,
+  // On non-Windows, this service runs in a regular utility process.
+  services.Add(RunRemovableStorageWriter);
 #endif
-  };
-  // clang-format on
-  return factory.get();
 }
 
-mojo::ServiceFactory* GetMainThreadServiceFactory() {
-  // clang-format off
-  static base::NoDestructor<mojo::ServiceFactory> factory {
-    RunFilePatcher,
-    RunUnzipper,
-    RunLanguageDetectionService,
-    RunQRCodeGeneratorService,
-    RunMachineLearningService,
+void RegisterMainThreadServices(mojo::ServiceFactory& services) {
+  services.Add(RunFilePatcher);
+  services.Add(RunUnzipper);
+  services.Add(RunLanguageDetectionService);
+  services.Add(RunQRCodeGeneratorService);
+  services.Add(RunMachineLearningService);
 
 #if !defined(OS_ANDROID)
-    RunProfileImporter,
-    RunMirroringService,
-    RunSpeechRecognitionService,
+  services.Add(RunProfileImporter);
+  services.Add(RunMirroringService);
+  services.Add(RunSpeechRecognitionService);
 #endif
 
 #if defined(OS_WIN)
-    RunQuarantineService,
-    RunWindowsUtility,
-    RunWindowsIconReader,
+  services.Add(RunQuarantineService);
+  services.Add(RunWindowsUtility);
+  services.Add(RunWindowsIconReader);
 #endif  // defined(OS_WIN)
 
 #if BUILDFLAG(ENABLE_PRINTING) && defined(OS_CHROMEOS)
-    RunCupsIppParser,
+  services.Add(RunCupsIppParser);
 #endif
 
 #if BUILDFLAG(FULL_SAFE_BROWSING) || defined(OS_CHROMEOS)
-    RunFileUtil,
+  services.Add(RunFileUtil);
 #endif
 
 #if BUILDFLAG(ENABLE_EXTENSIONS) && !defined(OS_WIN)
-    // On Windows, this service runs in an elevated utility process.
-    RunRemovableStorageWriter,
+  // On Windows, this service runs in an elevated utility process.
+  services.Add(RunRemovableStorageWriter);
 #endif
 
 #if BUILDFLAG(ENABLE_EXTENSIONS) || defined(OS_ANDROID)
-    RunMediaParserFactory,
+  services.Add(RunMediaParserFactory);
 #endif
 
 #if BUILDFLAG(ENABLE_PRINT_PREVIEW) || \
     (BUILDFLAG(ENABLE_PRINTING) && defined(OS_WIN))
-    RunPrintingService,
+  services.Add(RunPrintingService);
 #endif
 
 #if BUILDFLAG(ENABLE_PRINTING)
-    RunPrintCompositor,
+  services.Add(RunPrintCompositor);
 #endif
 
 #if BUILDFLAG(ENABLE_PAINT_PREVIEW)
-    RunPaintPreviewCompositor,
+  services.Add(RunPaintPreviewCompositor);
 #endif
 
 #if defined(OS_CHROMEOS)
-    RunImeService,
-    RunSharing,
-    RunTtsService,
+  services.Add(RunImeService);
+  services.Add(RunSharing);
+  services.Add(RunTtsService);
 #if BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
-    RunAssistantAudioDecoder,
+  services.Add(RunAssistantAudioDecoder);
 #endif
 #endif
-  };
-  // clang-format on
-  return factory.get();
 }
 
-mojo::ServiceFactory* GetIOThreadServiceFactory() {
-  // clang-format off
-  static base::NoDestructor<mojo::ServiceFactory> factory {
+void RegisterIOThreadServices(mojo::ServiceFactory& services) {
 #if !defined(OS_ANDROID)
-    RunProxyResolver,
-#endif  // !defined(OS_ANDROID)
-  };
-  // clang-format on
-  return factory.get();
+  services.Add(RunProxyResolver);
+#endif
 }

chrome/utility/services.h

@@ -12,8 +12,8 @@ class ServiceFactory;
 // Helpers to run out-of-process services in a dedicated utility process. All
 // out-of-process services will need to have their implementation hooked up in
 // one of these helpers.
-mojo::ServiceFactory* GetElevatedMainThreadServiceFactory();
-mojo::ServiceFactory* GetMainThreadServiceFactory();
-mojo::ServiceFactory* GetIOThreadServiceFactory();
+void RegisterElevatedMainThreadServices(mojo::ServiceFactory& services);
+void RegisterMainThreadServices(mojo::ServiceFactory& services);
+void RegisterIOThreadServices(mojo::ServiceFactory& services);
 
 #endif  // CHROME_UTILITY_SERVICES_H_

content/browser/service_process_host_browsertest.cc

@@ -2,8 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <string.h>
+
+#include "base/memory/shared_memory_mapping.h"
+#include "base/memory/unsafe_shared_memory_region.h"
 #include "base/optional.h"
 #include "base/run_loop.h"
+#include "base/stl_util.h"
 #include "base/test/bind_test_util.h"
 #include "base/time/time.h"
 #include "base/timer/elapsed_timer.h"
@@ -88,6 +93,36 @@ IN_PROC_BROWSER_TEST_F(ServiceProcessHostBrowserTest, RemoteDisconnectQuits) {
   observer.WaitForDeath();
 }
 
+IN_PROC_BROWSER_TEST_F(ServiceProcessHostBrowserTest, AllMessagesReceived) {
+  // Verifies that messages sent right before disconnection are always received
+  // and dispatched by the service before it self-terminates.
+  EchoServiceProcessObserver observer;
+  auto echo_service = ServiceProcessHost::Launch<echo::mojom::EchoService>();
+
+  const size_t kBufferSize = 256;
+  const std::string kMessages[] = {
+      "I thought we were having steamed clams.",
+      "D'oh, no! I said steamed hams. That's what I call hamburgers.",
+      "You call hamburgers, \"steamed hams?\"",
+      "Yes. It's a regional dialect."};
+  auto region = base::UnsafeSharedMemoryRegion::Create(kBufferSize);
+  base::WritableSharedMemoryMapping mapping = region.Map();
+  memset(mapping.memory(), 0, kBufferSize);
+
+  // Send several messages, since it helps to verify a lack of raciness between
+  // service-side message dispatch and service termination.
+  for (const auto& message : kMessages) {
+    ASSERT_LE(message.size(), kBufferSize);
+    echo_service->EchoStringToSharedMemory(message, region.Duplicate());
+  }
+  echo_service.reset();
+  observer.WaitForDeath();
+
+  const std::string& kLastMessage = kMessages[base::size(kMessages) - 1];
+  EXPECT_EQ(0,
+            memcmp(mapping.memory(), kLastMessage.data(), kLastMessage.size()));
+}
+
 IN_PROC_BROWSER_TEST_F(ServiceProcessHostBrowserTest, ObserveCrash) {
   EchoServiceProcessObserver observer;
   auto echo_service = ServiceProcessHost::Launch<echo::mojom::EchoService>();

content/public/utility/content_utility_client.cc

@@ -16,12 +16,4 @@ bool ContentUtilityClient::HandleServiceRequest(
   return false;
 }
 
-mojo::ServiceFactory* ContentUtilityClient::GetIOThreadServiceFactory() {
-  return nullptr;
-}
-
-mojo::ServiceFactory* ContentUtilityClient::GetMainThreadServiceFactory() {
-  return nullptr;
-}
-
 }  // namespace content

content/public/utility/content_utility_client.h

@@ -63,18 +63,16 @@ class CONTENT_EXPORT ContentUtilityClient {
       mojo::PendingReceiver<service_manager::mojom::Service> receiver);
 
   // Allows the embedder to handle an incoming service interface request to run
-  // a service on the IO thread. Should return a ServiceFactory instance which
-  // lives at least as long as the IO thread, or nullptr.
+  // a service on the IO thread.
   //
   // Only called from the IO thread.
-  virtual mojo::ServiceFactory* GetIOThreadServiceFactory();
+  virtual void RegisterIOThreadServices(mojo::ServiceFactory& services) {}
 
   // Allows the embedder to handle an incoming service interface request to run
-  // a service on the main thread. Should return a ServiceFactory instance which
-  // which effectively lives forever, or nullptr.
+  // a service on the main thread.
   //
   // Only called from the main thread.
-  virtual mojo::ServiceFactory* GetMainThreadServiceFactory();
+  virtual void RegisterMainThreadServices(mojo::ServiceFactory& services) {}
 
   virtual void RegisterNetworkBinders(
       service_manager::BinderRegistry* registry) {}

content/shell/utility/shell_content_utility_client.cc

@@ -172,11 +172,9 @@ bool ShellContentUtilityClient::HandleServiceRequest(
   return false;
 }
 
-mojo::ServiceFactory* ShellContentUtilityClient::GetIOThreadServiceFactory() {
-  static base::NoDestructor<mojo::ServiceFactory> factory{
-      RunEchoService,
-  };
-  return factory.get();
+void ShellContentUtilityClient::RegisterIOThreadServices(
+    mojo::ServiceFactory& services) {
+  services.Add(RunEchoService);
 }
 
 void ShellContentUtilityClient::RegisterNetworkBinders(

content/shell/utility/shell_content_utility_client.h

@@ -23,7 +23,7 @@ class ShellContentUtilityClient : public ContentUtilityClient {
   bool HandleServiceRequest(
       const std::string& service_name,
       mojo::PendingReceiver<service_manager::mojom::Service> receiver) override;
-  mojo::ServiceFactory* GetIOThreadServiceFactory() override;
+  void RegisterIOThreadServices(mojo::ServiceFactory& services) override;
   void RegisterNetworkBinders(
       service_manager::BinderRegistry* registry) override;
 

content/utility/services.cc

@@ -206,69 +206,35 @@ auto RunXrDeviceService(
 }
 #endif
 
-mojo::ServiceFactory& GetIOThreadServiceFactory() {
-  static base::NoDestructor<mojo::ServiceFactory> factory{
-      // The network service runs on the IO thread because it needs a message
-      // loop of type IO that can get notified when pipes have data.
-      RunNetworkService,
-  };
-  return *factory;
-}
+}  // namespace
 
-mojo::ServiceFactory& GetMainThreadServiceFactory() {
-  // clang-format off
-  static base::NoDestructor<mojo::ServiceFactory> factory{
-    RunAudio,
-#if BUILDFLAG(ENABLE_LIBRARY_CDMS)
-    RunCdmService,
-#endif
-    RunDataDecoder,
-    RunStorageService,
-    RunTracing,
-    RunVideoCapture,
-#if BUILDFLAG(ENABLE_VR) && !defined(OS_ANDROID)
-    RunXrDeviceService,
-#endif
-  };
-  // clang-format on
-  return *factory;
+void RegisterIOThreadServices(mojo::ServiceFactory& services) {
+  // The network service runs on the IO thread because it needs a message
+  // loop of type IO that can get notified when pipes have data.
+  services.Add(RunNetworkService);
+
+  // Add new IO-thread services above this line.
+  GetContentClient()->utility()->RegisterIOThreadServices(services);
 }
 
-}  // namespace
+void RegisterMainThreadServices(mojo::ServiceFactory& services) {
+  services.Add(RunAudio);
 
-void HandleServiceRequestOnIOThread(
-    mojo::GenericPendingReceiver receiver,
-    base::SequencedTaskRunner* main_thread_task_runner) {
-  if (GetIOThreadServiceFactory().MaybeRunService(&receiver))
-    return;
-
-  // If the request was handled already, we should not reach this point.
-  DCHECK(receiver.is_valid());
-  auto* embedder_factory =
-      GetContentClient()->utility()->GetIOThreadServiceFactory();
-  if (embedder_factory && embedder_factory->MaybeRunService(&receiver))
-    return;
-
-  DCHECK(receiver.is_valid());
-  main_thread_task_runner->PostTask(
-      FROM_HERE,
-      base::BindOnce(&HandleServiceRequestOnMainThread, std::move(receiver)));
-}
+  services.Add(RunDataDecoder);
+  services.Add(RunStorageService);
+  services.Add(RunTracing);
+  services.Add(RunVideoCapture);
 
-void HandleServiceRequestOnMainThread(mojo::GenericPendingReceiver receiver) {
-  if (GetMainThreadServiceFactory().MaybeRunService(&receiver))
-    return;
+#if BUILDFLAG(ENABLE_LIBRARY_CDMS)
+  services.Add(RunCdmService);
+#endif
 
-  // If the request was handled already, we should not reach this point.
-  DCHECK(receiver.is_valid());
-  auto* embedder_factory =
-      GetContentClient()->utility()->GetMainThreadServiceFactory();
-  if (embedder_factory && embedder_factory->MaybeRunService(&receiver))
-    return;
+#if BUILDFLAG(ENABLE_VR) && !defined(OS_ANDROID)
+  services.Add(RunXrDeviceService);
+#endif
 
-  DCHECK(receiver.is_valid());
-  DLOG(ERROR) << "Unhandled out-of-process service request for "
-              << receiver.interface_name().value();
+  // Add new main-thread services above this line.
+  GetContentClient()->utility()->RegisterMainThreadServices(services);
 }
 
 }  // namespace content

content/utility/services.h

@@ -5,17 +5,14 @@
 #ifndef CONTENT_UTILITY_SERVICES_H_
 #define CONTENT_UTILITY_SERVICES_H_
 
-#include "base/memory/scoped_refptr.h"
-#include "base/sequenced_task_runner.h"
-#include "mojo/public/cpp/bindings/generic_pending_receiver.h"
+namespace mojo {
+class ServiceFactory;
+}
 
 namespace content {
 
-void HandleServiceRequestOnIOThread(
-    mojo::GenericPendingReceiver receiver,
-    base::SequencedTaskRunner* main_thread_task_runner);
-
-void HandleServiceRequestOnMainThread(mojo::GenericPendingReceiver receiver);
+void RegisterIOThreadServices(mojo::ServiceFactory& services);
+void RegisterMainThreadServices(mojo::ServiceFactory& services);
 
 }  // namespace content