Shutdown holding space delegates while device is suspended

Some file systems are unmounted during device suspend (for example drive
FS), which was causing the holding space file system delegate's file
path removal detection to remove them from the holding space on device
suspend. To avoid this issue, holding space keyed service now observes
device suspend status and shuts down holding space delegates when the
device suspends. The delegates are re-initialized when suspend ends,
restoring the model from persistence again.

BUG=1152924

Change-Id: Iffe4586bcb87f47d687ceeff0fc76e60a443c0d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2568355
Commit-Queue: Toni Baržić <tbarzic@chromium.org>
Reviewed-by: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#833515}

chrome/browser/ui/ash/holding_space/BUILD.gn

@@ -20,9 +20,11 @@ source_set("browser_tests") {
     "//base/test:test_support",
     "//chrome/browser",
     "//chrome/browser/chromeos",
+    "//chrome/browser/chromeos:test_support",
     "//chrome/browser/extensions",
     "//chrome/browser/ui",
     "//chrome/test:test_support_ui",
+    "//chromeos/dbus/power:power",
     "//content/test:test_support",
     "//ui/aura",
     "//ui/base",

chrome/browser/ui/ash/holding_space/holding_space_downloads_delegate.cc

@@ -41,10 +41,6 @@ void HoldingSpaceDownloadsDelegate::Init() {
           : content::BrowserContext::GetDownloadManager(profile()));
 }
 
-void HoldingSpaceDownloadsDelegate::Shutdown() {
-  RemoveObservers();
-}
-
 void HoldingSpaceDownloadsDelegate::OnPersistenceRestored() {
   content::DownloadManager* download_manager =
       download_manager_for_testing

chrome/browser/ui/ash/holding_space/holding_space_downloads_delegate.h

@@ -47,7 +47,6 @@ class HoldingSpaceDownloadsDelegate : public HoldingSpaceKeyedServiceDelegate,
  private:
   // HoldingSpaceKeyedServiceDelegate:
   void Init() override;
-  void Shutdown() override;
   void OnPersistenceRestored() override;
 
   // content::DownloadManager::Observer:

chrome/browser/ui/ash/holding_space/holding_space_file_system_delegate.cc

@@ -102,10 +102,6 @@ void HoldingSpaceFileSystemDelegate::Init() {
                      base::Unretained(this)));
 }
 
-void HoldingSpaceFileSystemDelegate::Shutdown() {
-  volume_manager_observer_.RemoveAll();
-}
-
 void HoldingSpaceFileSystemDelegate::OnHoldingSpaceItemAdded(
     const HoldingSpaceItem* item) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -188,11 +184,17 @@ void HoldingSpaceFileSystemDelegate::OnVolumeMounted(
 void HoldingSpaceFileSystemDelegate::OnVolumeUnmounted(
     chromeos::MountError error_code,
     const file_manager::Volume& volume) {
-  model()->RemoveIf(base::BindRepeating(
-      [](const base::FilePath& volume_path, const HoldingSpaceItem* item) {
-        return volume_path.IsParent(item->file_path());
-      },
-      volume.mount_path()));
+  // Schedule task to remove items under the unmounted file path from the model.
+  // During suspend, some volumes get unmounted - for example, drive FS. The
+  // file system delegate gets shutdown to avoid removing items from unmounted
+  // volumes, but depending on the order in which observers are added to power
+  // manager dbus client, the file system delegate may get shutdown after
+  // unmounting a volume. To avoid observer ordering issues, schedule
+  // asynchronous task to remove unmounted items from the model.
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&HoldingSpaceFileSystemDelegate::RemoveItemsParentedByPath,
+                     weak_factory_.GetWeakPtr(), volume.mount_path()));
 }
 
 void HoldingSpaceFileSystemDelegate::OnFilePathChanged(
@@ -284,6 +286,15 @@ void HoldingSpaceFileSystemDelegate::RemoveWatch(
                                 file_system_watcher_->GetWeakPtr(), file_path));
 }
 
