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..da1de1a413b0a15aaae8dd029763b36aeb8b4cce 100644
--- a/chrome/browser/chrome_browser_main.cc
+++ b/chrome/browser/chrome_browser_main.cc
@@ -1328,6 +1328,18 @@ int ChromeBrowserMainParts::PreCreateThreadsImpl() {
   // initialize field trials and setup metrics. The field trials are needed by
   // IOThread's initialization which happens in BrowserProcess:PreCreateThreads.
   SetupFieldTrials();
+
+  // 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.

[gab, 2016/09/19 17:51:32]

Add:

// This instantiates the IOThread and must be after SetupMetrics() (**explain
why if it makes to here**?).

[robliao, 2016/09/19 19:17:30]

Done. What do you mean mean about the "explain why if it makes it to here". I
think we've hammered the issue home with the large comment on
MaybeInitializeTaskScheduler.

@@ -1336,18 +1348,6 @@ int ChromeBrowserMainParts::PreCreateThreadsImpl() {
   device::GeolocationProvider::SetGeolocationDelegate(

[gab, 2016/09/19 17:51:32]

Can we move this before SetupFieldTrials() and then add a big comment before
SetupFieldTrials() like:

// ***IMPORTANT***: do not add anything after this comment unless you know
specifically what you are doing. 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.

[robliao, 2016/09/19 19:17:30]

Looks safe to move (default constructor, global set but not dereferenced during
this init phase).

       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.
-  //
-  // To maintain scoping symmetry, if this line is moved, the corresponding
-  // shutdown call may also need to be moved.
-  MaybeInitializeTaskScheduler();
-
   return content::RESULT_CODE_NORMAL_EXIT;
 }
 
@@ -2169,13 +2169,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 +2185,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 the PreCreateThreads() on
+  // |browser_process_|. To maintain scoping symmetry, perform the shutdown

[gab, 2016/09/19 17:51:32]

s/before the PreCreateThreads on |browser_process_|/before invoking
|browser_process->PreCreateThreads()|/ ?

[robliao, 2016/09/19 19:17:30]

Done.

+  // after browser process PostDestroyThreads.

[gab, 2016/09/19 17:51:32]

Similarly: "after invoking |browser_process_->PostDestroyThreads()|" ?

[robliao, 2016/09/19 19:17:30]

Done.

+  base::TaskScheduler* task_scheduler = base::TaskScheduler::GetInstance();
+  if (task_scheduler)
+    task_scheduler->Shutdown();
+
   browser_shutdown::ShutdownPostThreadsStop(restart_flags);
   master_prefs_.reset();
   process_singleton_.reset();