Check for aborted in CrostiniManager OnVmShutdown

Refactor ReturnEarlyIfAborted function to only call
abort_callback_ if it is not already called since
it is possible that OnVmShutdown could call it,
and then another delayed callback could also call it.

Bug: 1013059
Change-Id: I33108b158ccc60e405c5ecf09d4cec3d65b3edec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1880242Reviewed-by: Nicholas Verne <nverne@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710142}

chrome/browser/chromeos/crostini/crostini_manager.cc

@@ -139,8 +139,7 @@ class CrostiniManager::CrostiniRestarter
       std::move(completed_callback_).Run(CrostiniResult::NOT_ALLOWED);
       return;
     }
-    if (is_aborted_) {
-      std::move(abort_callback_).Run();
+    if (ReturnEarlyIfAborted()) {
       return;
     }
     is_running_ = true;
@@ -170,6 +169,9 @@ class CrostiniManager::CrostiniRestarter
 
   // crostini::VmShutdownObserver
   void OnVmShutdown(const std::string& vm_name) override {
+    if (ReturnEarlyIfAborted()) {
+      return;
+    }
     if (vm_name == vm_name_) {
       LOG(WARNING) << "Unexpected VM shutdown during restart for " << vm_name;
       FinishRestart(CrostiniResult::RESTART_FAILED_VM_STOPPED);
@@ -184,6 +186,13 @@ class CrostiniManager::CrostiniRestarter
     ReportRestarterResult(CrostiniResult::RESTART_ABORTED);
   }
 
+  bool ReturnEarlyIfAborted() {
+    if (is_aborted_ && abort_callback_) {
+      std::move(abort_callback_).Run();
+    }
+    return is_aborted_;
+  }
+
   void OnContainerDownloading(int download_percent) {
     if (!is_running_) {
       return;
@@ -237,8 +246,7 @@ class CrostiniManager::CrostiniRestarter
     for (auto& observer : observer_list_) {
       observer.OnComponentLoaded(result);
     }
-    if (is_aborted_) {
-      std::move(abort_callback_).Run();
+    if (ReturnEarlyIfAborted()) {
       return;
     }
     if (result != CrostiniResult::SUCCESS) {
@@ -257,8 +265,7 @@ class CrostiniManager::CrostiniRestarter
     for (auto& observer : observer_list_) {
       observer.OnConciergeStarted(is_started);
     }
-    if (is_aborted_) {
-      std::move(abort_callback_).Run();
+    if (ReturnEarlyIfAborted()) {
       return;
     }
     if (!is_started) {
@@ -287,8 +294,7 @@ class CrostiniManager::CrostiniRestarter
     for (auto& observer : observer_list_) {
       observer.OnDiskImageCreated(success, status, disk_size_available);
     }
-    if (is_aborted_) {
-      std::move(abort_callback_).Run();
+    if (ReturnEarlyIfAborted()) {
       return;
     }
     if (!success) {
@@ -306,8 +312,7 @@ class CrostiniManager::CrostiniRestarter
     for (auto& observer : observer_list_) {
       observer.OnVmStarted(success);
     }
-    if (is_aborted_) {
-      std::move(abort_callback_).Run();
+    if (ReturnEarlyIfAborted()) {
       return;
     }
     if (!success) {
@@ -348,8 +353,7 @@ class CrostiniManager::CrostiniRestarter
     for (auto& observer : observer_list_) {
       observer.OnContainerCreated(result);
     }
-    if (is_aborted_) {
-      std::move(abort_callback_).Run();
+    if (ReturnEarlyIfAborted()) {
       return;
     }
     if (result != CrostiniResult::SUCCESS) {
@@ -370,8 +374,7 @@ class CrostiniManager::CrostiniRestarter
     for (auto& observer : observer_list_) {
       observer.OnContainerSetup(success);
     }
-    if (is_aborted_) {
-      std::move(abort_callback_).Run();
+    if (ReturnEarlyIfAborted()) {
       return;
     }
     if (!success) {
@@ -392,8 +395,7 @@ class CrostiniManager::CrostiniRestarter
     for (auto& observer : observer_list_) {
       observer.OnContainerStarted(result);
     }
-    if (is_aborted_) {
-      std::move(abort_callback_).Run();
+    if (ReturnEarlyIfAborted()) {
       return;
     }
     if (result != CrostiniResult::SUCCESS) {
@@ -435,8 +437,7 @@ class CrostiniManager::CrostiniRestarter
     for (auto& observer : observer_list_) {
       observer.OnSshKeysFetched(success);
     }
-    if (is_aborted_) {
-      std::move(abort_callback_).Run();
+    if (ReturnEarlyIfAborted()) {
       return;
     }
     if (!success) {
@@ -485,8 +486,7 @@ class CrostiniManager::CrostiniRestarter
                  << ", mount_path=" << mount_info.mount_path
                  << ", mount_type=" << mount_info.mount_type
                  << ", mount_condition=" << mount_info.mount_condition;
-      if (is_aborted_) {
-        std::move(abort_callback_).Run();
+      if (ReturnEarlyIfAborted()) {
         return;
       }
       FinishRestart(CrostiniResult::SSHFS_MOUNT_ERROR);
@@ -509,8 +509,7 @@ class CrostiniManager::CrostiniRestarter
 
     // Abort not checked until exiting this function.  On abort, do not
     // continue, but still remove observer and add volume as per above.
-    if (is_aborted_) {
-      std::move(abort_callback_).Run();
+    if (ReturnEarlyIfAborted()) {
       return;
     }
 

chrome/browser/chromeos/crostini/crostini_manager_unittest.cc

@@ -669,6 +669,13 @@ class CrostiniManagerRestartTest : public CrostiniManagerTest,
     if (abort_on_container_created_) {
       Abort();
     }
+    if (abort_then_stop_vm_) {
+      Abort();
+      vm_tools::concierge::VmStoppedSignal signal;
+      signal.set_owner_id(CryptohomeIdForProfile(profile()));
+      signal.set_name(kVmName);
+      crostini_manager()->OnVmStopped(signal);
+    }
   }
 
   void OnContainerStarted(CrostiniResult result) override {
@@ -732,6 +739,7 @@ class CrostiniManagerRestartTest : public CrostiniManagerTest,
   bool abort_on_container_setup_ = false;
   bool abort_on_ssh_keys_fetched_ = false;
   bool abort_on_container_mounted_ = false;
+  bool abort_then_stop_vm_ = false;
 
   // Used by SshfsMount().
   bool abort_on_mount_event_ = false;
@@ -971,6 +979,20 @@ TEST_F(CrostiniManagerRestartTest, AbortOnMountEventWithError) {
   chromeos::disks::DiskMountManager::Shutdown();
 }
 
+TEST_F(CrostiniManagerRestartTest, AbortThenStopVm) {
+  abort_then_stop_vm_ = true;
+  restart_id_ = crostini_manager()->RestartCrostini(
+      kVmName, kContainerName,
+      base::BindOnce(&CrostiniManagerRestartTest::RestartCrostiniCallback,
+                     base::Unretained(this), run_loop()->QuitClosure()),
+      this);
+  run_loop()->Run();
+  EXPECT_TRUE(fake_concierge_client_->create_disk_image_called());
+  EXPECT_TRUE(fake_concierge_client_->start_termina_vm_called());
+  EXPECT_FALSE(fake_concierge_client_->get_container_ssh_keys_called());
+  EXPECT_EQ(0, restart_crostini_callback_count_);
+}
+
 TEST_F(CrostiniManagerRestartTest, OnlyMountTerminaPenguin) {
   // Use names other than termina/penguin.  Will not mount sshfs.
   restart_id_ = crostini_manager()->RestartCrostini(