crostini: fix concurrent export import notifications bug

BUG=chromium:984442

Cq-Depend: chromium:1702346
Change-Id: Ifb96c0a69d5b1612445ae0f02e866bf466bf4643
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1703688
Commit-Queue: Julian Watson <juwa@google.com>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Auto-Submit: Julian Watson <juwa@google.com>
Cr-Commit-Position: refs/heads/master@{#679882}

chrome/app/chromeos_strings.grdp

@@ -3727,7 +3727,7 @@
     Backup couldn't be completed due to an error
   </message>
   <message name="IDS_CROSTINI_EXPORT_NOTIFICATION_MESSAGE_FAILED_IN_PROGRESS" desc="Message displayed in the notification when export (backup) of the Linux container fails due to another being in progress.">
-    Backup currently in progress for <ph name="CONTAINER_ID">$1<ex>(termina, penguin)</ex></ph>
+    Linux backup currently in progress
   </message>
   <message name="IDS_CROSTINI_IMPORT_NOTIFICATION_TITLE_RUNNING" desc="Title displayed in the notification while import (restore) of the Linux container is in progress.">
     Restoring Linux apps &amp; files
@@ -3745,7 +3745,7 @@
     Restoring couldn't be completed due to an error
   </message>
   <message name="IDS_CROSTINI_IMPORT_NOTIFICATION_MESSAGE_FAILED_IN_PROGRESS" desc="Message displayed in the notification when import (restore) of the Linux container fails due to another being in progress.">
-    Restore currently in progress for <ph name="CONTAINER_ID">$1<ex>(termina, penguin)</ex></ph>
+    Linux restore currently in progress
   </message>
   <message name="IDS_CROSTINI_IMPORT_NOTIFICATION_MESSAGE_FAILED_ARCHITECTURE" desc="Message displayed in the notification when import (restore) of the Linux container fails due to architecture mismatch.">
     Cannot import container architecture type <ph name="ARCHITECTURE_CONTAINER">$1<ex>arm64</ex></ph> with this device which is <ph name="ARCHITECTURE_DEVICE">$2<ex>x86_64</ex></ph>. You can try restoring this container into a different device, or you can access the files inside this container image by opening in Files app.

chrome/app/chromeos_strings_grdp/IDS_CROSTINI_EXPORT_NOTIFICATION_MESSAGE_FAILED_IN_PROGRESS.png.sha1

-3519b1b96658bf54a93cb2df6d8b5d711522c9f7
+1aef0ca7aca195b84ea1054fb9f39888b6080324
\ No newline at end of file

chrome/app/chromeos_strings_grdp/IDS_CROSTINI_IMPORT_NOTIFICATION_MESSAGE_FAILED_IN_PROGRESS.png.sha1

-3519b1b96658bf54a93cb2df6d8b5d711522c9f7
+d161e914ad82ea787e184e89ba0e5f3b095ba32c
\ No newline at end of file

chrome/browser/chromeos/crostini/crostini_export_import.cc

@@ -160,9 +160,18 @@ void CrostiniExportImport::Start(
     ContainerId container_id,
     base::FilePath path,
     CrostiniManager::CrostiniResultCallback callback) {
-  notifications_[container_id] =
-      std::make_unique<CrostiniExportImportNotification>(
-          profile_, this, type, GetUniqueNotificationId(), path);
+  auto* notification = CrostiniExportImportNotification::Create(
+      profile_, type, GetUniqueNotificationId(), path);
+
+  auto it = notifications_.find(container_id);
+  if (it != notifications_.end()) {
+    // An operation is in progress for this container, display an error to
+    // the user and exit.
+    notification->SetStatusFailedConcurrentOperation(it->second->type());
+    return;
+  } else {
+    notifications_.emplace_hint(it, container_id, notification);
+  }
 
   switch (type) {
     case ExportImportType::EXPORT:
@@ -196,7 +205,7 @@ void CrostiniExportImport::ExportAfterSharing(
     auto it = notifications_.find(container_id);
     DCHECK(it != notifications_.end()) << ContainerIdToString(container_id)
                                        << " has no notification to update";
-    it->second->SetStatusFailed();
+    RemoveNotification(it).SetStatusFailed();
     return;
   }
   CrostiniManager::GetForProfile(profile_)->ExportLxdContainer(
@@ -218,11 +227,11 @@ void CrostiniExportImport::OnExportComplete(
 
   ExportContainerResult enum_hist_result = ExportContainerResult::kSuccess;
   if (result == CrostiniResult::SUCCESS) {
-    switch (it->second->get_status()) {
+    switch (it->second->status()) {
       case CrostiniExportImportNotification::Status::RUNNING:
         UMA_HISTOGRAM_LONG_TIMES("Crostini.BackupTimeSuccess",
                                  base::Time::Now() - start);
-        it->second->SetStatusDone();
+        RemoveNotification(it).SetStatusDone();
         break;
       default:
         NOTREACHED();
@@ -242,9 +251,9 @@ void CrostiniExportImport::OnExportComplete(
     }
     UMA_HISTOGRAM_LONG_TIMES("Crostini.BackupTimeFailed",
                              base::Time::Now() - start);
-    DCHECK(it->second->get_status() ==
+    DCHECK(it->second->status() ==
            CrostiniExportImportNotification::Status::RUNNING);
-    it->second->SetStatusFailed();
+    RemoveNotification(it).SetStatusFailed();
   }
   UMA_HISTOGRAM_ENUMERATION("Crostini.Backup", enum_hist_result);
   std::move(callback).Run(result);
@@ -306,7 +315,7 @@ void CrostiniExportImport::ImportAfterSharing(
     auto it = notifications_.find(container_id);
     DCHECK(it != notifications_.end()) << ContainerIdToString(container_id)
                                        << " has no notification to update";
-    it->second->SetStatusFailed();
+    RemoveNotification(it).SetStatusFailed();
     return;
   }
   CrostiniManager::GetForProfile(profile_)->ImportLxdContainer(
@@ -322,16 +331,16 @@ void CrostiniExportImport::OnImportComplete(
     CrostiniManager::CrostiniResultCallback callback,
     CrostiniResult result) {
   auto it = notifications_.find(container_id);
-  DCHECK(it != notifications_.end())
-      << ContainerIdToString(container_id) << " has no notification to update";
 
   ImportContainerResult enum_hist_result = ImportContainerResult::kSuccess;
   if (result == CrostiniResult::SUCCESS) {
     UMA_HISTOGRAM_LONG_TIMES("Crostini.RestoreTimeSuccess",
                              base::Time::Now() - start);
-    switch (it->second->get_status()) {
+    DCHECK(it != notifications_.end()) << ContainerIdToString(container_id)
+                                       << " has no notification to update";
+    switch (it->second->status()) {
       case CrostiniExportImportNotification::Status::RUNNING:
-        it->second->SetStatusDone();
+        RemoveNotification(it).SetStatusDone();
         break;
       default:
         NOTREACHED();
@@ -361,12 +370,14 @@ void CrostiniExportImport::OnImportComplete(
     if (result == CrostiniResult::CONTAINER_EXPORT_IMPORT_FAILED ||
         result == CrostiniResult::CONTAINER_EXPORT_IMPORT_FAILED_VM_STOPPED ||
         result == CrostiniResult::CONTAINER_EXPORT_IMPORT_FAILED_VM_STARTED) {
-      DCHECK(it->second->get_status() ==
+      DCHECK(it != notifications_.end()) << ContainerIdToString(container_id)
+                                         << " has no notification to update";
+      DCHECK(it->second->status() ==
              CrostiniExportImportNotification::Status::RUNNING);
-      it->second->SetStatusFailed();
+      RemoveNotification(it).SetStatusFailed();
     } else {
-      DCHECK(it->second->get_status() ==
-             CrostiniExportImportNotification::Status::FAILED);
+      DCHECK(it == notifications_.end()) << ContainerIdToString(container_id)
+                                         << " has unexpected notification";
     }
     UMA_HISTOGRAM_LONG_TIMES("Crostini.RestoreTimeFailed",
                              base::Time::Now() - start);
@@ -404,13 +415,13 @@ void CrostiniExportImport::OnImportContainerProgress(
       break;
     // Failure, set error message.
     case ImportContainerProgressStatus::FAILURE_ARCHITECTURE:
-      it->second->SetStatusFailedArchitectureMismatch(architecture_container,
-                                                      architecture_device);
+      RemoveNotification(it).SetStatusFailedArchitectureMismatch(
+          architecture_container, architecture_device);
       break;
     case ImportContainerProgressStatus::FAILURE_SPACE:
       DCHECK_GE(minimum_required_space, available_space);
-      it->second->SetStatusFailedInsufficientSpace(minimum_required_space -
-                                                   available_space);
+      RemoveNotification(it).SetStatusFailedInsufficientSpace(
+          minimum_required_space - available_space);
       break;
     default:
       LOG(WARNING) << "Unknown Export progress status " << int(status);
@@ -422,22 +433,18 @@ std::string CrostiniExportImport::GetUniqueNotificationId() {
                             next_notification_id_++);
 }
 
-void CrostiniExportImport::NotificationCompleted(
-    const CrostiniExportImportNotification& notification) {
-  for (auto it = notifications_.begin(); it != notifications_.end(); ++it) {
-    if (it->second.get() == &notification) {
+CrostiniExportImportNotification& CrostiniExportImport::RemoveNotification(
+    std::map<ContainerId, CrostiniExportImportNotification*>::iterator it) {
+  DCHECK(it != notifications_.end());
+  auto& notification = *it->second;
   notifications_.erase(it);
-      return;
-    }
-  }
-  // Notification should always exist when this is called.
-  NOTREACHED();
+  return notification;
 }
 
 CrostiniExportImportNotification*
 CrostiniExportImport::GetNotificationForTesting(ContainerId container_id) {
   auto it = notifications_.find(container_id);
-  return it != notifications_.end() ? it->second.get() : nullptr;
+  return it != notifications_.end() ? it->second : nullptr;
 }
 
 }  // namespace crostini

chrome/browser/chromeos/crostini/crostini_export_import.h

@@ -82,10 +82,6 @@ class CrostiniExportImport : public KeyedService,
                        base::FilePath path,
                        CrostiniManager::CrostiniResultCallback callback);
 
-  // Called by the notification when it is closed so it can be destroyed.
-  void NotificationCompleted(
-      const CrostiniExportImportNotification& notification);
-
   CrostiniExportImportNotification* GetNotificationForTesting(
       ContainerId container_id);
 
@@ -160,10 +156,12 @@ class CrostiniExportImport : public KeyedService,
 
   std::string GetUniqueNotificationId();
 
+  CrostiniExportImportNotification& RemoveNotification(
+      std::map<ContainerId, CrostiniExportImportNotification*>::iterator it);
+
   Profile* profile_;
   scoped_refptr<ui::SelectFileDialog> select_folder_dialog_;
-  std::map<ContainerId, std::unique_ptr<CrostiniExportImportNotification>>
-      notifications_;
+  std::map<ContainerId, CrostiniExportImportNotification*> notifications_;
   // Notifications must have unique-per-profile identifiers.
   // A non-static member on a profile-keyed-service will suffice.
   int next_notification_id_;

chrome/browser/chromeos/crostini/crostini_export_import_notification.cc

@@ -29,12 +29,10 @@ constexpr char kNotifierCrostiniExportImportOperation[] =
 
 CrostiniExportImportNotification::CrostiniExportImportNotification(
     Profile* profile,
-    CrostiniExportImport* service,
     ExportImportType type,
     const std::string& notification_id,
     const base::FilePath& path)
     : profile_(profile),
-      service_(service),
       type_(type),
       path_(path),
       weak_ptr_factory_(this) {
@@ -136,6 +134,14 @@ void CrostiniExportImportNotification::SetStatusFailedInsufficientSpace(
       ui::FormatBytes(additional_required_space)));
 }
 
+void CrostiniExportImportNotification::SetStatusFailedConcurrentOperation(
+    ExportImportType in_progress_operation_type) {
+  SetStatusFailed(l10n_util::GetStringUTF16(
+      in_progress_operation_type == ExportImportType::EXPORT
+          ? IDS_CROSTINI_EXPORT_NOTIFICATION_MESSAGE_FAILED_IN_PROGRESS
+          : IDS_CROSTINI_IMPORT_NOTIFICATION_MESSAGE_FAILED_IN_PROGRESS));
+}
+
 void CrostiniExportImportNotification::SetStatusFailed(
     const base::string16& message) {
   status_ = Status::FAILED;
@@ -158,7 +164,7 @@ void CrostiniExportImportNotification::SetStatusFailed(
 void CrostiniExportImportNotification::Close(bool by_user) {
   closed_ = true;
   if (status_ != Status::RUNNING) {
-    service_->NotificationCompleted(*this);
+    delete this;
   }
 }
 

chrome/browser/chromeos/crostini/crostini_export_import_notification.h

@@ -11,6 +11,7 @@
 #include "base/files/file_path.h"
 #include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
+#include "chrome/browser/chromeos/crostini/crostini_util.h"
 #include "ui/message_center/public/cpp/notification_delegate.h"
 
 class Profile;
@@ -21,8 +22,6 @@ class Notification;
 
 namespace crostini {
 
-class CrostiniExportImport;
-
 enum class ExportImportType;
 
 // Notification for Crostini export and import.
@@ -31,11 +30,17 @@ class CrostiniExportImportNotification
  public:
   enum class Status { RUNNING, DONE, FAILED };
 
-  CrostiniExportImportNotification(Profile* profile,
-                                   CrostiniExportImport* service,
+  // Used to construct CrostiniExportImportNotification to ensure it controls
+  // its lifetime.
+  static CrostiniExportImportNotification* Create(
+      Profile* profile,
       ExportImportType type,
       const std::string& notification_id,
-                                   const base::FilePath& path);
+      const base::FilePath& path) {
+    return new CrostiniExportImportNotification(profile, type, notification_id,
+                                                path);
+  }
+
   virtual ~CrostiniExportImportNotification();
 
   void SetStatusRunning(int progress_percent);
@@ -45,8 +50,11 @@ class CrostiniExportImportNotification
       const std::string& architecture_container,
       const std::string& architecture_device);
   void SetStatusFailedInsufficientSpace(uint64_t additional_required_space);
+  void SetStatusFailedConcurrentOperation(
+      ExportImportType in_progress_operation_type);
 
-  Status get_status() const { return status_; }
+  Status status() const { return status_; }
+  ExportImportType type() const { return type_; }
   // Getters for testing.
   message_center::Notification* get_notification() {
     return notification_.get();
@@ -58,11 +66,14 @@ class CrostiniExportImportNotification
              const base::Optional<base::string16>& reply) override;
 
  private:
+  CrostiniExportImportNotification(Profile* profile,
+                                   ExportImportType type,
+                                   const std::string& notification_id,
+                                   const base::FilePath& path);
+
   void SetStatusFailed(const base::string16& message);
 
   Profile* profile_;
-  // These notifications are owned by the export service.
-  CrostiniExportImport* service_;
   ExportImportType type_;
   base::FilePath path_;
   Status status_ = Status::RUNNING;

chrome/browser/chromeos/crostini/crostini_export_import_unittest.cc

@@ -138,7 +138,7 @@ TEST_F(CrostiniExportImportTest, TestDeprecatedExportSuccess) {
   CrostiniExportImportNotification* notification =
       crostini_export_import()->GetNotificationForTesting(container_id_);
   ASSERT_NE(notification, nullptr);
-  EXPECT_EQ(notification->get_status(),
+  EXPECT_EQ(notification->status(),
             CrostiniExportImportNotification::Status::RUNNING);
   EXPECT_EQ(notification->get_notification()->progress(), 0);
 
@@ -146,7 +146,7 @@ TEST_F(CrostiniExportImportTest, TestDeprecatedExportSuccess) {
   SendExportProgress(vm_tools::cicerone::
                          ExportLxdContainerProgressSignal_Status_EXPORTING_PACK,
                      {.progress_percent = 20});
-  EXPECT_EQ(notification->get_status(),
+  EXPECT_EQ(notification->status(),
             CrostiniExportImportNotification::Status::RUNNING);
   EXPECT_EQ(notification->get_notification()->progress(), 10);
 
@@ -155,7 +155,7 @@ TEST_F(CrostiniExportImportTest, TestDeprecatedExportSuccess) {
       vm_tools::cicerone::
           ExportLxdContainerProgressSignal_Status_EXPORTING_DOWNLOAD,
       {.progress_percent = 20});
-  EXPECT_EQ(notification->get_status(),
+  EXPECT_EQ(notification->status(),
             CrostiniExportImportNotification::Status::RUNNING);
   EXPECT_EQ(notification->get_notification()->progress(), 60);
 
@@ -165,14 +165,14 @@ TEST_F(CrostiniExportImportTest, TestDeprecatedExportSuccess) {
       vm_tools::cicerone::
           ExportLxdContainerProgressSignal_Status_EXPORTING_DOWNLOAD,
       {.progress_percent = 40});
-  EXPECT_EQ(notification->get_status(),
+  EXPECT_EQ(notification->status(),
             CrostiniExportImportNotification::Status::RUNNING);
   EXPECT_EQ(notification->get_notification()->progress(), 60);
 
   // Done.
   SendExportProgress(
       vm_tools::cicerone::ExportLxdContainerProgressSignal_Status_DONE);
-  EXPECT_EQ(notification->get_status(),
+  EXPECT_EQ(notification->status(),
             CrostiniExportImportNotification::Status::DONE);
 }
 
@@ -183,7 +183,7 @@ TEST_F(CrostiniExportImportTest, TestExportSuccess) {
   CrostiniExportImportNotification* notification =
       crostini_export_import()->GetNotificationForTesting(container_id_);
   ASSERT_NE(notification, nullptr);
-  EXPECT_EQ(notification->get_status(),
+  EXPECT_EQ(notification->status(),
             CrostiniExportImportNotification::Status::RUNNING);
   EXPECT_EQ(notification->get_notification()->progress(), 0);
 
@@ -195,7 +195,7 @@ TEST_F(CrostiniExportImportTest, TestExportSuccess) {
        .total_bytes = 100,
        .files_streamed = 30,
        .bytes_streamed = 10});
-  EXPECT_EQ(notification->get_status(),
+  EXPECT_EQ(notification->status(),
             CrostiniExportImportNotification::Status::RUNNING);
   EXPECT_EQ(notification->get_notification()->progress(), 20);
 
@@ -207,7 +207,7 @@ TEST_F(CrostiniExportImportTest, TestExportSuccess) {
        .total_bytes = 100,
        .files_streamed = 55,
        .bytes_streamed = 66});
-  EXPECT_EQ(notification->get_status(),
+  EXPECT_EQ(notification->status(),
             CrostiniExportImportNotification::Status::RUNNING);
   EXPECT_EQ(notification->get_notification()->progress(), 60);
 
@@ -220,14 +220,14 @@ TEST_F(CrostiniExportImportTest, TestExportSuccess) {
        .total_bytes = 100,
        .files_streamed = 90,
        .bytes_streamed = 85});
-  EXPECT_EQ(notification->get_status(),
+  EXPECT_EQ(notification->status(),
             CrostiniExportImportNotification::Status::RUNNING);
   EXPECT_EQ(notification->get_notification()->progress(), 60);
 
   // Done.
   SendExportProgress(
       vm_tools::cicerone::ExportLxdContainerProgressSignal_Status_DONE);
-  EXPECT_EQ(notification->get_status(),
+  EXPECT_EQ(notification->status(),
             CrostiniExportImportNotification::Status::DONE);
 }
 
@@ -241,7 +241,7 @@ TEST_F(CrostiniExportImportTest, TestExportFail) {
   // Failed.
   SendExportProgress(
       vm_tools::cicerone::ExportLxdContainerProgressSignal_Status_FAILED);
-  EXPECT_EQ(notification->get_status(),
+  EXPECT_EQ(notification->status(),
             CrostiniExportImportNotification::Status::FAILED);
 }
 
@@ -252,7 +252,7 @@ TEST_F(CrostiniExportImportTest, TestImportSuccess) {
   CrostiniExportImportNotification* notification =
       crostini_export_import()->GetNotificationForTesting(container_id_);
   ASSERT_NE(notification, nullptr);
-  EXPECT_EQ(notification->get_status(),
+  EXPECT_EQ(notification->status(),
             CrostiniExportImportNotification::Status::RUNNING);
   EXPECT_EQ(notification->get_notification()->progress(), 0);
 
@@ -261,7 +261,7 @@ TEST_F(CrostiniExportImportTest, TestImportSuccess) {
       vm_tools::cicerone::
           ImportLxdContainerProgressSignal_Status_IMPORTING_UPLOAD,
       {.progress_percent = 20});
-  EXPECT_EQ(notification->get_status(),
+  EXPECT_EQ(notification->status(),
             CrostiniExportImportNotification::Status::RUNNING);
   EXPECT_EQ(notification->get_notification()->progress(), 10);
 
@@ -270,7 +270,7 @@ TEST_F(CrostiniExportImportTest, TestImportSuccess) {
       vm_tools::cicerone::
           ImportLxdContainerProgressSignal_Status_IMPORTING_UNPACK,
       {.progress_percent = 20});
-  EXPECT_EQ(notification->get_status(),
+  EXPECT_EQ(notification->status(),
             CrostiniExportImportNotification::Status::RUNNING);
   EXPECT_EQ(notification->get_notification()->progress(), 60);
 
@@ -280,14 +280,14 @@ TEST_F(CrostiniExportImportTest, TestImportSuccess) {
       vm_tools::cicerone::
           ImportLxdContainerProgressSignal_Status_IMPORTING_UNPACK,
       {.progress_percent = 40});
-  EXPECT_EQ(notification->get_status(),
+  EXPECT_EQ(notification->status(),
             CrostiniExportImportNotification::Status::RUNNING);
   EXPECT_EQ(notification->get_notification()->progress(), 60);
 
   // Done.
   SendImportProgress(
       vm_tools::cicerone::ImportLxdContainerProgressSignal_Status_DONE);
-  EXPECT_EQ(notification->get_status(),
+  EXPECT_EQ(notification->status(),
             CrostiniExportImportNotification::Status::DONE);
 }
 
@@ -301,7 +301,7 @@ TEST_F(CrostiniExportImportTest, TestImportFail) {
   // Failed.
   SendImportProgress(
       vm_tools::cicerone::ImportLxdContainerProgressSignal_Status_FAILED);
-  EXPECT_EQ(notification->get_status(),
+  EXPECT_EQ(notification->status(),
             CrostiniExportImportNotification::Status::FAILED);
   std::string msg("Restoring couldn't be completed due to an error");
   EXPECT_EQ(notification->get_notification()->message(),
@@ -319,7 +319,7 @@ TEST_F(CrostiniExportImportTest, TestImportFailArchitecture) {
   SendImportProgress(
       vm_tools::cicerone::
           ImportLxdContainerProgressSignal_Status_FAILED_ARCHITECTURE);
-  EXPECT_EQ(notification->get_status(),
+  EXPECT_EQ(notification->status(),
             CrostiniExportImportNotification::Status::FAILED);
   std::string msg(
       "Cannot import container architecture type arch_con with this device "
@@ -344,7 +344,7 @@ TEST_F(CrostiniExportImportTest, TestImportFailSpace) {
           .available_space = 20ul * 1'024 * 1'024 * 1'024,    // 20Gb
           .min_required_space = 35ul * 1'024 * 1'024 * 1'024  // 35Gb
       });
-  EXPECT_EQ(notification->get_status(),
+  EXPECT_EQ(notification->status(),
             CrostiniExportImportNotification::Status::FAILED);
   std::string msg =
       "Cannot restore due to lack of storage space. Free up 15.0 GB from the "