arc: simplify upgrade failure handling

Simplify the upgrade failure handling by ignoring the result of the dbus
call, and instead waiting for the ArcInstanceStopped signal before
signaling a failure. Also check for disk space in Chrome instead of in
session_manager.

This is the first change in a series that will have Chrome call to
Upstart directly to start the container, cutting out session_manager
entirely.

BUG=b:133248517
TEST=Run test cases and manually test container startup/shutdown.

Change-Id: I7c5ef4be82b5fbb2deca1fae7a2a0ebc06952e26
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1623631
Commit-Queue: Chris Morin <cmtm@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662829}

chromeos/dbus/session_manager/fake_session_manager_client.cc

@@ -580,25 +580,17 @@ void FakeSessionManagerClient::StartArcMiniContainer(
 
 void FakeSessionManagerClient::UpgradeArcContainer(
     const login_manager::UpgradeArcContainerRequest& request,
-    base::OnceClosure success_callback,
-    UpgradeErrorCallback error_callback) {
+    VoidDBusMethodCallback callback) {
   last_upgrade_arc_request_ = request;
 
-  if (!arc_available_) {
-    PostReply(FROM_HERE, std::move(error_callback), false);
-    return;
-  }
-  if (low_disk_) {
+  PostReply(FROM_HERE, std::move(callback), !force_upgrade_failure_);
+  if (force_upgrade_failure_) {
+    // Emulate ArcInstanceStopped signal propagation.
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
         base::BindOnce(&FakeSessionManagerClient::NotifyArcInstanceStopped,
-                       weak_ptr_factory_.GetWeakPtr(),
-                       login_manager::ArcContainerStopReason::LOW_DISK_SPACE));
-    PostReply(FROM_HERE, std::move(error_callback), true);
-    return;
+                       weak_ptr_factory_.GetWeakPtr()));
   }
-  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
-                                                std::move(success_callback));
 }
 
 void FakeSessionManagerClient::StopArcInstance(
@@ -613,8 +605,8 @@ void FakeSessionManagerClient::StopArcInstance(
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::BindOnce(&FakeSessionManagerClient::NotifyArcInstanceStopped,
-                     weak_ptr_factory_.GetWeakPtr(),
-                     login_manager::ArcContainerStopReason::USER_REQUEST));
+                     weak_ptr_factory_.GetWeakPtr()));
+
   container_running_ = false;
 }
 
@@ -637,10 +629,9 @@ void FakeSessionManagerClient::GetArcStartTime(
       arc_available_ ? base::make_optional(arc_start_time_) : base::nullopt);
 }
 
-void FakeSessionManagerClient::NotifyArcInstanceStopped(
-    login_manager::ArcContainerStopReason reason) {
+void FakeSessionManagerClient::NotifyArcInstanceStopped() {
   for (auto& observer : observers_)
-    observer.ArcInstanceStopped(reason);
+    observer.ArcInstanceStopped();
 }
 
 bool FakeSessionManagerClient::GetFlagsForUser(

chromeos/dbus/session_manager/fake_session_manager_client.h

@@ -112,8 +112,7 @@ class COMPONENT_EXPORT(SESSION_MANAGER) FakeSessionManagerClient
       VoidDBusMethodCallback callback) override;
   void UpgradeArcContainer(
       const login_manager::UpgradeArcContainerRequest& request,
-      base::OnceClosure success_callback,
-      UpgradeErrorCallback error_callback) override;
+      VoidDBusMethodCallback callback) override;
   void StopArcInstance(VoidDBusMethodCallback callback) override;
   void SetArcCpuRestriction(
       login_manager::ContainerCpuRestrictionState restriction_state,
@@ -123,7 +122,7 @@ class COMPONENT_EXPORT(SESSION_MANAGER) FakeSessionManagerClient
   void GetArcStartTime(DBusMethodCallback<base::TimeTicks> callback) override;
 
   // Notifies observers as if ArcInstanceStopped signal is received.
-  void NotifyArcInstanceStopped(login_manager::ArcContainerStopReason);
+  void NotifyArcInstanceStopped();
 
   // Returns true if flags for |cryptohome_id| have been set. If the return
   // value is |true|, |*out_flags_for_user| is filled with the flags passed to
@@ -230,12 +229,13 @@ class COMPONENT_EXPORT(SESSION_MANAGER) FakeSessionManagerClient
   }
 
   void set_arc_available(bool available) { arc_available_ = available; }
