Refactor ArcBridgeServiceImpl part 1.

This CL refactors ArcBridgeServiceImpl focusing on the
retry mechanism.
- With this CL, it starts to use OneShotTimer, which supports
cancel operation.
- Rename "reconnect" to "restart" for wording consistency.
- Introduce restart_delay_ for testing, instead of special
  bool flag. Also, for restarting of testing, get rid of
  synchronous PrerequisitesChanged() call, instead PostTask always.
- Extract StartArcSession() for sharing PrerequisitesChanged()
  and restarting.
- Added more DCHECKs.

BUG=657687
BUG=b/31079732
TEST=Ran bots.

Review-Url: https://codereview.chromium.org/2582003002
Cr-Commit-Position: refs/heads/master@{#439850}

components/arc/arc_bridge_service_impl.cc

@@ -22,13 +22,18 @@
 #include "components/prefs/pref_service.h"
 
 namespace arc {
+
 namespace {
-constexpr int64_t kReconnectDelayInSeconds = 5;
+
+constexpr base::TimeDelta kDefaultRestartDelay =
+    base::TimeDelta::FromSeconds(5);
+
 }  // namespace
 
 ArcBridgeServiceImpl::ArcBridgeServiceImpl(
     const scoped_refptr<base::TaskRunner>& blocking_task_runner)
     : session_started_(false),
+      restart_delay_(kDefaultRestartDelay),
       factory_(base::Bind(ArcSession::Create, this, blocking_task_runner)),
       weak_factory_(this) {}
 
@@ -57,13 +62,18 @@ void ArcBridgeServiceImpl::RequestStop() {
 
 void ArcBridgeServiceImpl::OnShutdown() {
   DCHECK(CalledOnValidThread());
+
   VLOG(1) << "OnShutdown";
-  if (!session_started_)
-    return;
   session_started_ = false;
-  reconnect_ = false;
-  if (arc_session_)
+  restart_timer_.Stop();
+  if (arc_session_) {
+    if (state() != State::STOPPING)
+      SetState(State::STOPPING);
     arc_session_->OnShutdown();
+  }
+  // ArcSession::OnShutdown() invokes OnSessionStopped() synchronously.
+  // In the observer method, |arc_session_| should be destroyed.
+  DCHECK(!arc_session_);
 }
 
 void ArcBridgeServiceImpl::SetArcSessionFactoryForTesting(
@@ -72,8 +82,12 @@ void ArcBridgeServiceImpl::SetArcSessionFactoryForTesting(
   factory_ = factory;
 }
 
-void ArcBridgeServiceImpl::DisableReconnectDelayForTesting() {
-  use_delay_before_reconnecting_ = false;
+void ArcBridgeServiceImpl::SetRestartDelayForTesting(
+    const base::TimeDelta& restart_delay) {
+  DCHECK_EQ(state(), State::STOPPED);
+  DCHECK(!arc_session_);
+  DCHECK(!restart_timer_.IsRunning());
+  restart_delay_ = restart_delay;
 }
 
 void ArcBridgeServiceImpl::PrerequisitesChanged() {
@@ -82,19 +96,16 @@ void ArcBridgeServiceImpl::PrerequisitesChanged() {
           << "state=" << static_cast<uint32_t>(state())
           << ", session_started=" << session_started_;
   if (state() == State::STOPPED) {
-    if (!session_started_)
+    if (!session_started_) {
+      // We were explicitly asked to stop, so do not restart.
+      restart_timer_.Stop();
       return;
+    }
+    DCHECK(!restart_timer_.IsRunning());
     VLOG(0) << "Prerequisites met, starting ARC";
-    SetStopReason(StopReason::SHUTDOWN);
-
-    if (arc_session_)
-      arc_session_->RemoveObserver(this);
-
-    SetState(State::CONNECTING);
-    arc_session_ = factory_.Run();
-    arc_session_->AddObserver(this);
-    arc_session_->Start();
+    StartArcSession();
   } else {
+    DCHECK(!restart_timer_.IsRunning());
     if (session_started_)
       return;
     VLOG(0) << "Prerequisites stopped being met, stopping ARC";
@@ -102,16 +113,30 @@ void ArcBridgeServiceImpl::PrerequisitesChanged() {
   }
 }
 
+void ArcBridgeServiceImpl::StartArcSession() {
+  DCHECK(CalledOnValidThread());
+  DCHECK_EQ(state(), State::STOPPED);
+  DCHECK(!arc_session_);
+  DCHECK(!restart_timer_.IsRunning());
+
+  VLOG(1) << "Starting ARC instance";
+  SetStopReason(StopReason::SHUTDOWN);
+  arc_session_ = factory_.Run();
+  arc_session_->AddObserver(this);
+  SetState(State::CONNECTING);
+  arc_session_->Start();
+}
+
 void ArcBridgeServiceImpl::StopInstance() {
   DCHECK(CalledOnValidThread());
-  if (state() == State::STOPPED || state() == State::STOPPING) {
+  DCHECK_NE(state(), State::STOPPED);
+  DCHECK(!restart_timer_.IsRunning());
+
+  if (state() == State::STOPPING) {
     VLOG(1) << "StopInstance() called when ARC is not running";
     return;
   }
 
-  // We were explicitly asked to stop, so do not reconnect.
-  reconnect_ = false;
-
   VLOG(1) << "Stopping ARC";
   DCHECK(arc_session_.get());
   SetState(State::STOPPING);
@@ -127,39 +152,47 @@ void ArcBridgeServiceImpl::OnSessionReady() {
     return;
   }
 
-  // The container can be considered to have been successfully launched, so
-  // restart if the connection goes down without being requested.
-  reconnect_ = true;
   VLOG(0) << "ARC ready";
   SetState(State::READY);
 }
 
 void ArcBridgeServiceImpl::OnSessionStopped(StopReason stop_reason) {
   DCHECK(CalledOnValidThread());
+  DCHECK(!restart_timer_.IsRunning());
+
   VLOG(0) << "ARC stopped: " << stop_reason;
   arc_session_->RemoveObserver(this);
   arc_session_.reset();
-  SetStopReason(stop_reason);
-  SetState(State::STOPPED);
 
-  if (reconnect_) {
+  // If READY, ARC instance unexpectedly crashed so we need to restart it
+  // automatically.
+  // If STOPPING, at least once RequestStop() is called. If |session_started_|
+  // is true, RequestStart() is following so schedule to restart ARC session.
+  // Otherwise, do nothing.
+  // If CONNECTING, ARC instance has not been booted properly, so do not
+  // restart it automatically.
+  if (state() == State::READY ||
+      (state() == State::STOPPING && session_started_)) {
+    // This check is for READY case. In READY case |session_started_| should be
+    // always true, because if once RequestStop() is called, the state() will
+    // be set to STOPPING.
+    DCHECK(session_started_);
+
     // There was a previous invocation and it crashed for some reason. Try
-    // starting the container again.
-    reconnect_ = false;
-    VLOG(0) << "ARC reconnecting";
-    if (use_delay_before_reconnecting_) {
-      // Instead of immediately trying to restart the container, give it some
-      // time to finish tearing down in case it is still in the process of
-      // stopping.
-      base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-          FROM_HERE, base::Bind(&ArcBridgeServiceImpl::PrerequisitesChanged,
-                                weak_factory_.GetWeakPtr()),
-          base::TimeDelta::FromSeconds(kReconnectDelayInSeconds));
-    } else {
-      // Restart immediately.
-      PrerequisitesChanged();
-    }
+    // starting ARC instance later again.
+    // Note that even |restart_delay_| is 0 (for testing), it needs to
+    // PostTask, because observer callback may call RequestStart()/Stop(),
+    // which can change restarting.
+    VLOG(0) << "ARC restarting";
+    restart_timer_.Start(FROM_HERE, restart_delay_,
+                         base::Bind(&ArcBridgeServiceImpl::StartArcSession,
+                                    weak_factory_.GetWeakPtr()));
   }
+
+  // TODO(hidehiko): Consider to let observers know whether there is scheduled
+  // restarting event, or not.
+  SetStopReason(stop_reason);
+  SetState(State::STOPPED);
 }
 
 }  // namespace arc

components/arc/arc_bridge_service_impl.h

@@ -13,6 +13,8 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/task_runner.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
 #include "components/arc/arc_bridge_service.h"
 #include "components/arc/arc_session_observer.h"
 #include "mojo/public/cpp/bindings/binding.h"
@@ -45,10 +47,10 @@ class ArcBridgeServiceImpl : public ArcBridgeService,
   // Returns the current ArcSession instance for testing purpose.
   ArcSession* GetArcSessionForTesting() { return arc_session_.get(); }
 
-  // Normally, reconnecting after connection shutdown happens after a short
-  // delay. When testing, however, we'd like it to happen immediately to avoid
-  // adding unnecessary delays.
-  void DisableReconnectDelayForTesting();
+  // Normally, automatic restarting happens after a short delay. When testing,
+  // however, we'd like it to happen immediately to avoid adding unnecessary
+  // delays.
+  void SetRestartDelayForTesting(const base::TimeDelta& restart_delay);
 
  private:
   // If all pre-requisites are true (ARC is available, it has been enabled, and
@@ -56,6 +58,9 @@ class ArcBridgeServiceImpl : public ArcBridgeService,
   // and the pre-requisites stop being true, stop ARC.
   void PrerequisitesChanged();
 
+  // Starts to run an ARC instance.
+  void StartArcSession();
+
   // Stops the running instance.
   void StopInstance();
 
@@ -68,12 +73,10 @@ class ArcBridgeServiceImpl : public ArcBridgeService,
   // If the user's session has started.
   bool session_started_;
 
-  // If the instance had already been started but the connection to it was
-  // lost. This should make the instance restart.
-  bool reconnect_ = false;
-
-  // Delay the reconnection.
-  bool use_delay_before_reconnecting_ = true;
+  // Instead of immediately trying to restart the container, give it some time
+  // to finish tearing down in case it is still in the process of stopping.
+  base::TimeDelta restart_delay_;
+  base::OneShotTimer restart_timer_;
 
   // Factory to inject a fake ArcSession instance for testing.
   ArcSessionFactory factory_;

components/arc/arc_bridge_service_unittest.cc

@@ -155,7 +155,7 @@ TEST_F(ArcBridgeTest, BootFailure) {
 
 // If the instance is stopped, it should be re-started.
 TEST_F(ArcBridgeTest, Restart) {
-  arc_bridge_service()->DisableReconnectDelayForTesting();
+  arc_bridge_service()->SetRestartDelayForTesting(base::TimeDelta());
   EXPECT_TRUE(arc_bridge_service()->stopped());
 
   arc_bridge_service()->RequestStart();
@@ -164,6 +164,8 @@ TEST_F(ArcBridgeTest, Restart) {
   // Simulate a connection loss.
   ASSERT_TRUE(arc_session());
   arc_session()->StopWithReason(StopReason::CRASH);
+  EXPECT_TRUE(arc_bridge_service()->stopped());
+  base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(arc_bridge_service()->ready());
 
   arc_bridge_service()->RequestStop();
@@ -172,7 +174,7 @@ TEST_F(ArcBridgeTest, Restart) {
 
 // Makes sure OnSessionStopped is called on stop.
 TEST_F(ArcBridgeTest, OnSessionStopped) {
-  arc_bridge_service()->DisableReconnectDelayForTesting();
+  arc_bridge_service()->SetRestartDelayForTesting(base::TimeDelta());
   EXPECT_TRUE(arc_bridge_service()->stopped());
 
   arc_bridge_service()->RequestStart();
@@ -182,12 +184,16 @@ TEST_F(ArcBridgeTest, OnSessionStopped) {
   ASSERT_TRUE(arc_session());
   arc_session()->StopWithReason(StopReason::GENERIC_BOOT_FAILURE);
   EXPECT_EQ(StopReason::GENERIC_BOOT_FAILURE, stop_reason());
+  EXPECT_TRUE(arc_bridge_service()->stopped());
+  base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(arc_bridge_service()->ready());
 
   // Simulate crash.
   ASSERT_TRUE(arc_session());
   arc_session()->StopWithReason(StopReason::CRASH);
   EXPECT_EQ(StopReason::CRASH, stop_reason());
+  EXPECT_TRUE(arc_bridge_service()->stopped());
+  base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(arc_bridge_service()->ready());
 
   // Graceful stop.
@@ -197,7 +203,7 @@ TEST_F(ArcBridgeTest, OnSessionStopped) {
 }
 
 TEST_F(ArcBridgeTest, Shutdown) {
-  arc_bridge_service()->DisableReconnectDelayForTesting();
+  arc_bridge_service()->SetRestartDelayForTesting(base::TimeDelta());
   EXPECT_TRUE(arc_bridge_service()->stopped());
 
   arc_bridge_service()->RequestStart();