Migrate ServiceProcess and MockLaunchd to use RunLoop::QuitClosure().

Use of RunLoop::QuitCurrent*Deprecated() is deprecated, and risky,
potentially causing unexpected early-exits if run under a different
RunLoop to the one intended.

Bug: 844016, 859151
Change-Id: Ib38310c0feeec09dbaac6cb74ffd061476e30cf3
Reviewed-on: https://chromium-review.googlesource.com/1123763
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573449}

chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc

@@ -12,6 +12,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/message_loop/message_loop.h"
@@ -122,15 +123,15 @@ class TestServiceProcess : public ServiceProcess {
   TestServiceProcess() { }
   ~TestServiceProcess() override {}
 
-  bool Initialize(base::MessageLoopForUI* message_loop,
-                  ServiceProcessState* state);
+  bool Initialize(base::OnceClosure quit_closure,
+                  std::unique_ptr<ServiceProcessState> state);
 };
 
-bool TestServiceProcess::Initialize(base::MessageLoopForUI* message_loop,
-                                    ServiceProcessState* state) {
-  main_message_loop_ = message_loop;
-
-  service_process_state_.reset(state);
+bool TestServiceProcess::Initialize(
+    base::OnceClosure quit_closure,
+    std::unique_ptr<ServiceProcessState> state) {
+  quit_closure_ = std::move(quit_closure);
+  service_process_state_ = std::move(state);
 
   base::Thread::Options options(base::MessageLoop::TYPE_IO, 0);
   io_thread_.reset(new base::Thread("TestServiceProcess_IO"));
@@ -190,6 +191,7 @@ int CloudPrintMockService_Main(SetExpectationsCallback set_expectations) {
   CHECK(!user_data_dir.empty());
   CHECK(test_launcher_utils::OverrideUserDataDir(user_data_dir));
 
+  base::RunLoop run_loop;
 #if defined(OS_MACOSX)
   if (!command_line->HasSwitch(kTestExecutablePath))
     return kMissingSwitch;
@@ -197,11 +199,10 @@ int CloudPrintMockService_Main(SetExpectationsCallback set_expectations) {
       command_line->GetSwitchValuePath(kTestExecutablePath);
   EXPECT_FALSE(executable_path.empty());
   MockLaunchd mock_launchd(executable_path, main_message_loop.task_runner(),
-                           true, true);
+                           run_loop.QuitClosure(), true, true);
   Launchd::ScopedInstance use_mock(&mock_launchd);
 #endif
 
-
   ServiceProcessState* state(new ServiceProcessState);
   bool service_process_state_initialized = state->Initialize();
   EXPECT_TRUE(service_process_state_initialized);
@@ -213,7 +214,8 @@ int CloudPrintMockService_Main(SetExpectationsCallback set_expectations) {
 
   // Takes ownership of the pointer, but we can use it since we have the same
   // lifetime.
-  EXPECT_TRUE(service_process.Initialize(&main_message_loop, state));
+  EXPECT_TRUE(service_process.Initialize(run_loop.QuitClosure(),
+                                         base::WrapUnique(state)));
 
   // Needed for IPC.
   mojo::core::Init();
@@ -255,7 +257,7 @@ int CloudPrintMockService_Main(SetExpectationsCallback set_expectations) {
           service_process.io_task_runner(),
           base::ThreadTaskRunnerHandle::Get());
 
-  base::RunLoop().Run();
+  run_loop.Run();
   if (!Mock::VerifyAndClearExpectations(&server))
     return kExpectationsNotMet;
   if (!g_good_shutdown)
@@ -388,8 +390,9 @@ void CloudPrintProxyPolicyStartupTest::SetUp() {
   EXPECT_TRUE(MockLaunchd::MakeABundle(temp_dir_.GetPath(),
                                        "CloudPrintProxyTest", &bundle_path_,
                                        &executable_path_));
-  mock_launchd_.reset(new MockLaunchd(
-      executable_path_, base::ThreadTaskRunnerHandle::Get(), true, false));
+  mock_launchd_.reset(new MockLaunchd(executable_path_,
+                                      base::ThreadTaskRunnerHandle::Get(),
+                                      base::DoNothing(), true, false));
   scoped_launchd_instance_.reset(
       new Launchd::ScopedInstance(mock_launchd_.get()));
 #endif

chrome/common/mac/mock_launchd.cc

@@ -94,11 +94,13 @@ bool MockLaunchd::MakeABundle(const base::FilePath& dst,
 MockLaunchd::MockLaunchd(
     const base::FilePath& file,
     scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
+    base::OnceClosure quit_closure,
     bool create_socket,
     bool as_service)
     : file_(file),
       pipe_name_(GetServiceProcessServerName()),
       main_task_runner_(std::move(main_task_runner)),
+      quit_closure_(std::move(quit_closure)),
       create_socket_(create_socket),
       as_service_(as_service),
       restart_called_(false),
@@ -227,8 +229,7 @@ CFDictionaryRef MockLaunchd::CopyDictionaryByCheckingIn(CFErrorRef* error) {
 
 bool MockLaunchd::RemoveJob(CFStringRef label, CFErrorRef* error) {
   remove_called_ = true;
-  main_task_runner_->PostTask(
-      FROM_HERE, base::RunLoop::QuitCurrentWhenIdleClosureDeprecated());
+  std::move(quit_closure_).Run();
   return true;
 }
 
@@ -237,8 +238,7 @@ bool MockLaunchd::RestartJob(Domain domain,
                              CFStringRef name,
                              CFStringRef session_type) {
   restart_called_ = true;
-  main_task_runner_->PostTask(
-      FROM_HERE, base::RunLoop::QuitCurrentWhenIdleClosureDeprecated());
+  std::move(quit_closure_).Run();
   return true;
 }
 

chrome/common/mac/mock_launchd.h

@@ -10,6 +10,7 @@
 #include <memory>
 #include <string>
 
+#include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/mac/scoped_cftyperef.h"
 #include "base/memory/scoped_refptr.h"
@@ -31,6 +32,7 @@ class MockLaunchd : public Launchd {
 
   MockLaunchd(const base::FilePath& file,
               scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
+              base::OnceClosure quit_closure,
               bool create_socket,
               bool as_service);
   ~MockLaunchd() override;
@@ -63,6 +65,7 @@ class MockLaunchd : public Launchd {
   base::FilePath file_;
   std::string pipe_name_;
   scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
+  base::OnceClosure quit_closure_;
   std::unique_ptr<MultiProcessLock> running_lock_;
   bool create_socket_;
   bool as_service_;

chrome/common/service_process_util_mac_unittest.mm

@@ -48,8 +48,8 @@ class ServiceProcessStateFileManipulationTest : public ::testing::Test {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     ASSERT_TRUE(MockLaunchd::MakeABundle(GetTempDirPath(), "Test",
                                          &bundle_path_, &executable_path_));
-    mock_launchd_.reset(
-        new MockLaunchd(executable_path_, loop_.task_runner(), false, false));
+    mock_launchd_.reset(new MockLaunchd(executable_path_, loop_.task_runner(),
+                                        run_loop_.QuitClosure(), false, false));
     scoped_launchd_instance_.reset(
         new Launchd::ScopedInstance(mock_launchd_.get()));
     ASSERT_TRUE(service_process_state_.Initialize());

chrome/service/service_main.cc

@@ -32,11 +32,11 @@ int CloudPrintServiceProcessMain(
   if (!state->Initialize())
     return 0;
 
+  base::RunLoop run_loop;
   ServiceProcess service_process;
-  if (service_process.Initialize(&main_message_loop,
-                                 parameters.command_line,
-                                 state.release())) {
-    base::RunLoop().Run();
+  if (service_process.Initialize(run_loop.QuitClosure(),
+                                 parameters.command_line, std::move(state))) {
+    run_loop.Run();
   } else {
     LOG(ERROR) << "Service process failed to initialize";
   }

chrome/service/service_process.cc

@@ -131,16 +131,15 @@ void PrepareRestartOnCrashEnviroment(
 ServiceProcess::ServiceProcess()
     : shutdown_event_(base::WaitableEvent::ResetPolicy::MANUAL,
                       base::WaitableEvent::InitialState::NOT_SIGNALED),
-      main_message_loop_(NULL),
       enabled_services_(0),
       update_available_(false) {
   DCHECK(!g_service_process);
   g_service_process = this;
 }
 
-bool ServiceProcess::Initialize(base::MessageLoopForUI* message_loop,
+bool ServiceProcess::Initialize(base::OnceClosure quit_closure,
                                 const base::CommandLine& command_line,
-                                ServiceProcessState* state) {
+                                std::unique_ptr<ServiceProcessState> state) {
 #if defined(USE_GLIB)
   // g_type_init has been deprecated since version 2.35.
 #if !GLIB_CHECK_VERSION(2, 35, 0)
@@ -148,8 +147,8 @@ bool ServiceProcess::Initialize(base::MessageLoopForUI* message_loop,
   g_type_init();
 #endif
 #endif  // defined(USE_GLIB)
-  main_message_loop_ = message_loop;
-  service_process_state_.reset(state);
+  quit_closure_ = std::move(quit_closure);
+  service_process_state_ = std::move(state);
 
   // Initialize TaskScheduler.
   constexpr int kMaxBackgroundThreads = 1;
@@ -300,8 +299,7 @@ void ServiceProcess::Shutdown() {
 }
 
 void ServiceProcess::Terminate() {
-  main_message_loop_->task_runner()->PostTask(
-      FROM_HERE, base::RunLoop::QuitCurrentWhenIdleClosureDeprecated());
+  std::move(quit_closure_).Run();
 }
 
 void ServiceProcess::OnShutdown() {

chrome/service/service_process.h

@@ -50,11 +50,11 @@ class ServiceProcess : public ServiceIPCServer::Client,
   ServiceProcess();
   ~ServiceProcess() override;
 
-  // Initialize the ServiceProcess with the message loop that it should run on.
-  // ServiceProcess takes ownership of |state|.
-  bool Initialize(base::MessageLoopForUI* message_loop,
+  // Initialize the ServiceProcess. |quit_closure| will be run when the service
+  // process is ready to exit.
+  bool Initialize(base::OnceClosure quit_closure,
                   const base::CommandLine& command_line,
-                  ServiceProcessState* state);
+                  std::unique_ptr<ServiceProcessState> state);
 
   bool Teardown();
 
@@ -123,8 +123,8 @@ class ServiceProcess : public ServiceIPCServer::Client,
   // An event that will be signalled when we shutdown.
   base::WaitableEvent shutdown_event_;
 
-  // Pointer to the main message loop that host this object.
-  base::MessageLoop* main_message_loop_;
+  // Closure to run to cause the main message loop to exit.
+  base::OnceClosure quit_closure_;
 
   // Count of currently enabled services in this process.
   int enabled_services_;