Move MessageLoopForUI creation to BrowserTaskExecutor::Create

This is in preparation for the BrowserUIThreadScheduler where we
hope to have only one location where it's set up.

Bug: 863341, 872372
Change-Id: Ia7c07adc1d60280f2aed25f5226bfce80c6aba4b
Reviewed-on: https://chromium-review.googlesource.com/c/1461393
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#632797}

chrome/browser/chrome_browser_main_android.cc

@@ -71,18 +71,7 @@ int ChromeBrowserMainPartsAndroid::PreEarlyInitialization() {
 
   content::Compositor::Initialize();
 
-  // Chrome on Android does not use default MessageLoop. It has its own
-  // Android specific MessageLoop.
-  DCHECK(!main_message_loop_.get());
-
-  // Create the MessageLoop if doesn't yet exist (and bind it to the native Java
-  // loop). This is a critical point in the startup process.
-  {
-    TRACE_EVENT0("startup",
-      "ChromeBrowserMainPartsAndroid::PreEarlyInitialization:CreateUiMsgLoop");
-    if (!base::MessageLoopCurrent::IsSet())
-      main_message_loop_ = std::make_unique<base::MessageLoopForUI>();
-  }
+  CHECK(base::MessageLoopCurrent::IsSet());
 
   return ChromeBrowserMainParts::PreEarlyInitialization();
 }

chrome/browser/chrome_browser_main_android.h

@@ -26,7 +26,6 @@ class ChromeBrowserMainPartsAndroid : public ChromeBrowserMainParts {
   void ShowMissingLocaleMessageBox() override;
 
  private:
-  std::unique_ptr<base::MessageLoop> main_message_loop_;
   std::unique_ptr<android::ChromeBackupWatcher> backup_watcher_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeBrowserMainPartsAndroid);

content/app/content_main_runner_impl.cc

@@ -900,11 +900,11 @@ int ContentMainRunnerImpl::RunServiceManager(MainFunctionParams& main_params,
     }
 #endif
 
-    // Create a MessageLoop if one does not already exist for the current
-    // thread. This thread won't be promoted as BrowserThread::UI until
-    // BrowserMainLoop::MainMessageLoopStart().
-    if (!base::MessageLoopCurrentForUI::IsSet())
-      main_message_loop_ = std::make_unique<base::MessageLoopForUI>();
+    // Register the TaskExecutor for posting task to the BrowserThreads. It is
+    // incorrect to post to a BrowserThread before this point. This instantiates
+    // and binds the MessageLoopForUI on the main thread (but it's only labeled
+    // as BrowserThread::UI in BrowserMainLoop::MainMessageLoopStart).
+    BrowserTaskExecutor::Create();
 
     delegate_->PostEarlyInitialization(main_params.ui_task != nullptr);
 
@@ -913,9 +913,7 @@ int ContentMainRunnerImpl::RunServiceManager(MainFunctionParams& main_params,
       StartBrowserTaskScheduler();
     }
 
