[Servicification] Create BrowserThread::IO earlier.

In this CL, we split the creation of the BrowserThread::IO thread and
initialization of its delegate:

1. The BrowserThread::IO is created in the
   BrowserMainLoop::PostMainMessageLoopStart() after the UI thread
   is created. Adjust the tracing events in the BrowserMainLoop.
2. Simplify the logic in BrowserMainLoop::CreateThreads() since only
   the launcher thread is created there. Initializes the
   delegate of the BrowserThread::IO there.
3. Update the logic in the BrowserThreadImpl when checking whether
   the BrowserThread::IO has initialized.
4. Update BrowserMainLoopTest and BrowserProcessImplTest.

 This is the first step of starting ServiceManager earlier.

Bug: 729596
Change-Id: I7a08fd7c7bf5548f8039c8c38152ecfde3bd20c4
Reviewed-on: https://chromium-review.googlesource.com/905424
Commit-Queue: Xi Han <hanxi@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537318}

chrome/browser/browser_process_impl_unittest.cc

@@ -49,16 +49,22 @@ class BrowserProcessImplTest : public ::testing::Test {
     g_browser_process = stashed_browser_process_;
   }
 
-  // Creates the secondary threads (all threads except the UI thread).
+  // Creates the secondary thread (IO thread).
   // The UI thread needs to be alive while BrowserProcessImpl is alive, and is
   // managed separately.
   void StartSecondaryThreads() {
     base::TaskScheduler::GetInstance()->StartWithDefaultParams();
+    io_thread_->StartIOThread();
+  }
+
+  // Initializes the IO thread delegate and starts the ServiceManager.
+  void Initialize() {
+    io_thread_->InitIOThreadDelegate();
+
     // TestServiceManagerContext creation requires the task scheduler to be
     // started.
     service_manager_context_ =
         std::make_unique<content::TestServiceManagerContext>();
-    io_thread_->StartIOThread();
   }
 
   // Destroys the secondary threads (all threads except the UI thread).
