Don't (un)install crostini while it's being (un)installed

This CL changes CrostiniManager to not try to install/restart crostini
while it is being uninstalled, and to abort any running restarts if
the uninstall process starts. It does not update the UI to prevent the
user from trying to do both operations at the same time.

Bug: 919153, 928339
Change-Id: Iccb829228e902c24a1ce653c1c244ad789bebe41
Reviewed-on: https://chromium-review.googlesource.com/c/1453956
Commit-Queue: Fergus Dall <sidereal@google.com>
Reviewed-by: Nicholas Verne <nverne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#629479}

chrome/browser/chromeos/crostini/crostini_manager.cc

@@ -8,6 +8,7 @@
 #include <string>
 #include <vector>
 
+#include "base/barrier_closure.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/no_destructor.h"
@@ -106,8 +107,10 @@ class CrostiniManager::CrostiniRestarter
 
   void Restart() {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-    if (is_aborted_)
+    if (is_aborted_) {
+      std::move(abort_callback_).Run();
       return;
+    }
     is_running_ = true;
     // Skip to the end immediately if testing.
     if (crostini_manager_->skip_restart_for_testing()) {
@@ -128,9 +131,10 @@ class CrostiniManager::CrostiniRestarter
 
   void RunCallback(CrostiniResult result) { std::move(callback_).Run(result); }
 
-  void Abort() {
+  void Abort(CrostiniManager::AbortRestartCallback callback) {
     is_aborted_ = true;
     observer_list_.Clear();
+    abort_callback_ = std::move(callback);
   }
 
   void OnContainerDownloading(int download_percent) {
@@ -146,6 +150,7 @@ class CrostiniManager::CrostiniRestarter
   CrostiniManager::RestartId restart_id() const { return restart_id_; }
   std::string vm_name() const { return vm_name_; }
   std::string container_name() const { return container_name_; }
+  bool is_aborted() const { return is_aborted_; }
 
  private:
   friend class base::RefCountedThreadSafe<CrostiniRestarter>;
@@ -165,8 +170,10 @@ class CrostiniManager::CrostiniRestarter
     for (auto& observer : observer_list_) {
       observer.OnComponentLoaded(result);
     }
-    if (is_aborted_)
+    if (is_aborted_) {
+      std::move(abort_callback_).Run();
       return;
+    }
     if (result != CrostiniResult::SUCCESS) {
       FinishRestart(result);
       return;
@@ -183,8 +190,10 @@ class CrostiniManager::CrostiniRestarter
     for (auto& observer : observer_list_) {
       observer.OnConciergeStarted(result);
     }
-    if (is_aborted_)
+    if (is_aborted_) {
+      std::move(abort_callback_).Run();
       return;
+    }
     if (!is_started) {
       LOG(ERROR) << "Failed to start Concierge service.";
       FinishRestart(result);
@@ -197,8 +206,10 @@ class CrostiniManager::CrostiniRestarter
 
   void ListVmDisksFinished(CrostiniResult result, int64_t disk_space_taken) {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-    if (is_aborted_)
+    if (is_aborted_) {
+      std::move(abort_callback_).Run();
       return;
+    }
     if (result != CrostiniResult::SUCCESS) {
       LOG(ERROR) << "Failed to list disk images.";
       FinishRestart(result);
@@ -246,8 +257,10 @@ class CrostiniManager::CrostiniRestarter
     for (auto& observer : observer_list_) {
       observer.OnDiskImageCreated(result, status, disk_size_available);
     }
-    if (is_aborted_)
+    if (is_aborted_) {
+      std::move(abort_callback_).Run();
       return;
+    }
     if (result != CrostiniResult::SUCCESS) {
       LOG(ERROR) << "Failed to create disk image.";
       FinishRestart(result);
@@ -264,8 +277,10 @@ class CrostiniManager::CrostiniRestarter
     for (auto& observer : observer_list_) {
       observer.OnVmStarted(result);
     }
-    if (is_aborted_)
+    if (is_aborted_) {
+      std::move(abort_callback_).Run();
       return;
+    }
     if (result != CrostiniResult::SUCCESS) {
       LOG(ERROR) << "Failed to Start Termina VM.";
       FinishRestart(result);
@@ -282,8 +297,10 @@ class CrostiniManager::CrostiniRestarter
     for (auto& observer : observer_list_) {
       observer.OnContainerCreated(result);
     }
-    if (is_aborted_)
+    if (is_aborted_) {
+      std::move(abort_callback_).Run();
       return;
+    }
     if (result != CrostiniResult::SUCCESS) {
       LOG(ERROR) << "Failed to Create Lxd Container.";
       FinishRestart(result);
@@ -301,8 +318,10 @@ class CrostiniManager::CrostiniRestarter
     for (auto& observer : observer_list_) {
       observer.OnContainerStarted(result);
     }
-    if (is_aborted_)
+    if (is_aborted_) {
+      std::move(abort_callback_).Run();
       return;
+    }
     if (result != CrostiniResult::SUCCESS) {
       LOG(ERROR) << "Failed to Start Lxd Container.";
       FinishRestart(result);
@@ -320,8 +339,10 @@ class CrostiniManager::CrostiniRestarter
     for (auto& observer : observer_list_) {
       observer.OnContainerSetup(result);
     }
-    if (is_aborted_)
+    if (is_aborted_) {
+      std::move(abort_callback_).Run();
       return;
+    }
     if (result != CrostiniResult::SUCCESS) {
       LOG(ERROR) << "Failed to set up Lxd Container user.";
       FinishRestart(result);
@@ -353,8 +374,10 @@ class CrostiniManager::CrostiniRestarter
     for (auto& observer : observer_list_) {
       observer.OnSshKeysFetched(result);
     }
-    if (is_aborted_)
+    if (is_aborted_) {
+      std::move(abort_callback_).Run();
       return;
+    }
     if (result != crostini::CrostiniResult::SUCCESS) {
       LOG(ERROR) << "Failed to get ssh keys.";
       FinishRestart(result);
@@ -413,8 +436,10 @@ class CrostiniManager::CrostiniRestarter
 
     // Abort not checked until end of function.  On abort, do not continue,
     // but still remove observer and add volume as per above.
-    if (is_aborted_)
+    if (is_aborted_) {
+      std::move(abort_callback_).Run();
       return;
+    }
 
     FinishRestart(CrostiniResult::SUCCESS);
   }
@@ -428,6 +453,7 @@ class CrostiniManager::CrostiniRestarter
   std::string container_name_;
   std::string source_path_;
   CrostiniManager::RestartCrostiniCallback callback_;
+  CrostiniManager::AbortRestartCallback abort_callback_;
   base::ObserverList<CrostiniManager::RestartObserver>::Unchecked
       observer_list_;
   CrostiniManager::RestartId restart_id_;
@@ -1465,6 +1491,15 @@ CrostiniManager::RestartId CrostiniManager::RestartCrostini(
     RestartCrostiniCallback callback,
     RestartObserver* observer) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  // Currently, |remove_crostini_callbacks_| is only used just before running
+  // CrostiniRemover. If that changes, then we should check for a currently
+  // running uninstaller in some other way.
+  if (!remove_crostini_callbacks_.empty()) {
+    LOG(ERROR)
+        << "Tried to install crostini while crostini uninstaller is running";
+    std::move(callback).Run(CrostiniResult::CROSTINI_UNINSTALLER_RUNNING);
+    return kUninitializedRestartId;
+  }
 
   auto restarter = base::MakeRefCounted<CrostiniRestarter>(
       profile_, weak_ptr_factory_.GetWeakPtr(), std::move(vm_name),
@@ -1484,7 +1519,8 @@ CrostiniManager::RestartId CrostiniManager::RestartCrostini(
 }
 
 void CrostiniManager::AbortRestartCrostini(
-    CrostiniManager::RestartId restart_id) {
+    CrostiniManager::RestartId restart_id,
+    AbortRestartCallback callback) {
   auto restarter_it = restarters_by_id_.find(restart_id);
   if (restarter_it == restarters_by_id_.end()) {
     // This can happen if a user cancels the install flow at the exact right
@@ -1492,32 +1528,43 @@ void CrostiniManager::AbortRestartCrostini(
     LOG(ERROR) << "Aborting a restarter that already finished";
     return;
   }
-  restarter_it->second->Abort();
+  restarter_it->second->Abort(base::BindOnce(
+      &CrostiniManager::OnAbortRestartCrostini, weak_ptr_factory_.GetWeakPtr(),
+      restart_id, std::move(callback)));
+}
 
+void CrostiniManager::OnAbortRestartCrostini(
+    CrostiniManager::RestartId restart_id,
+    AbortRestartCallback callback) {
+  auto restarter_it = restarters_by_id_.find(restart_id);
   auto key = std::make_pair(restarter_it->second->vm_name(),
                             restarter_it->second->container_name());
-  auto range = restarters_by_container_.equal_range(key);
-  for (auto it = range.first; it != range.second; ++it) {
-    if (it->second == restart_id) {
-      restarters_by_container_.erase(it);
-      break;
+  if (restarter_it != restarters_by_id_.end()) {
+    auto range = restarters_by_container_.equal_range(key);
+    for (auto it = range.first; it != range.second; ++it) {
+      if (it->second == restart_id) {
+        restarters_by_container_.erase(it);
+        break;
+      }
     }
+    // This invalidates the iterator and potentially destroys the restarter, so
+    // those shouldn't be accessed after this.
+    restarters_by_id_.erase(restarter_it);
   }
 
-  // This invalidates the iterator and potentially destroys the restarter, so
-  // those shouldn't be accessed after this.
-  restarters_by_id_.erase(restarter_it);
-
   // Kick off the "next" (in no order) pending Restart() if any.
   auto pending_it = restarters_by_container_.find(key);
   if (pending_it != restarters_by_container_.end()) {
     auto restarter = restarters_by_id_[pending_it->second];
     restarter->Restart();
   }
+
+  std::move(callback).Run();
 }
 
 bool CrostiniManager::IsRestartPending(RestartId restart_id) {
-  return restarters_by_id_.find(restart_id) != restarters_by_id_.end();
+  auto it = restarters_by_id_.find(restart_id);
+  return it != restarters_by_id_.end() && !it->second->is_aborted();
 }
 
 void CrostiniManager::AddShutdownContainerCallback(
@@ -2195,11 +2242,19 @@ void CrostiniManager::OnGetContainerSshKeys(
 void CrostiniManager::RemoveCrostini(std::string vm_name,
                                      RemoveCrostiniCallback callback) {
   AddRemoveCrostiniCallback(std::move(callback));
+
   auto crostini_remover = base::MakeRefCounted<CrostiniRemover>(
       profile_, std::move(vm_name),
       base::BindOnce(&CrostiniManager::OnRemoveCrostini,
                      weak_ptr_factory_.GetWeakPtr()));
-  crostini_remover->RemoveCrostini();
+
+  auto abort_callback = base::BarrierClosure(
+      restarters_by_id_.size(),
+      base::BindOnce(&CrostiniRemover::RemoveCrostini, crostini_remover));
+
+  for (auto restarter_it : restarters_by_id_) {
+    AbortRestartCrostini(restarter_it.first, abort_callback);
+  }
 }
 
 void CrostiniManager::OnRemoveCrostini(CrostiniResult result) {

chrome/browser/chromeos/crostini/crostini_manager.h

@@ -60,6 +60,7 @@ enum class CrostiniResult {
   ATTACH_USB_FAILED,
   DETACH_USB_FAILED,
   LIST_USB_FAILED,
+  CROSTINI_UNINSTALLER_RUNNING,
   UNKNOWN_USB_DEVICE,
   UNKNOWN_ERROR,
 };
@@ -218,6 +219,8 @@ class CrostiniManager : public KeyedService,
   using SearchAppCallback =
       base::OnceCallback<void(const std::vector<std::string>& package_names)>;
 
+  using AbortRestartCallback = base::OnceCallback<void()>;
+
   // Observer class for the Crostini restart flow.
   class RestartObserver {
    public:
@@ -454,8 +457,10 @@ class CrostiniManager : public KeyedService,
                             RestartObserver* observer = nullptr);
 
   // Aborts a restart. A "next" restarter with the same <vm_name,
-  // container_name> will run, if there is one.
-  void AbortRestartCrostini(RestartId restart_id);
+  // container_name> will run, if there is one. |callback| will be called once
+  // the restart has finished aborting
+  void AbortRestartCrostini(RestartId restart_id,
+                            AbortRestartCallback callback);
 
   // Returns true if the Restart corresponding to |restart_id| is not yet
   // complete.
@@ -700,6 +705,10 @@ class CrostiniManager : public KeyedService,
 
   void FinishRestart(CrostiniRestarter* restarter, CrostiniResult result);
 
+  // Callback for CrostiniManager::AbortRestartCrostini
+  void OnAbortRestartCrostini(RestartId restart_id,
+                              AbortRestartCallback callback);
+
   // Callback for CrostiniManager::RemoveCrostini.
   void OnRemoveCrostini(CrostiniResult result);
 

chrome/browser/chromeos/crostini/crostini_manager_unittest.cc

@@ -675,6 +675,12 @@ class CrostiniManagerRestartTest : public CrostiniManagerTest,
     std::move(closure).Run();
   }
 
+  void RemoveCrostiniCallback(base::OnceClosure closure,
+                              CrostiniResult result) {
+    remove_crostini_callback_count_++;
+    std::move(closure).Run();
+  }
+
   // CrostiniManager::RestartObserver
   void OnComponentLoaded(CrostiniResult result) override {
     if (abort_on_component_loaded_) {
@@ -730,7 +736,8 @@ class CrostiniManagerRestartTest : public CrostiniManagerTest,
 
  protected:
   void Abort() {
-    crostini_manager()->AbortRestartCrostini(restart_id_);
+    crostini_manager()->AbortRestartCrostini(restart_id_,
+                                             base::DoNothing::Once());
     run_loop()->Quit();
   }
 
@@ -751,6 +758,8 @@ class CrostiniManagerRestartTest : public CrostiniManagerTest,
 
   CrostiniManager::RestartId restart_id_ =
       CrostiniManager::kUninitializedRestartId;
+  const CrostiniManager::RestartId uninitialized_id_ =
+      CrostiniManager::kUninitializedRestartId;
   bool abort_on_component_loaded_ = false;
   bool abort_on_concierge_started_ = false;
   bool abort_on_disk_image_created_ = false;
@@ -760,6 +769,7 @@ class CrostiniManagerRestartTest : public CrostiniManagerTest,
   bool abort_on_container_setup_ = false;
   bool abort_on_ssh_keys_fetched_ = false;
   int restart_crostini_callback_count_ = 0;
+  int remove_crostini_callback_count_ = 0;
   chromeos::disks::MockDiskMountManager* disk_mount_manager_mock_;
 };
 
@@ -1154,4 +1164,96 @@ TEST_F(CrostiniManagerTest, GetLinuxPackageInfoFromAptSuccess) {
   run_loop()->Run();
 }
 
+TEST_F(CrostiniManagerRestartTest, RestartThenUninstall) {
+  restart_id_ = crostini_manager()->RestartCrostini(
+      kVmName, kContainerName,
+      base::BindOnce(&CrostiniManagerRestartTest::RestartCrostiniCallback,
+                     base::Unretained(this), base::DoNothing::Once()));
+
+  EXPECT_TRUE(crostini_manager()->IsRestartPending(restart_id_));
+
+  crostini_manager()->RemoveCrostini(
+      kVmName,
+      base::BindOnce(&CrostiniManagerRestartTest::RemoveCrostiniCallback,
+                     base::Unretained(this), run_loop()->QuitClosure()));
+
+  EXPECT_FALSE(crostini_manager()->IsRestartPending(restart_id_));
+
+  run_loop()->Run();
+
+  // Aborts don't call the restart callback. If that changes, everything that
+  // calls RestartCrostini will need to be checked to make sure they handle it
+  // in a sensible way.
+  EXPECT_EQ(0, restart_crostini_callback_count_);
+  EXPECT_EQ(1, remove_crostini_callback_count_);
+}
+
+TEST_F(CrostiniManagerRestartTest, RestartMultipleThenUninstall) {
+  CrostiniManager::RestartId id1, id2, id3;
+  id1 = crostini_manager()->RestartCrostini(
+      kVmName, kContainerName,
+      base::BindOnce(&CrostiniManagerRestartTest::RestartCrostiniCallback,
+                     base::Unretained(this), base::DoNothing::Once()));
+  id2 = crostini_manager()->RestartCrostini(
+      kVmName, kContainerName,
+      base::BindOnce(&CrostiniManagerRestartTest::RestartCrostiniCallback,
+                     base::Unretained(this), base::DoNothing::Once()));
+  id3 = crostini_manager()->RestartCrostini(
+      kVmName, kContainerName,
+      base::BindOnce(&CrostiniManagerRestartTest::RestartCrostiniCallback,
+                     base::Unretained(this), base::DoNothing::Once()));
+
+  EXPECT_TRUE(crostini_manager()->IsRestartPending(id1));
+  EXPECT_TRUE(crostini_manager()->IsRestartPending(id2));
+  EXPECT_TRUE(crostini_manager()->IsRestartPending(id3));
+
+  crostini_manager()->RemoveCrostini(
+      kVmName,
+      base::BindOnce(&CrostiniManagerRestartTest::RemoveCrostiniCallback,
+                     base::Unretained(this), run_loop()->QuitClosure()));
+
+  EXPECT_FALSE(crostini_manager()->IsRestartPending(id1));
+  EXPECT_FALSE(crostini_manager()->IsRestartPending(id2));
+  EXPECT_FALSE(crostini_manager()->IsRestartPending(id3));
+
+  run_loop()->Run();
+
+  // Aborts don't call the restart callback. If that changes, everything that
+  // calls RestartCrostini will need to be checked to make sure they handle it
+  // in a sensible way.
+  EXPECT_EQ(0, restart_crostini_callback_count_);
+  EXPECT_EQ(1, remove_crostini_callback_count_);
+}
+
+TEST_F(CrostiniManagerRestartTest, UninstallThenRestart) {
+  // Install crostini first so that the uninstaller doesn't terminate before we
+  // can call the installer again
+  restart_id_ = crostini_manager()->RestartCrostini(
+      kVmName, kContainerName,
+      base::BindOnce(&CrostiniManagerRestartTest::RestartCrostiniCallback,
+                     base::Unretained(this), run_loop()->QuitClosure()));
+
+  EXPECT_TRUE(crostini_manager()->IsRestartPending(restart_id_));
+
+  run_loop()->Run();
+
+  base::RunLoop run_loop2;
+  crostini_manager()->RemoveCrostini(
+      kVmName,
+      base::BindOnce(&CrostiniManagerRestartTest::RemoveCrostiniCallback,
+                     base::Unretained(this), run_loop2.QuitClosure()));
+
+  restart_id_ = crostini_manager()->RestartCrostini(
+      kVmName, kContainerName,
+      base::BindOnce(&CrostiniManagerRestartTest::RestartCrostiniCallback,
+                     base::Unretained(this), base::DoNothing::Once()));
+
+  EXPECT_EQ(uninitialized_id_, restart_id_);
+
+  run_loop2.Run();
+
+  EXPECT_EQ(2, restart_crostini_callback_count_);
+  EXPECT_EQ(1, remove_crostini_callback_count_);
+}
+
 }  // namespace crostini

chrome/browser/ui/views/crostini/crostini_installer_view.cc

@@ -193,7 +193,7 @@ bool CrostiniInstallerView::Cancel() {
     // Abort the long-running flow, and prevent our RestartObserver methods
     // being called after "this" has been destroyed.
     crostini::CrostiniManager::GetForProfile(profile_)->AbortRestartCrostini(
-        restart_id_);
+        restart_id_, base::DoNothing());
 
     SetupResult result = SetupResult::kUserCancelledStart;
     result = static_cast<SetupResult>(static_cast<int>(result) +