Clean up LifecycleTest to use QuitClosure and BarrierClosure.

Bug: 863166
Change-Id: I024feeb9c3e687632841da11e44222dc1b1f8acb
Reviewed-on: https://chromium-review.googlesource.com/1136722
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575041}

services/service_manager/tests/lifecycle/lifecycle_unittest.cc

@@ -4,7 +4,9 @@
 
 #include <memory>
 
+#include "base/barrier_closure.h"
 #include "base/bind.h"
+#include "base/callback.h"
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
@@ -30,15 +32,6 @@ const char kTestPackageAppNameA[] = "lifecycle_unittest_package_app_a";
 const char kTestPackageAppNameB[] = "lifecycle_unittest_package_app_b";
 const char kTestName[] = "lifecycle_unittest";
 
-void QuitLoop(base::RunLoop* loop) {
-  loop->Quit();
-}
-
-void DecrementCountAndQuitWhenZero(base::RunLoop* loop, size_t* count) {
-  if (!--(*count))
-    loop->Quit();
-}
-
 struct Instance {
   Instance() : pid(0) {}
   Instance(const Identity& identity, uint32_t pid)
@@ -51,8 +44,10 @@ struct Instance {
 class InstanceState : public mojom::ServiceManagerListener {
  public:
   InstanceState(mojom::ServiceManagerListenerRequest request,
-                base::RunLoop* loop)
-      : binding_(this, std::move(request)), loop_(loop) {}
+                base::OnceClosure on_init_complete)
+      : binding_(this, std::move(request)),
+        on_init_complete_(std::move(on_init_complete)),
+        on_destruction_(destruction_loop_.QuitClosure()) {}
   ~InstanceState() override {}
 
   bool HasInstanceForName(const std::string& name) const {
@@ -61,12 +56,10 @@ class InstanceState : public mojom::ServiceManagerListener {
   size_t GetNewInstanceCount() const {
     return instances_.size() - initial_instances_.size();
   }
-  void WaitForInstanceDestruction(base::RunLoop* loop) {
-    DCHECK(!destruction_loop_);
-    destruction_loop_ = loop;
-    // First of all check to see if we should be spinning this loop at all -
-    // the app(s) we're waiting on quitting may already have quit.
-    TryToQuitDestructionLoop();
+  void WaitForInstanceDestruction() {
+    // If the instances have already stopped then |destruction_loop_.Run()|
+    // will immediately return.
+    destruction_loop_.Run();
   }
 
  private:
@@ -77,7 +70,7 @@ class InstanceState : public mojom::ServiceManagerListener {
       initial_instances_[i.identity.name()] = i;
       instances_[i.identity.name()] = i;
     }
-    loop_->Quit();
+    std::move(on_init_complete_).Run();
   }
   void OnServiceCreated(mojom::RunningServiceInfoPtr instance) override {
     instances_[instance->identity.name()] =
@@ -102,14 +95,8 @@ class InstanceState : public mojom::ServiceManagerListener {
         break;
       }
     }
-    TryToQuitDestructionLoop();
-  }
-
-  void TryToQuitDestructionLoop() {
-    if (!GetNewInstanceCount() && destruction_loop_) {
-      destruction_loop_->Quit();
-      destruction_loop_ = nullptr;
-    }
+    if (GetNewInstanceCount() == 0)
+      std::move(on_destruction_).Run();
   }
   void OnServicePIDReceived(const service_manager::Identity& identity,
                             uint32_t pid) override {
@@ -127,11 +114,12 @@ class InstanceState : public mojom::ServiceManagerListener {
   std::map<std::string, Instance> initial_instances_;
 
   mojo::Binding<mojom::ServiceManagerListener> binding_;
-  base::RunLoop* loop_;
+  base::OnceClosure on_init_complete_;
 
   // Set when the client wants to wait for this object to track the destruction
   // of an instance before proceeding.
-  base::RunLoop* destruction_loop_ = nullptr;
+  base::RunLoop destruction_loop_;
+  base::OnceClosure on_destruction_;
 
   DISALLOW_COPY_AND_ASSIGN(InstanceState);
 };
@@ -162,7 +150,7 @@ class LifecycleTest : public test::ServiceTest {
   void InitPackage() {
     test::mojom::LifecycleControlPtr lifecycle = ConnectTo(kTestPackageName);
     base::RunLoop loop;
-    lifecycle.set_connection_error_handler(base::Bind(&QuitLoop, &loop));
+    lifecycle.set_connection_error_handler(loop.QuitClosure());
     lifecycle->GracefulQuit();
     loop.Run();
   }
@@ -190,18 +178,12 @@ class LifecycleTest : public test::ServiceTest {
 
   void PingPong(test::mojom::LifecycleControl* lifecycle) {
     base::RunLoop loop;
-    lifecycle->Ping(base::Bind(&QuitLoop, &loop));
+    lifecycle->Ping(loop.QuitClosure());
     loop.Run();
   }
 
   InstanceState* instances() { return instances_.get(); }
 
-  void WaitForInstanceDestruction() {
-    base::RunLoop loop;
-    instances()->WaitForInstanceDestruction(&loop);
-    loop.Run();
-  }
-
  private:
   std::unique_ptr<InstanceState> TrackInstances() {
     mojom::ServiceManagerPtr service_manager;
@@ -209,7 +191,8 @@ class LifecycleTest : public test::ServiceTest {
                                &service_manager);
     mojom::ServiceManagerListenerPtr listener;
     base::RunLoop loop;
-    InstanceState* state = new InstanceState(MakeRequest(&listener), &loop);
+    InstanceState* state =
+        new InstanceState(MakeRequest(&listener), loop.QuitClosure());
     service_manager->AddListener(std::move(listener));
     loop.Run();
     return base::WrapUnique(state);
@@ -227,11 +210,11 @@ TEST_F(LifecycleTest, Standalone_GracefulQuit) {
   EXPECT_EQ(1u, instances()->GetNewInstanceCount());
 
   base::RunLoop loop;
-  lifecycle.set_connection_error_handler(base::Bind(&QuitLoop, &loop));
+  lifecycle.set_connection_error_handler(loop.QuitClosure());
   lifecycle->GracefulQuit();
   loop.Run();
 
-  WaitForInstanceDestruction();
+  instances()->WaitForInstanceDestruction();
   EXPECT_FALSE(instances()->HasInstanceForName(kTestAppName));
   EXPECT_EQ(0u, instances()->GetNewInstanceCount());
 }
@@ -248,11 +231,11 @@ TEST_F(LifecycleTest, Standalone_Crash) {
   EXPECT_EQ(1u, instances()->GetNewInstanceCount());
 
   base::RunLoop loop;
-  lifecycle.set_connection_error_handler(base::Bind(&QuitLoop, &loop));
+  lifecycle.set_connection_error_handler(loop.QuitClosure());
   lifecycle->Crash();
   loop.Run();
 
-  WaitForInstanceDestruction();
+  instances()->WaitForInstanceDestruction();
   EXPECT_FALSE(instances()->HasInstanceForName(kTestAppName));
   EXPECT_EQ(0u, instances()->GetNewInstanceCount());
 }
@@ -264,10 +247,10 @@ TEST_F(LifecycleTest, Standalone_CloseServiceManagerConnection) {
   EXPECT_EQ(1u, instances()->GetNewInstanceCount());
 
   base::RunLoop loop;
-  lifecycle.set_connection_error_handler(base::Bind(&QuitLoop, &loop));
+  lifecycle.set_connection_error_handler(loop.QuitClosure());
   lifecycle->CloseServiceManagerConnection();
 
-  WaitForInstanceDestruction();
+  instances()->WaitForInstanceDestruction();
 
   // |lifecycle| pipe should still be valid.
   PingPong(lifecycle.get());
@@ -283,11 +266,11 @@ TEST_F(LifecycleTest, PackagedApp_GracefulQuit) {
   EXPECT_EQ(2u, instances()->GetNewInstanceCount());
 
   base::RunLoop loop;
-  lifecycle.set_connection_error_handler(base::Bind(&QuitLoop, &loop));
+  lifecycle.set_connection_error_handler(loop.QuitClosure());
   lifecycle->GracefulQuit();
   loop.Run();
 
-  WaitForInstanceDestruction();
+  instances()->WaitForInstanceDestruction();
   EXPECT_FALSE(instances()->HasInstanceForName(kTestPackageName));
   EXPECT_FALSE(instances()->HasInstanceForName(kTestAppName));
   EXPECT_EQ(0u, instances()->GetNewInstanceCount());
@@ -308,11 +291,11 @@ TEST_F(LifecycleTest, PackagedApp_Crash) {
   EXPECT_EQ(2u, instances()->GetNewInstanceCount());
 
   base::RunLoop loop;
-  lifecycle.set_connection_error_handler(base::Bind(&QuitLoop, &loop));
+  lifecycle.set_connection_error_handler(loop.QuitClosure());
   lifecycle->Crash();
   loop.Run();
 
-  WaitForInstanceDestruction();
+  instances()->WaitForInstanceDestruction();
   EXPECT_FALSE(instances()->HasInstanceForName(kTestPackageName));
   EXPECT_FALSE(instances()->HasInstanceForName(kTestPackageAppNameA));
   EXPECT_EQ(0u, instances()->GetNewInstanceCount());
@@ -342,18 +325,17 @@ TEST_F(LifecycleTest, PackagedApp_CrashCrashesOtherProvidedApp) {
   EXPECT_EQ(3u, instance_count);
 
   base::RunLoop loop;
-  lifecycle_a.set_connection_error_handler(
-      base::Bind(&DecrementCountAndQuitWhenZero, &loop, &instance_count));
-  lifecycle_b.set_connection_error_handler(
-      base::Bind(&DecrementCountAndQuitWhenZero, &loop, &instance_count));
-  lifecycle_package.set_connection_error_handler(
-      base::Bind(&DecrementCountAndQuitWhenZero, &loop, &instance_count));
+  base::RepeatingClosure quit_on_last =
+      base::BarrierClosure(instance_count, loop.QuitClosure());
+  lifecycle_a.set_connection_error_handler(quit_on_last);
+  lifecycle_b.set_connection_error_handler(quit_on_last);
+  lifecycle_package.set_connection_error_handler(quit_on_last);
 
   // Now crash one of the packaged apps.
   lifecycle_a->Crash();
   loop.Run();
 
-  WaitForInstanceDestruction();
+  instances()->WaitForInstanceDestruction();
   EXPECT_FALSE(instances()->HasInstanceForName(kTestPackageName));
   EXPECT_FALSE(instances()->HasInstanceForName(kTestPackageAppNameA));
   EXPECT_FALSE(instances()->HasInstanceForName(kTestPackageAppNameB));
@@ -379,18 +361,17 @@ TEST_F(LifecycleTest, PackagedApp_GracefulQuitPackageQuitsAll) {
   EXPECT_EQ(3u, instance_count);
 
   base::RunLoop loop;
-  lifecycle_a.set_connection_error_handler(
-      base::Bind(&DecrementCountAndQuitWhenZero, &loop, &instance_count));
-  lifecycle_b.set_connection_error_handler(
-      base::Bind(&DecrementCountAndQuitWhenZero, &loop, &instance_count));
-  lifecycle_package.set_connection_error_handler(
-      base::Bind(&DecrementCountAndQuitWhenZero, &loop, &instance_count));
+  base::RepeatingClosure quit_on_last =
+      base::BarrierClosure(instance_count, loop.QuitClosure());
+  lifecycle_a.set_connection_error_handler(quit_on_last);
+  lifecycle_b.set_connection_error_handler(quit_on_last);
+  lifecycle_package.set_connection_error_handler(quit_on_last);
 
   // Now quit the package. All the packaged apps should close.
   lifecycle_package->GracefulQuit();
   loop.Run();
 
-  WaitForInstanceDestruction();
+  instances()->WaitForInstanceDestruction();
   EXPECT_FALSE(instances()->HasInstanceForName(kTestPackageName));
   EXPECT_FALSE(instances()->HasInstanceForName(kTestPackageAppNameA));
   EXPECT_FALSE(instances()->HasInstanceForName(kTestPackageAppNameB));
@@ -406,11 +387,11 @@ TEST_F(LifecycleTest, Exe_GracefulQuit) {
   EXPECT_EQ(1u, instances()->GetNewInstanceCount());
 
   base::RunLoop loop;
-  lifecycle.set_connection_error_handler(base::Bind(&QuitLoop, &loop));
+  lifecycle.set_connection_error_handler(loop.QuitClosure());
   lifecycle->GracefulQuit();
   loop.Run();
 
-  WaitForInstanceDestruction();
+  instances()->WaitForInstanceDestruction();
   EXPECT_FALSE(instances()->HasInstanceForName(kTestExeName));
   EXPECT_EQ(0u, instances()->GetNewInstanceCount());
 
@@ -431,11 +412,12 @@ TEST_F(LifecycleTest, MAYBE_Exe_TerminateProcess) {
   EXPECT_EQ(1u, instances()->GetNewInstanceCount());
 
   base::RunLoop loop;
-  lifecycle.set_connection_error_handler(base::Bind(&QuitLoop, &loop));
+  lifecycle.set_connection_error_handler(loop.QuitClosure());
   process.Terminate(9, true);
+  LOG(ERROR) << "RUN";
   loop.Run();
 
-  WaitForInstanceDestruction();
+  instances()->WaitForInstanceDestruction();
   EXPECT_FALSE(instances()->HasInstanceForName(kTestExeName));
   EXPECT_EQ(0u, instances()->GetNewInstanceCount());
 }