Fix ArcBridgeBootstrap race issues.

components/arc/arc_bridge_bootstrap.cc

Index: components/arc/arc_bridge_bootstrap.cc
diff --git a/components/arc/arc_bridge_bootstrap.cc b/components/arc/arc_bridge_bootstrap.cc
index e06fdb1a704a472fe8458b50a929d578db22bcf0..d85a81f0006dd8060b11b1995221261e2e024dd3 100644
--- a/components/arc/arc_bridge_bootstrap.cc
+++ b/components/arc/arc_bridge_bootstrap.cc
@@ -6,6 +6,7 @@
 
 #include <fcntl.h>
 #include <grp.h>
+#include <poll.h>
 #include <unistd.h>
 
 #include <utility>
@@ -25,6 +26,7 @@
 #include "chromeos/dbus/dbus_method_call_status.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/session_manager_client.h"
+#include "components/arc/arc_bridge_host_impl.h"
 #include "components/user_manager/user_manager.h"
 #include "ipc/unix_domain_socket_util.h"
 #include "mojo/edk/embedder/embedder.h"
@@ -62,61 +64,144 @@ chromeos::SessionManagerClient* GetSessionManagerClient() {
   return chromeos::DBusThreadManager::Get()->GetSessionManagerClient();
 }
 
+// Creates a pair of pipes. Returns true on success, otherwise false.
+// On success, |read_fd| will be set to the fd of the read side, and
+// |write_fd| will be set to the one of write side.
+bool CreatePipe(base::ScopedFD* read_fd, base::ScopedFD* write_fd) {
+  int fds[2];
+  if (pipe2(fds, O_NONBLOCK | O_CLOEXEC) < 0) {
+    PLOG(ERROR) << "Failed to create a pipe.";
+    return false;
+  }
+
+  read_fd->reset(fds[0]);
+  write_fd->reset(fds[1]);
+  return true;
+}
+
+// Waits until |raw_socket_fd| is readable.
+// The operation may be cancelled originally triggered by user interaction to
+// disable ARC, or ARC instance is unexpectedly stopped (e.g. crash).
+// To notify such a situation, |raw_cancel_fd| is also passed to here, and the
+// write side will be closed in such a case.
+bool WaitForSocketReadable(int raw_socket_fd, int raw_cancel_fd) {
+  struct pollfd fds[2] = {
+      {raw_socket_fd, POLLIN, 0}, {raw_cancel_fd, POLLIN, 0},
+  };
+
+  if (HANDLE_EINTR(poll(fds, arraysize(fds), -1)) <= 0) {
+    PLOG(ERROR) << "Poll() is failed.";

[Luis Héctor Chávez, 2016/08/01 17:47:38]

nit: Just "poll()" is fine.

[hidehiko, 2016/08/02 11:36:21]

Done.

+    return false;
+  }
+
+  if (fds[1].revents) {
+    // Notified that Stop() is invoked. Cancel the Mojo connecting.
+    VLOG(0) << "Stop() is called during the ConnectMojo()";
+    return false;
+  }
+
+  DCHECK(fds[0].revents);
+  return true;
+}
+
+// TODO(hidehiko): Refactor more to make this class unittest-able, for at least
+// state-machine part.
 class ArcBridgeBootstrapImpl : public ArcBridgeBootstrap,
                                public chromeos::SessionManagerClient::Observer {
  public:
   // The possible states of the bootstrap connection.  In the normal flow,
   // the state changes in the following sequence:
   //
-  // STOPPED
+  // NOT_STARTED
   //   Start() ->
-  // DISK_SPACE_CHECKING
-  //   CheckDiskSpace() -> OnDiskSpaceChecked() ->
-  // SOCKET_CREATING
+  // CHECKING_DISK_SPACE
+  //   OnDiskSpaceChecked() ->
+  // CREATING_SOCKET
   //   CreateSocket() -> OnSocketCreated() ->
-  // STARTING
-  //   StartArcInstance() -> OnInstanceStarted() ->
-  // STARTED
-  //   AcceptInstanceConnection() -> OnInstanceConnected() ->
-  // READY
+  // STARTING_INSTANCE
+  //   -> OnInstanceStarted() ->
+  // CONNECTING_MOJO
+  //   ConnectMojo() -> OnMojoConnected() ->
+  // RUNNING
   //
-  // When Stop() or AbortBoot() is called from any state, either because an
-  // operation resulted in an error or because the user is logging out:
+  // At any phase, Stop() can be called. It does not immediately stop the

[Luis Héctor Chávez, 2016/08/01 17:47:38]

nt: s/phase/state/ throughout this comment.

[hidehiko, 2016/08/02 11:36:21]

Done.

+  // instance, but will eventually stop it.
+  // The actual stop will be notified via OnStopped() of the |delegate_|.
   //
-  // (any)
-  //   Stop()/AbortBoot() ->
-  // STOPPING
-  //   StopInstance() ->
-  // STOPPED
+  // When Stop() is called, it makes various behavior based on the current
+  // phase.
   //
-  // When the instance crashes while it was ready, it will be stopped:
-  //   READY -> STOPPING -> STOPPED
-  // and then restarted:
-  //   STOPPED -> DISK_SPACE_CHECKING -> ... -> READY).
+  // NOT_STARTED:
+  //   Do nothing. Immediately transition to the STOPPED.
+  // CHECKING_DISK_SPACE, CREATING_SOCKET:
+  //   The main task of those phases runs on WorkerPool thread. So, Stop()
+  //   just sets the flag and return. On the main task completion, a callback
+  //   will run on the main (practically UI) thread, and the flag is checked
+  //   at the beginning of them. This should work under the assumption that

[Luis Héctor Chávez, 2016/08/01 17:47:38]

Why is this an assumption? (Or rather, why does this need to be explicitly
pointed out? I was under the impression that UI thread work always had to take
little time).

[hidehiko, 2016/08/02 11:36:21]

If the background task is very heavy or blocking forever, the stopping mechanism
won't work well, because their callback where |stop_requested_| is checked is
called with long delay, or will never be called.

Mentioning "blocking" situation may be simpler case? WDYT?

+  //   the main tasks do not take much time.
+  // STARTING_INSTANCE:
+  //   The ARC instance is starting via SessionManager. So, similar to
+  //   CHECKING_DISK_SPACE/CREATING_SOCKET cases, Stop() just sets the flag
+  //   and return. In a callback, it checks if ARC instance is successfully

[Luis Héctor Chávez, 2016/08/01 17:47:37]

nit: s/In a callback/In its callback/.

[hidehiko, 2016/08/02 11:36:21]

Done.

+  //   started or not. In case of success, a request to stop the ARC instance
+  //   is sent to SessionManager. Its completion will be notified via
+  //   ArcInstanceStopped. Otherwise, it just turns into STOPPED state.
+  // CONNECTING_MOJO:
+  //   The main task runs on WorkerPool thread, but it is blocking call.
+  //   So, Stop() sends a request to cancel the blocking by closing the pipe
+  //   whose read side is also polled. Then, in its callback, similar to
+  //   STARTING_INSTANCE, a request to stop the ARC instance is sent to
+  //   SessionManager, and ArcInstanceStopped handles remaining procedure.
+  // RUNNING:
+  //   There is no more callback which runs on normal flow, so Stop() requests
+  //   to stop the ARC instance via SessionManager.
+  //
+  // Another trigger to change the state coming from outside of this class
+  // is an event ArcInstanceStopped() sent from SessionManager, when ARC
+  // instace is crashed. ArcInstanceStopped() turns the state into STOPPED

[Luis Héctor Chávez, 2016/08/01 17:47:37]

nit: s/instace is crashed/instance unexpectedly terminates/. Android itself can
still request a reboot/shutdown and it will cause an ArcInstanceStopped() event
to be fired.

[hidehiko, 2016/08/02 11:36:21]

Done.

+  // immediately.
+  // This happens only when STARTING_INSTANCE, CONNECTING_MOJO or RUNNING
+  // state.
+  //
+  // STARTING_INSTANCE:
+  //   In OnInstanceStarted(), |state_| is checked at the beginning. If it is
+  //   STOPPED, then ArcInstanceStopped() is called. Do nothing in that case.
+  // CONNECTING_MOJO:
+  //   Similar to Stop() case above, ArcInstanceStopped() also notifies to
+  //   WorkerPool() thread to cancel it to unblock the thread. In
+  //   OnMojoConnected(), similar to OnInstanceStarted(), check if |state_| is
+  //   STOPPED, then do nothing.
+  // RUNNING:
+  //   It is not necessary to do anything special here.
+  //
+  // In NOT_STARTED or STOPPED state, the instance can be safely destructed.

[Luis Héctor Chávez, 2016/08/01 17:47:38]

What happens if the instance is destroyed in any other state? e.g. User logs off
in the middle of some other state?

[hidehiko, 2016/08/02 11:36:21]

It would be enter to mysterious ChromeOS state, but if it is like shutting down
of the whole ChromeOS process, it wouldn't be problematic I think.

Before, starting instance, some operation may run on WorkerPool. That's fine, if
it's in shutdown state.

After starting, ARC instance may be remaining. If it is in ChromeOS shutdown, it
shouldn't be much problematic. (If there are more Start() request after the
destruction, it would be conflicting, though).

