Reland "Start ServiceManger before creating BrowserMainLoop."

This relands commit f481306. The CL
got reverted because BrowserMainLoopTest.CreateThreadsInSingleProcess
is falling on Windows. In this CL, we remove the call of
BrowserMainLoop#InitilaizeMojo() which isn't necessary for the test.

Beside, also re-enable two tests which were disabled when the reverting
CL landed:
 * RenderThreadImplBrowserTest.NonResourceDispatchIPCTasksDontGoThroughScheduler

The original cl description is:
This CL instantiates the ServiceManagerContext before creating
the BrowserMainRunner. It splits the startup path into two,
with/without starting the full browser. The changes are implemented
behind a flag "allow-start-service-manager-only".

Bug: 846846,902311
Change-Id: I6e3f6518e414e1298e57b55bd188879461d8f342
Reviewed-on: https://chromium-review.googlesource.com/c/1327413
Commit-Queue: Xi Han <[email protected]>
Reviewed-by: Gabriel Charette <[email protected]>
Reviewed-by: John Abd-El-Malek <[email protected]>
Cr-Commit-Position: refs/heads/master@{#611340}

content/app/DEPS

@@ -1,4 +1,5 @@
 include_rules = [
+  "+components/download",
   "+components/tracing",
   "+content",
   "+device/bluetooth",

content/app/content_main_runner_impl.cc

@@ -39,6 +39,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/trace_event/trace_event.h"
+#include "components/download/public/common/download_task_runner.h"
 #include "components/tracing/common/trace_startup.h"
 #include "content/app/mojo/mojo_init.h"
 #include "content/browser/browser_process_sub_thread.h"
@@ -153,6 +154,10 @@
 #include "services/service_manager/zygote/host/zygote_host_impl_linux.h"
 #endif
 
+#if defined(OS_ANDROID)
+#include "content/browser/android/browser_startup_controller.h"
+#endif
+
 namespace content {
 extern int GpuMain(const content::MainFunctionParams&);
 #if BUILDFLAG(ENABLE_PLUGINS)
@@ -167,6 +172,11 @@ namespace content {
 
 namespace {
 
+#if !defined(CHROME_MULTIPLE_DLL_CHILD)
+const char kAllowStartingServiceManagerOnly[] =
+    "allow-start-service-manager-only";
+#endif
+
 #if defined(V8_USE_EXTERNAL_STARTUP_DATA) && defined(OS_ANDROID)
 #if defined __LP64__
 #define kV8SnapshotDataDescriptor kV8Snapshot64DataDescriptor
@@ -857,20 +867,26 @@ int ContentMainRunnerImpl::Run(bool start_service_manager_only) {
   RegisterMainThreadFactories();
 
 #if !defined(CHROME_MULTIPLE_DLL_CHILD)
-  // The thread used to start the ServiceManager is handed-off to
-  // BrowserMain() which may elect to promote it (e.g. to BrowserThread::IO).
-  if (process_type.empty()) {
-    startup_data_ = std::make_unique<StartupDataImpl>();
+  if (process_type.empty())
+    return RunServiceManager(main_params, start_service_manager_only);
+#endif  // !defined(CHROME_MULTIPLE_DLL_CHILD)
+
+  return RunOtherNamedProcessTypeMain(process_type, main_params, delegate_);
+}
 
+#if !defined(CHROME_MULTIPLE_DLL_CHILD)
+int ContentMainRunnerImpl::RunServiceManager(MainFunctionParams& main_params,
+                                             bool start_service_manager_only) {
+  if (is_browser_main_loop_started_)
+    return -1;
+
+  if (!service_manager_context_) {
     if (delegate_->ShouldCreateFeatureList()) {
       DCHECK(!field_trial_list_);
       field_trial_list_ = SetUpFieldTrialsAndFeatureList();
       delegate_->PostFieldTrialInitialization();
     }
 
-    startup_data_->thread = BrowserProcessSubThread::CreateIOThread();
-    main_params.startup_data = startup_data_.get();
-
     if (GetContentClient()->browser()->ShouldCreateTaskScheduler()) {
       // Create and start the TaskScheduler early to allow upcoming code to use
       // the post_task.h API.
@@ -904,12 +920,32 @@ int ContentMainRunnerImpl::Run(bool start_service_manager_only) {
     // incorrect to post to a BrowserThread before this point.
     BrowserTaskExecutor::Create();
 
-    return RunBrowserProcessMain(main_params, delegate_);
+    // The thread used to start the ServiceManager is handed-off to
+    // BrowserMain() which may elect to promote it (e.g. to BrowserThread::IO).
+    service_manager_thread_ = BrowserProcessSubThread::CreateIOThread();
+    service_manager_context_.reset(
+        new ServiceManagerContext(service_manager_thread_->task_runner()));
+    download::SetIOTaskRunner(service_manager_thread_->task_runner());
+#if defined(OS_ANDROID)
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(&ServiceManagerStartupComplete));
+#endif
   }
-#endif  // !defined(CHROME_MULTIPLE_DLL_CHILD)
 
-  return RunOtherNamedProcessTypeMain(process_type, main_params, delegate_);
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          kAllowStartingServiceManagerOnly) &&
+      start_service_manager_only) {
+    return -1;
+  }
+
+  is_browser_main_loop_started_ = true;
+  startup_data_ = std::make_unique<StartupDataImpl>();
+  startup_data_->thread = std::move(service_manager_thread_);
+  startup_data_->service_manager_context = service_manager_context_.get();
+  main_params.startup_data = startup_data_.get();
+  return RunBrowserProcessMain(main_params, delegate_);
 }
+#endif  // !defined(CHROME_MULTIPLE_DLL_CHILD)
 
 void ContentMainRunnerImpl::Shutdown() {
   DCHECK(is_initialized_);

content/app/content_main_runner_impl.h

@@ -12,10 +12,12 @@
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/field_trial.h"
 #include "build/build_config.h"
+#include "content/browser/service_manager/service_manager_context.h"
 #include "content/browser/startup_data_impl.h"
 #include "content/public/app/content_main.h"
 #include "content/public/app/content_main_runner.h"
 #include "content/public/common/content_client.h"
+#include "content/public/common/main_function_params.h"
 
 #if defined(OS_WIN)
 #include "sandbox/win/src/sandbox_types.h"
@@ -46,6 +48,20 @@ class ContentMainRunnerImpl : public ContentMainRunner {
   void Shutdown() override;
 
  private:
+#if !defined(CHROME_MULTIPLE_DLL_CHILD)
+  int RunServiceManager(MainFunctionParams& main_function_params,
+                        bool start_service_manager_only);
+
+  bool is_browser_main_loop_started_ = false;
+
+  std::unique_ptr<base::MessageLoop> main_message_loop_;
+
+  std::unique_ptr<StartupDataImpl> startup_data_;
+  std::unique_ptr<base::FieldTrialList> field_trial_list_;
+  std::unique_ptr<BrowserProcessSubThread> service_manager_thread_;
+  std::unique_ptr<ServiceManagerContext> service_manager_context_;
+#endif  // !defined(CHROME_MULTIPLE_DLL_CHILD)
+
   // True if the runner has been initialized.
   bool is_initialized_ = false;
 
@@ -73,14 +89,6 @@ class ContentMainRunnerImpl : public ContentMainRunner {
 
   CreatedMainPartsClosure* created_main_parts_closure_ = nullptr;
 
-#if !defined(CHROME_MULTIPLE_DLL_CHILD)
-  std::unique_ptr<base::MessageLoop> main_message_loop_;
-
-  std::unique_ptr<StartupDataImpl> startup_data_;
-
-  std::unique_ptr<base::FieldTrialList> field_trial_list_;
-#endif  // !defined(CHROME_MULTIPLE_DLL_CHILD)
-
   DISALLOW_COPY_AND_ASSIGN(ContentMainRunnerImpl);
 };
 

content/browser/browser_main_loop.cc

@@ -561,6 +561,7 @@ void BrowserMainLoop::Init() {
     // This is always invoked before |io_thread_| is initialized (i.e. never
     // resets it).
     io_thread_ = std::move(startup_data->thread);
+    service_manager_context_ = startup_data->service_manager_context;
   }
 
   parts_.reset(
@@ -1081,7 +1082,9 @@ void BrowserMainLoop::ShutdownThreadsAndCleanUp() {
     BrowserGpuChannelHostFactory::instance()->CloseChannel();
 
   // Shutdown the Service Manager and IPC.
-  service_manager_context_.reset();
+  if (service_manager_context_)
+    service_manager_context_->ShutDown();
+  owned_service_manager_context_.reset();
   mojo_ipc_support_.reset();
 
   if (save_file_manager_)
@@ -1535,9 +1538,13 @@ void BrowserMainLoop::InitializeMojo() {
       base::CreateSingleThreadTaskRunnerWithTraits({BrowserThread::IO}),
       mojo::core::ScopedIPCSupport::ShutdownPolicy::FAST));
 
-  service_manager_context_.reset(
-      new ServiceManagerContext(io_thread_->task_runner()));
+  if (!service_manager_context_) {
+    owned_service_manager_context_ =
+        std::make_unique<ServiceManagerContext>(io_thread_->task_runner());
+    service_manager_context_ = owned_service_manager_context_.get();
+  }
   ServiceManagerContext::StartBrowserConnection();
+
 #if defined(OS_MACOSX)
   mojo::core::SetMachPortProvider(MachBroker::GetInstance());
 #endif  // defined(OS_MACOSX)

content/browser/browser_main_loop.h

@@ -354,8 +354,10 @@ class CONTENT_EXPORT BrowserMainLoop {
       gpu_data_manager_visual_proxy_;
 #endif
 
+  ServiceManagerContext* service_manager_context_ = nullptr;
+  std::unique_ptr<ServiceManagerContext> owned_service_manager_context_;
+
   // Members initialized in |BrowserThreadsStarted()| --------------------------
-  std::unique_ptr<ServiceManagerContext> service_manager_context_;
   std::unique_ptr<mojo::core::ScopedIPCSupport> mojo_ipc_support_;
 
   // |user_input_monitor_| has to outlive |audio_manager_|, so declared first.

content/browser/service_manager/service_manager_context.cc

@@ -827,6 +827,10 @@ ServiceManagerContext::ServiceManagerContext(
 }
 
 ServiceManagerContext::~ServiceManagerContext() {
+  ShutDown();
+}
+
+void ServiceManagerContext::ShutDown() {
   // NOTE: The in-process ServiceManager MUST be destroyed before the browser
   // process-wide ServiceManagerConnection. Otherwise it's possible for the
   // ServiceManager to receive connection requests for service:content_browser
@@ -837,6 +841,7 @@ ServiceManagerContext::~ServiceManagerContext() {
     ServiceManagerConnection::DestroyForProcess();
   service_manager_thread_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&DestroyConnectorOnIOThread));
+  packaged_services_connection_.reset();
 }
 
 // static
@@ -860,22 +865,12 @@ void ServiceManagerContext::StartBrowserConnection() {
   RegisterCommonBrowserInterfaces(browser_connection);
   browser_connection->Start();
 
-  bool network_service_enabled =
-      base::FeatureList::IsEnabled(network::features::kNetworkService);
-  bool network_service_in_process =
-      base::FeatureList::IsEnabled(features::kNetworkServiceInProcess) ||
-      base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kSingleProcess);
-  if (!network_service_enabled) {
-    // Create the in-process NetworkService object so that its getter is
-    // available on the IO thread.
-    GetNetworkService();
-  } else if (!network_service_in_process) {
-    // Start the network service process as soon as possible, since it is
-    // critical to start up performance.
-    browser_connection->GetConnector()->WarmService(
-        service_manager::ServiceFilter::ByName(mojom::kNetworkServiceName));
-  }
+  if (base::FeatureList::IsEnabled(network::features::kNetworkService))
+    return;
+
+  // Create the in-process NetworkService object so that its getter is
+  // available on the IO thread.
+  GetNetworkService();
 }
 
 // static

content/browser/service_manager/service_manager_context.h

@@ -51,6 +51,9 @@ class CONTENT_EXPORT ServiceManagerContext {
 
   static base::DeferredSequencedTaskRunner* GetAudioServiceRunner();
 
+  // Shutdowns the ServiceManager and the connections to the ServiceManager.
+  void ShutDown();
+
  private:
   class InProcessServiceManagerContext;
 

content/browser/startup_data_impl.h

@@ -12,13 +12,15 @@
 
 namespace content {
 
+class ServiceManagerContext;
+
 // The browser implementation of StartupData.
 struct StartupDataImpl : public StartupData {
   StartupDataImpl();
   ~StartupDataImpl() override;
 
-  // TODO(hanxi): add ServiceManagerContext* here.
   std::unique_ptr<BrowserProcessSubThread> thread;
+  ServiceManagerContext* service_manager_context;
 };
 
 }  // namespace content

content/renderer/render_thread_impl_browsertest.cc

@@ -26,7 +26,6 @@
 #include "base/test/scoped_feature_list.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread_task_runner_handle.h"
-#include "build/build_config.h"
 #include "content/app/mojo/mojo_init.h"
 #include "content/common/in_process_child_thread_params.h"
 #include "content/common/service_manager/child_connection.h"
@@ -300,19 +299,9 @@ class RenderThreadImplBrowserTest : public testing::Test {
   InputHandlerManagerDestroyedAfterCompositorThread
 #endif
 
-// TODO(crbug.com/902311): We should not disable
-// |NonResourceDispatchIPCTasksDontGoThroughScheduler| for Windows.
-#if defined(OS_WIN) && !defined(LEAK_SANITIZER)
-#define MAYBE_NonResourceDispatchIPCTasksDontGoThroughScheduler \
-  DISABLED_NonResourceDispatchIPCTasksDontGoThroughScheduler
-#else
-#define MAYBE_NonResourceDispatchIPCTasksDontGoThroughScheduler \
-  NonResourceDispatchIPCTasksDontGoThroughScheduler
-#endif
-
 // Disabled under LeakSanitizer due to memory leaks.
 TEST_F(RenderThreadImplBrowserTest,
-       WILL_LEAK(MAYBE_NonResourceDispatchIPCTasksDontGoThroughScheduler)) {
+       WILL_LEAK(NonResourceDispatchIPCTasksDontGoThroughScheduler)) {
   // NOTE other than not being a resource message, the actual message is
   // unimportant.