@@ -109,8 +115,11 @@ class BrowserProcessImplTest : public ::testing::Test {
 TEST_F(BrowserProcessImplTest, MAYBE_LifeCycle) {
   // Setup the BrowserProcessImpl and the threads.
   browser_process_impl()->Init();
-  browser_process_impl()->PreCreateThreads(*command_line());
+  // A lightweigh IO Thread is created before the
+  // BrowserThreadImpl::PreCreateThreads is called. https://crbug.com/729596.
   StartSecondaryThreads();
+  browser_process_impl()->PreCreateThreads(*command_line());
+  Initialize();
   browser_process_impl()->PreMainMessageLoopRun();
 
   // Force the creation of the NTPResourceCache, to test the destruction order.

content/browser/browser_main_loop.cc

@@ -738,6 +738,11 @@ void BrowserMainLoop::MainMessageLoopStart() {
 }
 
 void BrowserMainLoop::PostMainMessageLoopStart() {
+  {
+    TRACE_EVENT0("startup",
+                 "BrowserMainLoop::Subsystem:CreateBrowserThread::IO");
+    InitializeIOThread();
+  }
   {
     TRACE_EVENT0("startup", "BrowserMainLoop::Subsystem:SystemMonitor");
     system_monitor_.reset(new base::SystemMonitor);
@@ -1034,89 +1039,44 @@ int BrowserMainLoop::CreateThreads() {
 
   base::SequencedWorkerPool::EnableWithRedirectionToTaskSchedulerForProcess();
 
-  base::Thread::Options io_message_loop_options;
-  io_message_loop_options.message_loop_type = base::MessageLoop::TYPE_IO;
-
-  // Start threads in the order they occur in the BrowserThread::ID enumeration,
-  // except for BrowserThread::UI which is the main thread.
-  //
-  // Must be size_t so we can increment it.
-  for (size_t thread_id = BrowserThread::UI + 1;
-       thread_id < BrowserThread::ID_COUNT;
-       ++thread_id) {
-    // If this thread ID is backed by a real thread, |thread_to_start| will be
-    // set to the appropriate BrowserProcessSubThread*. And |options| can be
-    // updated away from its default.
-    std::unique_ptr<BrowserProcessSubThread>* thread_to_start = nullptr;
-    base::Thread::Options options;
-    // If |message_loop| is not nullptr, then this BrowserThread will use this
-    // message loop instead of creating a new thread. Note that means this
-    // thread will not be joined on shutdown, and may cause use-after-free if
-    // anything tries to access objects deleted by AtExitManager, such as
-    // non-leaky LazyInstance.
-    base::MessageLoop* message_loop = nullptr;
-
-    // Otherwise this thread ID will be backed by a SingleThreadTaskRunner using
-    // |task_traits| (to be set below instead of |thread_to_start|).
-    base::TaskTraits task_traits;
-
-    switch (thread_id) {
-      case BrowserThread::PROCESS_LAUNCHER:
-        TRACE_EVENT_BEGIN1("startup",
-            "BrowserMainLoop::CreateThreads:start",
-            "Thread", "BrowserThread::PROCESS_LAUNCHER");
+  TRACE_EVENT_BEGIN1("startup", "BrowserMainLoop::CreateThreads:start",
+                     "Thread", "BrowserThread::PROCESS_LAUNCHER");
+
 #if defined(OS_ANDROID)
-        // Android specializes Launcher thread so it is accessible in java.
-        // Note Android never does clean shutdown, so shutdown use-after-free
-        // concerns are not a problem in practice.
-        message_loop = android::LauncherThread::GetMessageLoop();
-        DCHECK(message_loop);
-        thread_to_start = &process_launcher_thread_;
+  // Android specializes Launcher thread so it is accessible in java.
+  // Note Android never does clean shutdown, so shutdown use-after-free
+  // concerns are not a problem in practice.
+  base::MessageLoop* message_loop = android::LauncherThread::GetMessageLoop();
+  DCHECK(message_loop);
+  // This BrowserThread will use this message loop instead of creating a new
+  // thread. Note that means this/ thread will not be joined on shutdown, and
+  // may cause use-after-free if anything tries to access objects deleted by
+  // AtExitManager, such as non-leaky LazyInstance.
+  process_launcher_thread_.reset(new BrowserProcessSubThread(
+      BrowserThread::PROCESS_LAUNCHER, message_loop));
 #else   // defined(OS_ANDROID)
-        // TODO(gab): WithBaseSyncPrimitives() is likely not required here.
-        task_traits = {base::MayBlock(), base::WithBaseSyncPrimitives(),
-                       base::TaskPriority::USER_BLOCKING,
-                       base::TaskShutdownBehavior::BLOCK_SHUTDOWN};
+  // This thread ID will be backed by a SingleThreadTaskRunner using
+  // |task_traits|.
+  // TODO(gab): WithBaseSyncPrimitives() is likely not required here.
+  base::TaskTraits task_traits = {base::MayBlock(),
+                                  base::WithBaseSyncPrimitives(),
+                                  base::TaskPriority::USER_BLOCKING,
+                                  base::TaskShutdownBehavior::BLOCK_SHUTDOWN};
+  scoped_refptr<base::SingleThreadTaskRunner> redirection_task_runner =
+      base::CreateSingleThreadTaskRunnerWithTraits(
+          task_traits, base::SingleThreadTaskRunnerThreadMode::DEDICATED);
+  DCHECK(redirection_task_runner);
+  BrowserThreadImpl::RedirectThreadIDToTaskRunner(
+      BrowserThread::PROCESS_LAUNCHER, std::move(redirection_task_runner));
 #endif  // defined(OS_ANDROID)
-        break;
-      case BrowserThread::IO:
-        TRACE_EVENT_BEGIN1("startup",
-            "BrowserMainLoop::CreateThreads:start",
-            "Thread", "BrowserThread::IO");
-        thread_to_start = &io_thread_;
-        options = io_message_loop_options;
-#if defined(OS_ANDROID) || defined(OS_CHROMEOS)
-        // Up the priority of the |io_thread_| as some of its IPCs relate to
-        // display tasks.
-        options.priority = base::ThreadPriority::DISPLAY;
-#endif
-        break;
-      case BrowserThread::UI:        // Falls through.
-      case BrowserThread::ID_COUNT:  // Falls through.
-        NOTREACHED();
-        break;
-    }
 
-    BrowserThread::ID id = static_cast<BrowserThread::ID>(thread_id);
-
-    if (thread_to_start) {
-      (*thread_to_start)
-          .reset(message_loop ? new BrowserProcessSubThread(id, message_loop)
-                              : new BrowserProcessSubThread(id));
-      // Start the thread if an existing |message_loop| wasn't provided.
-      if (!message_loop && !(*thread_to_start)->StartWithOptions(options))
-        LOG(FATAL) << "Failed to start the browser thread: id == " << id;
-    } else {
-      scoped_refptr<base::SingleThreadTaskRunner> redirection_task_runner =
-          base::CreateSingleThreadTaskRunnerWithTraits(
-              task_traits, base::SingleThreadTaskRunnerThreadMode::DEDICATED);
-      DCHECK(redirection_task_runner);
-      BrowserThreadImpl::RedirectThreadIDToTaskRunner(
-          id, std::move(redirection_task_runner));
-    }
+  // |io_thread_| is created by |PostMainMessageLoopStart()|, but its
+  // full initialization is deferred until this point because it requires
+  // several dependencies we don't want to depend on so early in startup.
+  DCHECK(io_thread_);
+  io_thread_->InitIOThreadDelegate();
 
-    TRACE_EVENT_END0("startup", "BrowserMainLoop::CreateThreads:start");
-  }
+  TRACE_EVENT_END0("startup", "BrowserMainLoop::CreateThreads:start");
   created_threads_ = true;
   return result_code_;
 }
@@ -1349,6 +1309,10 @@ base::SequencedTaskRunner* BrowserMainLoop::audio_service_runner() {
   return audio_service_runner_.get();
 }
 
+void BrowserMainLoop::InitializeIOThreadForTesting() {
+  InitializeIOThread();
+}
+
 #if !defined(OS_ANDROID)
 viz::FrameSinkManagerImpl* BrowserMainLoop::GetFrameSinkManager() const {
   return frame_sink_manager_impl_.get();
@@ -1696,6 +1660,20 @@ void BrowserMainLoop::MainMessageLoopRun() {
 #endif
 }
 
+void BrowserMainLoop::InitializeIOThread() {
+  base::Thread::Options options;
+  options.message_loop_type = base::MessageLoop::TYPE_IO;
+#if defined(OS_ANDROID) || defined(OS_CHROMEOS)
+  // Up the priority of the |io_thread_| as some of its IPCs relate to
+  // display tasks.
+  options.priority = base::ThreadPriority::DISPLAY;
+#endif
+
+  io_thread_.reset(new BrowserProcessSubThread(BrowserThread::IO));
+  if (!io_thread_->StartWithOptions(options))
+    LOG(FATAL) << "Failed to start the browser thread: IO";
+}
+
 void BrowserMainLoop::InitializeMojo() {
   if (!parsed_command_line_.HasSwitch(switches::kSingleProcess)) {
     // Disallow mojo sync calls in the browser process. Note that we allow sync

content/browser/browser_main_loop.h

@@ -159,6 +159,8 @@ class CONTENT_EXPORT BrowserMainLoop {
   // through stopping threads to PostDestroyThreads.
   void ShutdownThreadsAndCleanUp();
 
+  void InitializeIOThreadForTesting();
+
   int GetResultCode() const { return result_code_; }
 
   media::AudioManager* audio_manager() const { return audio_manager_.get(); }
@@ -247,6 +249,7 @@ class CONTENT_EXPORT BrowserMainLoop {
 
   void MainMessageLoopRun();
 
+  void InitializeIOThread();
   void InitializeMojo();
   base::FilePath GetStartupTraceFileName(
       const base::CommandLine& command_line) const;

content/browser/browser_main_loop_unittest.cc

@@ -27,6 +27,7 @@ TEST(BrowserMainLoopTest, CreateThreadsInSingleProcess) {
         *scoped_command_line.GetProcessCommandLine());
     BrowserMainLoop browser_main_loop(main_function_params);
     browser_main_loop.MainMessageLoopStart();
+    browser_main_loop.InitializeIOThreadForTesting();
     browser_main_loop.CreateThreads();
     EXPECT_GE(base::TaskScheduler::GetInstance()
                   ->GetMaxConcurrentNonBlockedTasksWithTraitsDeprecated(

content/browser/browser_thread_impl.cc

@@ -131,11 +131,38 @@ struct BrowserThreadGlobals {
   // by BrowserThreadGlobals, rather by whoever calls
   // BrowserThread::SetIOThreadDelegate.
   BrowserThreadDelegateAtomicPtr io_thread_delegate = 0;
+
+  // This locks protects |is_io_thread_initialized|. Do not read or modify this
+  // variable without holding this lock.
+  base::Lock io_thread_lock;
+
+  // A flag indicates whether the BrowserThreadDelegate of the BrowserThread::IO
+  // thread has been initialized.
+  bool is_io_thread_initialized = false;
 };
 
 base::LazyInstance<BrowserThreadGlobals>::Leaky
     g_globals = LAZY_INSTANCE_INITIALIZER;
 
+void InitIOThreadDelegateOnIOThread() {
+  BrowserThreadDelegateAtomicPtr delegate =
+      base::subtle::NoBarrier_Load(&g_globals.Get().io_thread_delegate);
+  if (delegate)
+    reinterpret_cast<BrowserThreadDelegate*>(delegate)->Init();
+}
+
+bool IsIOThreadInitialized() {
+  BrowserThreadGlobals& globals = g_globals.Get();
+  base::AutoLock lock(globals.io_thread_lock);
+  return globals.is_io_thread_initialized;
+}
+
+void SetIsIOThreadInitialized(bool is_io_thread_initialized) {
+  BrowserThreadGlobals& globals = g_globals.Get();
+  base::AutoLock lock(globals.io_thread_lock);
+  globals.is_io_thread_initialized = is_io_thread_initialized;
+}
+
 }  // namespace
 
 BrowserThreadImpl::BrowserThreadImpl(ID identifier)
@@ -162,10 +189,9 @@ BrowserThreadImpl::BrowserThreadImpl(ID identifier,
 }
 
 void BrowserThreadImpl::Init() {
-  BrowserThreadGlobals& globals = g_globals.Get();
-
 #if DCHECK_IS_ON()
   {
+    BrowserThreadGlobals& globals = g_globals.Get();
     base::AutoLock lock(globals.lock);
     // |globals| should already have been initialized for |identifier_| in
     // BrowserThreadImpl::StartWithOptions(). If this isn't the case it's likely
@@ -182,13 +208,6 @@ void BrowserThreadImpl::Init() {
     base::RunLoop::DisallowNestingOnCurrentThread();
     message_loop()->DisallowTaskObservers();
   }
-
-  if (identifier_ == BrowserThread::IO) {
-    BrowserThreadDelegateAtomicPtr delegate =
-        base::subtle::NoBarrier_Load(&globals.io_thread_delegate);
-    if (delegate)
-      reinterpret_cast<BrowserThreadDelegate*>(delegate)->Init();
-  }
 }
 
 // We disable optimizations for this block of functions so the compiler doesn't
@@ -252,11 +271,12 @@ void BrowserThreadImpl::Run(base::RunLoop* run_loop) {
 void BrowserThreadImpl::CleanUp() {
   BrowserThreadGlobals& globals = g_globals.Get();
 
-  if (identifier_ == BrowserThread::IO) {
+  if (identifier_ == BrowserThread::IO && IsIOThreadInitialized()) {
     BrowserThreadDelegateAtomicPtr delegate =
         base::subtle::NoBarrier_Load(&globals.io_thread_delegate);
     if (delegate)
       reinterpret_cast<BrowserThreadDelegate*>(delegate)->CleanUp();
+    SetIsIOThreadInitialized(false);
   }
 
   // Change the state to SHUTDOWN so that PostTaskHelper stops accepting tasks
@@ -290,6 +310,14 @@ void BrowserThreadImpl::ResetGlobalsForTesting(BrowserThread::ID identifier) {
     SetIOThreadDelegate(nullptr);
 }
 
+void BrowserThreadImpl::InitIOThreadDelegate() {
+  DCHECK(!IsIOThreadInitialized());
+
+  SetIsIOThreadInitialized(true);
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                          base::BindOnce(&InitIOThreadDelegateOnIOThread));
+}
+
 BrowserThreadImpl::~BrowserThreadImpl() {
   // All Thread subclasses must call Stop() in the destructor. This is
   // doubly important here as various bits of code check they are on
@@ -476,8 +504,13 @@ bool BrowserThread::IsThreadInitialized(ID identifier) {
   base::AutoLock lock(globals.lock);
   DCHECK_GE(identifier, 0);
   DCHECK_LT(identifier, ID_COUNT);
-  return globals.states[identifier] == BrowserThreadState::INITIALIZED ||
-         globals.states[identifier] == BrowserThreadState::RUNNING;
+  bool running =
+      globals.states[identifier] == BrowserThreadState::INITIALIZED ||
+      globals.states[identifier] == BrowserThreadState::RUNNING;
+  if (identifier != BrowserThread::IO)
+    return running;
+
+  return running && IsIOThreadInitialized();
 }
 
 // static

content/browser/browser_thread_impl.h

@@ -43,6 +43,10 @@ class CONTENT_EXPORT BrowserThreadImpl : public BrowserThread,
   bool Start();
   bool StartWithOptions(const Options& options);
   bool StartAndWaitForTesting();
+  // Called only by the BrowserThread::IO thread to initialize its
+  // BrowserThreadDelegate after the thread is created. See
+  // https://crbug.com/729596.
+  void InitIOThreadDelegate();
 
   // Redirects tasks posted to |identifier| to |task_runner|.
   static void RedirectThreadIDToTaskRunner(

content/public/test/test_browser_thread.cc

@@ -101,6 +101,10 @@ bool TestBrowserThread::StartIOThread() {
   return impl_->StartWithOptions(options);
 }
 
+void TestBrowserThread::InitIOThreadDelegate() {
+  impl_->InitIOThreadDelegate();
+}
+
 void TestBrowserThread::Stop() {
   impl_->Stop();
 }

content/public/test/test_browser_thread.h

@@ -43,6 +43,9 @@ class TestBrowserThread {
   // Starts the thread with an IOThread message loop.
   bool StartIOThread();
 
+  // Initializes the BrowserThreadDelegate.
+  void InitIOThreadDelegate();
+
   // Stops the thread.
   void Stop();