Treat a package operations as failed on VM shutdown

Currently we just leave linux package operation notifications hanging
around uncompleted if the vm is shutdown before the operation
finished. Change this to have them switch to the error state on VM
shutdown. We don't try to detect shutdowns that don't go though the UI.

Bug: 966334
Change-Id: Ibdfba27291cd3f4aa3780b5b10a549dd4ea91b17
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1659556Reviewed-by: Nicholas Verne <nverne@chromium.org>
Commit-Queue: Fergus Dall <sidereal@google.com>
Cr-Commit-Position: refs/heads/master@{#669137}

chrome/browser/chromeos/crostini/crostini_manager.cc

@@ -1719,6 +1719,13 @@ void CrostiniManager::RemoveImportContainerProgressObserver(
   import_container_progress_observers_.RemoveObserver(observer);
 }
 
+void CrostiniManager::AddVmShutdownObserver(VmShutdownObserver* observer) {
+  vm_shutdown_observers_.AddObserver(observer);
+}
+void CrostiniManager::RemoveVmShutdownObserver(VmShutdownObserver* observer) {
+  vm_shutdown_observers_.RemoveObserver(observer);
+}
+
 void CrostiniManager::OnCreateDiskImage(
     CreateDiskImageCallback callback,
     base::Optional<vm_tools::concierge::CreateDiskImageResponse> reply) {
@@ -1883,6 +1890,11 @@ void CrostiniManager::OnStopVm(
       return;
     }
   }
+  // Notify observers
+  for (auto& observer : vm_shutdown_observers_) {
+    observer.OnVmShutdown(vm_name);
+  }
+
   // Remove from running_vms_, and other vm-keyed state.
   running_vms_.erase(vm_name);
   running_containers_.erase(vm_name);

chrome/browser/chromeos/crostini/crostini_manager.h

@@ -212,6 +212,12 @@ class InstallerViewStatusObserver : public base::CheckedObserver {
   virtual void OnCrostiniInstallerViewStatusChanged(bool open) = 0;
 };
 
+class VmShutdownObserver : public base::CheckedObserver {
+ public:
+  // Called when the given VM has shutdown.
+  virtual void OnVmShutdown(const std::string& vm_name) = 0;
+};
+
 // CrostiniManager is a singleton which is used to check arguments for
 // ConciergeClient and CiceroneClient. ConciergeClient is dedicated to
 // communication with the Concierge service, CiceroneClient is dedicated to
@@ -594,6 +600,10 @@ class CrostiniManager : public KeyedService,
   void RemoveImportContainerProgressObserver(
       ImportContainerProgressObserver* observer);
 
+  // Add/remove vm shutdown observers.
+  void AddVmShutdownObserver(VmShutdownObserver* observer);
+  void RemoveVmShutdownObserver(VmShutdownObserver* observer);
+
   // ConciergeClient::Observer:
   void OnContainerStartupFailed(
       const vm_tools::concierge::ContainerStartedSignal& signal) override;
@@ -917,6 +927,8 @@ class CrostiniManager : public KeyedService,
   base::ObserverList<ImportContainerProgressObserver>::Unchecked
       import_container_progress_observers_;
 
+  base::ObserverList<VmShutdownObserver> vm_shutdown_observers_;
+
   // Restarts by <vm_name, container_name>. Only one restarter flow is actually
   // running for a given container, other restarters will just have their
   // callback called when the running restarter completes.

chrome/browser/chromeos/crostini/crostini_package_service.cc

@@ -109,6 +109,7 @@ CrostiniPackageService::CrostiniPackageService(Profile* profile)
 
   manager->AddLinuxPackageOperationProgressObserver(this);
   manager->AddPendingAppListUpdatesObserver(this);
+  manager->AddVmShutdownObserver(this);
 }
 
 CrostiniPackageService::~CrostiniPackageService() = default;
@@ -117,6 +118,7 @@ void CrostiniPackageService::Shutdown() {
   CrostiniManager* manager = CrostiniManager::GetForProfile(profile_);
   manager->RemoveLinuxPackageOperationProgressObserver(this);
   manager->RemovePendingAppListUpdatesObserver(this);
+  manager->RemoveVmShutdownObserver(this);
 }
 
 void CrostiniPackageService::SetNotificationStateChangeCallbackForTesting(
@@ -203,6 +205,26 @@ void CrostiniPackageService::OnUninstallPackageProgress(
                                progress_percent);
 }
 
