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