-    // Register the TaskExecutor for posting task to the BrowserThreads. It is
-    // incorrect to post to a BrowserThread before this point.
-    BrowserTaskExecutor::Create();
+    BrowserTaskExecutor::PostFeatureListSetup();
 
     if (!base::FeatureList::IsEnabled(
             features::kAllowStartingServiceManagerOnly)) {
@@ -968,8 +966,8 @@ void ContentMainRunnerImpl::Shutdown() {
   }
 
 #if !defined(CHROME_MULTIPLE_DLL_CHILD)
-  // The message loop needs to be destroyed before |exit_manager_|.
-  main_message_loop_.reset();
+  // The BrowserTaskExecutor needs to be destroyed before |exit_manager_|.
+  BrowserTaskExecutor::Shutdown();
 #endif  // !defined(CHROME_MULTIPLE_DLL_CHILD)
 
 #if defined(OS_WIN)

content/app/content_main_runner_impl.h

@@ -9,7 +9,6 @@
 
 #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/service_manager/service_manager_context.h"
@@ -54,8 +53,6 @@ class ContentMainRunnerImpl : public ContentMainRunner {
 
   bool is_browser_main_loop_started_ = false;
 
-  std::unique_ptr<base::MessageLoop> main_message_loop_;
-
   std::unique_ptr<StartupDataImpl> startup_data_;
   std::unique_ptr<base::FieldTrialList> field_trial_list_;
   std::unique_ptr<BrowserProcessSubThread> service_manager_thread_;

content/browser/browser_main_loop.cc

@@ -681,10 +681,7 @@ void BrowserMainLoop::MainMessageLoopStart() {
   // PostMainMessageLoopStart() below.
 
   TRACE_EVENT0("startup", "BrowserMainLoop::MainMessageLoopStart");
-
-  if (!base::MessageLoopCurrentForUI::IsSet())
-    main_message_loop_ = std::make_unique<base::MessageLoopForUI>();
-
+  DCHECK(base::MessageLoopCurrentForUI::IsSet());
   InitializeMainThread();
 }
 

content/browser/browser_main_loop.h

@@ -36,7 +36,6 @@ class CommandLine;
 class FilePath;
 class HighResolutionTimerManager;
 class MemoryPressureMonitor;
-class MessageLoop;
 class PowerMonitor;
 class SingleThreadTaskRunner;
 class SystemMonitor;
@@ -303,7 +302,6 @@ class CONTENT_EXPORT BrowserMainLoop {
       scoped_execution_fence_;
 
   // Members initialized in |MainMessageLoopStart()| ---------------------------
-  std::unique_ptr<base::MessageLoop> main_message_loop_;
 
   // Members initialized in |PostMainMessageLoopStart()| -----------------------
   std::unique_ptr<BrowserProcessSubThread> io_thread_;

content/browser/browser_thread_impl.cc

@@ -24,10 +24,6 @@
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/content_browser_client.h"
 
-#if defined(OS_ANDROID)
-#include "base/android/task_scheduler/post_task_android.h"
-#endif
-
 namespace content {
 
 namespace {
@@ -107,12 +103,7 @@ BrowserThreadImpl::BrowserThreadImpl(
   DCHECK(!globals.task_runners[identifier_]);
   globals.task_runners[identifier_] = std::move(task_runner);
 
-  // TODO(alexclarke): Move this to the BrowserUIThreadScheduler.
   if (identifier_ == BrowserThread::ID::UI) {
-#if defined(OS_ANDROID)
-    base::PostTaskAndroid::SignalNativeSchedulerReady();
-#endif
-
 #if defined(OS_POSIX)
     // Allow usage of the FileDescriptorWatcher API on the UI thread, using the
     // IO thread to watch the file descriptors.
@@ -132,12 +123,6 @@ BrowserThreadImpl::~BrowserThreadImpl() {
   BrowserThreadGlobals& globals = GetBrowserThreadGlobals();
   DCHECK_CALLED_ON_VALID_THREAD(globals.main_thread_checker_);
 
-#if defined(OS_ANDROID)
-  // TODO(alexclarke): Move this to the BrowserUIThreadScheduler.
-  if (identifier_ == BrowserThread::ID::UI)
-    base::PostTaskAndroid::SignalNativeSchedulerShutdown();
-#endif
-
   DCHECK_EQ(base::subtle::NoBarrier_Load(&globals.states[identifier_]),
             BrowserThreadState::RUNNING);
   base::subtle::NoBarrier_Store(&globals.states[identifier_],

content/browser/browser_thread_unittest.cc

@@ -13,12 +13,14 @@
 #include "base/sequenced_task_runner_helpers.h"
 #include "base/single_thread_task_runner.h"
 #include "base/task/post_task.h"
+#include "base/task/sequence_manager/sequence_manager_impl.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "content/browser/browser_process_sub_thread.h"
 #include "content/browser/browser_thread_impl.h"
 #include "content/browser/scheduler/browser_task_executor.h"
+#include "content/browser/startup_helper.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/test/test_browser_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -40,10 +42,20 @@ class BrowserThreadTest : public testing::Test {
 
  protected:
   void SetUp() override {
-    BrowserTaskExecutor::Create();
-
     ui_thread_ = std::make_unique<BrowserProcessSubThread>(BrowserThread::UI);
-    ui_thread_->Start();
+    std::unique_ptr<base::sequence_manager::internal::SequenceManagerImpl>
+        sequence_manager = base::sequence_manager::internal::
+            SequenceManagerImpl::CreateUnbound(
+                base::sequence_manager::SequenceManager::Settings());
+    default_task_queue_ =
+        sequence_manager
+            ->CreateTaskQueueWithType<base::sequence_manager::TaskQueue>(
+                base::sequence_manager::TaskQueue::Spec("default_tq"));
+    sequence_manager->SetTaskRunner(default_task_queue_->task_runner());
+    BrowserTaskExecutor::CreateForTesting(default_task_queue_->task_runner());
+    base::Thread::Options ui_options;
+    ui_options.message_loop_base = sequence_manager.release();
+    ui_thread_->StartWithOptions(ui_options);
 
     io_thread_ = std::make_unique<BrowserProcessSubThread>(BrowserThread::IO);
     base::Thread::Options io_options;
@@ -97,6 +109,7 @@ class BrowserThreadTest : public testing::Test {
  private:
   std::unique_ptr<BrowserProcessSubThread> ui_thread_;
   std::unique_ptr<BrowserProcessSubThread> io_thread_;
+  scoped_refptr<base::sequence_manager::TaskQueue> default_task_queue_;
 
   base::test::ScopedTaskEnvironment scoped_task_environment_;
   // Must be set before Release() to verify the deletion is intentional. Will be

content/browser/scheduler/browser_task_executor.cc

@@ -9,8 +9,13 @@
 #include "base/bind.h"
 #include "base/deferred_sequenced_task_runner.h"
 #include "base/no_destructor.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "content/browser/browser_thread_impl.h"
 
+#if defined(OS_ANDROID)
+#include "base/android/task_scheduler/post_task_android.h"
+#endif
+
 namespace content {
 namespace {
 
@@ -144,19 +149,74 @@ scoped_refptr<AfterStartupTaskRunner> GetAfterStartupTaskRunnerForThreadImpl(
 
 }  // namespace
 
-BrowserTaskExecutor::BrowserTaskExecutor() = default;
+BrowserTaskExecutor::BrowserTaskExecutor(
+    std::unique_ptr<base::MessageLoopForUI> message_loop,
+    scoped_refptr<base::SingleThreadTaskRunner> ui_thread_task_runner)
+    : message_loop_(std::move(message_loop)),
+      ui_thread_task_runner_(std::move(ui_thread_task_runner)) {}
+
 BrowserTaskExecutor::~BrowserTaskExecutor() = default;
 
 // static
 void BrowserTaskExecutor::Create() {
   DCHECK(!g_browser_task_executor);
-  g_browser_task_executor = new BrowserTaskExecutor();
+  DCHECK(!base::ThreadTaskRunnerHandle::IsSet());
+  std::unique_ptr<base::MessageLoopForUI> message_loop =
+      std::make_unique<base::MessageLoopForUI>();
+  scoped_refptr<base::SingleThreadTaskRunner> ui_thread_task_runner =
+      message_loop->task_runner();
+  g_browser_task_executor = new BrowserTaskExecutor(
+      std::move(message_loop), std::move(ui_thread_task_runner));
+  base::RegisterTaskExecutor(BrowserTaskTraitsExtension::kExtensionId,
+                             g_browser_task_executor);
+#if defined(OS_ANDROID)
+  base::PostTaskAndroid::SignalNativeSchedulerReady();
+#endif
+}
+
+// static
+void BrowserTaskExecutor::CreateForTesting(
+    scoped_refptr<base::SingleThreadTaskRunner> ui_thread_task_runner) {
+  DCHECK(!g_browser_task_executor);
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
+  DCHECK(ui_thread_task_runner);
+  g_browser_task_executor =
+      new BrowserTaskExecutor(nullptr, std::move(ui_thread_task_runner));
   base::RegisterTaskExecutor(BrowserTaskTraitsExtension::kExtensionId,
                              g_browser_task_executor);
+#if defined(OS_ANDROID)
+  base::PostTaskAndroid::SignalNativeSchedulerReady();
+#endif
+}
+
+// static
+void BrowserTaskExecutor::PostFeatureListSetup() {
+  DCHECK(g_browser_task_executor);
+  // TODO(alexclarke): Forward to the BrowserUIThreadScheduler.
+}
+
+// static
+void BrowserTaskExecutor::Shutdown() {
+  if (!g_browser_task_executor)
+    return;
+
+  DCHECK(g_browser_task_executor->message_loop_);
+  // We don't delete either |g_browser_task_executor| or because other threads
+  // may PostTask or call BrowserTaskExecutor::GetTaskRunner while  we're
+  // tearing things down. We don't want to add locks so we just leak instead of
+  // dealing with that.For similar reasons we don't need to call
+  // PostTaskAndroid::SignalNativeSchedulerShutdown on Android. In tests however
+  // we need to clean up, so BrowserTaskExecutor::ResetForTesting should be
+  // called.
+  g_browser_task_executor->message_loop_.reset();
 }
 
 // static
 void BrowserTaskExecutor::ResetForTesting() {
+#if defined(OS_ANDROID)
+  base::PostTaskAndroid::SignalNativeSchedulerShutdown();
+#endif
+
   for (int i = 0; i < BrowserThread::ID_COUNT; ++i) {
     GetAfterStartupTaskRunnerForThreadImpl(static_cast<BrowserThread::ID>(i))
         ->Reset();
@@ -232,6 +292,8 @@ scoped_refptr<base::SingleThreadTaskRunner> BrowserTaskExecutor::GetTaskRunner(
   // policies instead.
   if (traits.priority() == base::TaskPriority::BEST_EFFORT)
     return GetAfterStartupTaskRunnerForThread(thread_id);
+  if (thread_id == BrowserThread::UI)
+    return ui_thread_task_runner_;
   return GetProxyTaskRunnerForThread(thread_id);
 }
 

content/browser/scheduler/browser_task_executor.h

@@ -5,8 +5,11 @@
 #ifndef CONTENT_BROWSER_SCHEDULER_BROWSER_TASK_EXECUTOR_H_
 #define CONTENT_BROWSER_SCHEDULER_BROWSER_TASK_EXECUTOR_H_
 
+#include <memory>
+
 #include "base/gtest_prod_util.h"
 #include "base/memory/scoped_refptr.h"
+#include "base/message_loop/message_loop.h"
 #include "base/task/task_executor.h"
 #include "build/build_config.h"
 #include "content/common/content_export.h"
@@ -18,10 +21,25 @@ namespace content {
 // browser process.
 class CONTENT_EXPORT BrowserTaskExecutor : public base::TaskExecutor {
  public:
-  // Creates and registers a BrowserTaskExecutor that facilitates posting tasks
-  // to a BrowserThread via //base/task/post_task.h.
+  // Creates and registers a BrowserTaskExecutor on the current thread, which
+  // owns a MessageLoopForUI. This facilitates posting tasks to a BrowserThread
+  // via //base/task/post_task.h.
   static void Create();
 
+  // Creates and registers a BrowserTaskExecutor on the current thread, which
+  // defers to a task runner from a previously created MessageLoopForUI.
+  static void CreateForTesting(
+      scoped_refptr<base::SingleThreadTaskRunner> ui_thread_task_runner);
+
+  // This must be called after the FeatureList has been initialized in order
+  // for scheduling experiments to function.
+  static void PostFeatureListSetup();
+
+  // Winds down the BrowserTaskExecutor, after this no tasks can be executed
+  // and the base::TaskExecutor APIs are non-functional but won't crash if
+  // called.
+  static void Shutdown();
+
   // Unregister and delete the TaskExecutor after a test.
   static void ResetForTesting();
 
@@ -57,7 +75,10 @@ class CONTENT_EXPORT BrowserTaskExecutor : public base::TaskExecutor {
   FRIEND_TEST_ALL_PREFIXES(BrowserTaskExecutorTest,
                            BestEffortTasksRunAfterStartup);
 
-  BrowserTaskExecutor();
+  // TODO(alexclarke): Replace with a BrowserUIThreadScheduler.
+  BrowserTaskExecutor(
+      std::unique_ptr<base::MessageLoopForUI> message_loop,
+      scoped_refptr<base::SingleThreadTaskRunner> ui_thread_task_runner);
   ~BrowserTaskExecutor() override;
 
   scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner(
@@ -73,6 +94,9 @@ class CONTENT_EXPORT BrowserTaskExecutor : public base::TaskExecutor {
   static scoped_refptr<base::SingleThreadTaskRunner>
   GetAfterStartupTaskRunnerForThread(BrowserThread::ID id);
 
+  std::unique_ptr<base::MessageLoopForUI> message_loop_;
+  scoped_refptr<base::SingleThreadTaskRunner> ui_thread_task_runner_;
+
   DISALLOW_COPY_AND_ASSIGN(BrowserTaskExecutor);
 };
 

content/browser/scheduler/browser_task_executor_unittest.cc

@@ -60,9 +60,8 @@ class BrowserTaskExecutorTest : public testing::Test {
 };
 
 TEST_F(BrowserTaskExecutorTest, EnsureUIThreadTraitPointsToExpectedQueue) {
-  EXPECT_EQ(
-      base::CreateSingleThreadTaskRunnerWithTraits({BrowserThread::UI}),
-      BrowserTaskExecutor::GetProxyTaskRunnerForThread(BrowserThread::UI));
+  EXPECT_EQ(base::CreateSingleThreadTaskRunnerWithTraits({BrowserThread::UI}),
+            thread_bundle_.GetMainThreadTaskRunner());
 }
 
 TEST_F(BrowserTaskExecutorTest, EnsureIOThreadTraitPointsToExpectedQueue) {

content/public/test/browser_test_base.cc

@@ -340,8 +340,10 @@ void BrowserTestBase::SetUp() {
   field_trial_list_ = SetUpFieldTrialsAndFeatureList();
   StartBrowserTaskScheduler();
   BrowserTaskExecutor::Create();
+  BrowserTaskExecutor::PostFeatureListSetup();
   // TODO(phajdan.jr): Check return code, http://crbug.com/374738 .
   BrowserMain(params);
+  BrowserTaskExecutor::ResetForTesting();
 #else
   GetContentMainParams()->ui_task = ui_task.release();
   GetContentMainParams()->created_main_parts_closure =

content/public/test/test_browser_thread_bundle.cc

@@ -84,7 +84,7 @@ void TestBrowserThreadBundle::Init() {
   CHECK(com_initializer_->Succeeded());
 #endif
 
-  BrowserTaskExecutor::Create();
+  BrowserTaskExecutor::CreateForTesting(base::ThreadTaskRunnerHandle::Get());
 
   if (HasIOMainLoop()) {
     CHECK(base::MessageLoopCurrentForIO::IsSet());