Remove some unnecessary Service Manager APIs

ContentBrowserClient::WillStartServiceManager exists exclusively to
initialize some Audio Service policy state very early in browser
lifetime. This was necessary at one point because the Service Manager
was a prerequisite for various service bits to be configured, and
Service Manager bringup happens very early in startup.

Because the Service Manager is no longer used to host the Audio Service,
the service is free to manage its own configuration independently. It
can instead now query the sandboxing policy on-demand when launching the
service process, eliminating any need for a ContentBrowserClient API.

This also deletes the unused ShouldTerminateOnServiceQuit
ContentBrowserClient API, as well as its supporting code within Service
Manager.

Bug: 977637
Change-Id: I3fa148cf4ceb9bb26f56e2b736d20a6cce2170dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2538333
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827551}

chrome/browser/chrome_content_browser_client.cc

@@ -3842,28 +3842,6 @@ bool ChromeContentBrowserClient::IsRendererCodeIntegrityEnabled() {
 
 #endif  // defined(OS_WIN)
 
-void ChromeContentBrowserClient::WillStartServiceManager() {
-#if defined(OS_WIN) || defined(OS_MAC) || \
-    (defined(OS_LINUX) && !defined(OS_CHROMEOS))
-  auto* chrome_feature_list_creator =
-      startup_data_.chrome_feature_list_creator();
-  // This has to run very early before ServiceManagerContext is created.
-  const policy::PolicyMap& policies =
-      chrome_feature_list_creator->browser_policy_connector()
-          ->GetPolicyService()
-          ->GetPolicies(policy::PolicyNamespace(policy::POLICY_DOMAIN_CHROME,
-                                                std::string()));
-  const base::Value* audio_sandbox_enabled_policy_value =
-      policies.GetValue(policy::key::kAudioSandboxEnabled);
-  if (audio_sandbox_enabled_policy_value) {
-    bool force_enable_audio_sandbox;
-    audio_sandbox_enabled_policy_value->GetAsBoolean(
-        &force_enable_audio_sandbox);
-    SetForceAudioServiceSandboxed(force_enable_audio_sandbox);
-  }
-#endif
-}
-
 void ChromeContentBrowserClient::OpenURL(
     content::SiteInstance* site_instance,
     const content::OpenURLParams& params,

chrome/browser/chrome_content_browser_client.h

@@ -429,7 +429,6 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
   void BindHostReceiverForRenderer(
       content::RenderProcessHost* render_process_host,
       mojo::GenericPendingReceiver receiver) override;
-  void WillStartServiceManager() override;
   void OpenURL(
       content::SiteInstance* site_instance,
       const content::OpenURLParams& params,

chrome/browser/media/audio_service_util.cc

@@ -4,22 +4,38 @@
 
 #include "chrome/browser/media/audio_service_util.h"
 
+#include <string>
+
 #include "base/feature_list.h"
+#include "base/optional.h"
+#include "base/values.h"
+#include "build/build_config.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/policy/chrome_browser_policy_connector.h"
+#include "components/policy/core/common/policy_map.h"
+#include "components/policy/core/common/policy_namespace.h"
+#include "components/policy/core/common/policy_service.h"
+#include "components/policy/policy_constants.h"
 #include "content/public/common/content_features.h"
 
-static base::Optional<bool> g_force_audio_service_sandbox;
-
 bool IsAudioServiceSandboxEnabled() {
-  return g_force_audio_service_sandbox.value_or(
+  base::Optional<bool> force_enable_audio_sandbox;
+#if defined(OS_WIN) || defined(OS_MAC) || \
+    (defined(OS_LINUX) && !defined(OS_CHROMEOS))
+  const policy::PolicyMap& policies =
+      g_browser_process->browser_policy_connector()
+          ->GetPolicyService()
+          ->GetPolicies(policy::PolicyNamespace(policy::POLICY_DOMAIN_CHROME,
+                                                std::string()));
+  const base::Value* audio_sandbox_enabled_policy_value =
+      policies.GetValue(policy::key::kAudioSandboxEnabled);
+  if (audio_sandbox_enabled_policy_value) {
+    force_enable_audio_sandbox.emplace();
+    audio_sandbox_enabled_policy_value->GetAsBoolean(
+        &force_enable_audio_sandbox.value());
+  }
+#endif
+  return force_enable_audio_sandbox.value_or(
       base::FeatureList::IsEnabled(features::kAudioServiceSandbox));
 }
 
-// If |force_audio_service_sandbox| is:
-// * null: use the default audio-service sandbox configuration (varies per
-//         platform)
-// * true: enable the audio-service sandbox, regardless of default settings.
-// * false: disable the audio-service sandbox, regardless of default settings.
-void SetForceAudioServiceSandboxed(
-    const base::Optional<bool>& force_audio_service_sandbox) {
-  g_force_audio_service_sandbox = force_audio_service_sandbox;
-}

chrome/browser/media/audio_service_util.h

@@ -5,16 +5,6 @@
 #ifndef CHROME_BROWSER_MEDIA_AUDIO_SERVICE_UTIL_H_
 #define CHROME_BROWSER_MEDIA_AUDIO_SERVICE_UTIL_H_
 
-#include "base/optional.h"
-
 bool IsAudioServiceSandboxEnabled();
 
-// If |force_audio_service_sandbox| is:
-// * null: use the default audio-service sandbox configuration (varies per
-//         platform)
-// * true: enable the audio-service sandbox, regardless of default settings.
-// * false: disable the audio-service sandbox, regardless of default settings.
-void SetForceAudioServiceSandboxed(
-    const base::Optional<bool>& force_audio_service_sandbox);
-
 #endif  // CHROME_BROWSER_MEDIA_AUDIO_SERVICE_UTIL_H_

content/browser/service_manager/service_manager_context.cc

@@ -289,28 +289,6 @@ class ServiceManagerContext::InProcessServiceManagerContext
                                   base::Token{}, base::Token::CreateRandom()),
         std::move(system_remote), metadata.BindNewPipeAndPassReceiver());
     metadata->SetPID(base::GetCurrentProcId());
-
-    service_manager_->SetInstanceQuitCallback(
-        base::BindOnce(&OnInstanceQuitOnServiceManagerThread,
-                       std::move(ui_thread_task_runner)));
-  }
-
-  static void OnInstanceQuitOnServiceManagerThread(
-      scoped_refptr<base::SequencedTaskRunner> ui_thread_task_runner,
-      const service_manager::Identity& id) {
-    ui_thread_task_runner->PostTask(FROM_HERE,
-                                    base::BindOnce(&OnInstanceQuit, id));
-  }
-
-  static void OnInstanceQuit(const service_manager::Identity& id) {
-    if (GetContentClient()->browser()->ShouldTerminateOnServiceQuit(id)) {
-      // Don't LOG(FATAL) because we don't want a browser crash report.
-      LOG(ERROR) << "Terminating because service '" << id.name()
-                 << "' quit unexpectedly.";
-      // Skip shutdown to reduce the risk that other code in the browser will
-      // respond to the service pipe closing.
-      exit(1);
-    }
   }
 
   void ShutDownOnServiceManagerThread() {
@@ -370,8 +348,6 @@ ServiceManagerContext::ServiceManagerContext(
   // GetConnectorForIOThread().
   g_io_thread_connector.Get() = system_connection->GetConnector()->Clone();
 
-  GetContentClient()->browser()->WillStartServiceManager();
-
   in_process_context_->Start(
       manifests, std::move(system_remote),
       base::BindRepeating(&ServiceManagerContext::RunServiceInstance,

content/public/browser/content_browser_client.cc

@@ -721,11 +721,6 @@ void ContentBrowserClient::RunServiceInstance(
     const service_manager::Identity& identity,
     mojo::PendingReceiver<service_manager::mojom::Service>* receiver) {}
 
-bool ContentBrowserClient::ShouldTerminateOnServiceQuit(
-    const service_manager::Identity& id) {
-  return false;
-}
-
 base::Optional<service_manager::Manifest>
 ContentBrowserClient::GetServiceManifestOverlay(base::StringPiece name) {
   return base::nullopt;

content/public/browser/content_browser_client.h

@@ -1086,9 +1086,6 @@ class CONTENT_EXPORT ContentBrowserClient {
       RenderProcessHost* render_process_host,
       mojo::GenericPendingReceiver receiver) {}
 
-  // Called just before the Service Manager is initialized.
-  virtual void WillStartServiceManager() {}
-
   // Handles a service instance request for a new service instance with identity
   // |identity|. If the client knows how to run the named service, it should
   // bind |*receiver| accordingly, in the browser process.
@@ -1101,11 +1098,6 @@ class CONTENT_EXPORT ContentBrowserClient {
       const service_manager::Identity& identity,
       mojo::PendingReceiver<service_manager::mojom::Service>* receiver);
 
-  // Allows the embedder to terminate the browser if a specific service instance
-  // quits or crashes.
-  virtual bool ShouldTerminateOnServiceQuit(
-      const service_manager::Identity& id);
-
   // Allows the embedder to amend service manifests for existing services.
   // Specifically, the sets of exposed and required capabilities, interface
   // filter capabilities (deprecated), packaged services, and preloaded files

content/shell/browser/shell_content_browser_client.cc

@@ -200,13 +200,6 @@ bool ShellContentBrowserClient::IsHandledURL(const GURL& url) {
   return false;
 }
 
-bool ShellContentBrowserClient::ShouldTerminateOnServiceQuit(
-    const service_manager::Identity& id) {
-  if (should_terminate_on_service_quit_callback_)
-    return std::move(should_terminate_on_service_quit_callback_).Run(id);
-  return false;
-}
-
 void ShellContentBrowserClient::AppendExtraCommandLineSwitches(
     base::CommandLine* command_line,
     int child_process_id) {

content/shell/browser/shell_content_browser_client.h

@@ -43,8 +43,6 @@ class ShellContentBrowserClient : public ContentBrowserClient {
   std::unique_ptr<BrowserMainParts> CreateBrowserMainParts(
       const MainFunctionParams& parameters) override;
   bool IsHandledURL(const GURL& url) override;
-  bool ShouldTerminateOnServiceQuit(
-      const service_manager::Identity& id) override;
   void AppendExtraCommandLineSwitches(base::CommandLine* command_line,
                                       int child_process_id) override;
   std::string GetAcceptLangs(BrowserContext* context) override;
@@ -129,10 +127,6 @@ class ShellContentBrowserClient : public ContentBrowserClient {
     select_client_certificate_callback_ =
         std::move(select_client_certificate_callback);
   }
-  void set_should_terminate_on_service_quit_callback(
-      base::OnceCallback<bool(const service_manager::Identity&)> callback) {
-    should_terminate_on_service_quit_callback_ = std::move(callback);
-  }
   void set_login_request_callback(
       base::OnceCallback<void(bool is_main_frame)> login_request_callback) {
     login_request_callback_ = std::move(login_request_callback);
@@ -183,8 +177,6 @@ class ShellContentBrowserClient : public ContentBrowserClient {
   static bool allow_any_cors_exempt_header_for_browser_;
 
   base::OnceClosure select_client_certificate_callback_;
-  base::OnceCallback<bool(const service_manager::Identity&)>
-      should_terminate_on_service_quit_callback_;
   base::OnceCallback<void(bool is_main_frame)> login_request_callback_;
   base::RepeatingCallback<void(const network::mojom::URLLoaderFactoryParams*,
                                const url::Origin&,

services/service_manager/service_manager.cc

@@ -190,11 +190,6 @@ ServiceManager::~ServiceManager() {
   instances_.clear();
 }
 
-void ServiceManager::SetInstanceQuitCallback(
-    base::OnceCallback<void(const Identity&)> callback) {
-  instance_quit_callback_ = std::move(callback);
-}
-
 ServiceInstance* ServiceManager::FindOrCreateMatchingTargetInstance(
     const ServiceInstance& source_instance,
     const ServiceFilter& partial_target_filter) {
@@ -404,9 +399,6 @@ void ServiceManager::OnInstanceStopped(const Identity& identity) {
   for (auto& listener : listeners_) {
     listener->OnServiceStopped(identity);
   }
-
-  if (!instance_quit_callback_.is_null())
-    std::move(instance_quit_callback_).Run(identity);
 }
 
 ServiceInstance* ServiceManager::GetExistingInstance(

services/service_manager/service_manager.h

@@ -112,12 +112,6 @@ class ServiceManager : public Service {
 
   ~ServiceManager() override;
 
-  // Provide a callback to be notified whenever an instance is destroyed.
-  // Typically the creator of the Service Manager will use this to determine
-  // when some set of services it created are destroyed, so it can shut down.
-  void SetInstanceQuitCallback(
-      base::OnceCallback<void(const Identity&)> callback);
-
   // Directly requests that the Service Manager start a new instance for
   // |service_name| if one is not already running.
   //
@@ -212,7 +206,6 @@ class ServiceManager : public Service {
   ServiceInstance* service_manager_instance_;
 
   mojo::RemoteSet<mojom::ServiceManagerListener> listeners_;
-  base::OnceCallback<void(const Identity&)> instance_quit_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(ServiceManager);
 };