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 1310 matching lines @@
   // SetupFieldTrials() and SetupMetrics().
   chromeos::CrosSettings::Initialize();
 #endif  // defined(OS_CHROMEOS)
 
   SetupOriginTrialsCommandLine();
 
   // Now the command line has been mutated based on about:flags, we can
   // 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.

   browser_process_->PreCreateThreads();
 
   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;
 }
 
 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 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();
   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);
 }