Initialize the Browser Task Scheduler Between Field Trials and Metrics

chrome/browser/chrome_browser_main.cc

Index: chrome/browser/chrome_browser_main.cc
diff --git a/chrome/browser/chrome_browser_main.cc b/chrome/browser/chrome_browser_main.cc
index ae2b74a779b20e593c5a69b271bf45957b70233a..2c5b28e33439476497d316863eee34e59dfedcbd 100644
--- a/chrome/browser/chrome_browser_main.cc
+++ b/chrome/browser/chrome_browser_main.cc
@@ -1324,30 +1324,38 @@ int ChromeBrowserMainParts::PreCreateThreadsImpl() {
 
   SetupOriginTrialsCommandLine();
 
+  device::GeolocationProvider::SetGeolocationDelegate(
+      new ChromeGeolocationDelegate());
+
+  // IMPORTANT
+  // Do not add anything below this line until you've verified your new code
+  // does not interfere with the critical initialization order below. Some of
+  // the calls below end up implicitly creating threads and as such new calls
+  // typically either belong before them or in a later startup phase.
+
   // Now the command line has been mutated based on about:flags, we can

[gab, 2016/09/19 20:05:20]

While you're here: s/Now the/Now that the/

[robliao, 2016/09/19 20:08:34]

I'll do this in a next change to make reverts easier in case something happens.

[robliao, 2016/09/19 20:11:20]

https://codereview.chromium.org/2350943002/ tracks this change.

   // initialize field trials and setup metrics. The field trials are needed by
   // IOThread's initialization which happens in BrowserProcess:PreCreateThreads.
   SetupFieldTrials();
-  SetupMetrics();
 
-  // ChromeOS needs ResourceBundle::InitSharedInstance to be called before this.
-  browser_process_->PreCreateThreads();
-
-  device::GeolocationProvider::SetGeolocationDelegate(
-      new ChromeGeolocationDelegate());
-
-  // This needs to be the last thing in PreCreateThreads() because the
-  // TaskScheduler needs to be created before any other threads are (by
-  // contract) but it creates threads itself so instantiating it earlier is also
-  // incorrect. It also has to be after SetupFieldTrials() to allow it to use
-  // field trials. Note: it could also be the first thing in CreateThreads() but
-  // being in chrome/ is convenient for now as the initialization uses
-  // variations parameters extensively.
-  //
+  // Task Scheduler initialization needs to be here for the following reasons:
+  //   * After |SetupFieldTrials()|: Initialization uses variations.
+  //   * Before |SetupMetrics()|: |SetupMetrics()| uses the blocking pool. The
+  //         Task Scheduler must do any necessary redirection before then.
+  //   * Near the end of |PreCreateThreads()|: The TaskScheduler needs to be
+  //         created before any other threads are (by contract) but it creates
+  //         threads itself so instantiating it earlier is also incorrect.
   // To maintain scoping symmetry, if this line is moved, the corresponding
   // shutdown call may also need to be moved.
   MaybeInitializeTaskScheduler();
 
+  SetupMetrics();
+
+  // ChromeOS needs ResourceBundle::InitSharedInstance to be called before this.
+  // This also instantiates the IOThread which requests the metrics service and
+  // must be after |SetupMetrics()|.
+  browser_process_->PreCreateThreads();
+
   return content::RESULT_CODE_NORMAL_EXIT;
 }
 
@@ -2169,13 +2177,6 @@ void ChromeBrowserMainParts::PostDestroyThreads() {
   // not finish.
   NOTREACHED();
 #else
-  // The TaskScheduler was initialized at the very end of PreCreateThreads. To
-  // maintain scoping symmetry, perform the shutdown here at the beginning of
-  // PostDestroyThreads.
-  base::TaskScheduler* task_scheduler = base::TaskScheduler::GetInstance();
-  if (task_scheduler)
-    task_scheduler->Shutdown();
-
   int restart_flags = restart_last_session_
                           ? browser_shutdown::RESTART_LAST_SESSION
                           : browser_shutdown::NO_FLAGS;
@@ -2192,6 +2193,14 @@ void ChromeBrowserMainParts::PostDestroyThreads() {
   // browser_shutdown takes care of deleting browser_process, so we need to
   // release it.
   ignore_result(browser_process_.release());
+
+  // The TaskScheduler was initialized before invoking
+  // |browser_process_->PreCreateThreads()|. To maintain scoping symmetry,
+  // perform the shutdown after |browser_process_->PostDestroyThreads()|.
+  base::TaskScheduler* task_scheduler = base::TaskScheduler::GetInstance();
+  if (task_scheduler)
+    task_scheduler->Shutdown();
+
   browser_shutdown::ShutdownPostThreadsStop(restart_flags);
   master_prefs_.reset();
   process_singleton_.reset();