+void HoldingSpaceFileSystemDelegate::RemoveItemsParentedByPath(
+    const base::FilePath& parent_path) {
+  model()->RemoveIf(base::BindRepeating(
+      [](const base::FilePath& parent_path, const HoldingSpaceItem* item) {
+        return parent_path.IsParent(item->file_path());
+      },
+      parent_path));
+}
+
 void HoldingSpaceFileSystemDelegate::ClearNonFinalizedItems() {
   model()->RemoveIf(base::BindRepeating(
       [](const HoldingSpaceItem* item) { return !item->IsFinalized(); }));

chrome/browser/ui/ash/holding_space/holding_space_file_system_delegate.h

@@ -42,7 +42,6 @@ class HoldingSpaceFileSystemDelegate : public HoldingSpaceKeyedServiceDelegate,
 
   // HoldingSpaceKeyedServiceDelegate:
   void Init() override;
-  void Shutdown() override;
   void OnHoldingSpaceItemAdded(const HoldingSpaceItem* item) override;
   void OnHoldingSpaceItemRemoved(const HoldingSpaceItem* item) override;
   void OnHoldingSpaceItemFinalized(const HoldingSpaceItem* item) override;
@@ -77,6 +76,10 @@ class HoldingSpaceFileSystemDelegate : public HoldingSpaceKeyedServiceDelegate,
   void AddWatch(const base::FilePath& file_path);
   void RemoveWatch(const base::FilePath& file_path);
 
+  // Removes items that are (transitively) parented by `parent_path` from the
+  // holding space model.
+  void RemoveItemsParentedByPath(const base::FilePath& parent_path);
+
   // Clears all non-finalized items from holding space model - runs with a delay
   // after profile initialization to clean up items from volumes that have not
   // been mounted during startup.

chrome/browser/ui/ash/holding_space/holding_space_keyed_service.cc

@@ -96,7 +96,7 @@ HoldingSpaceKeyedService::HoldingSpaceKeyedService(Profile* profile,
   }
 
   // Otherwise we need to wait for the profile to be added.
-  profile_manager_observer_.Add(profile_manager);
+  profile_manager_observer_.Observe(profile_manager);
 }
 
 HoldingSpaceKeyedService::~HoldingSpaceKeyedService() {
@@ -269,18 +269,47 @@ void HoldingSpaceKeyedService::AddItem(std::unique_ptr<HoldingSpaceItem> item) {
 }
 
 void HoldingSpaceKeyedService::Shutdown() {
-  for (auto& delegate : delegates_)
-    delegate->Shutdown();
+  ShutdownDelegates();
 }
 
 void HoldingSpaceKeyedService::OnProfileAdded(Profile* profile) {
   if (profile == profile_) {
-    profile_manager_observer_.Remove(GetProfileManager());
+    profile_manager_observer_.RemoveObservation();
     OnProfileReady();
   }
 }
 
 void HoldingSpaceKeyedService::OnProfileReady() {
+  // Observe suspend status - the delegates will be shutdown during suspend.
+  if (chromeos::PowerManagerClient::Get())
+    power_manager_observer_.Observe(chromeos::PowerManagerClient::Get());
+
+  InitializeDelegates();
+
+  HoldingSpaceController::Get()->RegisterClientAndModelForUser(
+      account_id_, &holding_space_client_, &holding_space_model_);
+}
+
+void HoldingSpaceKeyedService::SuspendImminent(
+    power_manager::SuspendImminent::Reason reason) {
+  // Shutdown all delegates and clear the model when device suspends - some
+  // volumes may get unmounted during suspend, and may thus incorrectly get
+  // detected as deleted when device suspends - shutting down delegates during
+  // suspend avoids this issue, as it also disables file removal detection.
+  ShutdownDelegates();
+
+  // Clear the model as it will get restored from persistence when
+  // delegates are re-initialized after suspend.
+  holding_space_model_.RemoveAll();
+}
+
+void HoldingSpaceKeyedService::SuspendDone(base::TimeDelta sleep_duration) {
+  InitializeDelegates();
+}
+
+void HoldingSpaceKeyedService::InitializeDelegates() {
+  DCHECK(delegates_.empty());
+
   // The `HoldingSpaceDownloadsDelegate` monitors the status of downloads.
   delegates_.push_back(std::make_unique<HoldingSpaceDownloadsDelegate>(
       profile_, &holding_space_model_,
@@ -309,12 +338,13 @@ void HoldingSpaceKeyedService::OnProfileReady() {
     delegate->Init();
 }
 
+void HoldingSpaceKeyedService::ShutdownDelegates() {
+  delegates_.clear();
+}
+
 void HoldingSpaceKeyedService::OnPersistenceRestored() {
   for (auto& delegate : delegates_)
     delegate->NotifyPersistenceRestored();
-
-  HoldingSpaceController::Get()->RegisterClientAndModelForUser(
-      account_id_, &holding_space_client_, &holding_space_model_);
 }
 
 }  // namespace ash

chrome/browser/ui/ash/holding_space/holding_space_keyed_service.h

@@ -9,13 +9,14 @@
 #include <vector>
 
 #include "ash/public/cpp/holding_space/holding_space_model.h"
-#include "base/scoped_observer.h"
+#include "base/scoped_observation.h"
 #include "base/strings/string16.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/profiles/profile_manager_observer.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_client_impl.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_keyed_service_delegate.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_thumbnail_loader.h"
+#include "chromeos/dbus/power/power_manager_client.h"
 #include "components/account_id/account_id.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "url/gurl.h"
@@ -40,7 +41,8 @@ namespace ash {
 // *   Manages the temporary holding space per-profile data model.
 // *   Serves as an entry point to add holding space items from Chrome.
 class HoldingSpaceKeyedService : public KeyedService,
-                                 public ProfileManagerObserver {
+                                 public ProfileManagerObserver,
+                                 public chromeos::PowerManagerClient::Observer {
  public:
   HoldingSpaceKeyedService(Profile* profile, const AccountId& account_id);
   HoldingSpaceKeyedService(const HoldingSpaceKeyedService& other) = delete;
@@ -102,9 +104,22 @@ class HoldingSpaceKeyedService : public KeyedService,
   // ProfileManagerObserver:
   void OnProfileAdded(Profile* profile) override;
 
+  // PowerManagerClient::Observer
+  void SuspendImminent(power_manager::SuspendImminent::Reason reason) override;
+  void SuspendDone(base::TimeDelta sleep_duration) override;
+
   // Invoked when the associated profile is ready.
   void OnProfileReady();
 
+  // Creates and initializes holding space delegates. Called when the associated
+  // profile finishes initialization, or when device suspend ends (the delegates
+  // are shutdown during suspend).
+  void InitializeDelegates();
+
+  // Shuts down and destroys existing holding space delegates. Called on
+  // profile shutdown, or when device suspend starts.
+  void ShutdownDelegates();
+
   // Invoked when holding space persistence has been restored.
   void OnPersistenceRestored();
 
@@ -121,9 +136,13 @@ class HoldingSpaceKeyedService : public KeyedService,
   // service. They operate autonomously of one another.
   std::vector<std::unique_ptr<HoldingSpaceKeyedServiceDelegate>> delegates_;
 
-  ScopedObserver<ProfileManager, ProfileManagerObserver>
+  base::ScopedObservation<ProfileManager, ProfileManagerObserver>
       profile_manager_observer_{this};
 
+  base::ScopedObservation<chromeos::PowerManagerClient,
+                          chromeos::PowerManagerClient::Observer>
+      power_manager_observer_{this};
+
   base::WeakPtrFactory<HoldingSpaceKeyedService> weak_factory_{this};
 };
 

chrome/browser/ui/ash/holding_space/holding_space_keyed_service_delegate.cc

@@ -19,8 +19,6 @@ ProfileManager* GetProfileManager() {
 
 HoldingSpaceKeyedServiceDelegate::~HoldingSpaceKeyedServiceDelegate() = default;
 
-void HoldingSpaceKeyedServiceDelegate::Shutdown() {}
-
 void HoldingSpaceKeyedServiceDelegate::NotifyPersistenceRestored() {
   DCHECK(is_restoring_persistence_);
   is_restoring_persistence_ = false;

chrome/browser/ui/ash/holding_space/holding_space_keyed_service_delegate.h

@@ -19,16 +19,12 @@ class HoldingSpaceKeyedServiceDelegate : public HoldingSpaceModelObserver {
  public:
   ~HoldingSpaceKeyedServiceDelegate() override;
 
-  // Invoked by `HoldingSpaceKeyedService` to initialize the delegate
-  // immediately after its construction. Delegates accepting callbacks from
-  // the service should *not* invoke callbacks during construction but are free
-  // to do so during or anytime after initialization.
+  // Invoked by `HoldingSpaceKeyedService` to initialize the delegate.
+  // Called immediately after the delegate's construction. Delegates accepting
+  // callbacks from the service should *not* invoke callbacks during
+  // construction but are free to do so during or anytime after initialization.
   virtual void Init() = 0;
 
-  // Invoked by `HoldingSpaceKeyedService` when the service is shutting down.
-  // Delegates should perform any necessary clean up.
-  virtual void Shutdown();
-
   // Invoked by `HoldingSpaceKeyedService` to notify delegates when holding
   // space persistence has been restored.
   void NotifyPersistenceRestored();

chrome/browser/ui/ash/holding_space/holding_space_keyed_service_unittest.cc

@@ -1085,6 +1085,7 @@ TEST_F(HoldingSpaceKeyedServiceTest, RemoveItemsFromUnmountedVolumes) {
   EXPECT_EQ(3u, holding_space_model->items().size());
 
   test_mount_1.reset();
+  base::RunLoop().RunUntilIdle();
 
   EXPECT_EQ(1u, GetProfile()
                     ->GetPrefs()