Split Removable Media Watchers to be one per media

Currently, Removable Media is watched at the top-level directory under
/media/removable. Unfortunately, this only works if the removable media
was attached before the inotify is attached (since inotify does not
detect mount event).

This changes listen to the mount events from cros-disks through
ArcVolumeManager (to synchronize with Android volume mounting) and
create a FilePathWatcher per removable media.

BUG=b:150883493
TEST=Mount invisible removable media, the media changes are indexed.
TEST=Reboot DUT with non-visible removable media attached, still indexed.
TEST=Change the removable media to be visible, still indexed.
TEST=Reboot DUT with visible media attached, still indexed
TEST=Change the removable media to be invisible, still indexed.

Change-Id: Ia02012676681e025431d74f2c3f9b951a77ca783
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2362376
Auto-Submit: Risan <risan@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Commit-Queue: Risan <risan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#806716}

chrome/browser/chromeos/arc/file_system_watcher/arc_file_system_watcher_service.h

@@ -5,15 +5,18 @@
 #ifndef CHROME_BROWSER_CHROMEOS_ARC_FILE_SYSTEM_WATCHER_ARC_FILE_SYSTEM_WATCHER_SERVICE_H_
 #define CHROME_BROWSER_CHROMEOS_ARC_FILE_SYSTEM_WATCHER_ARC_FILE_SYSTEM_WATCHER_SERVICE_H_
 
+#include <map>
 #include <memory>
 #include <string>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "components/arc/mojom/file_system.mojom-forward.h"
 #include "components/arc/session/connection_observer.h"
