Quit browser RunLoop via injected Closure rather than via QuitCurrent*.

Use of QuitCurrent*Deprecated() is ambiguous, since RunLoops can be
nested, and so is unsafe to use in general.

TBR: bartfab
Bug: 845966, 844016, 749312
Change-Id: I537862ea02c4ed67887db32993f40e53f614b3cd
Reviewed-on: https://chromium-review.googlesource.com/1019967Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562046}

chrome/browser/apps/app_shim/app_shim_quit_interactive_uitest_mac.mm

@@ -121,7 +121,7 @@ IN_PROC_BROWSER_TEST_F(AppShimQuitTest, QuitWithKeyEvent) {
             atStart:NO];
 
   // This will time out if the event above does not terminate Chrome.
-  content::RunMessageLoop();
+  RunUntilBrowserProcessQuits();
 
   EXPECT_FALSE(handler_->FindHost(profile(), extension_id_));
   EXPECT_TRUE(browser_shutdown::IsTryingToQuit());

chrome/browser/apps/app_window_interactive_uitest.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/apps/app_window_interactive_uitest.h"
 
 #include "build/build_config.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window.h"
@@ -516,8 +517,8 @@ IN_PROC_BROWSER_TEST_F(AppWindowHiddenKeepAliveTest, StaysHidden) {
 
   for (auto* browser : *BrowserList::GetInstance())
     browser->window()->Close();
-  // This will time out if the command above does not terminate Chrome.
-  content::RunMessageLoop();
+
+  RunUntilBrowserProcessQuits();
 }
 
 // A window that is created hidden but shown soon after should keep Chrome

chrome/browser/browser_process_impl.cc

@@ -310,6 +310,23 @@ void BrowserProcessImpl::Init() {
   webrtc_event_log_manager_ = WebRtcEventLogManager::CreateSingletonInstance();
 }
 
+#if !defined(OS_ANDROID)
+void BrowserProcessImpl::SetQuitClosure(base::OnceClosure quit_closure) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(!quit_closure_);
+  quit_closure_ = std::move(quit_closure);
+}
+#endif
+
+#if defined(OS_MACOSX)
+base::OnceClosure BrowserProcessImpl::SwapQuitClosure(
+    base::OnceClosure quit_closure) {
+  DCHECK(quit_closure);
+  std::swap(quit_closure, quit_closure_);
+  return quit_closure;
+}
+#endif
+
 BrowserProcessImpl::~BrowserProcessImpl() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 #if BUILDFLAG(ENABLE_EXTENSIONS)
@@ -1369,6 +1386,7 @@ void BrowserProcessImpl::Unpin() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   release_last_reference_callstack_ = base::debug::StackTrace();
 
+  DCHECK(!shutting_down_);
   shutting_down_ = true;
 #if BUILDFLAG(ENABLE_PRINTING)
   // Wait for the pending print jobs to finish. Don't do this later, since
@@ -1391,9 +1409,10 @@ void BrowserProcessImpl::Unpin() {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(ChromeBrowserMainPartsMac::DidEndMainMessageLoop));
 #endif
-  base::RunLoop::QuitCurrentWhenIdleDeprecated();
 
 #if !defined(OS_ANDROID)
+  std::move(quit_closure_).Run();
+
   chrome::ShutdownIfNeeded();
 #endif  // !defined(OS_ANDROID)
 }

chrome/browser/browser_process_impl.h

@@ -83,6 +83,20 @@ class BrowserProcessImpl : public BrowserProcess,
   // Called to complete initialization.
   void Init();
 
+#if !defined(OS_ANDROID)
+  // Called by ChromeBrowserMainParts to provide a closure that will quit the
+  // browser main message-loop.
+  void SetQuitClosure(base::OnceClosure quit_closure);
+#endif
+
+#if defined(OS_MACOSX)
+  // TODO(https://crbug.com/845966): Replaces the current |quit_closure_| and
+  // returns it. Used by the Cocoa first-run dialog to swap-out the RunLoop set
+  // by ChromeBrowserMainParts' for one that will cause the first-run dialog's
+  // modal RunLoop to exit.
+  base::OnceClosure SwapQuitClosure(base::OnceClosure quit_closure);
+#endif
+
   // Called before the browser threads are created.
   void PreCreateThreads(const base::CommandLine& command_line);
 
@@ -376,6 +390,11 @@ class BrowserProcessImpl : public BrowserProcess,
 
   std::unique_ptr<prefs::InProcessPrefServiceFactory> pref_service_factory_;
 
