Fix ArcBridgeBootstrap race issues.

components/arc/arc_bridge_service_unittest.cc

Index: components/arc/arc_bridge_service_unittest.cc
diff --git a/components/arc/arc_bridge_service_unittest.cc b/components/arc/arc_bridge_service_unittest.cc
index cff48893c182a1a056df97f970f13aea4287b4b7..0585672798b752f30a63f097bd1b405985661c3c 100644
--- a/components/arc/arc_bridge_service_unittest.cc
+++ b/components/arc/arc_bridge_service_unittest.cc
@@ -25,10 +25,12 @@ class DummyObserver : public ArcBridgeService::Observer {};
 
 }  // namespace
 
-class ArcBridgeTest : public testing::Test, public ArcBridgeService::Observer {
+// TODO(hidehiko): ArcBridgeTest gets complicated and has stale code.
+// Simplify the code.
+class ArcBridgeTest : public testing::Test,
+                      public ArcBridgeService::Observer {
  public:
-  ArcBridgeTest() : ready_(false) {}
-  ~ArcBridgeTest() override {}
+  ArcBridgeTest() = default;
 
   void OnBridgeReady() override {
     state_ = ArcBridgeService::State::READY;
@@ -36,6 +38,7 @@ class ArcBridgeTest : public testing::Test, public ArcBridgeService::Observer {
   }
 
   void OnBridgeStopped(ArcBridgeService::StopReason stop_reason) override {
+    // The instance is already destructed in ArcBridgeServiceImpl::OnStopped().
     state_ = ArcBridgeService::State::STOPPED;
     stop_reason_ = stop_reason;
     message_loop_.task_runner()->PostTask(FROM_HERE,
@@ -44,12 +47,30 @@ class ArcBridgeTest : public testing::Test, public ArcBridgeService::Observer {
 
   bool ready() const { return ready_; }
   ArcBridgeService::State state() const { return state_; }
+  FakeArcBridgeBootstrap* bootstrap() const {
+    return static_cast<FakeArcBridgeBootstrap*>(
+        service_->GetBootstrapForTesting());
+  }
 
  protected:
   std::unique_ptr<ArcBridgeServiceImpl> service_;
-  std::unique_ptr<FakeArcBridgeInstance> instance_;
   ArcBridgeService::StopReason stop_reason_;
 
+  static std::unique_ptr<ArcBridgeBootstrap> CreateSuspendedBootstrap() {
+    std::unique_ptr<FakeArcBridgeBootstrap> bootstrap(
+        new FakeArcBridgeBootstrap);

[Luis Héctor Chávez, 2016/09/15 23:08:49]

same comment about default/value initialization. In this case you can use auto
bootstrap = base::MakeUnique<FakeArcBridgeBootstrap>() with absolutely no
penalty. Same below.

[hidehiko, 2016/09/20 13:35:19]

Done. Replaced "unique_ptr + new" by "auto + MakeUnique" for all occurrence in
this CL.

+    bootstrap->SuspendBoot();
+    return bootstrap;
+  }
+
+  static std::unique_ptr<ArcBridgeBootstrap> CreateBootFailureBootstrap(
+      ArcBridgeService::StopReason reason) {
+    std::unique_ptr<FakeArcBridgeBootstrap> bootstrap(
+        new FakeArcBridgeBootstrap);
+    bootstrap->EnableBootFailureEmulation(reason);
+    return bootstrap;
+  }
+
  private:
   void SetUp() override {
     chromeos::DBusThreadManager::Initialize();
@@ -58,22 +79,20 @@ class ArcBridgeTest : public testing::Test, public ArcBridgeService::Observer {
     state_ = ArcBridgeService::State::STOPPED;
     stop_reason_ = ArcBridgeService::StopReason::SHUTDOWN;
 
-    instance_.reset(new FakeArcBridgeInstance());
-    service_.reset(new ArcBridgeServiceImpl(
-        base::MakeUnique<FakeArcBridgeBootstrap>(instance_.get())));
-
+    service_.reset(new ArcBridgeServiceImpl);
+    service_->SetArcBridgeBootstrapFactoryForTesting(
+        base::Bind(FakeArcBridgeBootstrap::Create));
     service_->AddObserver(this);
   }
 
   void TearDown() override {
     service_->RemoveObserver(this);
-    instance_.reset();
     service_.reset();
 
     chromeos::DBusThreadManager::Shutdown();
   }
 
-  bool ready_;
+  bool ready_ = false;
   ArcBridgeService::State state_;
   base::MessageLoopForUI message_loop_;
 
@@ -87,7 +106,6 @@ TEST_F(ArcBridgeTest, Basic) {
   ASSERT_EQ(ArcBridgeService::State::STOPPED, state());
 
   service_->HandleStartup();
-  instance_->WaitForInitCall();
   ASSERT_EQ(ArcBridgeService::State::READY, state());
 
   service_->Shutdown();
@@ -99,30 +117,41 @@ TEST_F(ArcBridgeTest, Basic) {
 TEST_F(ArcBridgeTest, ShutdownMidStartup) {
   ASSERT_FALSE(ready());
 
+  service_->SetArcBridgeBootstrapFactoryForTesting(
+      base::Bind(ArcBridgeTest::CreateSuspendedBootstrap));
   service_->HandleStartup();
-  // WaitForInitCall() omitted.
-  ASSERT_EQ(ArcBridgeService::State::READY, state());
+  ASSERT_FALSE(service_->stopped());
+  ASSERT_FALSE(service_->ready());
 
   service_->Shutdown();
   ASSERT_EQ(ArcBridgeService::State::STOPPED, state());
 }
 
+// If the boot procedure is failed, then restarting mechanism should not
+// triggered.
+TEST_F(ArcBridgeTest, BootFailure) {
+  ASSERT_TRUE(service_->stopped());
+
+  service_->SetArcBridgeBootstrapFactoryForTesting(
+      base::Bind(ArcBridgeTest::CreateBootFailureBootstrap,
+                 ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE));
+  service_->HandleStartup();
+  EXPECT_EQ(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE, stop_reason_);
+  ASSERT_TRUE(service_->stopped());
+}
+
 // If the instance is stopped, it should be re-started.
 TEST_F(ArcBridgeTest, Restart) {
   ASSERT_FALSE(ready());
-  ASSERT_EQ(0, instance_->init_calls());
 
   service_->HandleStartup();
-  instance_->WaitForInitCall();
   ASSERT_EQ(ArcBridgeService::State::READY, state());
-  ASSERT_EQ(1, instance_->init_calls());
 
   // Simulate a connection loss.
   service_->DisableReconnectDelayForTesting();
-  instance_->Stop(ArcBridgeService::StopReason::CRASH);
-  instance_->WaitForInitCall();
-  ASSERT_EQ(ArcBridgeService::State::READY, state());
-  ASSERT_EQ(2, instance_->init_calls());
+  ASSERT_TRUE(bootstrap());
+  bootstrap()->StopWithReason(ArcBridgeService::StopReason::CRASH);
+  ASSERT_TRUE(service_->ready());
 
   service_->Shutdown();
   ASSERT_EQ(ArcBridgeService::State::STOPPED, state());
@@ -134,20 +163,20 @@ TEST_F(ArcBridgeTest, OnBridgeStopped) {
 
   service_->DisableReconnectDelayForTesting();
   service_->HandleStartup();
-  instance_->WaitForInitCall();
   ASSERT_EQ(ArcBridgeService::State::READY, state());
 
   // Simulate boot failure.
-  instance_->Stop(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE);
-  instance_->WaitForInitCall();
-  ASSERT_EQ(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE, stop_reason_);
-  ASSERT_EQ(ArcBridgeService::State::READY, state());
+  ASSERT_TRUE(bootstrap());
+  bootstrap()->StopWithReason(
+      ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE);
+  EXPECT_EQ(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE, stop_reason_);
+  ASSERT_TRUE(service_->ready());
 
   // Simulate crash.
-  instance_->Stop(ArcBridgeService::StopReason::CRASH);
-  instance_->WaitForInitCall();
-  ASSERT_EQ(ArcBridgeService::StopReason::CRASH, stop_reason_);
-  ASSERT_EQ(ArcBridgeService::State::READY, state());
+  ASSERT_TRUE(bootstrap());
+  bootstrap()->StopWithReason(ArcBridgeService::StopReason::CRASH);
+  EXPECT_EQ(ArcBridgeService::StopReason::CRASH, stop_reason_);
+  ASSERT_TRUE(service_->ready());
 
   // Graceful shutdown.
   service_->Shutdown();
@@ -158,8 +187,11 @@ TEST_F(ArcBridgeTest, OnBridgeStopped) {
 // Removing the same observer more than once should be okay.
 TEST_F(ArcBridgeTest, RemoveObserverTwice) {
   ASSERT_FALSE(ready());
-  service_->RemoveObserver(this);
-  // The teardown method will also remove |this|.
+  auto dummy_observer = base::MakeUnique<DummyObserver>();
+  service_->AddObserver(dummy_observer.get());
+  // Call RemoveObserver() twice.
+  service_->RemoveObserver(dummy_observer.get());
+  service_->RemoveObserver(dummy_observer.get());
 }
 
 // Removing an unknown observer should be allowed.