arc: Stop ARCVM instance and ARC Upstart jobs in /data migration screen

When the migration screen is shown, there can be a stale ARCVM instance
and ARC-related Upstart jobs running from the previous session, which
should be stopped before starting the migration.

Bug: b:258278176
Test: unit_tests --gtest_filter="ArcVmDataMigrationScreenTest.*"
Test: ash_components_unittests --gtest_filter="ArcVmClientAdapterTest.*"
Test: --enable-features=ArcVmDataMigration
Test: Manually confirm that ARCVM and all ARC-related Upstart jobs are
stopped when the migration screen is shown.

Change-Id: I47134f66c5cc84c0f8b2eb48cc65fdf1c88b0129
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4022732
Commit-Queue: Youkichi Hosoi <youkichihosoi@chromium.org>
Reviewed-by: Yury Khmel <khmel@chromium.org>
Reviewed-by: Danila Kuzmin <dkuzmin@google.com>
Cr-Commit-Position: refs/heads/main@{#1104904}

ash/components/arc/session/arc_vm_client_adapter.cc

@@ -73,18 +73,6 @@
 namespace arc {
 namespace {
 
-// The "_2d" in job names below corresponds to "-". Upstart escapes characters
-// that aren't valid in D-Bus object paths with underscore followed by its
-// ascii code in hex. So "arc_2dcreate_2ddata" becomes "arc-create-data".
-constexpr const char kArcVmPerBoardFeaturesJobName[] =
-    "arcvm_2dper_2dboard_2dfeatures";
-constexpr char kArcVmPreLoginServicesJobName[] =
-    "arcvm_2dpre_2dlogin_2dservices";
-constexpr char kArcVmPostLoginServicesJobName[] =
-    "arcvm_2dpost_2dlogin_2dservices";
-constexpr char kArcVmPostVmStartServicesJobName[] =
-    "arcvm_2dpost_2dvm_2dstart_2dservices";
-
 constexpr const char kArcVmBootNotificationServerSocketPath[] =
     "/run/arcvm_boot_notification_server/host.socket";
 
@@ -617,23 +605,27 @@ class ArcVmClientAdapter : public ArcClientAdapter,
     }
 
     start_params_ = std::move(params);
-    std::vector<std::string> enviroment;
-    if (start_params_.disable_ureadahead)
-      enviroment.emplace_back("DISABLE_UREADAHEAD=1");
+
     std::deque<JobDesc> jobs{
         // Note: the first Upstart job is a task, and the callback for the start
         // request won't be called until the task finishes. When the callback is
         // called with true, it is ensured that the per-board features files
         // exist.
         JobDesc{kArcVmPerBoardFeaturesJobName, UpstartOperation::JOB_START, {}},
-        JobDesc{
-            kArcVmMediaSharingServicesJobName, UpstartOperation::JOB_STOP, {}},
-        JobDesc{
-            kArcVmPostVmStartServicesJobName, UpstartOperation::JOB_STOP, {}},
-        JobDesc{kArcVmPostLoginServicesJobName, UpstartOperation::JOB_STOP, {}},
-        JobDesc{kArcVmPreLoginServicesJobName,
-                UpstartOperation::JOB_STOP_AND_START, std::move(enviroment)},
     };
+
+    for (const char* job : kArcVmUpstartJobsToBeStoppedOnRestart) {
+      jobs.emplace_back(job, UpstartOperation::JOB_STOP,
+                        std::vector<std::string>());
+    }
+
+    std::vector<std::string> environment;
+    if (start_params_.disable_ureadahead) {
+      environment.emplace_back("DISABLE_UREADAHEAD=1");
+    }
+    jobs.emplace_back(kArcVmPreLoginServicesJobName,
+                      UpstartOperation::JOB_START, std::move(environment));
+
     ConfigureUpstartJobs(
         std::move(jobs),
         base::BindOnce(

ash/components/arc/session/arc_vm_client_adapter.h

@@ -5,6 +5,7 @@
 #ifndef ASH_COMPONENTS_ARC_SESSION_ARC_VM_CLIENT_ADAPTER_H_
 #define ASH_COMPONENTS_ARC_SESSION_ARC_VM_CLIENT_ADAPTER_H_
 
+#include <array>
 #include <memory>
 #include <string>
 
@@ -46,10 +47,30 @@ enum class ArcBinaryTranslationType {
 // 3328 is chosen because it's a rounded number (i.e. 3328 % 256 == 0).
 constexpr size_t k32bitVmRamMaxMib = 3328;
 
-// Name of upstart job to start ARCVM services for sharing media directories
-// like MyFiles.
+// Names of Upstart jobs that are managed in the ARCVM boot sequence.
+// The "_2d" in job names below corresponds to "-". Upstart escapes characters
+// that aren't valid in D-Bus object paths with underscore followed by its
+// ascii code in hex. So "arc_2dcreate_2ddata" becomes "arc-create-data".
 constexpr char kArcVmMediaSharingServicesJobName[] =
     "arcvm_2dmedia_2dsharing_2dservices";
+constexpr const char kArcVmPerBoardFeaturesJobName[] =
+    "arcvm_2dper_2dboard_2dfeatures";
+constexpr char kArcVmPreLoginServicesJobName[] =
+    "arcvm_2dpre_2dlogin_2dservices";
+constexpr char kArcVmPostLoginServicesJobName[] =
+    "arcvm_2dpost_2dlogin_2dservices";
+constexpr char kArcVmPostVmStartServicesJobName[] =
+    "arcvm_2dpost_2dvm_2dstart_2dservices";
+
+// List of Upstart jobs that can outlive ARC sessions (e.g. after Chrome crash,
+// Chrome restart on a feature flag change) and thus should be stopped at the
+// beginning of the ARCVM boot sequence.
+constexpr std::array<const char*, 4> kArcVmUpstartJobsToBeStoppedOnRestart = {
+    kArcVmPreLoginServicesJobName,
+    kArcVmPostLoginServicesJobName,
+    kArcVmPostVmStartServicesJobName,
+    kArcVmMediaSharingServicesJobName,
+};
 
 // For better unit-testing.
 class ArcVmClientAdapterDelegate {

ash/components/arc/session/arc_vm_client_adapter_unittest.cc

@@ -61,16 +61,8 @@
 namespace arc {
 namespace {
 
-constexpr const char kArcVmPerBoardFeaturesJobName[] =
-    "arcvm_2dper_2dboard_2dfeatures";
 constexpr const char kArcVmBootNotificationServerAddressPrefix[] =
     "\0test_arcvm_boot_notification_server";
-constexpr char kArcVmPreLoginServicesJobName[] =
-    "arcvm_2dpre_2dlogin_2dservices";
-constexpr char kArcVmPostLoginServicesJobName[] =
-    "arcvm_2dpost_2dlogin_2dservices";
-constexpr char kArcVmPostVmStartServicesJobName[] =
-    "arcvm_2dpost_2dvm_2dstart_2dservices";
 
 // Disk path contained in CreateDiskImageResponse().
 constexpr const char kCreatedDiskImagePath[] = "test/data.img";
@@ -794,25 +786,27 @@ TEST_F(ArcVmClientAdapterTest, StartMiniArc_StopArcVmPreLoginServicesJobFail) {
   StopArcInstance();
 }
 
-// Tests that StartMiniArc()'s JOB_STOP_AND_START for
-// |kArcVmPreLoginServicesJobName| is properly implemented.
+// Tests that |kArcVmPreLoginServicesJobName| is properly stopped and then
+// started in StartMiniArc().
 TEST_F(ArcVmClientAdapterTest, StartMiniArc_JobRestart) {
   StartRecordingUpstartOperations();
   StartMiniArc();
 
   const auto& ops = upstart_operations();
   // Find the STOP operation for the job.
-  auto it =
-      base::ranges::find_if(ops, [](const UpstartOperation& op) {
-        return op.type == UpstartOperationType::STOP &&
-               kArcVmPreLoginServicesJobName == op.name;
-      });
-  ASSERT_NE(ops.end(), it);
+  auto it = base::ranges::find_if(ops, [](const UpstartOperation& op) {
+    return op.type == UpstartOperationType::STOP &&
+           op.name == kArcVmPreLoginServicesJobName;
+  });
+  ASSERT_NE(it, ops.end());
   ++it;
-  ASSERT_NE(ops.end(), it);
-  // The next operation must be START for the job.
-  EXPECT_EQ(it->name, kArcVmPreLoginServicesJobName);
-  EXPECT_EQ(UpstartOperationType::START, it->type);
+  ASSERT_NE(it, ops.end());
+  // Find the START operation for the job.
+  it = base::ranges::find_if(it, ops.end(), [](const UpstartOperation& op) {
+    return op.type == UpstartOperationType::START &&
+           op.name == kArcVmPreLoginServicesJobName;
+  });
+  ASSERT_NE(it, ops.end());
 }
 
 // Tests that StopArcInstance() eventually notifies the observer.

chrome/browser/ash/BUILD.gn

@@ -5268,6 +5268,7 @@ source_set("unit_tests") {
     "login/saml/password_expiry_notification_unittest.cc",
     "login/saml/password_sync_token_login_checker_unittest.cc",
     "login/saml/password_sync_token_verifier_unittest.cc",
+    "login/screens/arc_vm_data_migration_screen_unittest.cc",
     "login/screens/chromevox_hint/chromevox_hint_detector_unittest.cc",
     "login/screens/encryption_migration_screen_unittest.cc",
     "login/screens/network_screen_unittest.cc",

chrome/browser/ash/login/screens/arc_vm_data_migration_screen.cc

@@ -4,14 +4,18 @@
 
 #include "chrome/browser/ash/login/screens/arc_vm_data_migration_screen.h"
 
+#include <deque>
+
 #include "ash/components/arc/arc_prefs.h"
 #include "ash/components/arc/arc_util.h"
+#include "ash/components/arc/session/arc_vm_client_adapter.h"
 #include "ash/components/arc/session/arc_vm_data_migration_status.h"
 #include "ash/public/cpp/session/scoped_screen_lock_blocker.h"
 #include "ash/session/session_controller_impl.h"
 #include "ash/shell.h"
 #include "base/logging.h"
 #include "base/notreached.h"
+#include "chrome/browser/ash/profiles/profile_helper.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chromeos/ash/components/dbus/spaced/spaced_client.h"
@@ -79,6 +83,8 @@ void ArcVmDataMigrationScreen::ShowImpl() {
   // login user, and thus the primary profile is available at this point.
   profile_ = ProfileManager::GetPrimaryUserProfile();
   DCHECK(profile_);
+  user_id_hash_ = ProfileHelper::GetUserIdHashFromProfile(profile_);
+  DCHECK(!user_id_hash_.empty());
 
   GetWakeLock()->RequestWakeLock();
 
@@ -88,8 +94,7 @@ void ArcVmDataMigrationScreen::ShowImpl() {
       Shell::Get()->session_controller()->GetScopedScreenLockBlocker();
 
   view_->Show();
-  // TODO(b/258278176): Stop stale ARCVM and Upstart jobs while loading.
-  SetUpInitialView();
+  StopArcVmInstanceAndArcUpstartJobs();
 }
 
 void ArcVmDataMigrationScreen::HideImpl() {
@@ -111,6 +116,83 @@ void ArcVmDataMigrationScreen::OnUserAction(const base::Value::List& args) {
   }
 }
 
+void ArcVmDataMigrationScreen::StopArcVmInstanceAndArcUpstartJobs() {
+  DCHECK(ConciergeClient::Get());
+
+  // Check whether ARCVM is running. At this point ArcSessionManager is not
+  // initialized yet, but a stale ARCVM instance can be running.
+  vm_tools::concierge::GetVmInfoRequest request;
+  request.set_name(arc::kArcVmName);
+  request.set_owner_id(user_id_hash_);
+  ConciergeClient::Get()->GetVmInfo(
+      std::move(request),
+      base::BindOnce(&ArcVmDataMigrationScreen::OnGetVmInfoResponse,
+                     weak_ptr_factory_.GetWeakPtr()));
+}
+
+void ArcVmDataMigrationScreen::OnGetVmInfoResponse(
+    absl::optional<vm_tools::concierge::GetVmInfoResponse> response) {
+  if (!response.has_value()) {
+    LOG(ERROR) << "GetVmInfo for ARCVM failed: No D-Bus response";
+    HandleFatalError();
+    return;
+  }
+
+  // Unsuccessful response means that ARCVM is not running, because concierge
+  // looks at the list of running VMs. See concierge's Service::GetVmInfo().
+  if (!response->success()) {
+    VLOG(1) << "ARCVM is not running";
+    StopArcUpstartJobs();
+    return;
+  }
+
+  // ARCVM is running. Send the StopVmRequest signal and wait for OnVmStopped()
+  // to be invoked.
+  VLOG(1) << "ARCVM is running. Sending StopVmRequest to concierge";
+  concierge_observation_.Observe(ConciergeClient::Get());
+  vm_tools::concierge::StopVmRequest request;
+  request.set_name(arc::kArcVmName);
+  request.set_owner_id(user_id_hash_);
+  ConciergeClient::Get()->StopVm(
+      std::move(request),
+      base::BindOnce(&ArcVmDataMigrationScreen::OnStopVmResponse,
+                     weak_ptr_factory_.GetWeakPtr()));
+}
+
+void ArcVmDataMigrationScreen::OnStopVmResponse(
+    absl::optional<vm_tools::concierge::StopVmResponse> response) {
+  if (!response.has_value() || !response->success()) {
+    LOG(ERROR) << "StopVm for ARCVM failed: "
+               << (response.has_value() ? response->failure_reason()
+                                        : "No D-Bus response");
+    concierge_observation_.Reset();
+    HandleFatalError();
+  }
+}
+
+void ArcVmDataMigrationScreen::StopArcUpstartJobs() {
+  std::deque<arc::JobDesc> jobs;
+  for (const char* job : arc::kArcVmUpstartJobsToBeStoppedOnRestart) {
+    jobs.emplace_back(job, arc::UpstartOperation::JOB_STOP,
+                      std::vector<std::string>());
+  }
+  arc::ConfigureUpstartJobs(
+      std::move(jobs),
+      base::BindOnce(&ArcVmDataMigrationScreen::OnArcUpstartJobsStopped,
+                     weak_ptr_factory_.GetWeakPtr()));
+}
+
+void ArcVmDataMigrationScreen::OnArcUpstartJobsStopped(bool result) {
+  // |result| is true when there are no stale Upstart jobs.
+  if (!result) {
+    LOG(ERROR) << "Failed to stop ARC Upstart jobs";
+    HandleFatalError();
+    return;
+  }
+
+  SetUpInitialView();
+}
+
 void ArcVmDataMigrationScreen::SetUpInitialView() {
   arc::ArcVmDataMigrationStatus data_migration_status =
       arc::GetArcVmDataMigrationStatus(profile_->GetPrefs());
@@ -171,6 +253,20 @@ void ArcVmDataMigrationScreen::OnGetFreeDiskSpace(
   chromeos::PowerManagerClient::Get()->RequestStatusUpdate();
 }
 
+void ArcVmDataMigrationScreen::OnVmStarted(
+    const vm_tools::concierge::VmStartedSignal& signal) {}
+
+void ArcVmDataMigrationScreen::OnVmStopped(
+    const vm_tools::concierge::VmStoppedSignal& signal) {
+  if (signal.name() != arc::kArcVmName) {
+    return;
+  }
+
+  VLOG(1) << "ARCVM is stopped";
+  concierge_observation_.Reset();
+  StopArcUpstartJobs();
+}
+
 void ArcVmDataMigrationScreen::UpdateUIState(
     ArcVmDataMigrationScreenView::UIState state) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

chrome/browser/ash/login/screens/arc_vm_data_migration_screen.h

@@ -11,6 +11,7 @@
 #include "chrome/browser/ash/login/screens/base_screen.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/webui/ash/login/arc_vm_data_migration_screen_handler.h"
+#include "chromeos/ash/components/dbus/concierge/concierge_client.h"
 #include "chromeos/dbus/power/power_manager_client.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "services/device/public/mojom/wake_lock.mojom.h"
@@ -21,6 +22,7 @@ namespace ash {
 class ScopedScreenLockBlocker;
 
 class ArcVmDataMigrationScreen : public BaseScreen,
+                                 public ConciergeClient::VmObserver,
                                  public chromeos::PowerManagerClient::Observer {
  public:
   explicit ArcVmDataMigrationScreen(
@@ -38,27 +40,47 @@ class ArcVmDataMigrationScreen : public BaseScreen,
   void HideImpl() override;
   void OnUserAction(const base::Value::List& args) override;
 
+  // Stops ARCVM instance and ARC-related Upstart jobs that have outlived the
+  // previous session.
+  void StopArcVmInstanceAndArcUpstartJobs();
+
+  void OnGetVmInfoResponse(
+      absl::optional<vm_tools::concierge::GetVmInfoResponse> response);
+  void OnStopVmResponse(
+      absl::optional<vm_tools::concierge::StopVmResponse> response);
+
+  void StopArcUpstartJobs();
+  void OnArcUpstartJobsStopped(bool result);
+
   void SetUpInitialView();
 
   void OnGetFreeDiskSpace(absl::optional<int64_t> reply);
 
+  // ConciergeClient::VmObserver overrides:
+  void OnVmStarted(const vm_tools::concierge::VmStartedSignal& signal) override;
+  void OnVmStopped(const vm_tools::concierge::VmStoppedSignal& signal) override;
+
   void UpdateUIState(ArcVmDataMigrationScreenView::UIState state);
 
   void HandleSkip();
   void HandleUpdate();
 
-  void HandleFatalError();
+  virtual void HandleFatalError();
 
-  device::mojom::WakeLock* GetWakeLock();
+  virtual device::mojom::WakeLock* GetWakeLock();
 
   Profile* profile_;
+  std::string user_id_hash_;
 
   ArcVmDataMigrationScreenView::UIState current_ui_state_ =
       ArcVmDataMigrationScreenView::UIState::kLoading;
 
   double battery_percent_ = 100.0;
   bool is_connected_to_charger_ = true;
 
+  base::ScopedObservation<ConciergeClient, ConciergeClient::VmObserver>
+      concierge_observation_{this};
+
   base::ScopedObservation<chromeos::PowerManagerClient,
                           chromeos::PowerManagerClient::Observer>
       power_manager_observation_{this};