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..d0f2d997423cc5ac60d6956b8bdd3abf864cbb2b 100644
--- a/components/arc/arc_bridge_service_unittest.cc
+++ b/components/arc/arc_bridge_service_unittest.cc
@@ -25,10 +25,14 @@ 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 ArcBridgeServiceImpl::Delegate {
  public:
   ArcBridgeTest() : ready_(false) {}
-  ~ArcBridgeTest() override {}
+  ~ArcBridgeTest() override {};

[Luis Héctor Chávez, 2016/09/07 23:38:32]

remove the ;? (also, = default?)

[hidehiko, 2016/09/08 16:53:41]

Oops. Fixed.

 
   void OnBridgeReady() override {
     state_ = ArcBridgeService::State::READY;
@@ -36,7 +40,9 @@ 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;
+    bootstrap_ = nullptr;
     stop_reason_ = stop_reason;
     message_loop_.task_runner()->PostTask(FROM_HERE,
                                           message_loop_.QuitWhenIdleClosure());
@@ -44,12 +50,19 @@ class ArcBridgeTest : public testing::Test, public ArcBridgeService::Observer {
 
   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();
@@ -58,25 +71,47 @@ 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())));
+    boot_failure_emulation_enabled_ = false;
+    boot_suspended_ = false;
 
+    service_.reset(new ArcBridgeServiceImpl);

[Luis Héctor Chávez, 2016/09/07 23:38:32]

Any reason why you're not using parentheses? I've seen a couple of bugs caused
by omitting them and not being super explicit with all the member
initialization.

[hidehiko, 2016/09/08 16:53:41]

For consistency, with variables on stack. As you may know, "T var()" means a
function prototype, taking no-args and returning T. "T()" in some context means
anonymous function.

I'm interested in the problem caused by missing parenthesis. Could you share,
just before adding ()s?

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

There was a recent discussion about this in chromium-dev when doing the
WrapUnique -> MakeUnique migration:

https://groups.google.com/a/chromium.org/d/topic/chromium-dev/iQgMedVA8-k/dis...

There was another similar-but-not-quite discussion about it here, where side
effects of one such initialization were non-obvious:

https://groups.google.com/a/chromium.org/d/topic/chromium-dev/xH1-ac_nDCY/dis...

I know none of these apply here, but I had to go look at the constructor to make
sure. 

In any case, something that came off the first thread I shared is that the C++
Dos and Don'ts were updated to reflect this decision:

https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...

"Don't make future maintainers guess whether you left off the '()' on purpose.
[...] If you're intentionally leaving off the "()" as an optimisation, please
leave a comment."

so, please leave a comment if you absolutely want to leave the parens out, but I
encourage to add them since there is no optimization IIUC. In this context it's
impossible for the most vexing parse to kick in here since it's prefixed by
'new', but in that case I would have suggested using C++11's uniform
initialization.

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

I see. Thank you for explanation and references.
I used () in this CL for consistency. We can replace it by {} as you suggested
anytime we want later, I think.

+    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.

[Luis Héctor Chávez, 2016/09/07 23:38:32]

This looks dangerous :S

Anyways, is there any chance that the factory pattern can help here? If you
construct the factory with the flags, maybe you don't need to access the object
later?

[hidehiko, 2016/09/08 16:53:41]

Ok, now I rewrote it under Factory redesign.

+    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);
 };
 
@@ -87,7 +122,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 +133,39 @@ TEST_F(ArcBridgeTest, Basic) {
 TEST_F(ArcBridgeTest, ShutdownMidStartup) {
   ASSERT_FALSE(ready());
 
+  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());
 }
 
+// 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());
 
   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 +177,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 +201,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.