arc: Notify ARC instance failures via callbacks.

StopReason is passed to following observer methods:
- ArcBridgeBootstrap::Observer::OnStopped
- ArcBridgeService::Observer::OnBridgeStopped

Those callbacks can now tell the reason the bridge stopped.

I am planning to use this soon to show notifications on failures.
See the bug entry for more details.

BUG=chromium:625923
TEST=components_unittests --gtest_filter=ArcBridgeTest.*
TEST=Manually verified that ARC boots.

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

chrome/browser/chromeos/arc/arc_auth_service.cc

@@ -192,7 +192,8 @@ void ArcAuthService::OnInstanceReady() {
       binding_.CreateInterfacePtrAndBind());
 }
 
-void ArcAuthService::OnBridgeStopped() {
+void ArcAuthService::OnBridgeStopped(ArcBridgeService::StopReason reason) {
+  // TODO(crbug.com/625923): Use |reason| to report more detailed errors.
   if (waiting_for_reply_) {
     // Using SERVICE_UNAVAILABLE instead of UNKNOWN_ERROR, since the latter
     // causes this code to not try to stop ARC, so it would retry without the

chrome/browser/chromeos/arc/arc_auth_service.h

@@ -124,7 +124,7 @@ class ArcAuthService : public ArcService,
   void RemoveObserver(Observer* observer);
 
   // ArcBridgeService::Observer:
-  void OnBridgeStopped() override;
+  void OnBridgeStopped(ArcBridgeService::StopReason reason) override;
 
   // InstanceHolder<mojom::AuthInstance>::Observer:
   void OnInstanceReady() override;

chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc

@@ -237,7 +237,7 @@ ArcAppListPrefs::ArcAppListPrefs(const base::FilePath& base_path,
   bridge_service->app()->AddObserver(this);
   bridge_service->AddObserver(this);
   if (!bridge_service->ready())
-    OnBridgeStopped();
+    OnBridgeStopped(arc::ArcBridgeService::StopReason::SHUTDOWN);
 }
 
 ArcAppListPrefs::~ArcAppListPrefs() {
@@ -563,7 +563,8 @@ void ArcAppListPrefs::OnOptInEnabled(bool enabled) {
     RemoveAllApps();
 }
 
-void ArcAppListPrefs::OnBridgeStopped() {
+void ArcAppListPrefs::OnBridgeStopped(
+    arc::ArcBridgeService::StopReason reason) {
   DisableAllApps();
 }
 

chrome/browser/ui/app_list/arc/arc_app_list_prefs.h

@@ -209,7 +209,7 @@ class ArcAppListPrefs
   ArcAppListPrefs(const base::FilePath& base_path, PrefService* prefs);
 
   // arc::ArcBridgeService::Observer:
-  void OnBridgeStopped() override;
+  void OnBridgeStopped(arc::ArcBridgeService::StopReason reason) override;
 
   // arc::InstanceHolder<arc::mojom::AppInstance>::Observer:
   void OnInstanceReady() override;

components/arc/arc_bridge_bootstrap.cc

@@ -73,11 +73,11 @@ class ArcBridgeBootstrapImpl : public ArcBridgeBootstrap,
   //   AcceptInstanceConnection() -> OnInstanceConnected() ->
   // READY
   //
-  // When Stop() is called from any state, either because an operation
-  // resulted in an error or because the user is logging out:
+  // When Stop() or AbortBoot() is called from any state, either because an
+  // operation resulted in an error or because the user is logging out:
   //
   // (any)
-  //   Stop() ->
+  //   Stop()/AbortBoot() ->
   // STOPPING
   //   StopInstance() ->
   // STOPPED
@@ -86,6 +86,9 @@ class ArcBridgeBootstrapImpl : public ArcBridgeBootstrap,
   //   READY -> STOPPING -> STOPPED
   // and then restarted:
   //   STOPPED -> SOCKET_CREATING -> ... -> READY).
+  //
+  // Note: Order of constants below matters. Please make sure to sort them
+  // in chronological order.
   enum class State {
     // ARC is not currently running.
     STOPPED,
@@ -114,6 +117,10 @@ class ArcBridgeBootstrapImpl : public ArcBridgeBootstrap,
   void Stop() override;
 
  private:
+  // Aborts ARC instance boot. This is called from various state-machine
+  // functions when they encounter an error during boot.
+  void AbortBoot(ArcBridgeService::StopReason reason);
+
   // Creates the UNIX socket on the bootstrap thread and then processes its
   // file descriptor.
   static base::ScopedFD CreateSocket();
@@ -135,6 +142,10 @@ class ArcBridgeBootstrapImpl : public ArcBridgeBootstrap,
   // The state of the bootstrap connection.
   State state_ = State::STOPPED;
 
+  // The reason the ARC instance is stopped.
+  ArcBridgeService::StopReason stop_reason_ =
+      ArcBridgeService::StopReason::SHUTDOWN;
+
   base::ThreadChecker thread_checker_;
 
   // WeakPtrFactory to use callbacks.
@@ -168,6 +179,7 @@ void ArcBridgeBootstrapImpl::Start() {
     VLOG(1) << "Start() called when instance is not stopped";
     return;
   }
+  stop_reason_ = ArcBridgeService::StopReason::SHUTDOWN;
   SetState(State::SOCKET_CREATING);
   base::PostTaskAndReplyWithResult(
       base::WorkerPool::GetTaskRunner(true).get(), FROM_HERE,
@@ -236,7 +248,7 @@ void ArcBridgeBootstrapImpl::OnSocketCreated(base::ScopedFD socket_fd) {
 
   if (!socket_fd.is_valid()) {
     LOG(ERROR) << "ARC: Error creating socket";
-    Stop();
+    AbortBoot(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE);
     return;
   }
 
@@ -261,7 +273,7 @@ void ArcBridgeBootstrapImpl::OnInstanceStarted(base::ScopedFD socket_fd,
     // Roll back the state to SOCKET_CREATING to avoid sending the D-Bus signal
     // to stop the failed instance.
     SetState(State::SOCKET_CREATING);
-    Stop();
+    AbortBoot(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE);
     return;
   }
   if (state_ != State::STARTING) {
@@ -318,12 +330,14 @@ void ArcBridgeBootstrapImpl::OnInstanceConnected(base::ScopedFD fd) {
   }
   if (!fd.is_valid()) {
     LOG(ERROR) << "Invalid handle";
+    AbortBoot(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE);
     return;
   }
   mojo::ScopedMessagePipeHandle server_pipe = mojo::edk::CreateMessagePipe(
       mojo::edk::ScopedPlatformHandle(mojo::edk::PlatformHandle(fd.release())));
   if (!server_pipe.is_valid()) {
     LOG(ERROR) << "Invalid pipe";
+    AbortBoot(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE);
     return;
   }
   SetState(State::READY);
@@ -339,11 +353,10 @@ void ArcBridgeBootstrapImpl::Stop() {
     VLOG(1) << "Stop() called when ARC is not running";
     return;
   }
-  if (state_ == State::SOCKET_CREATING) {
+  if (state_ < State::STARTING) {
     // This was stopped before the D-Bus command to start the instance. Skip
     // the D-Bus command to stop it.
-    SetState(State::STOPPING);
-    ArcInstanceStopped(true);
+    SetState(State::STOPPED);
     return;
   }
   SetState(State::STOPPING);
@@ -354,13 +367,26 @@ void ArcBridgeBootstrapImpl::Stop() {
       base::Bind(&DoNothingInstanceStopped));
 }
 
+void ArcBridgeBootstrapImpl::AbortBoot(ArcBridgeService::StopReason reason) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(reason != ArcBridgeService::StopReason::SHUTDOWN);
+  // In case of multiple errors, report the first one.
+  if (stop_reason_ == ArcBridgeService::StopReason::SHUTDOWN) {
+    stop_reason_ = reason;
+  }
+  Stop();
+}
+
 void ArcBridgeBootstrapImpl::ArcInstanceStopped(bool clean) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!clean)
+  if (!clean) {
     LOG(ERROR) << "ARC instance crashed";
-  DCHECK(delegate_);
+    // In case of multiple errors, report the first one.
+    if (stop_reason_ == ArcBridgeService::StopReason::SHUTDOWN) {
+      stop_reason_ = ArcBridgeService::StopReason::CRASH;
+    }
+  }
   SetState(State::STOPPED);
-  delegate_->OnStopped();
 }
 
 void ArcBridgeBootstrapImpl::SetState(State state) {
@@ -369,6 +395,10 @@ void ArcBridgeBootstrapImpl::SetState(State state) {
   DCHECK(state_ != state);
   state_ = state;
   VLOG(2) << "State: " << static_cast<uint32_t>(state_);
+  if (state_ == State::STOPPED) {
+    DCHECK(delegate_);
+    delegate_->OnStopped(stop_reason_);
+  }
 }
 
 }  // namespace

components/arc/arc_bridge_bootstrap.h

@@ -10,6 +10,7 @@
 #include "base/macros.h"
 #include "base/sequenced_task_runner.h"
 #include "base/single_thread_task_runner.h"
+#include "components/arc/arc_bridge_service.h"
 #include "components/arc/common/arc_bridge.mojom.h"
 
 namespace arc {
@@ -21,9 +22,12 @@ class ArcBridgeBootstrap {
  public:
   class Delegate {
    public:
+    // Called when the connection with ARC instance has been established.
     virtual void OnConnectionEstablished(
         mojom::ArcBridgeInstancePtr instance_ptr) = 0;
-    virtual void OnStopped() = 0;
+
+    // Called when ARC instance is stopped.
+    virtual void OnStopped(ArcBridgeService::StopReason reason) = 0;
   };
 
   // Creates a default instance of ArcBridgeBootstrap.

components/arc/arc_bridge_service.cc

@@ -17,8 +17,10 @@ namespace arc {
 ArcBridgeService* g_arc_bridge_service = nullptr;
 
 ArcBridgeService::ArcBridgeService()
-    : available_(false), state_(State::STOPPED), weak_factory_(this) {
-}
+    : available_(false),
+      state_(State::STOPPED),
+      stop_reason_(StopReason::SHUTDOWN),
+      weak_factory_(this) {}
 
 ArcBridgeService::~ArcBridgeService() {
   DCHECK(CalledOnValidThread());
@@ -167,7 +169,7 @@ void ArcBridgeService::SetState(State state) {
   if (state_ == State::READY)
     FOR_EACH_OBSERVER(Observer, observer_list(), OnBridgeReady());
   else if (state == State::STOPPED)
-    FOR_EACH_OBSERVER(Observer, observer_list(), OnBridgeStopped());
+    FOR_EACH_OBSERVER(Observer, observer_list(), OnBridgeStopped(stop_reason_));
 }
 
 void ArcBridgeService::SetAvailable(bool available) {
@@ -177,6 +179,11 @@ void ArcBridgeService::SetAvailable(bool available) {
   FOR_EACH_OBSERVER(Observer, observer_list(), OnAvailableChanged(available_));
 }
 
+void ArcBridgeService::SetStopReason(StopReason stop_reason) {
+  DCHECK(CalledOnValidThread());
+  stop_reason_ = stop_reason;
+}
+
 bool ArcBridgeService::CalledOnValidThread() {
   return thread_checker_.CalledOnValidThread();
 }

components/arc/arc_bridge_service.h

@@ -22,7 +22,6 @@ class CommandLine;
 
 namespace arc {
 
-class ArcBridgeBootstrap;
 class ArcBridgeTest;
 
 // The Chrome-side service that handles ARC instances and ARC bridge creation.
@@ -30,12 +29,26 @@ class ArcBridgeTest;
 // communication channel (the ARC bridge) used to send and receive messages.
 class ArcBridgeService : public mojom::ArcBridgeHost {
  public:
+  // Describes the reason the bridge is stopped.
+  enum class StopReason {
+    // ARC instance has been gracefully shut down.
+    SHUTDOWN,
+
+    // Errors occurred during the ARC instance boot. This includes any failures
+    // before the instance is actually attempted to be started, and also
+    // failures on bootstrapping IPC channels with Android.
+    GENERIC_BOOT_FAILURE,
+
+    // ARC instance has crashed.
+    CRASH,
+  };
+
   // Notifies life cycle events of ArcBridgeService.
   class Observer {
    public:
     // Called whenever the state of the bridge has changed.
     virtual void OnBridgeReady() {}
-    virtual void OnBridgeStopped() {}
+    virtual void OnBridgeStopped(StopReason reason) {}
 
     // Called whenever ARC's availability has changed for this system.
     virtual void OnAvailableChanged(bool available) {}
@@ -195,6 +208,11 @@ class ArcBridgeService : public mojom::ArcBridgeHost {
   // Changes the current availability and notifies all observers.
   void SetAvailable(bool availability);
 
+  // Sets the reason the bridge is stopped. This function must be always called
+  // before SetState(State::STOPPED) to report a correct reason with
+  // Observer::OnBridgeStopped().
+  void SetStopReason(StopReason stop_reason);
+
   base::ObserverList<Observer>& observer_list() { return observer_list_; }
 
   bool CalledOnValidThread();
@@ -208,6 +226,7 @@ class ArcBridgeService : public mojom::ArcBridgeHost {
   FRIEND_TEST_ALL_PREFIXES(ArcBridgeTest, Prerequisites);
   FRIEND_TEST_ALL_PREFIXES(ArcBridgeTest, ShutdownMidStartup);
   FRIEND_TEST_ALL_PREFIXES(ArcBridgeTest, Restart);
+  FRIEND_TEST_ALL_PREFIXES(ArcBridgeTest, OnBridgeStopped);
 
   // Instance holders.
   InstanceHolder<mojom::AppInstance> app_;
@@ -240,6 +259,9 @@ class ArcBridgeService : public mojom::ArcBridgeHost {
   // The current state of the bridge.
   ArcBridgeService::State state_;
 
+  // The reason the bridge is stopped.
+  StopReason stop_reason_;
+
   // WeakPtrFactory to use callbacks.
   base::WeakPtrFactory<ArcBridgeService> weak_factory_;
 

components/arc/arc_bridge_service_impl.cc

@@ -78,6 +78,7 @@ void ArcBridgeServiceImpl::PrerequisitesChanged() {
     if (!available() || !session_started_)
       return;
     VLOG(0) << "Prerequisites met, starting ARC";
+    SetStopReason(StopReason::SHUTDOWN);
     SetState(State::CONNECTING);
     bootstrap_->Start();
   } else {
@@ -136,8 +137,9 @@ void ArcBridgeServiceImpl::OnConnectionEstablished(
   SetState(State::READY);
 }
 
-void ArcBridgeServiceImpl::OnStopped() {
+void ArcBridgeServiceImpl::OnStopped(StopReason stop_reason) {
   DCHECK(CalledOnValidThread());
+  SetStopReason(stop_reason);
   SetState(State::STOPPED);
   VLOG(0) << "ARC stopped";
   if (reconnect_) {

components/arc/arc_bridge_service_impl.h

@@ -44,6 +44,7 @@ class ArcBridgeServiceImpl : public ArcBridgeService,
  private:
   friend class ArcBridgeTest;
   FRIEND_TEST_ALL_PREFIXES(ArcBridgeTest, Restart);
+  FRIEND_TEST_ALL_PREFIXES(ArcBridgeTest, OnBridgeStopped);
 
   // If all pre-requisites are true (ARC is available, it has been enabled, and
   // the session has started), and ARC is stopped, start ARC. If ARC is running
@@ -55,7 +56,7 @@ class ArcBridgeServiceImpl : public ArcBridgeService,
 
   // ArcBridgeBootstrap::Delegate:
   void OnConnectionEstablished(mojom::ArcBridgeInstancePtr instance) override;
-  void OnStopped() override;
+  void OnStopped(StopReason reason) override;
 
   // Called when the bridge channel is closed. This typically only happens when
   // the ARC instance crashes. This is not called during shutdown.

components/arc/arc_bridge_service_unittest.cc

@@ -34,8 +34,9 @@ class ArcBridgeTest : public testing::Test, public ArcBridgeService::Observer {
     ready_ = true;
   }
 
-  void OnBridgeStopped() override {
+  void OnBridgeStopped(ArcBridgeService::StopReason stop_reason) override {
     state_ = ArcBridgeService::State::STOPPED;
+    stop_reason_ = stop_reason;
     message_loop_.PostTask(FROM_HERE, message_loop_.QuitWhenIdleClosure());
   }
 
@@ -45,6 +46,7 @@ class ArcBridgeTest : public testing::Test, public ArcBridgeService::Observer {
  protected:
   std::unique_ptr<ArcBridgeServiceImpl> service_;
   std::unique_ptr<FakeArcBridgeInstance> instance_;
+  ArcBridgeService::StopReason stop_reason_;
 
  private:
   void SetUp() override {
@@ -52,6 +54,7 @@ class ArcBridgeTest : public testing::Test, public ArcBridgeService::Observer {
 
     ready_ = false;
     state_ = ArcBridgeService::State::STOPPED;
+    stop_reason_ = ArcBridgeService::StopReason::SHUTDOWN;
 
     instance_.reset(new FakeArcBridgeInstance());
     service_.reset(new ArcBridgeServiceImpl(
@@ -129,7 +132,7 @@ TEST_F(ArcBridgeTest, Restart) {
   // Simulate a connection loss.
   service_->DisableReconnectDelayForTesting();
   service_->OnChannelClosed();
-  instance_->SimulateCrash();
+  instance_->Stop(ArcBridgeService::StopReason::CRASH);
   instance_->WaitForInitCall();
   ASSERT_EQ(ArcBridgeService::State::READY, state());
   ASSERT_EQ(2, instance_->init_calls());
@@ -138,6 +141,36 @@ TEST_F(ArcBridgeTest, Restart) {
   ASSERT_EQ(ArcBridgeService::State::STOPPED, state());
 }
 
+// Makes sure OnBridgeStopped is called on stop.
+TEST_F(ArcBridgeTest, OnBridgeStopped) {
+  ASSERT_FALSE(ready());
+
+  service_->DisableReconnectDelayForTesting();
+  service_->SetAvailable(true);
+  service_->HandleStartup();
+  instance_->WaitForInitCall();
+  ASSERT_EQ(ArcBridgeService::State::READY, state());
+
+  // Simulate boot failure.
+  service_->OnChannelClosed();
+  instance_->Stop(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE);
+  instance_->WaitForInitCall();
+  ASSERT_EQ(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE, stop_reason_);
+  ASSERT_EQ(ArcBridgeService::State::READY, state());
+
+  // Simulate crash.
+  service_->OnChannelClosed();
+  instance_->Stop(ArcBridgeService::StopReason::CRASH);
+  instance_->WaitForInitCall();
+  ASSERT_EQ(ArcBridgeService::StopReason::CRASH, stop_reason_);
+  ASSERT_EQ(ArcBridgeService::State::READY, state());
+
+  // 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());

components/arc/test/fake_arc_bridge_bootstrap.cc

@@ -28,11 +28,13 @@ void FakeArcBridgeBootstrap::Start() {
 void FakeArcBridgeBootstrap::Stop() {
   DCHECK(delegate_);
   instance_->Unbind();
-  delegate_->OnStopped();
+  delegate_->OnStopped(ArcBridgeService::StopReason::SHUTDOWN);
 }
 
-void FakeArcBridgeBootstrap::OnCrashed() {
-  Stop();
+void FakeArcBridgeBootstrap::OnStopped(ArcBridgeService::StopReason reason) {
+  DCHECK(delegate_);
+  instance_->Unbind();
+  delegate_->OnStopped(reason);
 }
 
 }  // namespace arc

components/arc/test/fake_arc_bridge_bootstrap.h

@@ -24,7 +24,7 @@ class FakeArcBridgeBootstrap : public ArcBridgeBootstrap,
 
  private:
   // FakeArcBridgeInstance::Delegate:
-  void OnCrashed() override;
+  void OnStopped(ArcBridgeService::StopReason reason) override;
 
   // Owned by the caller.
   FakeArcBridgeInstance* instance_;

components/arc/test/fake_arc_bridge_instance.cc

@@ -31,10 +31,10 @@ void FakeArcBridgeInstance::WaitForInitCall() {
   binding_.WaitForIncomingMethodCall();
 }
 
-void FakeArcBridgeInstance::SimulateCrash() {
+void FakeArcBridgeInstance::Stop(ArcBridgeService::StopReason reason) {
   if (!delegate_)
     return;
-  delegate_->OnCrashed();
+  delegate_->OnStopped(reason);
 }
 
 }  // namespace arc

components/arc/test/fake_arc_bridge_instance.h

@@ -17,7 +17,7 @@ class FakeArcBridgeInstance : public mojom::ArcBridgeInstance {
   class Delegate {
    public:
     virtual ~Delegate() = default;
-    virtual void OnCrashed() = 0;
+    virtual void OnStopped(ArcBridgeService::StopReason reason) = 0;
   };
 
   FakeArcBridgeInstance();
@@ -41,8 +41,8 @@ class FakeArcBridgeInstance : public mojom::ArcBridgeInstance {
   // The number of times Init() has been called.
   int init_calls() const { return init_calls_; }
 
-  // Simulates a crash by calling Stop() on the ArcBridgeBoostrap.
-  void SimulateCrash();
+  // Stops the instance.
+  void Stop(ArcBridgeService::StopReason reason);
 
  private:
   Delegate* delegate_ = nullptr;

components/arc/user_data/arc_user_data_service.cc

@@ -31,7 +31,7 @@ ArcUserDataService::~ArcUserDataService() {
   arc_bridge_service()->RemoveObserver(this);
 }
 
-void ArcUserDataService::OnBridgeStopped() {
+void ArcUserDataService::OnBridgeStopped(ArcBridgeService::StopReason reason) {
   DCHECK(thread_checker_.CalledOnValidThread());
   const AccountId& account_id =
       user_manager::UserManager::Get()->GetPrimaryUser()->GetAccountId();

components/arc/user_data/arc_user_data_service.h

@@ -30,7 +30,7 @@ class ArcUserDataService : public ArcService,
   // ArcBridgeService::Observer:
   // Called whenever the arc bridge is stopped to potentially remove data if
   // the user has not opted in.
-  void OnBridgeStopped() override;
+  void OnBridgeStopped(ArcBridgeService::StopReason reason) override;
 
  private:
   base::ThreadChecker thread_checker_;