[FeatureList] Create default FeatureList early in PostEarlyInitialization.

This CL add a default implementation of
ContentMainDelegate::PostEarlyInitialization() to create FeatureList,
and remove the creation in BrowserMainLoop which happens after staring
the TaskScheduler in CL (https://crrev.com/c/1174955). This
fix browser tests failures when the TaskScheduler is starting before
FeatureList is created.

Bug: 848615, 729596
Change-Id: I9f9e85c785d41ad518db4170db53c4fc1d57e90d
Reviewed-on: https://chromium-review.googlesource.com/c/1228615
Commit-Queue: Xi Han <hanxi@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Paul Miller <paulmiller@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596712}

android_webview/browser/aw_browser_main_parts.cc

@@ -149,11 +149,6 @@ AwBrowserMainParts::AwBrowserMainParts(AwContentBrowserClient* browser_client)
 AwBrowserMainParts::~AwBrowserMainParts() {
 }
 
-bool AwBrowserMainParts::ShouldContentCreateFeatureList() {
-  // FeatureList will be created in AwFieldTrialCreator.
-  return false;
-}
-
 int AwBrowserMainParts::PreEarlyInitialization() {
   // Network change notifier factory must be singleton, only set factory
   // instance while it is not been created.

android_webview/browser/aw_browser_main_parts.h

@@ -30,7 +30,6 @@ class AwBrowserMainParts : public content::BrowserMainParts {
   ~AwBrowserMainParts() override;
 
   // Overriding methods from content::BrowserMainParts.
-  bool ShouldContentCreateFeatureList() override;
   int PreEarlyInitialization() override;
   int PreCreateThreads() override;
   void PreMainMessageLoopRun() override;

android_webview/lib/aw_main_delegate.cc

@@ -304,6 +304,13 @@ void AwMainDelegate::ProcessExiting(const std::string& process_type) {
   logging::CloseLogFile();
 }
 
+bool AwMainDelegate::ShouldCreateFeatureList() {
+  // TODO(https://crbug.com/887468): Move the creation of FeatureList from
+  // AwBrowserMainParts::PreCreateThreads() to
+  // AwMainDelegate::PostEarlyInitialization().
+  return false;
+}
+
 content::ContentBrowserClient* AwMainDelegate::CreateContentBrowserClient() {
   content_browser_client_.reset(new AwContentBrowserClient());
   return content_browser_client_.get();

android_webview/lib/aw_main_delegate.h

@@ -44,6 +44,7 @@ class AwMainDelegate : public content::ContentMainDelegate {
       const std::string& process_type,
       const content::MainFunctionParams& main_function_params) override;
   void ProcessExiting(const std::string& process_type) override;
+  bool ShouldCreateFeatureList() override;
   content::ContentBrowserClient* CreateContentBrowserClient() override;
   content::ContentGpuClient* CreateContentGpuClient() override;
   content::ContentRendererClient* CreateContentRendererClient() override;

chrome/app/chrome_main_delegate.cc

@@ -512,6 +512,11 @@ void ChromeMainDelegate::PostEarlyInitialization() {
   chrome_feature_list_creator_->CreateFeatureList();
   tracing_sampler_profiler_->OnMessageLoopStarted();
 }
+
+bool ChromeMainDelegate::ShouldCreateFeatureList() {
+  // Chrome creates the FeatureList, so content should not.
+  return false;
+}
 #endif
 
 bool ChromeMainDelegate::BasicStartupComplete(int* exit_code) {

chrome/app/chrome_main_delegate.h

@@ -67,6 +67,7 @@ class ChromeMainDelegate : public content::ContentMainDelegate {
   void PreCreateMainMessageLoop() override;
 #if !defined(CHROME_MULTIPLE_DLL_CHILD)
   void PostEarlyInitialization() override;
+  bool ShouldCreateFeatureList() override;
 #endif
 
   content::ContentBrowserClient* CreateContentBrowserClient() override;

chrome/browser/chrome_browser_main.cc

@@ -941,11 +941,6 @@ DLLEXPORT void __cdecl RelaunchChromeBrowserWithNewCommandLineIfNeeded() {
 
 // content::BrowserMainParts implementation ------------------------------------
 
-bool ChromeBrowserMainParts::ShouldContentCreateFeatureList() {
-  // Chrome creates the FeatureList, so no need for content to do the same.
-  return false;
-}
-
 int ChromeBrowserMainParts::PreEarlyInitialization() {
   TRACE_EVENT0("startup", "ChromeBrowserMainParts::PreEarlyInitialization");
   for (size_t i = 0; i < chrome_extra_parts_.size(); ++i)

chrome/browser/chrome_browser_main.h

@@ -61,7 +61,6 @@ class ChromeBrowserMainParts : public content::BrowserMainParts {
                          ChromeFeatureListCreator* chrome_feature_list_creator);
 
   // content::BrowserMainParts overrides.
-  bool ShouldContentCreateFeatureList() override;
   // These are called in-order by content::BrowserMainLoop.
   // Each stage calls the same stages in any ChromeBrowserMainExtraParts added
   // with AddParts() from ChromeContentBrowserClient::CreateBrowserMainParts.

chromecast/app/cast_main_delegate.cc

@@ -186,6 +186,13 @@ void CastMainDelegate::ZygoteForked() {
 }
 #endif  // defined(OS_LINUX)
 
+bool CastMainDelegate::ShouldCreateFeatureList() {
+  // TODO(https://crbug.com/887459): Move the creation of FeatureList from
+  // CastBrowserMainParts::PreCreateThreads() to
+  // CastMainDelegate::PostEarlyInitialization().
+  return false;
+}
+
 void CastMainDelegate::InitializeResourceBundle() {
   base::FilePath pak_file;
   CHECK(base::PathService::Get(FILE_CAST_PAK, &pak_file));

chromecast/app/cast_main_delegate.h

@@ -40,6 +40,7 @@ class CastMainDelegate : public content::ContentMainDelegate {
 #if defined(OS_LINUX)
   void ZygoteForked() override;
 #endif  // defined(OS_LINUX)
+  bool ShouldCreateFeatureList() override;
   content::ContentBrowserClient* CreateContentBrowserClient() override;
   content::ContentRendererClient* CreateContentRendererClient() override;
   content::ContentUtilityClient* CreateContentUtilityClient() override;

chromecast/browser/cast_browser_main_parts.cc

@@ -395,10 +395,6 @@ content::BrowserContext* CastBrowserMainParts::browser_context() {
   return cast_browser_process_->browser_context();
 }
 
-bool CastBrowserMainParts::ShouldContentCreateFeatureList() {
-  return false;
-}
-
 void CastBrowserMainParts::PreMainMessageLoopStart() {
   // GroupedHistograms needs to be initialized before any threads are created
   // to prevent race conditions between calls to Preregister and those threads

chromecast/browser/cast_browser_main_parts.h

@@ -80,7 +80,6 @@ class CastBrowserMainParts : public content::BrowserMainParts {
   content::BrowserContext* browser_context();
 
   // content::BrowserMainParts implementation:
-  bool ShouldContentCreateFeatureList() override;
   void PreMainMessageLoopStart() override;
   void PostMainMessageLoopStart() override;
   void ToolkitInitialized() override;

content/app/content_main_runner_impl.cc

@@ -45,6 +45,7 @@
 #include "content/browser/browser_thread_impl.h"
 #include "content/browser/scheduler/browser_task_executor.h"
 #include "content/browser/startup_data_impl.h"
+#include "content/browser/startup_helper.h"
 #include "content/common/content_constants_internal.h"
 #include "content/common/url_schemes.h"
 #include "content/public/app/content_main_delegate.h"
@@ -900,7 +901,13 @@ int ContentMainRunnerImpl::Run(bool start_service_manager_only) {
     if (!base::MessageLoopCurrentForUI::IsSet())
       main_message_loop_ = std::make_unique<base::MessageLoopForUI>();
 
+    if (delegate_->ShouldCreateFeatureList()) {
+      DCHECK(!field_trial_list_);
+      field_trial_list_ = SetUpFieldTrialsAndFeatureList();
+    }
+
     delegate_->PostEarlyInitialization();
+
     return RunBrowserProcessMain(main_params, delegate_);
   }
 #endif  // !defined(CHROME_MULTIPLE_DLL_CHILD)

content/app/content_main_runner_impl.h

@@ -10,6 +10,7 @@
 #include "base/callback_forward.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/message_loop/message_loop.h"
+#include "base/metrics/field_trial.h"
 #include "build/build_config.h"
 #include "content/browser/startup_data_impl.h"
 #include "content/public/app/content_main.h"
@@ -76,6 +77,8 @@ class ContentMainRunnerImpl : public ContentMainRunner {
   std::unique_ptr<base::MessageLoop> main_message_loop_;
 
   std::unique_ptr<StartupDataImpl> startup_data_;
+
+  std::unique_ptr<base::FieldTrialList> field_trial_list_;
 #endif  // !defined(CHROME_MULTIPLE_DLL_CHILD)
 
   DISALLOW_COPY_AND_ASSIGN(ContentMainRunnerImpl);

content/browser/BUILD.gn

@@ -592,6 +592,8 @@ jumbo_source_set("browser") {
     "code_cache/generated_code_cache_context.h",
     "startup_data_impl.cc",
     "startup_data_impl.h",
+    "startup_helper.cc",
+    "startup_helper.h",
 
     # NOTE: These files are here instead of in compositor_browser_sources
     # because the latter is not built on Android, whereas these files are

content/browser/browser_main_loop.cc

@@ -650,16 +650,6 @@ int BrowserMainLoop::EarlyInitialization() {
       return pre_early_init_error_code;
   }
 
-  if (!parts_ || parts_->ShouldContentCreateFeatureList()) {
-    // Note that we do not initialize a new FeatureList when calling this for
-    // the second time.
-    const base::CommandLine* command_line =
-        base::CommandLine::ForCurrentProcess();
-    base::FeatureList::InitializeInstance(
-        command_line->GetSwitchValueASCII(switches::kEnableFeatures),
-        command_line->GetSwitchValueASCII(switches::kDisableFeatures));
-  }
-
 #if defined(OS_ANDROID) || defined(OS_CHROMEOS)
   // Up the priority of the UI thread unless it was already high (since recent
   // versions of Android (O+) do this automatically).

content/browser/startup_helper.cc

+// Copyright 2018 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 "content/browser/startup_helper.h"
+
+#include "base/base_switches.h"
+#include "base/command_line.h"
+
+namespace content {
+
+std::unique_ptr<base::FieldTrialList> SetUpFieldTrialsAndFeatureList() {
+  auto field_trial_list = std::make_unique<base::FieldTrialList>(nullptr);
+  const base::CommandLine* command_line =
+      base::CommandLine::ForCurrentProcess();
+
+  // Ensure any field trials specified on the command line are initialized.
+  if (command_line->HasSwitch(::switches::kForceFieldTrials)) {
+    // Create field trials without activating them, so that this behaves in a
+    // consistent manner with field trials created from the server.
+    bool result = base::FieldTrialList::CreateTrialsFromString(
+        command_line->GetSwitchValueASCII(::switches::kForceFieldTrials),
+        std::set<std::string>());
+    CHECK(result) << "Invalid --" << ::switches::kForceFieldTrials
+                  << " list specified.";
+  }
+
+  base::FeatureList::InitializeInstance(
+      command_line->GetSwitchValueASCII(switches::kEnableFeatures),
+      command_line->GetSwitchValueASCII(switches::kDisableFeatures));
+  return field_trial_list;
+}
+
+}  // namespace content

content/browser/startup_helper.h

+// Copyright 2018 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.
+
+#ifndef CONTENT_BROWSER_STARTUP_HELPER_H_
+#define CONTENT_BROWSER_STARTUP_HELPER_H_
+
+#include "base/metrics/field_trial.h"
+#include "content/common/content_export.h"
+
+namespace content {
+
+// Setups fields trials and the FeatureList, and returns the unique pointer of
+// the field trials.
+std::unique_ptr<base::FieldTrialList> CONTENT_EXPORT
+SetUpFieldTrialsAndFeatureList();
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_STARTUP_HELPER_H_

content/public/app/content_main_delegate.cc

@@ -75,6 +75,10 @@ void ContentMainDelegate::OnServiceManagerInitialized(
     const base::Closure& quit_closure,
     service_manager::BackgroundServiceManager* service_manager) {}
 
+bool ContentMainDelegate::ShouldCreateFeatureList() {
+  return true;
+}
+
 ContentBrowserClient* ContentMainDelegate::CreateContentBrowserClient() {
 #if defined(CHROME_MULTIPLE_DLL_CHILD)
   return NULL;

content/public/app/content_main_delegate.h

@@ -129,9 +129,15 @@ class CONTENT_EXPORT ContentMainDelegate {
   // creating the main message loop.
   virtual void PreCreateMainMessageLoop() {}
 
-  // Allows the embedder to perform platform-specific initializatioion. For
-  // example, things that should be done right after TaskScheduler starts and
-  // the main MessageLoop was installed.
+  // Returns true if content should create field trials and initialize the
+  // FeatureList instance for this process. Default implementation returns true.
+  // Embedders that need to control when and/or how FeatureList should be
+  // created should override and return false.
+  virtual bool ShouldCreateFeatureList();
+
+  // Allows the embedder to perform its own initialization after content
+  // performed its own and already brought up MessageLoop, TaskScheduler, field
+  // tials and FeatureList (by default).
   virtual void PostEarlyInitialization() {}
 
  protected:

content/public/browser/browser_main_parts.cc

@@ -8,10 +8,6 @@
 
 namespace content {
 
-bool BrowserMainParts::ShouldContentCreateFeatureList() {
-  return true;
-}
-
 int BrowserMainParts::PreEarlyInitialization() {
   return service_manager::RESULT_CODE_NORMAL_EXIT;
 }

content/public/browser/browser_main_parts.h

@@ -54,11 +54,6 @@ class CONTENT_EXPORT BrowserMainParts {
   BrowserMainParts() {}
   virtual ~BrowserMainParts() {}
 
-  // Returns true if content should create a FeatureList. Default
-  // implementation returns true. Embedders that need to control when and/or
-  // how FeatureList should be created should override and return false.
-  virtual bool ShouldContentCreateFeatureList();
-
   // A return value other than RESULT_CODE_NORMAL_EXIT indicates error and is
   // used as the exit status.
   virtual int PreEarlyInitialization();

content/public/test/browser_test_base.cc

@@ -27,6 +27,7 @@
 #include "content/browser/browser_thread_impl.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/scheduler/browser_task_executor.h"
+#include "content/browser/startup_helper.h"
 #include "content/browser/tracing/tracing_controller_impl.h"
 #include "content/public/app/content_main.h"
 #include "content/public/browser/browser_task_traits.h"
@@ -326,6 +327,8 @@ void BrowserTestBase::SetUp() {
   params.ui_task = ui_task.release();
   params.created_main_parts_closure = created_main_parts_closure.release();
   base::TaskScheduler::Create("Browser");
+  DCHECK(!field_trial_list_);
+  field_trial_list_ = SetUpFieldTrialsAndFeatureList();
   BrowserTaskExecutor::Create();
   // TODO(phajdan.jr): Check return code, http://crbug.com/374738 .
   BrowserMain(params);

content/shell/browser/shell_browser_main_parts.cc

@@ -133,7 +133,6 @@ int ShellBrowserMainParts::PreEarlyInitialization() {
   net::NetworkChangeNotifier::SetFactory(
       new net::NetworkChangeNotifierFactoryAndroid());
 #endif
-  SetupFieldTrials();
   return service_manager::RESULT_CODE_NORMAL_EXIT;
 }
 
@@ -149,25 +148,6 @@ void ShellBrowserMainParts::InitializeMessageLoopContext() {
                          gfx::Size());
 }
 
-void ShellBrowserMainParts::SetupFieldTrials() {
-  DCHECK(!field_trial_list_);
-  field_trial_list_.reset(new base::FieldTrialList(nullptr));
-
-  const base::CommandLine* command_line =
-      base::CommandLine::ForCurrentProcess();
-
-  // Ensure any field trials specified on the command line are initialized.
-  if (command_line->HasSwitch(::switches::kForceFieldTrials)) {
-    // Create field trials without activating them, so that this behaves in a
-    // consistent manner with field trials created from the server.
-    bool result = base::FieldTrialList::CreateTrialsFromString(
-        command_line->GetSwitchValueASCII(::switches::kForceFieldTrials),
-        std::set<std::string>());
-    CHECK(result) << "Invalid --" << ::switches::kForceFieldTrials
-                  << " list specified.";
-  }
-}
-
 int ShellBrowserMainParts::PreCreateThreads() {
 #if defined(OS_ANDROID)
   const base::CommandLine* command_line =

content/shell/browser/shell_browser_main_parts.h

@@ -55,7 +55,6 @@ class ShellBrowserMainParts : public BrowserMainParts {
   }
 
  private:
-  void SetupFieldTrials();
 
   std::unique_ptr<net::NetLog> net_log_;
   std::unique_ptr<ShellBrowserContext> browser_context_;
@@ -65,10 +64,6 @@ class ShellBrowserMainParts : public BrowserMainParts {
   const MainFunctionParams parameters_;
   bool run_message_loop_;
 
-  // Statistical testing infrastructure for the entire browser. nullptr until
-  // |SetupFieldTrials()| is called.
-  std::unique_ptr<base::FieldTrialList> field_trial_list_;
-
   DISALLOW_COPY_AND_ASSIGN(ShellBrowserMainParts);
 };