While waiting for the mojo connection, the WorkerPool will be unblocked because
the |accept_cancel_pipe_| should be closed.

+  // Specifically, in STOPPED state, there may be inflight operations or
+  // pending callbacks. Though, what they do is just do-nothing conceptually
+  // and they can be safely ignored.
   //
   // Note: Order of constants below matters. Please make sure to sort them
   // in chronological order.
   enum class State {
-    // ARC is not currently running.
-    STOPPED,
+    // ARC is not yet started.
+    NOT_STARTED,
 
     // Checking the disk space.
-    DISK_SPACE_CHECKING,
+    CHECKING_DISK_SPACE,
 
     // An UNIX socket is being created.
-    SOCKET_CREATING,
+    CREATING_SOCKET,
 
     // The request to start the instance has been sent.
-    STARTING,
+    STARTING_INSTANCE,
 
     // The instance has started. Waiting for it to connect to the IPC bridge.
-    STARTED,
+    CONNECTING_MOJO,
 
-    // The instance is fully connected.
-    READY,
+    // The instance is fully set up.
+    RUNNING,
 
-    // The request to shut down the instance has been sent.
-    STOPPING,
+    // ARC is terminated.
+    STOPPED,
   };
 
   ArcBridgeBootstrapImpl();
@@ -127,37 +212,44 @@ 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);
+  // Completes the termination procedure.
+  void OnStopped(ArcBridgeService::StopReason reason);
 
   // Called after getting the device free disk space.
