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 d85b5562ee269be85d82263430393505c03e46da..6656012f76269216d9d26609a99e84ff3d987786 100644
--- a/components/arc/arc_bridge_service_unittest.cc
+++ b/components/arc/arc_bridge_service_unittest.cc
@@ -4,16 +4,10 @@
 
 #include <memory>
 
-#include "base/bind.h"
-#include "base/bind_helpers.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
-#include "base/run_loop.h"
-#include "chromeos/dbus/dbus_thread_manager.h"
 #include "components/arc/arc_bridge_service_impl.h"
 #include "components/arc/test/fake_arc_bridge_bootstrap.h"
-#include "components/arc/test/fake_arc_bridge_instance.h"
-#include "mojo/public/cpp/system/message_pipe.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace arc {
@@ -24,56 +18,71 @@ class DummyObserver : public ArcBridgeService::Observer {};
 
 }  // namespace
 
-class ArcBridgeTest : public testing::Test, public ArcBridgeService::Observer {
+class ArcBridgeTest : public testing::Test,
+                      public ArcBridgeService::Observer,
+                      public ArcBridgeServiceImpl::Delegate {
  public:
-  ArcBridgeTest() : ready_(false) {}
-  ~ArcBridgeTest() override {}
-
-  void OnBridgeReady() override {
-    state_ = ArcBridgeService::State::READY;
-    ready_ = true;
-  }
+  ArcBridgeTest() = default;
+  ~ArcBridgeTest() override = default;
 
   void OnBridgeStopped(ArcBridgeService::StopReason stop_reason) override {
-    state_ = ArcBridgeService::State::STOPPED;
+    // The instance is already destructed in ArcBridgeServiceImpl::OnStopped().
+    bootstrap_ = nullptr;
+
     stop_reason_ = stop_reason;
-    message_loop_.PostTask(FROM_HERE, message_loop_.QuitWhenIdleClosure());
   }
 
-  bool ready() const { return ready_; }
-  ArcBridgeService::State state() const { return state_; }
+  FakeArcBridgeBootstrap* bootstrap() const { return bootstrap_; }
 
  protected:
   std::unique_ptr<ArcBridgeServiceImpl> service_;
-  std::unique_ptr<FakeArcBridgeInstance> instance_;
   ArcBridgeService::StopReason stop_reason_;
 
+  void EnableBootFailureEmulation(ArcBridgeService::StopReason reason) {
+    boot_failure_emulation_enabled_ = true;
+    boot_failure_reason_ = reason;
+  }
+
+  void SuspendBoot() { boot_suspended_ = true; }
+
  private:
   void SetUp() override {
-    chromeos::DBusThreadManager::Initialize();
-
-    ready_ = false;
-    state_ = ArcBridgeService::State::STOPPED;
     stop_reason_ = ArcBridgeService::StopReason::SHUTDOWN;
 
-    instance_.reset(new FakeArcBridgeInstance());
-    service_.reset(new ArcBridgeServiceImpl(
-        base::MakeUnique<FakeArcBridgeBootstrap>(instance_.get())));
+    boot_failure_emulation_enabled_ = false;
+    boot_suspended_ = false;
 
+    service_.reset(new ArcBridgeServiceImpl);
+    service_->SetDelegateForTesting(this);
     service_->AddObserver(this);
   }
 
   void TearDown() override {
     service_->RemoveObserver(this);
-    instance_.reset();
     service_.reset();
+  }
 
-    chromeos::DBusThreadManager::Shutdown();
+  // ArcBridgeServiceImpl::Delegate.
+  std::unique_ptr<ArcBridgeBootstrap> CreateBootstrap() override {
+    // The testee ArcBridgeServiceImpl instance has the ownership. So, keep
+    // the raw pointer only here.
+    bootstrap_ = new FakeArcBridgeBootstrap;
+    if (boot_failure_emulation_enabled_) {
+      bootstrap_->EnableBootFailureEmulation(boot_failure_reason_);
+    }
+    if (boot_suspended_) {
+      bootstrap_->SuspendBoot();
+    }
+
+    return base::WrapUnique(bootstrap_);
   }
 
-  bool ready_;
-  ArcBridgeService::State state_;
-  base::MessageLoopForUI message_loop_;
+  // Tracks the current bootstrap instance. This depends on the implementation
+  // details of ArcBridgeServiceImpl.
+  FakeArcBridgeBootstrap* bootstrap_ = nullptr;
+  bool boot_failure_emulation_enabled_ = false;
+  ArcBridgeService::StopReason boot_failure_reason_;
+  bool boot_suspended_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(ArcBridgeTest);
 };
@@ -81,88 +90,98 @@ class ArcBridgeTest : public testing::Test, public ArcBridgeService::Observer {
 // Exercises the basic functionality of the ARC Bridge Service.  A message from
 // within the instance should cause the observer to be notified.
 TEST_F(ArcBridgeTest, Basic) {
-  ASSERT_FALSE(ready());
-  ASSERT_EQ(ArcBridgeService::State::STOPPED, state());
+  ASSERT_TRUE(service_->stopped());
 
   service_->HandleStartup();
-  instance_->WaitForInitCall();
-  ASSERT_EQ(ArcBridgeService::State::READY, state());
+  ASSERT_TRUE(service_->ready());
 
   service_->Shutdown();
-  ASSERT_EQ(ArcBridgeService::State::STOPPED, state());
+  ASSERT_TRUE(service_->stopped());
 }
 
 // If the ArcBridgeService is shut down, it should be stopped, even
 // mid-startup.
 TEST_F(ArcBridgeTest, ShutdownMidStartup) {
-  ASSERT_FALSE(ready());
+  ASSERT_TRUE(service_->stopped());
 
+  SuspendBoot();
   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());
+  ASSERT_TRUE(service_->stopped());
+}
+
+// If the boot procedure is failed, then restarting mechanism should not
+// triggered.
+TEST_F(ArcBridgeTest, BootFailure) {
+  ASSERT_TRUE(service_->stopped());
+
+  EnableBootFailureEmulation(
+      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());
+  ASSERT_TRUE(service_->stopped());
 
   service_->HandleStartup();
-  instance_->WaitForInitCall();
-  ASSERT_EQ(ArcBridgeService::State::READY, state());
-  ASSERT_EQ(1, instance_->init_calls());
+  ASSERT_TRUE(service_->ready());
 
   // 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());
+  ASSERT_TRUE(service_->stopped());
 }
 
 // Makes sure OnBridgeStopped is called on stop.
 TEST_F(ArcBridgeTest, OnBridgeStopped) {
-  ASSERT_FALSE(ready());
+  ASSERT_TRUE(service_->stopped());
 
   service_->DisableReconnectDelayForTesting();
   service_->HandleStartup();
-  instance_->WaitForInitCall();
-  ASSERT_EQ(ArcBridgeService::State::READY, state());
+  ASSERT_TRUE(service_->ready());
 
   // 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();
-  ASSERT_EQ(ArcBridgeService::StopReason::SHUTDOWN, stop_reason_);
-  ASSERT_EQ(ArcBridgeService::State::STOPPED, state());
+  EXPECT_EQ(ArcBridgeService::StopReason::SHUTDOWN, stop_reason_);
+  ASSERT_TRUE(service_->stopped());
 }
 
 // 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|.
+  ASSERT_TRUE(service_->stopped());
+  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.
 TEST_F(ArcBridgeTest, RemoveUnknownObserver) {
-  ASSERT_FALSE(ready());
+  ASSERT_TRUE(service_->stopped());
   auto dummy_observer = base::MakeUnique<DummyObserver>();
   service_->RemoveObserver(dummy_observer.get());
 }