+  void set_force_upgrade_failure(bool force_upgrade_failure) {
+    force_upgrade_failure_ = force_upgrade_failure;
+  }
   void set_arc_start_time(base::TimeTicks arc_start_time) {
     arc_start_time_ = arc_start_time;
   }
 
-  void set_low_disk(bool low_disk) { low_disk_ = low_disk; }
-
   void set_force_state_keys_missing(bool force_state_keys_missing) {
     force_state_keys_missing_ = force_state_keys_missing;
   }
@@ -289,9 +289,9 @@ class COMPONENT_EXPORT(SESSION_MANAGER) FakeSessionManagerClient
   bool force_state_keys_missing_ = false;
 
   bool arc_available_ = false;
+  bool force_upgrade_failure_ = false;
   base::TimeTicks arc_start_time_;
 
-  bool low_disk_ = false;
   bool container_running_ = false;
 
   // Contains last request passed to StartArcMiniContainer

chromeos/dbus/session_manager/session_manager_client.cc

@@ -419,10 +419,8 @@ class SessionManagerClientImpl : public SessionManagerClient {
 
   void UpgradeArcContainer(
       const login_manager::UpgradeArcContainerRequest& request,
-      base::OnceClosure success_callback,
-      UpgradeErrorCallback error_callback) override {
-    DCHECK(!success_callback.is_null());
-    DCHECK(!error_callback.is_null());
+      VoidDBusMethodCallback callback) override {
+    DCHECK(!callback.is_null());
     dbus::MethodCall method_call(
         login_manager::kSessionManagerInterface,
         login_manager::kSessionManagerUpgradeArcContainer);
@@ -430,11 +428,10 @@ class SessionManagerClientImpl : public SessionManagerClient {
 
     writer.AppendProtoAsArrayOfBytes(request);
 
-    session_manager_proxy_->CallMethodWithErrorResponse(
+    session_manager_proxy_->CallMethod(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::BindOnce(&SessionManagerClientImpl::OnUpgradeArcContainer,
-                       weak_ptr_factory_.GetWeakPtr(),
-                       std::move(success_callback), std::move(error_callback)));
+        base::BindOnce(&SessionManagerClientImpl::OnVoidMethod,
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
   void StopArcInstance(VoidDBusMethodCallback callback) override {
@@ -728,19 +725,8 @@ class SessionManagerClientImpl : public SessionManagerClient {
   }
 
   void ArcInstanceStoppedReceived(dbus::Signal* signal) {
-    dbus::MessageReader reader(signal);
-
-    auto reason = login_manager::ArcContainerStopReason::CRASH;
-    uint32_t value = 0;
-    if (reader.PopUint32(&value)) {
-      reason = static_cast<login_manager::ArcContainerStopReason>(value);
-    } else {
-      LOG(ERROR) << "Invalid signal: " << signal->ToString();
-      return;
-    }
-
     for (auto& observer : observers_)
-      observer.ArcInstanceStopped(reason);
+      observer.ArcInstanceStopped();
   }
 
   // Called when the object is connected to the signal.
@@ -795,21 +781,6 @@ class SessionManagerClientImpl : public SessionManagerClient {
     std::move(callback).Run(base::TimeTicks::FromInternalValue(ticks));
   }
 
-  void OnUpgradeArcContainer(base::OnceClosure success_callback,
-                             UpgradeErrorCallback error_callback,
-                             dbus::Response* response,
-                             dbus::ErrorResponse* error) {
-    if (!response) {
-      LOG(ERROR) << "Failed to call UpgradeArcContainer: "
-                 << (error ? error->ToString() : "(null)");
-      std::move(error_callback)
-          .Run(error && error->GetErrorName() ==
-                            login_manager::dbus_error::kLowFreeDisk);
-      return;
-    }
-    std::move(success_callback).Run();
-  }
-
   dbus::ObjectProxy* session_manager_proxy_ = nullptr;
   std::unique_ptr<BlockingMethodCaller> blocking_method_caller_;
   base::ObserverList<Observer>::Unchecked observers_;

chromeos/dbus/session_manager/session_manager_client.h

@@ -73,11 +73,7 @@ class COMPONENT_EXPORT(SESSION_MANAGER) SessionManagerClient {
     virtual void EmitLoginPromptVisibleCalled() {}
 
     // Called when the ARC instance is stopped after it had already started.
-    // |clean| is true if the instance was stopped as a result of an explicit
-    // request, false if it died unexpectedly.
-    // See details for StartArcInstanceCallback.
-    virtual void ArcInstanceStopped(
-        login_manager::ArcContainerStopReason reason) {}
+    virtual void ArcInstanceStopped() {}
   };
 
   // Interface for performing actions on behalf of the stub implementation.
@@ -338,16 +334,14 @@ class COMPONENT_EXPORT(SESSION_MANAGER) SessionManagerClient {
       const login_manager::StartArcMiniContainerRequest& request,
       VoidDBusMethodCallback callback) = 0;
 
-  // UpgradeArcContainer upgrades a mini-container to a full ARC container. In
-  // case of success, success_callback is called. In case of error,
-  // error_callback will be called with a |low_free_disk_space| signaling
-  // whether the failure was due to low free disk space.
-  using UpgradeErrorCallback =
-      base::OnceCallback<void(bool low_free_disk_space)>;
+  // UpgradeArcContainer upgrades a mini-container to a full ARC container. On
+  // upgrade failure, the container will be shutdown. The container shutdown
+  // will trigger the ArcInstanceStopped signal (as usual). There are no
+  // guarantees over whether this |callback| is invoked or the
+  // ArcInstanceStopped signal is received first.
   virtual void UpgradeArcContainer(
       const login_manager::UpgradeArcContainerRequest& request,
-      base::OnceClosure success_callback,
-      UpgradeErrorCallback error_callback) = 0;
+      VoidDBusMethodCallback callback) = 0;
 
   // Asynchronously stops the ARC instance.  Upon completion, invokes
   // |callback| with the result; true on success, false on failure (either

components/arc/session/arc_client_adapter.h

@@ -8,18 +8,12 @@
 #include <memory>
 #include <string>
 
-#include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/observer_list.h"
-#include "base/optional.h"
-#include "chromeos/dbus/login_manager/arc.pb.h"
 #include "chromeos/dbus/session_manager/session_manager_client.h"
 
 namespace arc {
 
-// TODO(yusukes): Move the enum and proto in system_api/ from login_manager's
-// namespace to arc and remove all the type aliases.
-using ArcContainerStopReason = login_manager::ArcContainerStopReason;
 using StartArcMiniContainerRequest =
     login_manager::StartArcMiniContainerRequest;
 using UpgradeArcContainerRequest = login_manager::UpgradeArcContainerRequest;
@@ -30,7 +24,7 @@ class ArcClientAdapter {
   class Observer {
    public:
     virtual ~Observer() = default;
-    virtual void ArcInstanceStopped(ArcContainerStopReason stop_reason) = 0;
+    virtual void ArcInstanceStopped() = 0;
   };
 
   // Creates a default instance of ArcClientAdapter.
@@ -42,15 +36,9 @@ class ArcClientAdapter {
   virtual void StartMiniArc(const StartArcMiniContainerRequest& request,
                             chromeos::VoidDBusMethodCallback callback) = 0;
 
-  // UpgradeArc upgrades a mini ARC instance to a full ARC instance. In case of
-  // success, success_callback is called. In case of error, |error_callback|
-  // will be called with a |low_free_disk_space| signaling whether the failure
-  // was due to low free disk space.
-  using UpgradeErrorCallback =
-      base::OnceCallback<void(bool low_free_disk_space)>;
+  // UpgradeArc upgrades a mini ARC instance to a full ARC instance.
   virtual void UpgradeArc(const UpgradeArcContainerRequest& request,
-                          base::OnceClosure success_callback,
-                          UpgradeErrorCallback error_callback) = 0;
+                          chromeos::VoidDBusMethodCallback callback) = 0;
 
   // Asynchronously stops the ARC instance.
   virtual void StopArcInstance() = 0;

components/arc/session/arc_container_client_adapter.cc

@@ -37,10 +37,9 @@ class ArcContainerClientAdapter
   }
 
   void UpgradeArc(const UpgradeArcContainerRequest& request,
-                  base::OnceClosure success_callback,
-                  UpgradeErrorCallback error_callback) override {
+                  chromeos::VoidDBusMethodCallback callback) override {
     chromeos::SessionManagerClient::Get()->UpgradeArcContainer(
-        request, std::move(success_callback), std::move(error_callback));
+        request, std::move(callback));
   }
 
   void StopArcInstance() override {
@@ -51,10 +50,9 @@ class ArcContainerClientAdapter
   }
 
   // chromeos::SessionManagerClient::Observer overrides:
-  void ArcInstanceStopped(
-      login_manager::ArcContainerStopReason stop_reason) override {
+  void ArcInstanceStopped() override {
     for (auto& observer : observer_list_)
-      observer.ArcInstanceStopped(stop_reason);
+      observer.ArcInstanceStopped();
   }
 
  private:

components/arc/session/arc_session_impl.cc

@@ -21,6 +21,7 @@
 #include "base/rand_util.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/system/sys_info.h"
 #include "base/task/post_task.h"
 #include "base/task/task_traits.h"
 #include "chromeos/constants/chromeos_switches.h"
@@ -80,17 +81,6 @@ bool WaitForSocketReadable(int raw_socket_fd, int raw_cancel_fd) {
   return true;
 }
 
-// Returns the ArcStopReason corresponding to the ARC instance staring failure.
-ArcStopReason GetArcStopReason(bool low_disk_space, bool stop_requested) {
-  if (stop_requested)
-    return ArcStopReason::SHUTDOWN;
-
-  if (low_disk_space)
-    return ArcStopReason::LOW_DISK_SPACE;
-
-  return ArcStopReason::GENERIC_BOOT_FAILURE;
-}
-
 // Converts ArcSupervisionTransition into
 // login_manager::UpgradeArcContainerRequest_SupervisionTransition.
 login_manager::UpgradeArcContainerRequest_SupervisionTransition
@@ -126,6 +116,7 @@ class ArcSessionDelegateImpl : public ArcSessionImpl::Delegate {
   base::ScopedFD ConnectMojo(base::ScopedFD socket_fd,
                              ConnectMojoCallback callback) override;
   void GetLcdDensity(GetLcdDensityCallback callback) override;
+  void GetFreeDiskSpace(GetFreeDiskSpaceCallback callback) override;
   version_info::Channel GetChannel() override;
 
  private:
@@ -208,6 +199,15 @@ void ArcSessionDelegateImpl::GetLcdDensity(GetLcdDensityCallback callback) {
       std::move(callback)));
 }
 
+void ArcSessionDelegateImpl::GetFreeDiskSpace(
+    GetFreeDiskSpaceCallback callback) {
+  PostTaskWithTraitsAndReplyWithResult(
+      FROM_HERE, {base::MayBlock()},
+      base::BindOnce(&base::SysInfo::AmountOfFreeDiskSpace,
+                     base::FilePath("/home")),
+      std::move(callback));
+}
+
 version_info::Channel ArcSessionDelegateImpl::GetChannel() {
   return channel_;
 }
@@ -448,11 +448,13 @@ void ArcSessionImpl::OnMiniInstanceStarted(bool result) {
   DCHECK_EQ(state_, State::STARTING_MINI_INSTANCE);
 
   if (!result) {
-    OnStopped(GetArcStopReason(false, stop_requested_));
+    LOG(ERROR) << "Failed to start ARC mini container";
+    OnStopped(ArcStopReason::GENERIC_BOOT_FAILURE);
     return;
   }
 
   VLOG(2) << "ARC mini container has been successfully started.";
+  state_ = State::RUNNING_MINI_INSTANCE;
 
   if (stop_requested_) {
     // The ARC instance has started to run. Request to stop.
@@ -460,8 +462,6 @@ void ArcSessionImpl::OnMiniInstanceStarted(bool result) {
     return;
   }
 
-  state_ = State::RUNNING_MINI_INSTANCE;
-
   if (upgrade_requested_)
     // RequestUpgrade() has been called during the D-Bus call.
     DoUpgrade();
@@ -473,6 +473,24 @@ void ArcSessionImpl::DoUpgrade() {
   VLOG(2) << "Upgrading an existing ARC mini instance";
   state_ = State::STARTING_FULL_INSTANCE;
 
+  // Getting the free disk space doesn't take long.
+  delegate_->GetFreeDiskSpace(base::BindOnce(&ArcSessionImpl::OnFreeDiskSpace,
+                                             weak_factory_.GetWeakPtr()));
+}
+
+void ArcSessionImpl::OnFreeDiskSpace(int64_t space) {
+  // Ensure there's sufficient space on disk for the container.
+  if (space == -1) {
+    LOG(ERROR) << "Could not determine free disk space";
+    StopArcInstance();
+    return;
+  } else if (space < kMinimumFreeDiskSpaceBytes) {
+    VLOG(1) << "There is not enough disk space to start the ARC container";
+    insufficient_disk_space_ = true;
+    StopArcInstance();
+    return;
+  }
+
   delegate_->CreateSocket(base::BindOnce(&ArcSessionImpl::OnSocketCreated,
                                          weak_factory_.GetWeakPtr()));
 }
@@ -490,7 +508,7 @@ void ArcSessionImpl::OnSocketCreated(base::ScopedFD socket_fd) {
 
   if (!socket_fd.is_valid()) {
     LOG(ERROR) << "ARC: Error creating socket";
-    OnStopped(ArcStopReason::GENERIC_BOOT_FAILURE);
+    StopArcInstance();
     return;
   }
 
@@ -535,18 +553,20 @@ void ArcSessionImpl::OnSocketCreated(base::ScopedFD socket_fd) {
         upgrade_params_.demo_session_apps_path.value());
   }
 
-  client_->UpgradeArc(
-      request,
-      base::BindOnce(&ArcSessionImpl::OnUpgraded, weak_factory_.GetWeakPtr(),
-                     std::move(socket_fd)),
-      base::BindOnce(&ArcSessionImpl::OnUpgradeError,
-                     weak_factory_.GetWeakPtr()));
+  client_->UpgradeArc(request, base::BindOnce(&ArcSessionImpl::OnUpgraded,
+                                              weak_factory_.GetWeakPtr(),
+                                              std::move(socket_fd)));
 }
 
-void ArcSessionImpl::OnUpgraded(base::ScopedFD socket_fd) {
+void ArcSessionImpl::OnUpgraded(base::ScopedFD socket_fd, bool result) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK_EQ(state_, State::STARTING_FULL_INSTANCE);
 
+  if (!result) {
+    LOG(ERROR) << "Failed to upgrade ARC container";
+    return;
+  }
+
   VLOG(2) << "ARC instance is successfully upgraded.";
 
   if (stop_requested_) {
@@ -567,10 +587,6 @@ void ArcSessionImpl::OnUpgraded(base::ScopedFD socket_fd) {
   }
 }
 
-void ArcSessionImpl::OnUpgradeError(bool low_disk_space) {
-  OnStopped(GetArcStopReason(low_disk_space, stop_requested_));
-}
-
 void ArcSessionImpl::OnMojoConnected(
     std::unique_ptr<mojom::ArcBridgeHost> arc_bridge_host) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
@@ -658,30 +674,28 @@ void ArcSessionImpl::StopArcInstance() {
   client_->StopArcInstance();
 }
 
-void ArcSessionImpl::ArcInstanceStopped(ArcContainerStopReason stop_reason) {
+void ArcSessionImpl::ArcInstanceStopped() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  VLOG(1) << "Notified that ARC instance is stopped "
-          << static_cast<uint32_t>(stop_reason);
+  DCHECK_NE(state_, State::STARTING_MINI_INSTANCE);
+  VLOG(1) << "Notified that ARC instance is stopped";
 
   // In case that crash happens during before the Mojo channel is connected,
   // unlock the ThreadPool's thread.
   accept_cancel_pipe_.reset();
 
-  // TODO(hidehiko): In new D-Bus signal, more detailed reason why ARC
-  // container is stopped. Check it in details.
   ArcStopReason reason;
   if (stop_requested_) {
     // If the ARC instance is stopped after its explicit request,
     // return SHUTDOWN.
     reason = ArcStopReason::SHUTDOWN;
-  } else if (stop_reason == ArcContainerStopReason::LOW_DISK_SPACE) {
+  } else if (insufficient_disk_space_) {
     // ARC mini container is stopped because of upgarde failure due to low
     // disk space.
     reason = ArcStopReason::LOW_DISK_SPACE;
-  } else if (stop_reason != ArcContainerStopReason::CRASH) {
-    // 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.
+  } else if (state_ == State::STARTING_FULL_INSTANCE ||
+             state_ == State::CONNECTING_MOJO) {
+    // If the ARC instance is stopped during the upgrade, but it is not
+    // explicitly requested, return GENERIC_BOOT_FAILURE for the case.
     reason = ArcStopReason::GENERIC_BOOT_FAILURE;
   } else {
     // Otherwise, this is caused by CRASH occured inside of the ARC instance.

components/arc/session/arc_session_impl.h

@@ -30,6 +30,8 @@ namespace mojom {
 class ArcBridgeHost;
 }  // namespace mojom
 
+constexpr int64_t kMinimumFreeDiskSpaceBytes = 64 << 20;  // 64MB
+
 class ArcSessionImpl : public ArcSession, public ArcClientAdapter::Observer {
  public:
   // The possible states of the session. Expected state changes are as follows.
@@ -87,10 +89,9 @@ class ArcSessionImpl : public ArcSession, public ArcClientAdapter::Observer {
   //   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 unexpectedly terminates. ArcInstanceStopped() turns the state into
-  // STOPPED immediately.
+  // Another trigger to change the state coming from outside of this class is an
+  // event, ArcInstanceStopped(), sent from SessionManager when the ARC instance
+  // terminates. ArcInstanceStopped() turns the state into STOPPED immediately.
   //
   // In NOT_STARTED or STOPPED state, the instance can be safely destructed.
   // Specifically, in STOPPED state, there may be inflight operations or
@@ -156,6 +157,10 @@ class ArcSessionImpl : public ArcSession, public ArcClientAdapter::Observer {
     // callback will cancel the pending callback.
     virtual void GetLcdDensity(GetLcdDensityCallback callback) = 0;
 
+    // Gets the available disk space under /home. The result is in bytes.
+    using GetFreeDiskSpaceCallback = base::OnceCallback<void(int64_t)>;
+    virtual void GetFreeDiskSpace(GetFreeDiskSpaceCallback callback) = 0;
+
     // Returns the channel for the installation.
     virtual version_info::Channel GetChannel() = 0;
   };
@@ -190,7 +195,7 @@ class ArcSessionImpl : public ArcSession, public ArcClientAdapter::Observer {
 
   // D-Bus callback for UpgradeArcContainer(). |socket_fd| should be a socket
   // which should be accept(2)ed to connect ArcBridgeService Mojo channel.
-  void OnUpgraded(base::ScopedFD socket_fd);
+  void OnUpgraded(base::ScopedFD socket_fd, bool result);
 
   // D-Bus callback for UpgradeArcContainer when the upgrade fails.
   // |low_free_disk_space| signals whether the failure was due to low free disk
@@ -205,7 +210,7 @@ class ArcSessionImpl : public ArcSession, public ArcClientAdapter::Observer {
   void StopArcInstance();
 
   // ArcClientAdapter::Observer:
-  void ArcInstanceStopped(ArcContainerStopReason stop_reason) override;
+  void ArcInstanceStopped() override;
 
   // Completes the termination procedure. Note that calling this may end up with
   // deleting |this| because the function calls observers' OnSessionStopped().
@@ -214,6 +219,9 @@ class ArcSessionImpl : public ArcSession, public ArcClientAdapter::Observer {
   // LCD density for the device is available.
   void OnLcdDensity(int32_t lcd_density);
 
+  // Free disk space under /home in bytes.
+  void OnFreeDiskSpace(int64_t space);
+
   // Checks whether a function runs on the thread where the instance is
   // created.
   THREAD_CHECKER(thread_checker_);
@@ -233,6 +241,9 @@ class ArcSessionImpl : public ArcSession, public ArcClientAdapter::Observer {
   // Whether the full container has been requested
   bool upgrade_requested_ = false;
 
+  // Whether there's insufficient disk space to start the container.
+  bool insufficient_disk_space_ = 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_;

components/arc/session/arc_session_impl_unittest.cc

@@ -90,6 +90,10 @@ class FakeDelegate : public ArcSessionImpl::Delegate {
       lcd_density_callback_ = std::move(callback);
   }
 
+  void GetFreeDiskSpace(GetFreeDiskSpaceCallback callback) override {
+    std::move(callback).Run(free_disk_space_);
+  }
+
   version_info::Channel GetChannel() override {
     return version_info::Channel::DEFAULT;
   }
@@ -100,6 +104,8 @@ class FakeDelegate : public ArcSessionImpl::Delegate {
     std::move(lcd_density_callback_).Run(lcd_density_);
   }
 
+  void SetFreeDiskSpace(int64_t space) { free_disk_space_ = space; }
+
  private:
   void PostCallback(ConnectMojoCallback callback) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
@@ -112,6 +118,7 @@ class FakeDelegate : public ArcSessionImpl::Delegate {
   int32_t lcd_density_ = 0;
   bool success_ = true;
   bool suspend_ = false;
+  int64_t free_disk_space_ = kMinimumFreeDiskSpaceBytes * 2;
   ConnectMojoCallback pending_callback_;
   GetLcdDensityCallback lcd_density_callback_;
 
@@ -198,10 +205,6 @@ class ArcSessionImplTest : public testing::Test {
         user_manager::UserManager::Get());
   }
 
-  void EmulateDBusFailure() {
-    chromeos::FakeSessionManagerClient::Get()->set_arc_available(false);
-  }
-
   std::unique_ptr<ArcSessionImpl, ArcSessionDeleter> CreateArcSession(
       std::unique_ptr<ArcSessionImpl::Delegate> delegate = nullptr,
       int32_t lcd_density = 160) {
@@ -243,7 +246,7 @@ TEST_F(ArcSessionImplTest, MiniInstance_Success) {
 // SessionManagerClient::StartArcMiniContainer() reports an error, causing the
 // mini-container start to fail.
 TEST_F(ArcSessionImplTest, MiniInstance_DBusFail) {
-  EmulateDBusFailure();
+  chromeos::FakeSessionManagerClient::Get()->set_arc_available(false);
 
   auto arc_session = CreateArcSession();
   TestArcSessionObserver observer(arc_session.get());
@@ -262,10 +265,13 @@ TEST_F(ArcSessionImplTest, MiniInstance_DBusFail) {
 // causing the container upgrade to fail to start container with reason
 // LOW_DISK_SPACE.
 TEST_F(ArcSessionImplTest, Upgrade_LowDisk) {
-  chromeos::FakeSessionManagerClient::Get()->set_low_disk(true);
+  auto delegate = std::make_unique<FakeDelegate>();
+  delegate->SetFreeDiskSpace(kMinimumFreeDiskSpaceBytes / 2);
+
   // Set up. Start mini-container. The mini-container doesn't use the disk, so
   // there being low disk space won't cause it to start.
-  auto arc_session = CreateArcSession();
+  auto arc_session = CreateArcSession(std::move(delegate));
+
   base::RunLoop run_loop;
   TestArcSessionObserver observer(arc_session.get(), &run_loop);
   ASSERT_NO_FATAL_FAILURE(SetupMiniContainer(arc_session.get(), &observer));
@@ -306,7 +312,7 @@ TEST_F(ArcSessionImplTest, Upgrade_DBusFail) {
   ASSERT_NO_FATAL_FAILURE(SetupMiniContainer(arc_session.get(), &observer));
 
   // Hereafter, let SessionManagerClient::UpgradeArcContainer() fail.
-  EmulateDBusFailure();
+  chromeos::FakeSessionManagerClient::Get()->set_force_upgrade_failure(true);
 
   // Then upgrade, which should fail.
   arc_session->RequestUpgrade(DefaultUpgradeParams());
@@ -514,33 +520,6 @@ TEST_F(ArcSessionImplTest,
   EXPECT_TRUE(observer.on_session_stopped_args()->upgrade_requested);
 }
 
-// Stop is requested, but at the same time
-// SessionManagerClient::StartArcMiniContainer() reports an error. Then, it
-// should be handled as regular SHUTDOWN, because graceful shutdown itself is
-// difficult and sometimes reports unexpected error although it succeeds.
-TEST_F(ArcSessionImplTest, Stop_ConflictWithFailure) {
-  // Let SessionManagerClient::StartArcMiniContainer() fail.
-  EmulateDBusFailure();
-
-  auto arc_session = CreateArcSession();
-  TestArcSessionObserver observer(arc_session.get());
-  arc_session->StartMiniInstance();
-  ASSERT_EQ(ArcSessionImpl::State::STARTING_MINI_INSTANCE,
-            arc_session->GetStateForTesting());
-
-  arc_session->Stop();
-  base::RunLoop().RunUntilIdle();
-
-  EXPECT_EQ(ArcSessionImpl::State::STOPPED, arc_session->GetStateForTesting());
-  ASSERT_TRUE(observer.on_session_stopped_args().has_value());
-  // Even if D-Bus reports an error, if Stop() is invoked, it will be handled
-  // as clean shutdown.
-  EXPECT_EQ(ArcStopReason::SHUTDOWN,
-            observer.on_session_stopped_args()->reason);
-  EXPECT_FALSE(observer.on_session_stopped_args()->was_running);
-  EXPECT_FALSE(observer.on_session_stopped_args()->upgrade_requested);
-}
-
 // Emulating crash.
 TEST_F(ArcSessionImplTest, ArcStopInstance) {
   auto arc_session = CreateArcSession();
@@ -552,8 +531,7 @@ TEST_F(ArcSessionImplTest, ArcStopInstance) {
             arc_session->GetStateForTesting());
 
   // Deliver the ArcInstanceStopped D-Bus signal.
-  chromeos::FakeSessionManagerClient::Get()->NotifyArcInstanceStopped(
-      login_manager::ArcContainerStopReason::CRASH);
+  chromeos::FakeSessionManagerClient::Get()->NotifyArcInstanceStopped();
 
   EXPECT_EQ(ArcSessionImpl::State::STOPPED, arc_session->GetStateForTesting());
   ASSERT_TRUE(observer.on_session_stopped_args().has_value());

components/arc/session/arc_vm_client_adapter.cc

@@ -13,18 +13,13 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/task/post_task.h"
-#include "chromeos/dbus/session_manager/session_manager_client.h"
+#include "chromeos/dbus/login_manager/arc.pb.h"
 #include "chromeos/dbus/upstart/upstart_client.h"
 
 namespace arc {
 
 namespace {
 
-// TODO(yusukes): Move ArcContainerStopReason to arc:: and stop including
-// chromeos/dbus/session_manager/session_manager_client.h.
-constexpr login_manager::ArcContainerStopReason kDummyReason =
-    login_manager::ArcContainerStopReason::SESSION_MANAGER_SHUTDOWN;
-
 // The conversion of upstart job names to dbus object paths is undocumented. See
 // arc_data_remover.cc for more information.
 constexpr char kArcVmUpstartJob[] = "arcvm";
@@ -45,8 +40,7 @@ class ArcVmClientAdapter : public ArcClientAdapter {
   }
 
   void UpgradeArc(const UpgradeArcContainerRequest& request,
-                  base::OnceClosure success_callback,
-                  UpgradeErrorCallback error_callback) override {
+                  chromeos::VoidDBusMethodCallback callback) override {
     // TODO(yusukes): Consider doing the same as crostini rather than taking to
     // Upstart.
     VLOG(1) << "Starting arcvm";
@@ -57,10 +51,7 @@ class ArcVmClientAdapter : public ArcClientAdapter {
         // arc_session_impl.cc fills the |account_id| field, and it is always
         // guaranteed that the ID is not for Incognito mode and is a valid one.
         // TODO(yusukes): Pass other fields of the |request| to the job.
-        {"CHROMEOS_USER=" + request.account_id()},
-        base::BindOnce(&ArcVmClientAdapter::OnArcInstanceUpgraded,
-                       weak_factory_.GetWeakPtr(), std::move(success_callback),
-                       std::move(error_callback)));
+        {"CHROMEOS_USER=" + request.account_id()}, std::move(callback));
   }
 
   void StopArcInstance() override {
@@ -76,22 +67,12 @@ class ArcVmClientAdapter : public ArcClientAdapter {
   }
 
  private:
-  void OnArcInstanceUpgraded(base::OnceClosure success_callback,
-                             UpgradeErrorCallback error_callback,
-                             bool result) {
-    VLOG(1) << "OnArcInstanceUpgraded result=" << result;
-    if (result)
-      std::move(success_callback).Run();
-    else
-      std::move(error_callback).Run(/*low_free_disk_space=*/false);
-  }
-
   void OnArcInstanceStopped(bool result) {
     VLOG(1) << "OnArcInstanceStopped result=" << result;
     if (!result)
       LOG(WARNING) << "Failed to stop arcvm. Instance not running?";
     for (auto& observer : observer_list_)
-      observer.ArcInstanceStopped(kDummyReason);
+      observer.ArcInstanceStopped();
   }
 
   // For callbacks.