Fix ArcBridgeBootstrap race issues.

components/arc/arc_bridge_service_unittest.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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 "base/single_thread_task_runner.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 {
 
 namespace {
 
 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;
     ready_ = true;
   }
 
   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,
                                           message_loop_.QuitWhenIdleClosure());
   }
 
   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();
 
     ready_ = false;
     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_;
 
   DISALLOW_COPY_AND_ASSIGN(ArcBridgeTest);
 };
 
 // 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());
 
   service_->HandleStartup();
-  instance_->WaitForInitCall();
   ASSERT_EQ(ArcBridgeService::State::READY, state());
 
   service_->Shutdown();
   ASSERT_EQ(ArcBridgeService::State::STOPPED, state());
 }
 
 // If the ArcBridgeService is shut down, it should be stopped, even
 // mid-startup.
 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());
 }
 
 // Makes sure OnBridgeStopped is called on stop.
 TEST_F(ArcBridgeTest, OnBridgeStopped) {
   ASSERT_FALSE(ready());
 
   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();
   ASSERT_EQ(ArcBridgeService::StopReason::SHUTDOWN, stop_reason_);
   ASSERT_EQ(ArcBridgeService::State::STOPPED, state());
 }
 
 // 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.
 TEST_F(ArcBridgeTest, RemoveUnknownObserver) {
   ASSERT_FALSE(ready());
   auto dummy_observer = base::MakeUnique<DummyObserver>();
   service_->RemoveObserver(dummy_observer.get());
 }
 
 }  // namespace arc