Discard invalid iterators in StartQueuedOperation that could cause crashes

Bug: 1015341
Change-Id: I711702f6e7ba4f5881bc227ab04f695e494c47fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1868771
Auto-Submit: Fergus Dall <sidereal@google.com>
Commit-Queue: Nicholas Verne <nverne@chromium.org>
Reviewed-by: Nicholas Verne <nverne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707705}

chrome/browser/chromeos/crostini/crostini_package_service.cc

@@ -554,10 +554,14 @@ void CrostiniPackageService::StartQueuedOperation(
                                    PackageOperationStatus::SUCCEEDED, 100);
     }
 
-    // Clean up memory.
-    if (uninstall_queue.empty()) {
+    // As recursive calls to StartQueuedOperation might delete |uninstall_queue|
+    // and invalidate |uninstall_queue_iter| we must look it up again.
+    uninstall_queue_iter = queued_uninstalls_.find(container_id);
+    if (uninstall_queue_iter != queued_uninstalls_.end() &&
+        uninstall_queue_iter->second.empty()) {
+      // Clean up memory.
       queued_uninstalls_.erase(uninstall_queue_iter);
-      // Invalidates uninstall_queue.
+      // Invalidates |uninstall_queue_iter|.
     }
     return;
   }
@@ -590,10 +594,15 @@ void CrostiniPackageService::StartQueuedOperation(
                        weak_ptr_factory_.GetWeakPtr(), vm_name, container_name,
                        std::move(callback)));
 
-    // Clean up memory.
-    if (install_queue.empty()) {
+    // InstallLinuxPackage shouldn't be able to recursively call this method,
+    // but as future proofing consider |install_queue_iter| to be invalidated
+    // anyway.
+    install_queue_iter = queued_installs_.find(container_id);
+    if (install_queue_iter != queued_installs_.end() &&
+        install_queue_iter->second.empty()) {
+      // Clean up memory.
       queued_installs_.erase(install_queue_iter);
-      // Invalidates install_queue.
+      // Invalidates |install_queue_iter|.
     }
     return;
   }

chrome/browser/chromeos/crostini/crostini_package_service_unittest.cc

@@ -904,6 +904,8 @@ TEST_F(CrostiniPackageServiceTest, SecondUninstallStartsWhenFirstFails) {
 }
 
 TEST_F(CrostiniPackageServiceTest, DuplicateUninstallSucceeds) {
+  // Use three uninstalls as a regression test for crbug.com/1015341
+  service_->QueueUninstallApplication(kDefaultAppId);
   service_->QueueUninstallApplication(kDefaultAppId);
   service_->QueueUninstallApplication(kDefaultAppId);
 
@@ -921,6 +923,7 @@ TEST_F(CrostiniPackageServiceTest, DuplicateUninstallSucceeds) {
       Printable(notification_display_service_->GetDisplayedNotificationsForType(
           NotificationHandler::Type::TRANSIENT)),
       UnorderedElementsAre(IsUninstallSuccessNotification(DEFAULT_APP),
+                           IsUninstallSuccessNotification(DEFAULT_APP),
                            IsUninstallSuccessNotification(DEFAULT_APP)));
 }