-  void OnDiskSpaceChecked(int64_t disk_free_bytes);
+  void OnDiskSpaceObtained(int64_t disk_free_bytes);
 
   // Creates the UNIX socket on the bootstrap thread and then processes its
   // file descriptor.
   static base::ScopedFD CreateSocket();
   void OnSocketCreated(base::ScopedFD fd);
 
+  // DBus callbacks.
+  void OnInstanceStarted(base::ScopedFD socket_fd, bool success);
+
   // Synchronously accepts a connection on |socket_fd| and then processes the
   // connected socket's file descriptor.
-  static base::ScopedFD AcceptInstanceConnection(base::ScopedFD socket_fd);
-  void OnInstanceConnected(base::ScopedFD fd);
-
-  void SetState(State state);
+  static base::ScopedFD ConnectMojo(base::ScopedFD socket_fd,
+                                    base::ScopedFD cancel_fd);
+  void OnMojoConnected(base::ScopedFD fd);
 
-  // DBus callbacks.
-  void OnInstanceStarted(base::ScopedFD socket_fd, bool success);
+  // Request to stop ARC instance via DBus.
+  void StopArcInstance();
 
   // chromeos::SessionManagerClient::Observer:
   void ArcInstanceStopped(bool clean) override;
 
   // The state of the bootstrap connection.
-  State state_ = State::STOPPED;
+  State state_ = State::NOT_STARTED;
 
-  // The reason the ARC instance is stopped.
-  ArcBridgeService::StopReason stop_reason_ =
-      ArcBridgeService::StopReason::SHUTDOWN;
+  // When Stop() is called, thsi flag is set.
+  bool stop_requested_ = false;
+
+  // In CONNECTING_MOJO state, this is set to the write side of the pipe
+  // to notify cancelling of the procedure.
+  base::ScopedFD accept_cancel_pipe_;
+
+  // Mojo endpoint.
+  std::unique_ptr<mojom::ArcBridgeHost> arc_bridge_host_;
 
   base::ThreadChecker thread_checker_;
 
@@ -178,7 +270,7 @@ ArcBridgeBootstrapImpl::ArcBridgeBootstrapImpl()
 
 ArcBridgeBootstrapImpl::~ArcBridgeBootstrapImpl() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(state_ == State::STOPPED || state_ == State::STOPPING);
+  DCHECK(state_ == State::NOT_STARTED || state_ == State::STOPPED);
   chromeos::SessionManagerClient* client = GetSessionManagerClient();
   if (client == nullptr)
     return;
@@ -188,38 +280,92 @@ ArcBridgeBootstrapImpl::~ArcBridgeBootstrapImpl() {
 void ArcBridgeBootstrapImpl::Start() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(delegate_);
-  if (state_ != State::STOPPED) {
-    VLOG(1) << "Start() called when instance is not stopped";
-    return;
-  }
-  stop_reason_ = ArcBridgeService::StopReason::SHUTDOWN;
+  DCHECK_EQ(state_, State::NOT_STARTED);
+  state_ = State::CHECKING_DISK_SPACE;
+
   // TODO(crbug.com/628124): Move disk space checking logic to session_manager.
-  SetState(State::DISK_SPACE_CHECKING);
   base::PostTaskAndReplyWithResult(
       base::WorkerPool::GetTaskRunner(true).get(), FROM_HERE,
       base::Bind(&base::SysInfo::AmountOfFreeDiskSpace,
                  base::FilePath(kDiskCheckPath)),
-      base::Bind(&ArcBridgeBootstrapImpl::OnDiskSpaceChecked,
+      base::Bind(&ArcBridgeBootstrapImpl::OnDiskSpaceObtained,
                  weak_factory_.GetWeakPtr()));
 }
 
-void ArcBridgeBootstrapImpl::OnDiskSpaceChecked(int64_t disk_free_bytes) {
+void ArcBridgeBootstrapImpl::Stop() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(delegate_);
+
+  // For second time or later, just do nothing.
+  // It is already in the stopping phase.
+  if (stop_requested_)
+    return;
+
+  stop_requested_ = true;
+  arc_bridge_host_.reset();
+  switch (state_) {
+    case State::NOT_STARTED:
+      OnStopped(ArcBridgeService::StopReason::SHUTDOWN);
+      return;
+
+    case State::CHECKING_DISK_SPACE:
+    case State::CREATING_SOCKET:
+    case State::STARTING_INSTANCE:
+      // Before starting the ARC instance, we do nothing here.
+      // At some point, a callback will be invoked on UI thread,
+      // and stopping procedure will be run there.
+      return;
+
+    case State::CONNECTING_MOJO:
+      // Mojo connection is being waited on a WorkerPool thread.
+      // Request to cancel it. Following stopping procedure will run
+      // in its callback.
+      DCHECK(accept_cancel_pipe_.get());
+      accept_cancel_pipe_.reset();
+      return;
+
+    case State::RUNNING:
+      // Now ARC instance is running. Request to stop it.
+      StopArcInstance();
+      return;
+
+    case State::STOPPED:
+      // The instance is already stopped. Do nothing.
+      return;
+  }
+}
+
+void ArcBridgeBootstrapImpl::OnStopped(ArcBridgeService::StopReason reason) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  // OnStopped() should be called once per instance.
+  DCHECK_NE(state_, State::STOPPED);
+  arc_bridge_host_.reset();
+  state_ = State::STOPPED;
+  delegate_->OnStopped(reason);
+}
+
+void ArcBridgeBootstrapImpl::OnDiskSpaceObtained(int64_t disk_free_bytes) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (state_ != State::DISK_SPACE_CHECKING) {
+  DCHECK_EQ(state_, State::CHECKING_DISK_SPACE);
+
+  if (stop_requested_) {
     VLOG(1) << "Stop() called while checking disk space";
+    OnStopped(ArcBridgeService::StopReason::SHUTDOWN);
     return;
   }
+
   if (disk_free_bytes < 0) {
     LOG(ERROR) << "ARC: Failed to get free disk space";
-    AbortBoot(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE);
+    OnStopped(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE);
     return;
   }
   if (disk_free_bytes < kCriticalDiskFreeBytes) {
     LOG(ERROR) << "ARC: The device is too low on disk space to start ARC";
-    AbortBoot(ArcBridgeService::StopReason::LOW_DISK_SPACE);
+    OnStopped(ArcBridgeService::StopReason::LOW_DISK_SPACE);
     return;
   }
-  SetState(State::SOCKET_CREATING);
+
+  state_ = State::CREATING_SOCKET;
   base::PostTaskAndReplyWithResult(
       base::WorkerPool::GetTaskRunner(true).get(), FROM_HERE,
       base::Bind(&ArcBridgeBootstrapImpl::CreateSocket),
@@ -232,22 +378,10 @@ base::ScopedFD ArcBridgeBootstrapImpl::CreateSocket() {
   base::FilePath socket_path(kArcBridgeSocketPath);
 
   int raw_fd = -1;
-  if (!IPC::CreateServerUnixDomainSocket(socket_path, &raw_fd)) {
+  if (!IPC::CreateServerUnixDomainSocket(socket_path, &raw_fd))
     return base::ScopedFD();
-  }
   base::ScopedFD socket_fd(raw_fd);
 
-  // Make socket blocking.
-  int flags = HANDLE_EINTR(fcntl(socket_fd.get(), F_GETFL));
-  if (flags == -1) {
-    PLOG(ERROR) << "fcntl(F_GETFL)";
-    return base::ScopedFD();
-  }
-  if (HANDLE_EINTR(fcntl(socket_fd.get(), F_SETFL, flags & ~O_NONBLOCK)) < 0) {
-    PLOG(ERROR) << "fcntl(O_NONBLOCK)";
-    return base::ScopedFD();
-  }
-
   // Change permissions on the socket.
   struct group arc_bridge_group;
   struct group* arc_bridge_group_res = nullptr;
@@ -279,18 +413,21 @@ base::ScopedFD ArcBridgeBootstrapImpl::CreateSocket() {
 
 void ArcBridgeBootstrapImpl::OnSocketCreated(base::ScopedFD socket_fd) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (state_ != State::SOCKET_CREATING) {
+  DCHECK_EQ(state_, State::CREATING_SOCKET);
+
+  if (stop_requested_) {
     VLOG(1) << "Stop() called while connecting";
+    OnStopped(ArcBridgeService::StopReason::SHUTDOWN);
     return;
   }
-  SetState(State::STARTING);
 
   if (!socket_fd.is_valid()) {
     LOG(ERROR) << "ARC: Error creating socket";
-    AbortBoot(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE);
+    OnStopped(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE);
     return;
   }
 
+  state_ = State::STARTING_INSTANCE;
   user_manager::UserManager* user_manager = user_manager::UserManager::Get();
   DCHECK(user_manager->GetPrimaryUser());
   const cryptohome::Identification cryptohome_id(
@@ -307,31 +444,57 @@ void ArcBridgeBootstrapImpl::OnSocketCreated(base::ScopedFD socket_fd) {
 void ArcBridgeBootstrapImpl::OnInstanceStarted(base::ScopedFD socket_fd,
                                                bool success) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  if (state_ == State::STOPPED) {
+    // This is the case that error is notified via DBus before the
+    // OnInstanceStarted() callback is invoked. The stopping procedure has
+    // been run, so do nothing.
+    return;
+  }
+
+  DCHECK_EQ(state_, State::STARTING_INSTANCE);
+
+  if (stop_requested_) {
+    if (success) {
+      // The ARC instance has started to run. Request to stop.
+      StopArcInstance();
+      return;
+    }
+    OnStopped(ArcBridgeService::StopReason::SHUTDOWN);
+    return;
+  }
+
   if (!success) {
     LOG(ERROR) << "Failed to start ARC instance";
-    // Roll back the state to SOCKET_CREATING to avoid sending the D-Bus signal
-    // to stop the failed instance.
-    SetState(State::SOCKET_CREATING);
-    AbortBoot(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE);
+    OnStopped(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE);
     return;
   }
-  if (state_ != State::STARTING) {
-    VLOG(1) << "Stop() called when ARC is not running";
+
+  state_ = State::CONNECTING_MOJO;
+
+  // Prepare a pipe so that AcceptInstanceConnection can be interrupted on
+  // Stop().
+  base::ScopedFD cancel_fd;
+  if (!CreatePipe(&cancel_fd, &accept_cancel_pipe_)) {
+    OnStopped(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE);
     return;
   }
-  SetState(State::STARTED);
 
   base::PostTaskAndReplyWithResult(
       base::WorkerPool::GetTaskRunner(true).get(), FROM_HERE,
-      base::Bind(&ArcBridgeBootstrapImpl::AcceptInstanceConnection,
-                 base::Passed(&socket_fd)),
-      base::Bind(&ArcBridgeBootstrapImpl::OnInstanceConnected,
+      base::Bind(&ArcBridgeBootstrapImpl::ConnectMojo, base::Passed(&socket_fd),
+                 base::Passed(&cancel_fd)),
+      base::Bind(&ArcBridgeBootstrapImpl::OnMojoConnected,
                  weak_factory_.GetWeakPtr()));
 }
 
 // static
-base::ScopedFD ArcBridgeBootstrapImpl::AcceptInstanceConnection(
-    base::ScopedFD socket_fd) {
+base::ScopedFD ArcBridgeBootstrapImpl::ConnectMojo(base::ScopedFD socket_fd,
+                                                   base::ScopedFD cancel_fd) {
+  if (!WaitForSocketReadable(socket_fd.get(), cancel_fd.get())) {
+    VLOG(0) << "Mojo connecting is cancelled.";
+    return base::ScopedFD();
+  }
+
   int raw_fd = -1;
   if (!IPC::ServerOnConnect(socket_fd.get(), &raw_fd)) {
     return base::ScopedFD();
@@ -361,44 +524,52 @@ base::ScopedFD ArcBridgeBootstrapImpl::AcceptInstanceConnection(
   return scoped_fd;
 }
 
-void ArcBridgeBootstrapImpl::OnInstanceConnected(base::ScopedFD fd) {
+void ArcBridgeBootstrapImpl::OnMojoConnected(base::ScopedFD fd) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (state_ != State::STARTED) {
-    VLOG(1) << "Stop() called when ARC is not running";
+
+  if (state_ == State::STOPPED) {
+    // This is the case that error is notified via DBus before the
+    // OnMojoConnected() callback is invoked. The stopping procedure has
+    // been run, so do nothing.
     return;
   }
+
+  DCHECK_EQ(state_, State::CONNECTING_MOJO);
+  accept_cancel_pipe_.reset();
+
+  if (stop_requested_) {
+    StopArcInstance();
+    return;
+  }
+
   if (!fd.is_valid()) {
     LOG(ERROR) << "Invalid handle";
-    AbortBoot(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE);
+    StopArcInstance();
     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);
+    StopArcInstance();
     return;
   }
-  SetState(State::READY);
+
   mojom::ArcBridgeInstancePtr instance;
   instance.Bind(mojo::InterfacePtrInfo<mojom::ArcBridgeInstance>(
       std::move(server_pipe), 0u));
-  delegate_->OnConnectionEstablished(std::move(instance));
+  arc_bridge_host_.reset(new ArcBridgeHostImpl(std::move(instance)));
+
+  state_ = State::RUNNING;
+  delegate_->OnReady();
 }
 
-void ArcBridgeBootstrapImpl::Stop() {
+void ArcBridgeBootstrapImpl::StopArcInstance() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (state_ == State::STOPPED || state_ == State::STOPPING) {
-    VLOG(1) << "Stop() called when ARC is not running";
-    return;
-  }
-  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::STOPPED);
-    return;
-  }
-  SetState(State::STOPPING);
+  DCHECK(state_ == State::STARTING_INSTANCE ||
+         state_ == State::CONNECTING_MOJO || state_ == State::RUNNING);
+
   // Notification will arrive through ArcInstanceStopped().
   chromeos::SessionManagerClient* session_manager_client =
       chromeos::DBusThreadManager::Get()->GetSessionManagerClient();
@@ -406,38 +577,27 @@ 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) {
-    LOG(ERROR) << "ARC instance crashed";
-    // In case of multiple errors, report the first one.
-    if (stop_reason_ == ArcBridgeService::StopReason::SHUTDOWN) {
-      stop_reason_ = ArcBridgeService::StopReason::CRASH;
-    }
-  }
-  SetState(State::STOPPED);
-}
-
-void ArcBridgeBootstrapImpl::SetState(State state) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  // DCHECK on enum classes not supported.
-  DCHECK(state_ != state);
-  state_ = state;
-  VLOG(2) << "State: " << static_cast<uint32_t>(state_);

[Luis Héctor Chávez, 2016/08/01 17:47:38]

We're losing some amount of useful logging :( Is it possible to keep this just
for the logging?

[hidehiko, 2016/08/02 11:36:21]

Instead of introducing SetState, added VLOGs for each state transitions.

-  if (state_ == State::STOPPED) {
-    DCHECK(delegate_);
-    delegate_->OnStopped(stop_reason_);
+  // In case that crash happens during connecting Mojo, we unlock the

[Luis Héctor Chávez, 2016/08/01 17:47:38]

nit: s/during connecting Mojo/before the Mojo channel is connected/

[hidehiko, 2016/08/02 11:36:21]

This could call before starting Mojo channel connection, depending on
SessionManager's implementation. So, technically this sounds more suitable to
the real? Welcome better phrases :-).

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

Right, but for the purposes of resetting |accept_cancel_pipe_| we only care if
the stopped signal arrives before the Mojo channel is connected, since
afterwards it does not matter. This was mostly a grammar nit, btw :P

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

Makes sense. Done. Thank you.

+  // WorkerPool thread.
+  accept_cancel_pipe_.reset();
+
+  ArcBridgeService::StopReason reason;
+  if (stop_requested_) {
+    // If the ARC instance is stopped after its explicit request,
+    // returns SHUTDOWN.

[Luis Héctor Chávez, 2016/08/01 17:47:38]

nit: s/returns/return/

[hidehiko, 2016/08/02 11:36:21]

Done.

+    reason = ArcBridgeService::StopReason::SHUTDOWN;
+  } else if (clean) {
+    // If the ARC instance is stopped, but it is not explicitly requested,
+    // then this is triggered by some failure during the starting procedure.
+    // Return GENERIC_BOOT_FAILURE for the case.
+    reason = ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE;

[Luis Héctor Chávez, 2016/08/01 17:47:37]

Have you actually seen this case happen? We should probably look into it.

[hidehiko, 2016/08/02 11:36:21]

No, practically.

+  } else {
+    // Otherwise, this is caused by CRASH occured inside of the ARC instance.
+    reason = ArcBridgeService::StopReason::CRASH;
   }
+  OnStopped(reason);
 }
 
 }  // namespace