Improve holding space item restoration for Android files

Defers item validity checks for Android file until connection to ARC
file system service is ready. Without this, there was a race condition
between holding space service and ARC initialization, where item
validity checks were performed before the backing file system was ready.

Also, given that ARC is only initialized for primary profiles, makes
holding space file system delegate ignore Android files in non-primary
profile - they are kept in the persistent storage, but are kept out of
the holding space model.

BUG=1165890

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

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

@@ -4,6 +4,9 @@
 
 #include "chrome/browser/ui/ash/holding_space/holding_space_file_system_delegate.h"
 
+#include <set>
+#include <string>
+
 #include "ash/public/cpp/holding_space/holding_space_constants.h"
 #include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "ash/public/cpp/holding_space/holding_space_model.h"
@@ -14,12 +17,43 @@
 #include "base/task/task_traits.h"
 #include "base/task/thread_pool.h"
 #include "base/threading/sequenced_task_runner_handle.h"
+#include "chrome/browser/chromeos/arc/arc_util.h"
+#include "chrome/browser/chromeos/file_manager/path_util.h"
 #include "chrome/browser/chromeos/fileapi/file_change_service_factory.h"
+#include "chrome/browser/profiles/profile.h"
+#include "components/arc/arc_service_manager.h"
+#include "components/arc/session/arc_bridge_service.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 
 namespace ash {
 
+namespace {
+
+arc::ConnectionHolder<arc::mojom::FileSystemInstance,
+                      arc::mojom::FileSystemHost>*
+GetArcFileSystem() {
+  if (!arc::ArcServiceManager::Get())
+    return nullptr;
+  return arc::ArcServiceManager::Get()->arc_bridge_service()->file_system();
+}
+
+// Returns whether
+// *   the ARC is enabled for `profile`, and
+// *   connection to ARC file system service is *not* established at the time.
+bool IsArcFileSystemDisconnected(Profile* profile) {
+  return arc::IsArcPlayStoreEnabledForProfile(profile) && GetArcFileSystem() &&
+         !GetArcFileSystem()->IsConnected();
+}
+
+// Returns whether the item is backed by an Android file. Can be used with
+// non-finalized items.
+bool ItemBackedByAndroidFile(const HoldingSpaceItem* item) {
+  return file_manager::util::GetAndroidFilesPath().IsParent(item->file_path());
+}
+
+}  // namespace
+
 // HoldingSpaceFileSystemDelegate::FileSystemWatcher ---------------------------
 
 class HoldingSpaceFileSystemDelegate::FileSystemWatcher {
@@ -99,6 +133,19 @@ HoldingSpaceFileSystemDelegate::~HoldingSpaceFileSystemDelegate() {
                                           file_system_watcher_.release());
 }
 
+void HoldingSpaceFileSystemDelegate::OnConnectionReady() {
+  // Schedule validity checks for android items.
+  for (auto& item : model()->items()) {
+    if (!ItemBackedByAndroidFile(item.get()))
+      continue;
+
+    holding_space_util::ValidityRequirement requirements;
+    if (item->type() != HoldingSpaceItem::Type::kPinnedFile)
+      requirements.must_be_newer_than = kMaxFileAge;
+    ScheduleFilePathValidityCheck({item->file_path(), requirements});
+  }
+}
+
 void HoldingSpaceFileSystemDelegate::Init() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   file_system_watcher_ = std::make_unique<FileSystemWatcher>(
@@ -113,6 +160,9 @@ void HoldingSpaceFileSystemDelegate::Init() {
         file_manager::VolumeManager::Get(profile()));
   }
 
+  if (GetArcFileSystem())
+    arc_file_system_observer_.Observe(GetArcFileSystem());
+
   // Schedule a task to clean up items that belong to volumes that haven't been
   // mounted in a reasonable amount of time.
   // The primary goal of handling the delayed volume mount is to support volumes
@@ -126,6 +176,9 @@ void HoldingSpaceFileSystemDelegate::Init() {
 void HoldingSpaceFileSystemDelegate::OnHoldingSpaceItemsAdded(
     const std::vector<const HoldingSpaceItem*>& items) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  const bool arc_file_system_disconnected =
+      IsArcFileSystemDisconnected(profile());
   for (const HoldingSpaceItem* item : items) {
     if (item->IsFinalized()) {
       // Watch the directory containing `item`'s backing file. If the directory
@@ -152,6 +205,11 @@ void HoldingSpaceFileSystemDelegate::OnHoldingSpaceItemsAdded(
     if (file_system_url.is_empty())
       continue;
 
+    // Defer validity checks (and finalization) for android files if the
+    // ARC file system connection is not yet ready.
+    if (arc_file_system_disconnected && ItemBackedByAndroidFile(item))
+      continue;
+
     holding_space_util::ValidityRequirement requirements;
     if (item->type() != HoldingSpaceItem::Type::kPinnedFile)
       requirements.must_be_newer_than = kMaxFileAge;
@@ -350,17 +408,32 @@ void HoldingSpaceFileSystemDelegate::OnFilePathValidityChecksComplete(
     std::vector<base::FilePath> valid_paths,
     std::vector<base::FilePath> invalid_paths) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  const bool arc_file_system_disconnected =
+      IsArcFileSystemDisconnected(profile());
+
   // Remove items with invalid paths.
-  // When `file_path` is removed, we need to remove any associated items.
   model()->RemoveIf(base::BindRepeating(
-      [](const std::vector<base::FilePath>* invalid_paths,
+      [](bool arc_file_system_disconnected,
+         const std::vector<base::FilePath>* invalid_paths,
          const HoldingSpaceItem* item) {
+        // Avoid removing Android files if connection to ARC file system has
+        // been lost (e.g. Android container might have crashed). Validity
+        // checks will be re-run once the file system gets connected.
+        if (arc_file_system_disconnected && ItemBackedByAndroidFile(item))
+          return false;
+
         return base::Contains(*invalid_paths, item->file_path());
       },
-      &invalid_paths));
+      arc_file_system_disconnected, &invalid_paths));
 
   std::vector<const HoldingSpaceItem*> items_to_finalize;
   for (auto& item : model()->items()) {
+    // Defer finalization of items backed by android files if the connection to
+    // ARC file system service has been lost.
+    if (arc_file_system_disconnected && ItemBackedByAndroidFile(item.get()))
+      continue;
+
     if (!item->IsFinalized() &&
         base::Contains(valid_paths, item->file_path())) {
       items_to_finalize.push_back(item.get());
@@ -414,7 +487,19 @@ void HoldingSpaceFileSystemDelegate::RemoveItemsParentedByPath(
 void HoldingSpaceFileSystemDelegate::ClearNonFinalizedItems() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   model()->RemoveIf(base::BindRepeating(
-      [](const HoldingSpaceItem* item) { return !item->IsFinalized(); }));
+      [](Profile* profile, const HoldingSpaceItem* item) {
+        if (item->IsFinalized())
+          return false;
+
+        // Do not remove items whose path can be resolved to a file system URL.
+        // In this case, the associated mount point has been mounted, but the
+        // finalization may have been delayed - for example due to issues/delays
+        // with initializing ARC.
+        const GURL url = holding_space_util::ResolveFileSystemUrl(
+            profile, item->file_path());
+        return url.is_empty();
+      },
+      profile()));
 }
 
 }  // namespace ash

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

@@ -17,6 +17,9 @@
 #include "chrome/browser/chromeos/fileapi/file_change_service_observer.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_keyed_service_delegate.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_util.h"
+#include "components/arc/mojom/file_system.mojom-forward.h"
+#include "components/arc/session/connection_holder.h"
+#include "components/arc/session/connection_observer.h"
 
 namespace base {
 class FilePath;
@@ -33,6 +36,7 @@ namespace ash {
 class HoldingSpaceFileSystemDelegate
     : public HoldingSpaceKeyedServiceDelegate,
       public chromeos::FileChangeServiceObserver,
+      public arc::ConnectionObserver<arc::mojom::FileSystemInstance>,
       public file_manager::VolumeManagerObserver {
  public:
   HoldingSpaceFileSystemDelegate(Profile* profile, HoldingSpaceModel* model);
@@ -65,6 +69,9 @@ class HoldingSpaceFileSystemDelegate
   void OnFileMoved(const storage::FileSystemURL& src,
                    const storage::FileSystemURL& dst) override;
 
+  // arc::ConnectionObserver<arc::mojom::FileSystemInstance>:
+  void OnConnectionReady() override;
+
   // Invoked when the specified `file_path` has changed.
   void OnFilePathChanged(const base::FilePath& file_path, bool error);
 
@@ -131,6 +138,12 @@ class HoldingSpaceFileSystemDelegate
                           file_manager::VolumeManagerObserver>
       volume_manager_observer_{this};
 
+  base::ScopedObservation<
+      arc::ConnectionHolder<arc::mojom::FileSystemInstance,
+                            arc::mojom::FileSystemHost>,
+      arc::ConnectionObserver<arc::mojom::FileSystemInstance>>
+      arc_file_system_observer_{this};
+
   base::WeakPtrFactory<HoldingSpaceFileSystemDelegate> weak_factory_{this};
 };
 

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

@@ -7,12 +7,28 @@
 #include "ash/public/cpp/holding_space/holding_space_constants.h"
 #include "ash/public/cpp/holding_space/holding_space_image.h"
 #include "ash/public/cpp/holding_space/holding_space_item.h"
+#include "chrome/browser/chromeos/file_manager/path_util.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/scoped_user_pref_update.h"
 
 namespace ash {
 
+namespace {
+
+// Returns whether the item should be ignored by the holding space model. This
+// returns true if the item is not supported in the current context, but may
+// be otherwise supported. For example, returns true for ARC file system
+// backed items in a secondary user profile.
+bool ShouldIgnoreItem(Profile* profile, const HoldingSpaceItem* item) {
+  return file_manager::util::GetAndroidFilesPath().IsParent(
+             item->file_path()) &&
+         !chromeos::ProfileHelper::IsPrimaryProfile(profile);
+}
+
+}  // namespace
+
 // static
 constexpr char HoldingSpacePersistenceDelegate::kPersistencePath[];
 
@@ -109,6 +125,8 @@ void HoldingSpacePersistenceDelegate::RestoreModelFromPersistence() {
             base::Value::AsDictionaryValue(persisted_holding_space_item),
             base::BindOnce(&holding_space_util::ResolveImage,
                            base::Unretained(thumbnail_loader_)));
+    if (ShouldIgnoreItem(profile(), holding_space_item.get()))
+      continue;
     item_restored_callback_.Run(std::move(holding_space_item));
   }
   // Notify completion of persistence restoration.