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..d95d1f25c5a30c1fe9eeb74010801b2fb6a7c292 100644
--- a/chrome/browser/chrome_browser_main.cc
+++ b/chrome/browser/chrome_browser_main.cc
@@ -1328,6 +1328,22 @@ 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.
+  // Note: It could also be the first thing in CreateThreads() but being in

[fdoray, 2016/09/16 18:20:28]

This Note is not so true if SetupMetrics() is in PreCreateThreads() and uses the
blocking pool.

[robliao, 2016/09/16 21:35:40]

Indeed. Removed.

+  // 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();
+
   SetupMetrics();
 
   // ChromeOS needs ResourceBundle::InitSharedInstance to be called before this.
@@ -1336,18 +1352,6 @@ int ChromeBrowserMainParts::PreCreateThreadsImpl() {
   device::GeolocationProvider::SetGeolocationDelegate(
       new ChromeGeolocationDelegate());
 
-  // This needs to be the last thing in PreCreateThreads() because the

[gab, 2016/09/16 19:57:04]

I think we do want this to remain the last thing. Can we instead move
SetupMetrics() to PreMainMessageLoopRun() or something with all the other
services?

I want the invariant that no threads are created until the very end of
PreCreateThreads() to eventually be true.

[robliao, 2016/09/16 21:35:40]

This will have to stay here since browser_process_->PreCreateThreads initializes
the IOThread, which needs the metrics service.

The nice part of this is that it is the absolute earliest time we can init. The
possible drawback as you point out is that we create threads before the end of
PreCreateThreads. One way to address this is to have a two-phase startup, init
the structures here, buffer, and then start the threads at CreateThreads. I'm
not so sure I'm thrilled about that structure, but it's something to think
about.

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

Agreed I'm also not thrilled about buffering either just for the sake of making
PreCreateThreads() correct.

-  // 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 +2173,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 +2189,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 browser process PreCreateThreads.

[fdoray, 2016/09/16 18:20:28]

"before the call to PreCreateThreads() on |browser_process_|"

It took me a few seconds to understand what you meant by "browser process".

[robliao, 2016/09/16 21:35:40]

Done.

+  // 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();