Unit test for CrostiniPackageService

Add unit test for CrostiniPackageService. Also fixes a corner case found
by unit test (queuing uninstalls while install starts up).

BUG=822514
TEST=Ran unit test

Change-Id: Ieede7119febda6a2fb67ca9835e8d859ddf08400
Reviewed-on: https://chromium-review.googlesource.com/c/1401611
Commit-Queue: Ian Barkley-Yeung <iby@chromium.org>
Reviewed-by: Timothy Loh <timloh@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#630485}

chrome/browser/chromeos/BUILD.gn

@@ -2214,6 +2214,7 @@ source_set("unit_tests") {
     "child_accounts/usage_time_limit_processor_unittest.cc",
     "child_accounts/usage_time_state_notifier_unittest.cc",
     "crostini/crostini_manager_unittest.cc",
+    "crostini/crostini_package_service_unittest.cc",
     "crostini/crostini_share_path_unittest.cc",
     "crostini/crosvm_metrics_unittest.cc",
     "crostini/crosvm_process_list_unittest.cc",

chrome/browser/chromeos/crostini/crostini_manager.cc

@@ -352,6 +352,13 @@ class CrostiniManager::CrostiniRestarter
 
   void SetUpLxdContainerUserFinished(CrostiniResult result) {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    // The restarter shouldn't outlive the CrostiniManager but it can when
+    // skip_restart_for_testing is set.
+    if (!crostini_manager_) {
+      LOG(ERROR) << "CrostiniManager deleted";
+      return;
+    }
+
     // Tell observers.
     for (auto& observer : observer_list_) {
       observer.OnContainerSetup(result);

chrome/browser/chromeos/crostini/crostini_package_service.cc

@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/no_destructor.h"
+#include "base/stl_util.h"
 #include "base/strings/strcat.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -117,6 +118,11 @@ void CrostiniPackageService::Shutdown() {
   manager->RemoveLinuxPackageOperationProgressObserver(this);
 }
 
+void CrostiniPackageService::SetNotificationStateChangeCallbackForTesting(
+    StateChangeCallback state_change_callback) {
+  testing_state_change_callback_ = std::move(state_change_callback);
+}
+
 void CrostiniPackageService::NotificationCompleted(
     CrostiniPackageNotification* notification) {
   for (auto it = finished_notifications_.begin();
@@ -147,6 +153,9 @@ void CrostiniPackageService::InstallLinuxPackage(
     const std::string& container_name,
     const std::string& package_path,
     CrostiniManager::InstallLinuxPackageCallback callback) {
+  const ContainerIdentifier container_id(vm_name, container_name);
+  containers_with_pending_installs_.insert(container_id);
+
   CrostiniManager::GetForProfile(profile_)->InstallLinuxPackage(
       vm_name, container_name, package_path,
       base::BindOnce(&CrostiniPackageService::OnInstallLinuxPackage,
@@ -213,8 +222,8 @@ std::string CrostiniPackageService::ContainerIdentifierToString(
 
 bool CrostiniPackageService::ContainerHasRunningOperation(
     const ContainerIdentifier& container_id) const {
-  return running_notifications_.find(container_id) !=
-         running_notifications_.end();
+  return base::ContainsKey(running_notifications_, container_id) ||
+         base::ContainsKey(containers_with_pending_installs_, container_id);
 }
 
 void CrostiniPackageService::CreateRunningNotification(
@@ -279,6 +288,9 @@ void CrostiniPackageService::UpdatePackageOperationStatus(
       StartQueuedUninstall(container_id);
     }
   }
+  if (testing_state_change_callback_) {
+    testing_state_change_callback_.Run(status);
+  }
 }
 
 void CrostiniPackageService::OnGetLinuxPackageInfo(
@@ -295,9 +307,18 @@ void CrostiniPackageService::OnInstallLinuxPackage(
     CrostiniManager::InstallLinuxPackageCallback callback,
     CrostiniResult result) {
   std::move(callback).Run(result);
-  if (result != CrostiniResult::SUCCESS)
-    return;
   const ContainerIdentifier container_id(vm_name, container_name);
+  containers_with_pending_installs_.erase(container_id);
+  if (result != CrostiniResult::SUCCESS) {
+    // We never show a notification for this failed install, so this is our only
+    // chance to kick off uninstalled queued behind the install.
+    auto queued_iter = queued_uninstalls_.find(container_id);
+    if (queued_iter != queued_uninstalls_.end() &&
+        !queued_iter->second.empty()) {
+      StartQueuedUninstall(container_id);
+    }
+    return;
+  }
   CreateRunningNotification(
       container_id,
       CrostiniPackageNotification::NotificationType::PACKAGE_INSTALL,

chrome/browser/chromeos/crostini/crostini_package_service.h

@@ -8,10 +8,12 @@
 #include <map>
 #include <memory>
 #include <queue>
+#include <set>
 #include <string>
 #include <utility>
 #include <vector>
 
+#include "base/callback_forward.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/chromeos/crostini/crostini_manager.h"
 #include "chrome/browser/chromeos/crostini/crostini_package_notification.h"
@@ -24,11 +26,19 @@ namespace crostini {
 class CrostiniPackageService : public KeyedService,
                                public LinuxPackageOperationProgressObserver {
  public:
+  using StateChangeCallback =
+      base::RepeatingCallback<void(PackageOperationStatus)>;
+
   static CrostiniPackageService* GetForProfile(Profile* profile);
 
   explicit CrostiniPackageService(Profile* profile);
   ~CrostiniPackageService() override;
 
+  // For testing: Set a callback that will be called each time a notification
+  // is set to a new state.
+  void SetNotificationStateChangeCallbackForTesting(
+      StateChangeCallback state_change_callback);
+
   // KeyedService:
   void Shutdown() override;
 
@@ -147,6 +157,11 @@ class CrostiniPackageService : public KeyedService,
   std::map<ContainerIdentifier, std::unique_ptr<CrostiniPackageNotification>>
       running_notifications_;
 
+  // Containers that have an install waiting for its initial response. We don't
+  // display notifications for these, but they still need to cause uninstalls
+  // to queue.
+  std::set<ContainerIdentifier> containers_with_pending_installs_;
+
   // Uninstalls we want to run when the current one is done.
   std::map<ContainerIdentifier, std::queue<QueuedUninstall>> queued_uninstalls_;
 
@@ -156,6 +171,9 @@ class CrostiniPackageService : public KeyedService,
   std::vector<std::unique_ptr<CrostiniPackageNotification>>
       finished_notifications_;
 
+  // Called each time a notification is set to a new state.
+  StateChangeCallback testing_state_change_callback_;
+
   int next_notification_id_ = 0;
 
   base::WeakPtrFactory<CrostiniPackageService> weak_ptr_factory_;

chromeos/dbus/fake_cicerone_client.cc

@@ -114,6 +114,7 @@ void FakeCiceroneClient::GetContainerAppIcons(
 void FakeCiceroneClient::GetLinuxPackageInfo(
     const vm_tools::cicerone::LinuxPackageInfoRequest& request,
     DBusMethodCallback<vm_tools::cicerone::LinuxPackageInfoResponse> callback) {
+  most_recent_linux_package_info_request_ = request;
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::BindOnce(std::move(callback), get_linux_package_info_response_));
@@ -123,18 +124,28 @@ void FakeCiceroneClient::InstallLinuxPackage(
     const vm_tools::cicerone::InstallLinuxPackageRequest& request,
     DBusMethodCallback<vm_tools::cicerone::InstallLinuxPackageResponse>
         callback) {
+  most_recent_install_linux_package_request_ = request;
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::BindOnce(std::move(callback), install_linux_package_response_));
 }
 
+void FakeCiceroneClient::SetOnUninstallPackageOwningFileCallback(
+    UninstallPackageOwningFileCallback callback) {
+  uninstall_package_owning_file_callback_ = std::move(callback);
+}
+
 void FakeCiceroneClient::UninstallPackageOwningFile(
     const vm_tools::cicerone::UninstallPackageOwningFileRequest& request,
     DBusMethodCallback<vm_tools::cicerone::UninstallPackageOwningFileResponse>
         callback) {
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback),
-                                uninstall_package_owning_file_response_));
+  if (uninstall_package_owning_file_callback_) {
+    uninstall_package_owning_file_callback_.Run(request, std::move(callback));
+  } else {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(std::move(callback),
+                                  uninstall_package_owning_file_response_));
+  }
 }
 
 void FakeCiceroneClient::WaitForServiceToBeAvailable(
@@ -278,4 +289,18 @@ void FakeCiceroneClient::NotifyImportLxdContainerProgress(
   }
 }
 
+void FakeCiceroneClient::InstallLinuxPackageProgress(
+    const vm_tools::cicerone::InstallLinuxPackageProgressSignal& signal) {
+  for (auto& observer : observer_list_) {
+    observer.OnInstallLinuxPackageProgress(signal);
+  }
+}
+
+void FakeCiceroneClient::UninstallPackageProgress(
+    const vm_tools::cicerone::UninstallPackageProgressSignal& signal) {
+  for (auto& observer : observer_list_) {
+    observer.OnUninstallPackageProgress(signal);
+  }
+}
+
 }  // namespace chromeos

chromeos/dbus/fake_cicerone_client.h

@@ -15,6 +15,10 @@ namespace chromeos {
 class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeCiceroneClient
     : public CiceroneClient {
  public:
+  using UninstallPackageOwningFileCallback = base::RepeatingCallback<void(
+      const vm_tools::cicerone::UninstallPackageOwningFileRequest&,
+      DBusMethodCallback<
+          vm_tools::cicerone::UninstallPackageOwningFileResponse>)>;
   FakeCiceroneClient();
   ~FakeCiceroneClient() override;
 
@@ -86,9 +90,15 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeCiceroneClient
       DBusMethodCallback<vm_tools::cicerone::InstallLinuxPackageResponse>
           callback) override;
 
+  // Sets a callback to be called during any call to UninstallPackageOwningFile.
+  void SetOnUninstallPackageOwningFileCallback(
+      UninstallPackageOwningFileCallback callback);
+
   // Fake version of the method that uninstalls an application inside a running
-  // Container. |callback| is called after the method call finishes. This does
-  // not cause progress events to be fired.
+  // Container. If SetOnUninstallPackageOwningFileCallback has been called, it
+  // just triggers that callback. Otherwise, it generates a task to call
+  // |callback| with the response from
+  // set_uninstall_package_owning_file_response.
   void UninstallPackageOwningFile(
       const vm_tools::cicerone::UninstallPackageOwningFileRequest& request,
       DBusMethodCallback<vm_tools::cicerone::UninstallPackageOwningFileResponse>
@@ -216,12 +226,22 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeCiceroneClient
     container_app_icon_response_ = container_app_icon_response;
   }
 
+  const vm_tools::cicerone::LinuxPackageInfoRequest&
+  get_most_recent_linux_package_info_request() const {
+    return most_recent_linux_package_info_request_;
+  }
+
   void set_linux_package_info_response(
       const vm_tools::cicerone::LinuxPackageInfoResponse&
           get_linux_package_info_response) {
     get_linux_package_info_response_ = get_linux_package_info_response;
   }
 
+  const vm_tools::cicerone::InstallLinuxPackageRequest&
+  get_most_recent_install_linux_package_request() const {
+    return most_recent_install_linux_package_request_;
+  }
+
   void set_install_linux_package_response(
       const vm_tools::cicerone::InstallLinuxPackageResponse&
           install_linux_package_response) {
@@ -284,6 +304,10 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeCiceroneClient
       const vm_tools::cicerone::ExportLxdContainerProgressSignal& signal);
   void NotifyImportLxdContainerProgress(
       const vm_tools::cicerone::ImportLxdContainerProgressSignal& signal);
+  void InstallLinuxPackageProgress(
+      const vm_tools::cicerone::InstallLinuxPackageProgressSignal& signal);
+  void UninstallPackageProgress(
+      const vm_tools::cicerone::UninstallPackageProgressSignal& signal);
 
  protected:
   void Init(dbus::Bus* bus) override {}
@@ -310,7 +334,11 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeCiceroneClient
   vm_tools::cicerone::LaunchContainerApplicationResponse
       launch_container_application_response_;
   vm_tools::cicerone::ContainerAppIconResponse container_app_icon_response_;
+  vm_tools::cicerone::LinuxPackageInfoRequest
+      most_recent_linux_package_info_request_;
   vm_tools::cicerone::LinuxPackageInfoResponse get_linux_package_info_response_;
+  vm_tools::cicerone::InstallLinuxPackageRequest
+      most_recent_install_linux_package_request_;
   vm_tools::cicerone::InstallLinuxPackageResponse
       install_linux_package_response_;
   vm_tools::cicerone::UninstallPackageOwningFileResponse
@@ -325,6 +353,8 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeCiceroneClient
   vm_tools::cicerone::ExportLxdContainerResponse export_lxd_container_response_;
   vm_tools::cicerone::ImportLxdContainerResponse import_lxd_container_response_;
 
+  UninstallPackageOwningFileCallback uninstall_package_owning_file_callback_;
+
   base::ObserverList<Observer>::Unchecked observer_list_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeCiceroneClient);