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 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;
     ready_ = true;
   }
 
   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());
   }
 
   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);

[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);
 };
 
 // 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());
 
+  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());
 }
 
 // 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