+#if !defined(OS_ANDROID)
+  // Called to signal the process' main message loop to exit.
+  base::OnceClosure quit_closure_;
+#endif
+
   SEQUENCE_CHECKER(sequence_checker_);
 
   DISALLOW_COPY_AND_ASSIGN(BrowserProcessImpl);

chrome/browser/chrome_browser_main.cc

@@ -309,6 +309,11 @@ using content::BrowserThread;
 
 namespace {
 
+#if !defined(OS_ANDROID)
+// Holds the RunLoop for the non-Android MainMessageLoopRun() to Run().
+base::RunLoop* g_run_loop = nullptr;
+#endif
+
 // This function provides some ways to test crash and assertion handling
 // behavior of the program.
 void HandleTestParameters(const base::CommandLine& command_line) {
@@ -1309,6 +1314,12 @@ int ChromeBrowserMainParts::PreCreateThreadsImpl() {
   }
 
 #if !defined(OS_ANDROID)
+  // Create the RunLoop for MainMessageLoopRun() to use, and pass a copy of
+  // its QuitClosure to the BrowserProcessImpl to call when it is time to exit.
+  DCHECK(!g_run_loop);
+  g_run_loop = new base::RunLoop;
+  browser_process_->SetQuitClosure(g_run_loop->QuitWhenIdleClosure());
+
   // These members must be initialized before returning from this function.
   // Android doesn't use StartupBrowserCreator.
   browser_creator_.reset(new StartupBrowserCreator);
@@ -1843,7 +1854,7 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
   if (first_run::IsChromeFirstRun()) {
     first_run::AutoImport(profile_, master_prefs_->import_bookmarks_path);
 
-    // Note: this can pop the first run consent dialog on linux.
+    // Note: This can pop-up the first run consent dialog on Linux & Mac.
     first_run::DoPostImportTasks(profile_,
                                  master_prefs_->make_chrome_default_for_user);
 
@@ -2133,7 +2144,6 @@ bool ChromeBrowserMainParts::MainMessageLoopRun(int* result_code) {
   RecordBrowserStartupTime();
 
   DCHECK(base::MessageLoopForUI::IsCurrent());
-  base::RunLoop run_loop;
 
   performance_monitor::PerformanceMonitor::GetInstance()->StartGatherCycle();
 
@@ -2142,7 +2152,7 @@ bool ChromeBrowserMainParts::MainMessageLoopRun(int* result_code) {
   metrics::MetricsService::SetExecutionPhase(
       metrics::ExecutionPhase::MAIN_MESSAGE_LOOP_RUN,
       g_browser_process->local_state());
-  run_loop.Run();
+  g_run_loop->Run();
 
   return true;
 #endif  // defined(OS_ANDROID)
@@ -2239,3 +2249,12 @@ void ChromeBrowserMainParts::PostDestroyThreads() {
 void ChromeBrowserMainParts::AddParts(ChromeBrowserMainExtraParts* parts) {
   chrome_extra_parts_.push_back(parts);
 }
+
+#if !defined(OS_ANDROID)
+// static
+std::unique_ptr<base::RunLoop> ChromeBrowserMainParts::TakeRunLoopForTest() {
+  auto run_loop = base::WrapUnique<base::RunLoop>(g_run_loop);
+  g_run_loop = nullptr;
+  return run_loop;
+}
+#endif

chrome/browser/chrome_browser_main.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "build/build_config.h"
 #include "chrome/browser/chrome_browser_field_trials.h"
 #include "chrome/browser/chrome_process_singleton.h"
 #include "chrome/browser/first_run/first_run.h"
@@ -50,6 +51,13 @@ class ChromeBrowserMainParts : public content::BrowserMainParts {
   // Add additional ChromeBrowserMainExtraParts.
   virtual void AddParts(ChromeBrowserMainExtraParts* parts);
 
+#if !defined(OS_ANDROID)
+  // Returns the RunLoop that would be run by MainMessageLoopRun. This is used
+  // by InProcessBrowserTests to allow them to run until the BrowserProcess is
+  // ready for the browser to exit.
+  static std::unique_ptr<base::RunLoop> TakeRunLoopForTest();
+#endif
+
  protected:
 #if !defined(OS_ANDROID)
   class DeferringTaskRunner;

chrome/browser/chromeos/login/auto_launched_kiosk_browsertest.cc

@@ -454,7 +454,7 @@ class AutoLaunchedKioskTest : public extensions::ExtensionApiTest {
 
     // Wait until the app terminates if it is still running.
     if (!app_window_registry->GetAppWindowsForApp(app_id).empty())
-      base::RunLoop().Run();
+      RunUntilBrowserProcessQuits();
     return true;
   }
 

chrome/browser/chromeos/login/kiosk_browsertest.cc

@@ -691,7 +691,7 @@ class KioskTest : public OobeBaseTest {
 
     // Wait until the app terminates if it is still running.
     if (!app_window_registry->GetAppWindowsForApp(test_app_id_).empty())
-      content::RunMessageLoop();
+      RunUntilBrowserProcessQuits();
 
     // Check that the app had been informed that it is running in a kiosk
     // session.

chrome/browser/chromeos/login/oobe_browsertest.cc

@@ -48,7 +48,7 @@ class OobeTest : public OobeBaseTest {
     if (LoginDisplayHost::default_host()) {
       base::ThreadTaskRunnerHandle::Get()->PostTask(
           FROM_HERE, base::BindOnce(&chrome::AttemptExit));
-      content::RunMessageLoop();
+      RunUntilBrowserProcessQuits();
     }
 
     OobeBaseTest::TearDownOnMainThread();

chrome/browser/chromeos/policy/device_system_use_24hour_clock_browsertest.cc

@@ -53,7 +53,7 @@ class SystemUse24HourClockPolicyTest
     if (LoginDisplayHost::default_host()) {
       base::ThreadTaskRunnerHandle::Get()->PostTask(
           FROM_HERE, base::BindOnce(&chrome::AttemptExit));
-      content::RunMessageLoop();
+      RunUntilBrowserProcessQuits();
     }
   }
 

chrome/browser/chromeos/policy/display_rotation_default_handler_browsertest.cc

@@ -93,7 +93,7 @@ class DisplayRotationDefaultTest
     if (chromeos::LoginDisplayHost::default_host()) {
       base::ThreadTaskRunnerHandle::Get()->PostTask(
           FROM_HERE, base::BindOnce(&chrome::AttemptExit));
-      content::RunMessageLoop();
+      RunUntilBrowserProcessQuits();
     }
   }
 

chrome/browser/chromeos/policy/unaffiliated_arc_allowed_browsertest.cc

@@ -87,7 +87,7 @@ class UnaffiliatedArcAllowedTest
     if (chromeos::LoginDisplayHost::default_host()) {
       base::ThreadTaskRunnerHandle::Get()->PostTask(
           FROM_HERE, base::BindOnce(&chrome::AttemptExit));
-      content::RunMessageLoop();
+      RunUntilBrowserProcessQuits();
     }
     arc::ArcSessionManager::Get()->Shutdown();
     DevicePolicyCrosBrowserTest::TearDownOnMainThread();

chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc

@@ -149,7 +149,7 @@ IN_PROC_BROWSER_TEST_F(UserCloudPolicyManagerTest, ErrorLoadingPolicy) {
       content::NotificationService::AllSources());
   GetLoginDisplay()->ShowSigninScreenForTest(kAccountId, kAccountPassword,
                                              kEmptyServices);
-  base::RunLoop().Run();
+  RunUntilBrowserProcessQuits();
   // Should not receive a SESSION_STARTED notification.
   ASSERT_EQ(0, observer.notification_count());
 

chrome/browser/chromeos/shutdown_policy_browsertest.cc

@@ -328,7 +328,7 @@ class ShutdownPolicyLoginTest : public ShutdownPolicyBaseTest {
     if (LoginDisplayHost::default_host()) {
       base::ThreadTaskRunnerHandle::Get()->PostTask(
           FROM_HERE, base::Bind(&chrome::AttemptExit));
-      content::RunMessageLoop();
+      RunUntilBrowserProcessQuits();
     }
   }
 

chrome/browser/ui/cocoa/first_run_dialog.mm

@@ -13,6 +13,7 @@
 #include "base/run_loop.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "chrome/browser/browser_process_impl.h"
 #include "chrome/browser/first_run/first_run.h"
 #include "chrome/browser/first_run/first_run_dialog.h"
 #include "chrome/browser/metrics/metrics_reporting_state.h"
@@ -137,10 +138,21 @@ void ShowFirstRunDialog(Profile* profile) {
 
   scoped_refptr<FirstRunShowBridge> bridge(new FirstRunShowBridge(self));
   base::RunLoop run_loop;
+
+  // Swap-out the BrowserProcessImpl's quit-closure with the one for our
+  // modal RunLoop, so that it can be exit either by the dialog being closed,
+  // or in response to SIGTERM being handled.
+  base::OnceClosure browser_quit_closure =
+      static_cast<BrowserProcessImpl*>(g_browser_process)
+          ->SwapQuitClosure(run_loop.QuitClosure());
+
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(&FirstRunShowBridge::ShowDialog, bridge.get(),
                             run_loop.QuitClosure()));
   run_loop.Run();
+
+  static_cast<BrowserProcessImpl*>(g_browser_process)
+      ->SwapQuitClosure(std::move(browser_quit_closure));
 }
 
 - (void)show {

chrome/browser/ui/tab_modal_confirm_dialog_browsertest.cc

@@ -133,7 +133,8 @@ IN_PROC_BROWSER_TEST_F(TabModalConfirmDialogTest, Navigate) {
 IN_PROC_BROWSER_TEST_F(TabModalConfirmDialogTest, Quit) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(&chrome::AttemptExit));
-  content::RunMessageLoop();
+  RunUntilBrowserProcessQuits();
+
   EXPECT_EQ(0, accepted_count_);
   EXPECT_EQ(0, canceled_count_);
   EXPECT_EQ(1, closed_count_);

chrome/test/base/in_process_browser_test.cc

@@ -21,6 +21,7 @@
 #include "build/build_config.h"
 #include "chrome/browser/after_startup_task_utils.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/chrome_browser_main.h"
 #include "chrome/browser/chrome_content_browser_client.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/devtools/devtools_window.h"
@@ -340,6 +341,10 @@ void InProcessBrowserTest::CloseAllBrowsers() {
 #endif
 }
 
+void InProcessBrowserTest::RunUntilBrowserProcessQuits() {
+  std::move(run_loop_)->Run();
+}
+
 // TODO(alexmos): This function should expose success of the underlying
 // navigation to tests, which should make sure navigations succeed when
 // appropriate. See https://crbug.com/425335
@@ -466,6 +471,10 @@ base::CommandLine InProcessBrowserTest::GetCommandLineForRelaunch() {
 void InProcessBrowserTest::PreRunTestOnMainThread() {
   AfterStartupTaskUtils::SetBrowserStartupIsCompleteForTesting();
 
+  // Take the ChromeBrowserMainParts' RunLoop to run ourself, when we
+  // want to wait for the browser to exit.
+  run_loop_ = ChromeBrowserMainParts::TakeRunLoopForTest();
+
   // Pump startup related events.
   content::RunAllPendingInMessageLoop();
 
@@ -527,6 +536,7 @@ void InProcessBrowserTest::PostRunTestOnMainThread() {
   content::RunAllPendingInMessageLoop();
 
   QuitBrowsers();
+
   // BrowserList should be empty at this point.
   CHECK(BrowserList::GetInstance()->empty());
 }
@@ -550,7 +560,7 @@ void InProcessBrowserTest::QuitBrowsers() {
   // shut down properly.
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(&chrome::AttemptExit));
-  content::RunMessageLoop();
+  RunUntilBrowserProcessQuits();
 
 #if defined(OS_MACOSX)
   // chrome::AttemptExit() will attempt to close all browsers by deleting

chrome/test/base/in_process_browser_test.h

@@ -152,6 +152,11 @@ class InProcessBrowserTest : public content::BrowserTestBase {
   // outstanding resources.
   void CloseAllBrowsers();
 
+  // Runs the main thread message loop until the BrowserProcess indicates
+  // we should quit. This will normally be called automatically during test
+  // teardown, but may instead be run manually by the test, if necessary.
+  void RunUntilBrowserProcessQuits();
+
   // Convenience methods for adding tabs to a Browser.
   void AddTabAtIndexToBrowser(Browser* browser,
                               int index,
@@ -247,6 +252,9 @@ class InProcessBrowserTest : public content::BrowserTestBase {
   // Browser created in BrowserMain().
   Browser* browser_;
 
+  // Used to run the process until the BrowserProcess signals the test to quit.
+  std::unique_ptr<base::RunLoop> run_loop_;
+
   // Temporary user data directory. Used only when a user data directory is not
   // specified in the command line.
   base::ScopedTempDir temp_user_data_dir_;