+void CrostiniPackageService::OnVmShutdown(const std::string& vm_name) {
+  // Making a notification as failed removes it from |running_notifications_|,
+  // which invalidates the iterators. To avoid this, we record all the
+  // containers that just shut down before removing any notifications.
+  std::vector<ContainerId> to_remove;
+  for (auto iter = running_notifications_.begin();
+       iter != running_notifications_.end(); iter++) {
+    if (iter->first.first == vm_name) {
+      to_remove.push_back(iter->first);
+    }
+  }
+  for (auto iter : to_remove) {
+    // Use a loop because removing a notification from the running set can cause
+    // a queued operation to start, which will also need to be removed.
+    while (ContainerHasRunningOperation(iter)) {
+      UpdatePackageOperationStatus(iter, PackageOperationStatus::FAILED, 0);
+    }
+  }
+}
+
 void CrostiniPackageService::QueueUninstallApplication(
     const std::string& app_id) {
   auto registration =

chrome/browser/chromeos/crostini/crostini_package_service.h

@@ -26,7 +26,8 @@ namespace crostini {
 
 class CrostiniPackageService : public KeyedService,
                                public LinuxPackageOperationProgressObserver,
-                               public PendingAppListUpdatesObserver {
+                               public PendingAppListUpdatesObserver,
+                               public VmShutdownObserver {
  public:
   using StateChangeCallback =
       base::RepeatingCallback<void(PackageOperationStatus)>;
@@ -87,6 +88,9 @@ class CrostiniPackageService : public KeyedService,
                                const std::string& container_name,
                                int count) override;
 
+  // VmShutdownObserver
+  void OnVmShutdown(const std::string& vm_name) override;
+
   // (Eventually) uninstall the package identified by |app_id|. If successfully
   // started, a system notification will be used to display further updates.
   void QueueUninstallApplication(const std::string& app_id);

chrome/browser/chromeos/crostini/crostini_package_service_unittest.cc

@@ -23,6 +23,7 @@
 #include "chromeos/dbus/cros_disks_client.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/fake_cicerone_client.h"
+#include "chromeos/dbus/fake_concierge_client.h"
 #include "chromeos/dbus/vm_applications/apps.pb.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
@@ -1037,6 +1038,28 @@ TEST_F(CrostiniPackageServiceTest,
       UnorderedElementsAre(IsUninstallSuccessNotification(DEFAULT_APP)));
 }
 
+TEST_F(CrostiniPackageServiceTest, UninstallNotificationFailsOnVmShutdown) {
+  // Use two apps to ensure one is queued up.
+  service_->QueueUninstallApplication(kDefaultAppId);
+  service_->QueueUninstallApplication(kSecondAppId);
+
+  base::RunLoop run_loop;
+  CrostiniManager::GetForProfile(profile_.get())
+      ->StopVm(kCrostiniDefaultVmName,
+               base::BindOnce(
+                   [](base::OnceClosure quit, crostini::CrostiniResult) {
+                     std::move(quit).Run();
+                   },
+                   run_loop.QuitClosure()));
+  run_loop.Run();
+
+  EXPECT_THAT(
+      Printable(notification_display_service_->GetDisplayedNotificationsForType(
+          NotificationHandler::Type::TRANSIENT)),
+      UnorderedElementsAre(IsUninstallFailedNotification(DEFAULT_APP),
+                           IsUninstallFailedNotification(SECOND_APP)));
+}
+
 TEST_F(CrostiniPackageServiceTest, ClosingSuccessNotificationWorks) {
   service_->QueueUninstallApplication(kDefaultAppId);
   StartAndSignalUninstall(UninstallPackageProgressSignal::SUCCEEDED);
@@ -1821,6 +1844,31 @@ TEST_F(CrostiniPackageServiceTest,
       UnorderedElementsAre(IsInstallSuccessNotification()));
 }
 
+TEST_F(CrostiniPackageServiceTest, InstallNotificationFailsOnVmShutdown) {
+  service_->InstallLinuxPackage(kCrostiniDefaultVmName,
+                                kCrostiniDefaultContainerName, kPackageFilePath,
+                                base::DoNothing());
+
+  base::RunLoop().RunUntilIdle();
+
+  StartAndSignalInstall(InstallLinuxPackageProgressSignal::INSTALLING);
+
+  base::RunLoop run_loop;
+  CrostiniManager::GetForProfile(profile_.get())
+      ->StopVm(kCrostiniDefaultVmName,
+               base::BindOnce(
+                   [](base::OnceClosure quit, crostini::CrostiniResult) {
+                     std::move(quit).Run();
+                   },
+                   run_loop.QuitClosure()));
+  run_loop.Run();
+
+  EXPECT_THAT(
+      Printable(notification_display_service_->GetDisplayedNotificationsForType(
+          NotificationHandler::Type::TRANSIENT)),
+      UnorderedElementsAre(IsInstallFailedNotification()));
+}
+
 TEST_F(CrostiniPackageServiceTest, UninstallsQueuesBehindStartingUpInstall) {
   CrostiniResult result = CrostiniResult::UNKNOWN_ERROR;
   service_->InstallLinuxPackage(