+#include "components/arc/volume_mounter/arc_volume_mounter_bridge.h"
 #include "components/keyed_service/core/keyed_service.h"
 
 namespace base {
@@ -33,7 +36,8 @@ class ArcBridgeService;
 // Android MediaProvider.
 class ArcFileSystemWatcherService
     : public KeyedService,
-      public ConnectionObserver<mojom::FileSystemInstance> {
+      public ConnectionObserver<mojom::FileSystemInstance>,
+      public ArcVolumeMounterBridge::Delegate {
  public:
   // Returns singleton instance for the given BrowserContext,
   // or nullptr if the browser |context| is not allowed to use ARC.
@@ -49,15 +53,25 @@ class ArcFileSystemWatcherService
   void OnConnectionReady() override;
   void OnConnectionClosed() override;
 
+  // ArcVolumeMounterBridge::Delegate overrides.
+  void StartWatchingRemovableMedia(const std::string& fs_uuid,
+                                   const std::string& mount_path,
+                                   base::OnceClosure callback) override;
+
+  void StopWatchingRemovableMedia(const std::string& fs_uuid) override;
+
  private:
   class FileSystemWatcher;
 
   void StartWatchingFileSystem();
-  void StopWatchingFileSystem();
+  void StopWatchingFileSystem(base::OnceClosure);
+
+  void TriggerSendAllMountEvents() const;
 
   std::unique_ptr<FileSystemWatcher> CreateAndStartFileSystemWatcher(
       const base::FilePath& cros_path,
-      const base::FilePath& android_path);
+      const base::FilePath& android_path,
+      base::OnceClosure callback);
   void OnFileSystemChanged(const std::vector<std::string>& paths);
 
   content::BrowserContext* const context_;
@@ -65,7 +79,9 @@ class ArcFileSystemWatcherService
 
   std::unique_ptr<FileSystemWatcher> downloads_watcher_;
   std::unique_ptr<FileSystemWatcher> myfiles_watcher_;
-  std::unique_ptr<FileSystemWatcher> removable_media_watcher_;
+  // A map from UUID to watcher.
+  std::map<std::string, std::unique_ptr<FileSystemWatcher>>
+      removable_media_watchers_;
 
   scoped_refptr<base::SequencedTaskRunner> file_task_runner_;
 

components/arc/BUILD.gn

@@ -118,6 +118,7 @@ static_library("arc") {
     "//components/exo",
     "//components/google/core/common",
     "//components/guest_os",
+    "//components/keyed_service/core",
     "//components/onc",
     "//components/prefs",
 

components/arc/volume_mounter/arc_volume_mounter_bridge.cc

@@ -70,9 +70,15 @@ ArcVolumeMounterBridge* ArcVolumeMounterBridge::GetForBrowserContextForTesting(
   return ArcVolumeMounterBridgeFactory::GetForBrowserContextForTesting(context);
 }
 
+// static
+KeyedServiceBaseFactory* ArcVolumeMounterBridge::GetFactory() {
+  return ArcVolumeMounterBridgeFactory::GetInstance();
+}
+
 ArcVolumeMounterBridge::ArcVolumeMounterBridge(content::BrowserContext* context,
                                                ArcBridgeService* bridge_service)
-    : arc_bridge_service_(bridge_service),
+    : delegate_(nullptr),
+      arc_bridge_service_(bridge_service),
       pref_service_(user_prefs::UserPrefs::Get(context)) {
   DCHECK(pref_service_);
   arc_bridge_service_->volume_mounter()->AddObserver(this);
@@ -95,6 +101,11 @@ ArcVolumeMounterBridge::~ArcVolumeMounterBridge() {
   arc_bridge_service_->volume_mounter()->RemoveObserver(this);
 }
 
+void ArcVolumeMounterBridge::Initialize(Delegate* delegate) {
+  delegate_ = delegate;
+  DCHECK(delegate_);
+}
+
 // Sends MountEvents of all existing MountPoints in cros-disks.
 void ArcVolumeMounterBridge::SendAllMountEvents() {
   SendMountEventForMyFiles();
@@ -150,19 +161,12 @@ void ArcVolumeMounterBridge::OnVisibleStoragesChanged() {
   }
 }
 
-void ArcVolumeMounterBridge::OnConnectionReady() {
-  // Deferring the SendAllMountEvents as a task to current thread to not
-  // block the mojo request since SendAllMountEvents might take non trivial
-  // amount of time.
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(&ArcVolumeMounterBridge::SendAllMountEvents,
-                                weak_ptr_factory_.GetWeakPtr()));
-}
-
 void ArcVolumeMounterBridge::OnMountEvent(
     DiskMountManager::MountEvent event,
     chromeos::MountError error_code,
     const DiskMountManager::MountPointInfo& mount_info) {
+  DCHECK(delegate_);
+
   // ArcVolumeMounter is limited for local storage, as Android's StorageManager
   // volume concept relies on assumption that it is local filesystem. Hence,
   // special volumes like DriveFS should not come through this path.
@@ -199,17 +203,35 @@ void ArcVolumeMounterBridge::OnMountEvent(
              << " is null during MountEvent " << event;
   }
 
-  mojom::VolumeMounterInstance* volume_mounter_instance =
-      ARC_GET_INSTANCE_FOR_METHOD(arc_bridge_service_->volume_mounter(),
-                                  OnMountEvent);
-
-  if (!volume_mounter_instance)
-    return;
-
   const bool visible = IsVisibleToAndroidApps(fs_uuid);
-  volume_mounter_instance->OnMountEvent(mojom::MountPointInfo::New(
-      event, mount_info.source_path, mount_info.mount_path, fs_uuid,
-      device_label, device_type, visible));
+  switch (event) {
+    case DiskMountManager::MountEvent::MOUNTING:
+      // Attach watcher to the directories. This is the best place to add the
+      // watcher, because if the watcher is attached after Android mounts (and
+      // performs full scan) the removable media, there might be a small time
+      // interval that has undetectable changes.
+      delegate_->StartWatchingRemovableMedia(
+          fs_uuid, mount_info.mount_path,
+          base::BindOnce(
+              &ArcVolumeMounterBridge::SendMountEventForRemovableMedia,
+              weak_ptr_factory_.GetWeakPtr(), event, mount_info.source_path,
+              mount_info.mount_path, fs_uuid, device_label, device_type,
+              visible));
+      break;
+    case DiskMountManager::MountEvent::UNMOUNTING:
+
+      // The actual ordering for the unmount event is not very important because
+      // during unmount, we don't care about accidentally ignoring changes.
+      // Hence, no synchronization is needed as we only care about cleaning up
+      // memory usage for watchers which is ok to be done at any time as long as
+      // it is done.
+      SendMountEventForRemovableMedia(event, mount_info.source_path,
+                                      mount_info.mount_path, fs_uuid,
+                                      device_label, device_type, visible);
+      delegate_->StopWatchingRemovableMedia(fs_uuid);
+      break;
+  }
+
   if (event == DiskMountManager::MountEvent::MOUNTING &&
       (device_type == chromeos::DeviceType::DEVICE_TYPE_USB ||
        device_type == chromeos::DeviceType::DEVICE_TYPE_SD)) {
@@ -220,6 +242,25 @@ void ArcVolumeMounterBridge::OnMountEvent(
   }
 }
 
+void ArcVolumeMounterBridge::SendMountEventForRemovableMedia(
+    DiskMountManager::MountEvent event,
+    const std::string& source_path,
+    const std::string& mount_path,
+    const std::string& fs_uuid,
+    const std::string& device_label,
+    chromeos::DeviceType device_type,
+    bool visible) {
+  mojom::VolumeMounterInstance* volume_mounter_instance =
+      ARC_GET_INSTANCE_FOR_METHOD(arc_bridge_service_->volume_mounter(),
+                                  OnMountEvent);
+
+  if (!volume_mounter_instance)
+    return;
+  volume_mounter_instance->OnMountEvent(
+      mojom::MountPointInfo::New(event, source_path, mount_path, fs_uuid,
+                                 device_label, device_type, visible));
+}
+
 void ArcVolumeMounterBridge::RequestAllMountPoints() {
   // Deferring the SendAllMountEvents as a task to current thread to not
   // block the mojo request since SendAllMountEvents might take non trivial

components/arc/volume_mounter/arc_volume_mounter_bridge.h

@@ -7,12 +7,14 @@
 
 #include <string>
 
+#include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "chromeos/disks/disk_mount_manager.h"
 #include "components/arc/mojom/volume_mounter.mojom.h"
 #include "components/arc/session/connection_observer.h"
 #include "components/keyed_service/core/keyed_service.h"
+#include "components/keyed_service/core/keyed_service_base_factory.h"
 #include "components/prefs/pref_change_registrar.h"
 
 namespace content {
@@ -31,6 +33,21 @@ class ArcVolumeMounterBridge
       public ConnectionObserver<mojom::VolumeMounterInstance>,
       public mojom::VolumeMounterHost {
  public:
+  class Delegate {
+   public:
+    // To be called by ArcVolumeMounter when a removable media is mounted. This
+    // create a watcher for the removable media.
+    virtual void StartWatchingRemovableMedia(const std::string& fs_uuid,
+                                             const std::string& mount_path,
+                                             base::OnceClosure callback) = 0;
+
+    // To be called by ArcVolumeMounter when a removable media is unmounted.
+    // This removees the watcher for the removable media..
+    virtual void StopWatchingRemovableMedia(const std::string& fs_uuid) = 0;
+
+   protected:
+    ~Delegate() = default;
+  };
   // Returns singleton instance for the given BrowserContext,
   // or nullptr if the browser |context| is not allowed to use ARC.
   static ArcVolumeMounterBridge* GetForBrowserContext(
@@ -38,13 +55,13 @@ class ArcVolumeMounterBridge
   static ArcVolumeMounterBridge* GetForBrowserContextForTesting(
       content::BrowserContext* context);
 
+  // Returns Factory instance for ArcVolumeMounterBridge.
+  static KeyedServiceBaseFactory* GetFactory();
+
   ArcVolumeMounterBridge(content::BrowserContext* context,
                          ArcBridgeService* bridge_service);
   ~ArcVolumeMounterBridge() override;
 
-  // ConnectionObserver<mojom::VolumeMounterInstance> overrides:
-  void OnConnectionReady() override;
-
   // chromeos::disks::DiskMountManager::Observer overrides:
   void OnMountEvent(chromeos::disks::DiskMountManager::MountEvent event,
                     chromeos::MountError error_code,
@@ -54,14 +71,28 @@ class ArcVolumeMounterBridge
   // mojom::VolumeMounterHost overrides:
   void RequestAllMountPoints() override;
 
- private:
+  // Initialize ArcVolumeMounterBridge with delegate.
+  void Initialize(Delegate* delegate);
+
+  // Send all existing mount events. Usually is called around service startup.
   void SendAllMountEvents();
 
+ private:
   void SendMountEventForMyFiles();
+  void SendMountEventForRemovableMedia(
+      chromeos::disks::DiskMountManager::MountEvent event,
+      const std::string& source_path,
+      const std::string& mount_path,
+      const std::string& fs_uuid,
+      const std::string& device_label,
+      chromeos::DeviceType device_type,
+      bool visible);
 
   bool IsVisibleToAndroidApps(const std::string& uuid) const;
   void OnVisibleStoragesChanged();
 
+  Delegate* delegate_;
+
   ArcBridgeService* const arc_bridge_service_;  // Owned by ArcServiceManager.
 
   PrefService* const pref_service_;