Initialize the Browser Task Scheduler Between Field Trials and Metrics

chrome/browser/chrome_browser_main.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chrome_browser_main.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 1306 matching lines @@
 #endif  // defined(OS_MACOSX)
 
 #if defined(OS_CHROMEOS)
   // Must be done after g_browser_process is constructed, before
   // SetupFieldTrials() and SetupMetrics().
   chromeos::CrosSettings::Initialize();
 #endif  // defined(OS_CHROMEOS)
 
   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;
 }
 
 void ChromeBrowserMainParts::MojoShellConnectionStarted(
     content::MojoShellConnection* connection) {
   for (size_t i = 0; i < chrome_extra_parts_.size(); ++i)
     chrome_extra_parts_[i]->MojoShellConnectionStarted(connection);
 }
 
 void ChromeBrowserMainParts::PreMainMessageLoopRun() {
@@ skipping 801 matching lines @@
 #endif  // defined(SYZYASAN)
 #endif  // defined(OS_ANDROID)
 }
 
 void ChromeBrowserMainParts::PostDestroyThreads() {
 #if defined(OS_ANDROID)
   // On Android, there is no quit/exit. So the browser's main message loop will
   // 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;
 
 #if BUILDFLAG(ENABLE_BACKGROUND)
   if (restart_flags) {
     restart_flags |= BackgroundModeManager::should_restart_in_background()
                          ? browser_shutdown::RESTART_IN_BACKGROUND
                          : browser_shutdown::NO_FLAGS;
   }
 #endif  // BUILDFLAG(ENABLE_BACKGROUND)
 
   browser_process_->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();
   device_event_log::Shutdown();
 
   // We need to do this check as late as possible, but due to modularity, this
   // may be the last point in Chrome.  This would be more effective if done at
   // a higher level on the stack, so that it is impossible for an early return
   // to bypass this code.  Perhaps we need a *final* hook that is called on all
   // paths from content/browser/browser_main.
   CHECK(metrics::MetricsService::UmaMetricsProperlyShutdown());
 
 #if defined(OS_CHROMEOS)
   chromeos::CrosSettings::Shutdown();
 #endif  // defined(OS_CHROMEOS)
 #endif  // defined(OS_ANDROID)
 }
 
 // Public members:
 
 void ChromeBrowserMainParts::AddParts(ChromeBrowserMainExtraParts* parts) {
   chrome_extra_parts_.push_back(parts);
 }