Use new ChromeOS session manager ARC API

The StartArcInstance dbus method has been split into
StartArcMiniContainer and UpgradeArcContainer. Both need to be called
sequentially for the ARC container to boot fully.

Bug: 65548422
Test: Ensure container starts fully
Test: ./out/Debug/unit_tests --gtest_filter='*Arc*'
Test: ./out/Debug/components_unittests --gtest_filter='*Arc*'
Change-Id: I60879e0c8b4ec6e93ee30fbabe0ae5e83dce2fc5
Reviewed-on: https://chromium-review.googlesource.com/917669Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Commit-Queue: Christopher Morin <cmtm@google.com>
Cr-Commit-Position: refs/heads/master@{#547629}

chromeos/dbus/fake_session_manager_client.cc

@@ -504,29 +504,38 @@ void FakeSessionManagerClient::GetServerBackedStateKeys(
   }
 }
 
-void FakeSessionManagerClient::StartArcInstance(
-    const login_manager::StartArcInstanceRequest& request,
-    StartArcInstanceCallback callback) {
-  last_start_arc_request_ = request;
-  StartArcInstanceResult result;
-  std::string container_instance_id;
+void FakeSessionManagerClient::StartArcMiniContainer(
+    const login_manager::StartArcMiniContainerRequest& request,
+    StartArcMiniContainerCallback callback) {
   if (!arc_available_) {
-    result = StartArcInstanceResult::UNKNOWN_ERROR;
-  } else if (low_disk_) {
-    result = StartArcInstanceResult::LOW_FREE_DISK_SPACE;
-  } else {
-    result = StartArcInstanceResult::SUCCESS;
-    if (container_instance_id_.empty()) {
-      // This is starting a new container.
-      base::Base64Encode(kFakeContainerInstanceId, &container_instance_id_);
-      // Note that empty |container_instance_id| should be returned if
-      // this is upgrade case, so assign only when starting a new container.
-      container_instance_id = container_instance_id_;
-    }
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(std::move(callback), base::nullopt));
+    return;
+  }
+  // This is starting a new container.
+  base::Base64Encode(kFakeContainerInstanceId, &container_instance_id_);
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(std::move(callback), container_instance_id_));
+}
+
+void FakeSessionManagerClient::UpgradeArcContainer(
+    const login_manager::UpgradeArcContainerRequest& request,
+    UpgradeArcContainerCallback success_callback,
+    UpgradeErrorCallback error_callback) {
+  last_upgrade_arc_request_ = request;
+
+  if (!arc_available_) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(std::move(error_callback), false));
+    return;
+  }
+  if (low_disk_) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(std::move(error_callback), true));
+    return;
   }
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), result,
-                                container_instance_id, base::ScopedFD()));
+      FROM_HERE, base::BindOnce(std::move(success_callback), base::ScopedFD()));
 }
 
 void FakeSessionManagerClient::StopArcInstance(

chromeos/dbus/fake_session_manager_client.h

@@ -86,8 +86,13 @@ class FakeSessionManagerClient : public SessionManagerClient {
                        const std::vector<std::string>& flags) override;
   void GetServerBackedStateKeys(StateKeysCallback callback) override;
 
-  void StartArcInstance(const login_manager::StartArcInstanceRequest& request,
-                        StartArcInstanceCallback callback) override;
+  void StartArcMiniContainer(
+      const login_manager::StartArcMiniContainerRequest& request,
+      StartArcMiniContainerCallback callback) override;
+  void UpgradeArcContainer(
+      const login_manager::UpgradeArcContainerRequest& request,
+      UpgradeArcContainerCallback success_callback,
+      UpgradeErrorCallback error_callback) override;
   void StopArcInstance(VoidDBusMethodCallback callback) override;
   void SetArcCpuRestriction(
       login_manager::ContainerCpuRestrictionState restriction_state,
@@ -140,8 +145,9 @@ class FakeSessionManagerClient : public SessionManagerClient {
   void set_device_local_account_policy(const std::string& account_id,
                                        const std::string& policy_blob);
 
-  const login_manager::StartArcInstanceRequest& last_start_arc_request() const {
-    return last_start_arc_request_;
+  const login_manager::UpgradeArcContainerRequest& last_upgrade_arc_request()
+      const {
+    return last_upgrade_arc_request_;
   }
 
   // Notify observers about a property change completion.
@@ -213,7 +219,7 @@ class FakeSessionManagerClient : public SessionManagerClient {
   std::string container_instance_id_;
 
   // Contains last requst passed to StartArcInstance
-  login_manager::StartArcInstanceRequest last_start_arc_request_;
+  login_manager::UpgradeArcContainerRequest last_upgrade_arc_request_;
 
   StubDelegate* delegate_;
 

chromeos/dbus/session_manager_client.cc

@@ -373,21 +373,41 @@ class SessionManagerClientImpl : public SessionManagerClient {
                        weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
-  void StartArcInstance(const login_manager::StartArcInstanceRequest& request,
-                        StartArcInstanceCallback callback) override {
+  void StartArcMiniContainer(
+      const login_manager::StartArcMiniContainerRequest& request,
+      StartArcMiniContainerCallback callback) override {
     DCHECK(!callback.is_null());
+    dbus::MethodCall method_call(
+        login_manager::kSessionManagerInterface,
+        login_manager::kSessionManagerStartArcMiniContainer);
+    dbus::MessageWriter writer(&method_call);
+
+    writer.AppendProtoAsArrayOfBytes(request);
+
+    session_manager_proxy_->CallMethod(
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::BindOnce(&SessionManagerClientImpl::OnStartArcMiniContainer,
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+  }
 
+  void UpgradeArcContainer(
+      const login_manager::UpgradeArcContainerRequest& request,
+      UpgradeArcContainerCallback success_callback,
+      UpgradeErrorCallback error_callback) override {
+    DCHECK(!success_callback.is_null());
+    DCHECK(!error_callback.is_null());
     dbus::MethodCall method_call(
         login_manager::kSessionManagerInterface,
-        login_manager::kSessionManagerStartArcInstance);
+        login_manager::kSessionManagerUpgradeArcContainer);
     dbus::MessageWriter writer(&method_call);
 
     writer.AppendProtoAsArrayOfBytes(request);
 
     session_manager_proxy_->CallMethodWithErrorResponse(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::BindOnce(&SessionManagerClientImpl::OnStartArcInstance,
-                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+        base::BindOnce(&SessionManagerClientImpl::OnUpgradeArcContainer,
+                       weak_ptr_factory_.GetWeakPtr(),
+                       std::move(success_callback), std::move(error_callback)));
   }
 
   void StopArcInstance(VoidDBusMethodCallback callback) override {
@@ -757,34 +777,44 @@ class SessionManagerClientImpl : public SessionManagerClient {
     std::move(callback).Run(base::TimeTicks::FromInternalValue(ticks));
   }
 
-  void OnStartArcInstance(StartArcInstanceCallback callback,
-                          dbus::Response* response,
-                          dbus::ErrorResponse* error) {
+  void OnStartArcMiniContainer(StartArcMiniContainerCallback callback,
+                               dbus::Response* response) {
     if (!response) {
-      LOG(ERROR) << "Failed to call StartArcInstance: "
-                 << (error ? error->ToString() : "(null)");
-      std::move(callback).Run(
-          error && error->GetErrorName() ==
-                       login_manager::dbus_error::kLowFreeDisk
-              ? StartArcInstanceResult::LOW_FREE_DISK_SPACE
-              : StartArcInstanceResult::UNKNOWN_ERROR,
-          std::string(), base::ScopedFD());
+      std::move(callback).Run(base::nullopt);
       return;
     }
 
     dbus::MessageReader reader(response);
     std::string container_instance_id;
-    base::ScopedFD server_socket;
-    if (!reader.PopString(&container_instance_id) ||
-        !reader.PopFileDescriptor(&server_socket)) {
+    if (!reader.PopString(&container_instance_id)) {
       LOG(ERROR) << "Invalid response: " << response->ToString();
-      std::move(callback).Run(StartArcInstanceResult::UNKNOWN_ERROR,
-                              std::string(), base::ScopedFD());
+      std::move(callback).Run(base::nullopt);
       return;
     }
+    std::move(callback).Run(std::move(container_instance_id));
+  }
 
-    std::move(callback).Run(StartArcInstanceResult::SUCCESS,
-                            container_instance_id, std::move(server_socket));
+  void OnUpgradeArcContainer(UpgradeArcContainerCallback 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;
+    }
+
+    dbus::MessageReader reader(response);
+    base::ScopedFD server_socket;
+    if (!reader.PopFileDescriptor(&server_socket)) {
+      LOG(ERROR) << "Invalid response: " << response->ToString();
+      std::move(error_callback).Run(false);
+      return;
+    }
+    std::move(success_callback).Run(std::move(server_socket));
   }
 
   dbus::ObjectProxy* session_manager_proxy_ = nullptr;

chromeos/dbus/session_manager_client.h

@@ -25,7 +25,8 @@ class Identification;
 }
 
 namespace login_manager {
-class StartArcInstanceRequest;
+class StartArcMiniContainerRequest;
+class UpgradeArcContainerRequest;
 }
 
 namespace chromeos {
@@ -280,36 +281,30 @@ class CHROMEOS_EXPORT SessionManagerClient : public DBusClient {
   // sync fails or there's no network, the callback is never invoked.
   virtual void GetServerBackedStateKeys(StateKeysCallback callback) = 0;
 
-  // Asynchronously starts the ARC instance for the user whose cryptohome is
-  // located by |cryptohome_id|.  Flag |disable_boot_completed_broadcast|
-  // blocks Android ACTION_BOOT_COMPLETED broadcast for 3rd party applications.
-  // Upon completion, invokes |callback| with the result.
-  // Running ARC requires some amount of disk space. LOW_FREE_DISK_SPACE will
-  // be returned when there is not enough free disk space for ARC.
-  // UNKNOWN_ERROR is returned for any other errors.
-  enum class StartArcInstanceResult {
-    SUCCESS,
-    UNKNOWN_ERROR,
-    LOW_FREE_DISK_SPACE,
-  };
-  // When ArcStartupMode is LOGIN_SCREEN, StartArcInstance starts a container
-  // with only a handful of ARC processes for Chrome OS login screen.
-  // |cryptohome_id|, |skip_boot_completed_broadcast|, and
-  // |scan_vendor_priv_app| are ignored when the mode is LOGIN_SCREEN.
-  enum class ArcStartupMode {
-    FULL,
-    LOGIN_SCREEN,
-  };
-  // In case of success, |container_instance_id| will be passed as its second
-  // param. The ID is passed to ArcInstanceStopped() to identify which instance
-  // is stopped.
-  using StartArcInstanceCallback =
-      base::OnceCallback<void(StartArcInstanceResult result,
-                              const std::string& container_instance_id,
-                              base::ScopedFD server_socket)>;
-  virtual void StartArcInstance(
-      const login_manager::StartArcInstanceRequest& request,
-      StartArcInstanceCallback callback) = 0;
+  // StartArcMiniContainer starts a container with only a handful of ARC
+  // processes for Chrome OS login screen.  In case of success, callback will be
+  // called with |container_instance_id| set to a string.  The ID is passed to
+  // ArcInstanceStopped() to identify which instance is stopped. In case of
+  // error, |container_instance_id| will be nullopt.
+  using StartArcMiniContainerCallback =
+      DBusMethodCallback<std::string /* container_instance_id */>;
+  virtual void StartArcMiniContainer(
+      const login_manager::StartArcMiniContainerRequest& request,
+      StartArcMiniContainerCallback callback) = 0;
+
+  // UpgradeArcContainer upgrades a mini-container to a full ARC container. In
+  // case of success, success_callback is called. |server_socket| should be
+  // accept(2)ed to connect to the ArcBridgeService Mojo channel.  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 UpgradeArcContainerCallback =
+      base::OnceCallback<void(base::ScopedFD server_socket)>;
+  using UpgradeErrorCallback =
+      base::OnceCallback<void(bool low_free_disk_space)>;
+  virtual void UpgradeArcContainer(
+      const login_manager::UpgradeArcContainerRequest& request,
+      UpgradeArcContainerCallback success_callback,
+      UpgradeErrorCallback error_callback) = 0;
 
   // Asynchronously stops the ARC instance.  Upon completion, invokes
   // |callback| with the result; true on success, false on failure (either

components/arc/arc_session_impl.cc

@@ -37,9 +37,6 @@ namespace arc {
 
 namespace {
 
-using StartArcInstanceResult =
-    chromeos::SessionManagerClient::StartArcInstanceResult;
-
 chromeos::SessionManagerClient* GetSessionManagerClient() {
   // If the DBusThreadManager or the SessionManagerClient aren't available,
   // there isn't much we can do. This should only happen when running tests.
@@ -92,22 +89,13 @@ bool WaitForSocketReadable(int raw_socket_fd, int raw_cancel_fd) {
 }
 
 // Returns the ArcStopReason corresponding to the ARC instance staring failure.
-ArcStopReason GetArcStopReason(StartArcInstanceResult result,
-                               bool stop_requested) {
+ArcStopReason GetArcStopReason(bool low_disk_space, bool stop_requested) {
   if (stop_requested)
     return ArcStopReason::SHUTDOWN;
 
-  switch (result) {
-    case StartArcInstanceResult::SUCCESS:
-      NOTREACHED();
-      break;
-    case StartArcInstanceResult::UNKNOWN_ERROR:
-      return ArcStopReason::GENERIC_BOOT_FAILURE;
-    case StartArcInstanceResult::LOW_FREE_DISK_SPACE:
-      return ArcStopReason::LOW_DISK_SPACE;
-  }
+  if (low_disk_space)
+    return ArcStopReason::LOW_DISK_SPACE;
 
-  NOTREACHED();
   return ArcStopReason::GENERIC_BOOT_FAILURE;
 }
 
@@ -273,13 +261,13 @@ void ArcSessionImpl::StartMiniInstance() {
 
   state_ = State::STARTING_MINI_INSTANCE;
   VLOG(2) << "Starting ARC mini instance";
-  login_manager::StartArcInstanceRequest request;
+
+  login_manager::StartArcMiniContainerRequest request;
   request.set_native_bridge_experiment(
       base::FeatureList::IsEnabled(arc::kNativeBridgeExperimentFeature));
-  request.set_for_login_screen(true);
 
   chromeos::SessionManagerClient* client = GetSessionManagerClient();
-  client->StartArcInstance(
+  client->StartArcMiniContainer(
       request, base::BindOnce(&ArcSessionImpl::OnMiniInstanceStarted,
                               weak_factory_.GetWeakPtr()));
 }
@@ -312,20 +300,16 @@ void ArcSessionImpl::RequestUpgrade() {
 }
 
 void ArcSessionImpl::OnMiniInstanceStarted(
-    chromeos::SessionManagerClient::StartArcInstanceResult result,
-    const std::string& container_instance_id,
-    base::ScopedFD socket_fd) {
+    base::Optional<std::string> container_instance_id) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK_EQ(state_, State::STARTING_MINI_INSTANCE);
 
-  if (result != StartArcInstanceResult::SUCCESS) {
-    LOG(ERROR) << "Failed to start ARC instance";
-    OnStopped(GetArcStopReason(result, stop_requested_));
+  if (!container_instance_id) {
+    OnStopped(GetArcStopReason(false, stop_requested_));
     return;
   }
 
-  DCHECK(!container_instance_id.empty());
-  container_instance_id_ = container_instance_id;
+  container_instance_id_ = std::move(*container_instance_id);
   VLOG(2) << "ARC mini instance is successfully started: "
           << container_instance_id_;
 
@@ -348,9 +332,7 @@ void ArcSessionImpl::DoUpgrade() {
   VLOG(2) << "Upgrading an existing ARC mini instance";
   state_ = State::STARTING_FULL_INSTANCE;
 
-  login_manager::StartArcInstanceRequest request;
-  request.set_native_bridge_experiment(
-      base::FeatureList::IsEnabled(arc::kNativeBridgeExperimentFeature));
+  login_manager::UpgradeArcContainerRequest request;
   user_manager::UserManager* user_manager = user_manager::UserManager::Get();
   DCHECK(user_manager->GetPrimaryUser());
 
@@ -373,33 +355,28 @@ void ArcSessionImpl::DoUpgrade() {
   if (packages_cache_mode_string == kPackagesCacheModeSkipCopy) {
     request.set_packages_cache_mode(
         login_manager::
-            StartArcInstanceRequest_PackageCacheMode_SKIP_SETUP_COPY_ON_INIT);
+            UpgradeArcContainerRequest_PackageCacheMode_SKIP_SETUP_COPY_ON_INIT);
   } else if (packages_cache_mode_string == kPackagesCacheModeCopy) {
     request.set_packages_cache_mode(
-        login_manager::StartArcInstanceRequest_PackageCacheMode_COPY_ON_INIT);
+        login_manager::
+            UpgradeArcContainerRequest_PackageCacheMode_COPY_ON_INIT);
   } else if (!packages_cache_mode_string.empty()) {
     VLOG(2) << "Invalid packages cache mode switch "
             << packages_cache_mode_string << ".";
   }
 
   chromeos::SessionManagerClient* client = GetSessionManagerClient();
-  client->StartArcInstance(request, base::BindOnce(&ArcSessionImpl::OnUpgraded,
-                                                   weak_factory_.GetWeakPtr()));
+  client->UpgradeArcContainer(
+      request,
+      base::BindOnce(&ArcSessionImpl::OnUpgraded, weak_factory_.GetWeakPtr()),
+      base::BindOnce(&ArcSessionImpl::OnUpgradeError,
+                     weak_factory_.GetWeakPtr()));
 }
 
-void ArcSessionImpl::OnUpgraded(
-    chromeos::SessionManagerClient::StartArcInstanceResult result,
-    const std::string& container_instance_id,  // unused
-    base::ScopedFD socket_fd) {
+void ArcSessionImpl::OnUpgraded(base::ScopedFD socket_fd) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK_EQ(state_, State::STARTING_FULL_INSTANCE);
 
-  if (result != StartArcInstanceResult::SUCCESS) {
-    LOG(ERROR) << "Failed to start ARC instance";
-    OnStopped(GetArcStopReason(result, stop_requested_));
-    return;
-  }
-
   VLOG(2) << "ARC instance is successfully upgraded.";
 
   if (stop_requested_) {
@@ -420,6 +397,10 @@ void ArcSessionImpl::OnUpgraded(
   }
 }
 
+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_);

components/arc/arc_session_impl.h

@@ -13,6 +13,7 @@
 #include "base/files/scoped_file.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/optional.h"
 #include "base/threading/thread_checker.h"
 #include "chromeos/dbus/session_manager_client.h"
 #include "components/arc/arc_session.h"
@@ -145,24 +146,20 @@ class ArcSessionImpl : public ArcSession,
   void OnShutdown() override;
 
  private:
-  // D-Bus callback for StartArcInstance() for a mini instance.
-  // In case of success, |container_instance_id| must not be empty, and
-  // |socket_fd| is /dev/null.
-  // TODO(hidehiko): Remove |socket_fd| from this callback.
-  void OnMiniInstanceStarted(
-      chromeos::SessionManagerClient::StartArcInstanceResult result,
-      const std::string& container_instance_id,
-      base::ScopedFD socket_fd);
+  // D-Bus callback for StartArcMiniContainer().
+  void OnMiniInstanceStarted(base::Optional<std::string> container_instance_id);
 
   // Sends a D-Bus message to upgrade to a full instance.
   void DoUpgrade();
 
-  // D-Bus callback for StartArcInstance() for an upgrade. |socket_fd| should be
-  // a socket which should be accept(2)ed to connect ArcBridgeService Mojo
-  // channel.
-  void OnUpgraded(chromeos::SessionManagerClient::StartArcInstanceResult result,
-                  const std::string& container_instance_id,  // unused
-                  base::ScopedFD socket_fd);
+  // 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);
+
+  // D-Bus callback for UpgradeArcContainer when the upgrade fails.
+  // |low_free_disk_space| signals whether the failure was due to low free disk
+  // space.
+  void OnUpgradeError(bool low_free_disk_space);
 
   // Called when Mojo connection is established (or canceled during the
   // connect.)