Reland "Shutdown holding space delegates while device is suspended"

This is a reland of 2a07cc57

Original change's description:
> 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}

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

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

@@ -9,6 +9,7 @@
 #include "ash/public/cpp/holding_space/holding_space_model.h"
 #include "base/files/file_path.h"
 #include "base/files/file_path_watcher.h"
+#include "base/files/file_util.h"
 #include "base/sequence_checker.h"
 #include "base/task/task_traits.h"
 #include "base/task/thread_pool.h"
@@ -32,15 +33,26 @@ class HoldingSpaceFileSystemDelegate::FileSystemWatcher {
   FileSystemWatcher& operator=(const FileSystemWatcher&) = delete;
   ~FileSystemWatcher() { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); }
 
-  void AddWatch(const base::FilePath& file_path) {
+  void AddWatchForParent(const base::FilePath& file_path) {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-    if (base::Contains(watchers_, file_path))
-      return;
-    watchers_[file_path] = std::make_unique<base::FilePathWatcher>();
-    watchers_[file_path]->Watch(
-        file_path, base::FilePathWatcher::Type::kNonRecursive,
-        base::Bind(&FileSystemWatcher::OnFilePathChanged,
-                   weak_factory_.GetWeakPtr()));
+
+    // Observe the file path parent directory for changes - this reduces the
+    // number of inotify requests, and works well enough for detecting file
+    // deletion.
+    const base::FilePath path_to_watch = file_path.DirName();
+
+    if (!base::Contains(watchers_, path_to_watch)) {
+      watchers_[path_to_watch] = std::make_unique<base::FilePathWatcher>();
+      watchers_[path_to_watch]->Watch(
+          path_to_watch, base::FilePathWatcher::Type::kNonRecursive,
+          base::Bind(&FileSystemWatcher::OnFilePathChanged,
+                     weak_factory_.GetWeakPtr()));
+    }
+
+    // If the target path got deleted while request to add a watcher was in
+    // flight, notify observers of path change immediately.
+    if (!base::PathExists(file_path))
+      OnFilePathChanged(path_to_watch, /*error=*/false);
   }
 
   void RemoveWatch(const base::FilePath& file_path) {
@@ -102,10 +114,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);
@@ -113,7 +121,7 @@ void HoldingSpaceFileSystemDelegate::OnHoldingSpaceItemAdded(
   if (item->IsFinalized()) {
     // Watch the directory containing `items`'s backing file. If the directory
     // is already being watched, this will no-op.
-    AddWatch(item->file_path().DirName());
+    AddWatchForParent(item->file_path());
     return;
   }
 
@@ -162,7 +170,7 @@ void HoldingSpaceFileSystemDelegate::OnHoldingSpaceItemRemoved(
 
 void HoldingSpaceFileSystemDelegate::OnHoldingSpaceItemFinalized(
     const HoldingSpaceItem* item) {
-  AddWatch(item->file_path().DirName());
+  AddWatchForParent(item->file_path());
 }
 
 void HoldingSpaceFileSystemDelegate::OnVolumeMounted(
@@ -188,11 +196,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(
@@ -269,10 +283,11 @@ void HoldingSpaceFileSystemDelegate::OnFilePathValidityChecksComplete(
   }
 }
 
-void HoldingSpaceFileSystemDelegate::AddWatch(const base::FilePath& file_path) {
+void HoldingSpaceFileSystemDelegate::AddWatchForParent(
+    const base::FilePath& file_path) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   file_system_watcher_runner_->PostTask(
-      FROM_HERE, base::BindOnce(&FileSystemWatcher::AddWatch,
+      FROM_HERE, base::BindOnce(&FileSystemWatcher::AddWatchForParent,
                                 file_system_watcher_->GetWeakPtr(), file_path));
 }
 
@@ -284,6 +299,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;
@@ -74,9 +73,15 @@ class HoldingSpaceFileSystemDelegate : public HoldingSpaceKeyedServiceDelegate,
       std::vector<base::FilePath> invalid_paths);
 
   // Adds/removes a watch for the specified `file_path`.
-  void AddWatch(const base::FilePath& file_path);
+  // Note that `AddWatchForParent` will add a watch for the `file_path`'s parent
+  // directory.
+  void AddWatchForParent(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()