Add a DiskMountManager method to refresh mount entries in addition to devices.

Lack of the info caused Chrome OS Files.app to fail finding external storages
after browser-only restart (like that from about:flags or from browser crash.)

BUG=356583

Review URL: https://codereview.chromium.org/379743004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@282558 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/file_manager/fake_disk_mount_manager.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/chromeos/file_manager/fake_disk_mount_manager.h"
 
+#include "base/callback.h"
 #include "base/stl_util.h"
 
 namespace file_manager {
@@ -62,7 +63,9 @@ FakeDiskMountManager::mount_points() const {
   return mount_points_;
 }
 
-void FakeDiskMountManager::RequestMountInfoRefresh() {
+void FakeDiskMountManager::EnsureMountInfoRefreshed(
+    const EnsureMountInfoRefreshedCallback& callback) {
+  callback.Run(true);
 }
 
 void FakeDiskMountManager::MountPath(const std::string& source_path,

chrome/browser/chromeos/file_manager/fake_disk_mount_manager.h

@@ -55,7 +55,8 @@ class FakeDiskMountManager : public chromeos::disks::DiskMountManager {
   virtual const Disk* FindDiskBySourcePath(
       const std::string& source_path) const OVERRIDE;
   virtual const MountPointMap& mount_points() const OVERRIDE;
-  virtual void RequestMountInfoRefresh() OVERRIDE;
+  virtual void EnsureMountInfoRefreshed(
+      const EnsureMountInfoRefreshedCallback& callback) OVERRIDE;
   virtual void MountPath(const std::string& source_path,
                          const std::string& source_format,
                          const std::string& mount_label,

chrome/browser/chromeos/file_manager/volume_manager.cc

@@ -310,6 +310,9 @@ void VolumeManager::Initialize() {
 
   // Subscribe to DiskMountManager.
   disk_mount_manager_->AddObserver(this);
+  disk_mount_manager_->EnsureMountInfoRefreshed(
+      base::Bind(&VolumeManager::OnDiskMountManagerRefreshed,
+                 weak_ptr_factory_.GetWeakPtr()));
 
   // Subscribe to FileSystemProviderService and register currently mounted
   // volumes for the profile.
@@ -326,54 +329,6 @@ void VolumeManager::Initialize() {
     }
   }
 
-  std::vector<VolumeInfo> archives;
-
-  const chromeos::disks::DiskMountManager::MountPointMap& mount_points =
-      disk_mount_manager_->mount_points();
-  for (chromeos::disks::DiskMountManager::MountPointMap::const_iterator it =
-           mount_points.begin();
-       it != mount_points.end();
-       ++it) {
-    if (it->second.mount_type == chromeos::MOUNT_TYPE_ARCHIVE) {
-      // Archives are mounted after other type of volumes. See below.
-      archives.push_back(CreateVolumeInfoFromMountPointInfo(it->second, NULL));
-      continue;
-    }
-    DoMountEvent(
-        chromeos::MOUNT_ERROR_NONE,
-        CreateVolumeInfoFromMountPointInfo(
-            it->second,
-            disk_mount_manager_->FindDiskBySourcePath(it->second.source_path)),
-            kNotRemounting);
-  }
-
-  // We mount archives only if they are opened from currently mounted volumes.
-  // To check the condition correctly in DoMountEvent, we care the order.
-  std::vector<bool> done(archives.size(), false);
-  for (size_t i = 0; i < archives.size(); ++i) {
-    if (!done[i]) {
-      std::vector<VolumeInfo> chain;
-      done[i] = true;
-      chain.push_back(archives[i]);
-
-      // If archives[i]'s source_path is in another archive, mount it first.
-      for (size_t parent = 0; parent < archives.size(); ++parent) {
-        if (!done[parent] &&
-            archives[parent].mount_path.IsParent(chain.back().source_path)) {
-          done[parent] = true;
-          chain.push_back(archives[parent]);
-          parent = 0;  // Search archives[parent]'s parent from the beginning.
-        }
-      }
-
-      // Mount from the tail of chain.
-      for (size_t i = chain.size(); i > 0; --i)
-        DoMountEvent(chromeos::MOUNT_ERROR_NONE, chain[i - 1], kNotRemounting);
-    }
-  }
-
-  disk_mount_manager_->RequestMountInfoRefresh();
-
   // Subscribe to Profile Preference change.
   pref_change_registrar_.Init(profile_->GetPrefs());
   pref_change_registrar_.Add(
@@ -787,6 +742,60 @@ void VolumeManager::OnRemovableStorageDetached(
   }
 }
 
+void VolumeManager::OnDiskMountManagerRefreshed(bool success) {
+  if (!success) {
+    LOG(ERROR) << "Failed to refresh disk mount manager";
+    return;
+  }
+
+  std::vector<VolumeInfo> archives;
+
+  const chromeos::disks::DiskMountManager::MountPointMap& mount_points =
+      disk_mount_manager_->mount_points();
+  for (chromeos::disks::DiskMountManager::MountPointMap::const_iterator it =
+           mount_points.begin();
+       it != mount_points.end();
+       ++it) {
+    if (it->second.mount_type == chromeos::MOUNT_TYPE_ARCHIVE) {
+      // Archives are mounted after other types of volume. See below.
+      archives.push_back(CreateVolumeInfoFromMountPointInfo(it->second, NULL));
+      continue;
+    }
+    DoMountEvent(
+        chromeos::MOUNT_ERROR_NONE,
+        CreateVolumeInfoFromMountPointInfo(
+            it->second,
+            disk_mount_manager_->FindDiskBySourcePath(it->second.source_path)),
+            kNotRemounting);
+  }
+
+  // We mount archives only if they are opened from currently mounted volumes.
+  // To check the condition correctly in DoMountEvent, we care about the order.
+  std::vector<bool> done(archives.size(), false);
+  for (size_t i = 0; i < archives.size(); ++i) {
+    if (done[i])
+      continue;
+
+    std::vector<VolumeInfo> chain;
+    done[i] = true;
+    chain.push_back(archives[i]);
+
+    // If archives[i]'s source_path is in another archive, mount it first.
+    for (size_t parent = i + 1; parent < archives.size(); ++parent) {
+      if (!done[parent] &&
+          archives[parent].mount_path.IsParent(chain.back().source_path)) {
+        done[parent] = true;
+        chain.push_back(archives[parent]);
+        parent = i + 1;  // Search archives[parent]'s parent from the beginning.
+      }
+    }
+
+    // Mount from the tail of chain.
+    for (size_t i = chain.size(); i > 0; --i)
+      DoMountEvent(chromeos::MOUNT_ERROR_NONE, chain[i - 1], kNotRemounting);
+  }
+}
+
 void VolumeManager::OnStorageMonitorInitialized() {
   std::vector<storage_monitor::StorageInfo> storages =
       storage_monitor::StorageMonitor::GetInstance()->GetAllAvailableStorages();

chrome/browser/chromeos/file_manager/volume_manager.h

@@ -206,6 +206,7 @@ class VolumeManager : public KeyedService,
   SnapshotManager* snapshot_manager() { return snapshot_manager_.get(); }
 
  private:
+  void OnDiskMountManagerRefreshed(bool success);
   void OnStorageMonitorInitialized();
   void OnPrivetVolumesAvailable(
       const local_discovery::PrivetVolumeLister::VolumeList& volumes);

chrome/browser/chromeos/imageburner/burn_device_handler_unittest.cc

@@ -80,7 +80,8 @@ class FakeDiskMountManager : public disks::DiskMountManager {
     // Note: mount_points_ will always be empty, now.
     return mount_points_;
   }
-  virtual void RequestMountInfoRefresh() OVERRIDE {}
+  virtual void EnsureMountInfoRefreshed(
+      const EnsureMountInfoRefreshedCallback& callback) OVERRIDE {}
   virtual void MountPath(const std::string& source_path,
                          const std::string& source_format,
                          const std::string& mount_label,

chromeos/disks/disk_mount_manager.cc

@@ -26,7 +26,9 @@ DiskMountManager* g_disk_mount_manager = NULL;
 // The DiskMountManager implementation.
 class DiskMountManagerImpl : public DiskMountManager {
  public:
-  DiskMountManagerImpl() : weak_ptr_factory_(this) {
+  DiskMountManagerImpl() :
+    already_refreshed_(false),
+    weak_ptr_factory_(this) {
     DBusThreadManager* dbus_thread_manager = DBusThreadManager::Get();
     DCHECK(dbus_thread_manager);
     cros_disks_client_ = dbus_thread_manager->GetCrosDisksClient();
@@ -155,7 +157,7 @@ class DiskMountManagerImpl : public DiskMountManager {
     }
 
     // We will send the same callback data object to all Unmount calls and use
-    // it to syncronize callbacks.
+    // it to synchronize callbacks.
     // Note: this implementation has a potential memory leak issue. For
     // example if this instance is destructed before all the callbacks for
     // Unmount are invoked, the memory pointed by |cb_data| will be leaked.
@@ -189,11 +191,22 @@ class DiskMountManagerImpl : public DiskMountManager {
   }
 
   // DiskMountManager override.
-  virtual void RequestMountInfoRefresh() OVERRIDE {
-    cros_disks_client_->EnumerateAutoMountableDevices(
-        base::Bind(&DiskMountManagerImpl::OnRequestMountInfo,
-                   weak_ptr_factory_.GetWeakPtr()),
-        base::Bind(&base::DoNothing));
+  virtual void EnsureMountInfoRefreshed(
+      const EnsureMountInfoRefreshedCallback& callback) OVERRIDE {
+    if (already_refreshed_) {
+      callback.Run(true);
+      return;
+    }
+
+    refresh_callbacks_.push_back(callback);
+    if (refresh_callbacks_.size() == 1) {
+      // If there's no in-flight refreshing task, start it.
+      cros_disks_client_->EnumerateAutoMountableDevices(
+          base::Bind(&DiskMountManagerImpl::RefreshAfterEnumerateDevices,
+                     weak_ptr_factory_.GetWeakPtr()),
+          base::Bind(&DiskMountManagerImpl::RefreshCompleted,
+                     weak_ptr_factory_.GetWeakPtr(), false));
+    }
   }
 
   // DiskMountManager override.
@@ -414,7 +427,7 @@ class DiskMountManagerImpl : public DiskMountManager {
     NotifyFormatStatusUpdate(FORMAT_COMPLETED, error_code, device_path);
   }
 
-  // Callbcak for GetDeviceProperties.
+  // Callback for GetDeviceProperties.
   void OnGetDeviceProperties(const DiskInfo& disk_info) {
     // TODO(zelidrag): Find a better way to filter these out before we
     // fetch the properties:
@@ -454,32 +467,64 @@ class DiskMountManagerImpl : public DiskMountManager {
     NotifyDiskStatusUpdate(is_new ? DISK_ADDED : DISK_CHANGED, disk);
   }
 
-  // Callbcak for RequestMountInfo.
-  void OnRequestMountInfo(const std::vector<std::string>& devices) {
-    std::set<std::string> current_device_set;
-    if (!devices.empty()) {
-      // Initiate properties fetch for all removable disks,
-      for (size_t i = 0; i < devices.size(); i++) {
-        current_device_set.insert(devices[i]);
-        // Initiate disk property retrieval for each relevant device path.
-        cros_disks_client_->GetDeviceProperties(
-            devices[i],
-            base::Bind(&DiskMountManagerImpl::OnGetDeviceProperties,
-                       weak_ptr_factory_.GetWeakPtr()),
-            base::Bind(&base::DoNothing));
-      }
-    }
-    // Search and remove disks that are no longer present.
+  // Part of EnsureMountInfoRefreshed(). Called after the list of devices are
+  // enumerated.
+  void RefreshAfterEnumerateDevices(const std::vector<std::string>& devices) {
+    std::set<std::string> current_device_set(devices.begin(), devices.end());
     for (DiskMap::iterator iter = disks_.begin(); iter != disks_.end(); ) {
       if (current_device_set.find(iter->first) == current_device_set.end()) {
-        Disk* disk = iter->second;
-        NotifyDiskStatusUpdate(DISK_REMOVED, disk);
         delete iter->second;
         disks_.erase(iter++);
       } else {
         ++iter;
       }
     }
+    RefreshDeviceAtIndex(devices, 0);
+  }
+
+  // Part of EnsureMountInfoRefreshed(). Called for each device to refresh info.
+  void RefreshDeviceAtIndex(const std::vector<std::string>& devices,
+                            size_t index) {
+    if (index == devices.size()) {
+      // All devices info retrieved. Proceed to enumerate mount point info.
+      cros_disks_client_->EnumerateMountEntries(
+          base::Bind(&DiskMountManagerImpl::RefreshAfterEnumerateMountEntries,
+                     weak_ptr_factory_.GetWeakPtr()),
+          base::Bind(&DiskMountManagerImpl::RefreshCompleted,
+                     weak_ptr_factory_.GetWeakPtr(), false));
+      return;
+    }
+
+    cros_disks_client_->GetDeviceProperties(
+        devices[index],
+        base::Bind(&DiskMountManagerImpl::RefreshAfterGetDeviceProperties,
+                   weak_ptr_factory_.GetWeakPtr(), devices, index + 1),
+        base::Bind(&DiskMountManagerImpl::RefreshCompleted,
+                   weak_ptr_factory_.GetWeakPtr(), false));
+  }
+
+  // Part of EnsureMountInfoRefreshed().
+  void RefreshAfterGetDeviceProperties(const std::vector<std::string>& devices,
+                                       size_t next_index,
+                                       const DiskInfo& disk_info) {
+    OnGetDeviceProperties(disk_info);
+    RefreshDeviceAtIndex(devices, next_index);
+  }
+
+  // Part of EnsureMountInfoRefreshed(). Called after mount entries are listed.
+  void RefreshAfterEnumerateMountEntries(
+      const std::vector<MountEntry>& entries) {
+    for (size_t i = 0; i < entries.size(); ++i)
+      OnMountCompleted(entries[i]);
+    RefreshCompleted(true);
+  }
+
+  // Part of EnsureMountInfoRefreshed(). Called when the refreshing is done.
+  void RefreshCompleted(bool success) {
+    already_refreshed_ = true;
+    for (size_t i = 0; i < refresh_callbacks_.size(); ++i)
+      refresh_callbacks_[i].Run(success);
+    refresh_callbacks_.clear();
   }
 
   // Callback to handle mount event signals.
@@ -580,6 +625,9 @@ class DiskMountManagerImpl : public DiskMountManager {
   typedef std::set<std::string> SystemPathPrefixSet;
   SystemPathPrefixSet system_path_prefixes_;
 
+  bool already_refreshed_;
+  std::vector<EnsureMountInfoRefreshedCallback> refresh_callbacks_;
+
   base::WeakPtrFactory<DiskMountManagerImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DiskMountManagerImpl);

chromeos/disks/disk_mount_manager.h

@@ -198,6 +198,9 @@ class CHROMEOS_EXPORT DiskMountManager {
   // A callback type for UnmountPath method.
   typedef base::Callback<void(MountError error_code)> UnmountPathCallback;
 
+  // A callback type for EnsureMountInfoRefreshed method.
+  typedef base::Callback<void(bool success)> EnsureMountInfoRefreshedCallback;
+
   // Implement this interface to be notified about disk/mount related events.
   class Observer {
    public:
@@ -236,8 +239,11 @@ class CHROMEOS_EXPORT DiskMountManager {
   // Gets the list of mount points.
   virtual const MountPointMap& mount_points() const = 0;
 
-  // Requests refreshing all the information about mounted disks.
-  virtual void RequestMountInfoRefresh() = 0;
+  // Refreshes all the information about mounting if it is not yet done and
+  // invokes |callback| when finished. If the information is already refreshed
+  // It just runs |callback| immediately.
+  virtual void EnsureMountInfoRefreshed(
+      const EnsureMountInfoRefreshedCallback& callback) = 0;
 
   // Mounts a device.
   // Note that the mount operation may fail. To find out the result, one should

chromeos/disks/mock_disk_mount_manager.cc

@@ -58,6 +58,9 @@ MockDiskMountManager::MockDiskMountManager() {
   ON_CALL(*this, FindDiskBySourcePath(_))
       .WillByDefault(Invoke(
           this, &MockDiskMountManager::FindDiskBySourcePathInternal));
+  ON_CALL(*this, EnsureMountInfoRefreshed(_))
+      .WillByDefault(Invoke(
+          this, &MockDiskMountManager::EnsureMountInfoRefreshedInternal));
 }
 
 MockDiskMountManager::~MockDiskMountManager() {
@@ -162,7 +165,7 @@ void MockDiskMountManager::SetupDefaultReplies() {
       .WillRepeatedly(ReturnRef(mount_points_));
   EXPECT_CALL(*this, FindDiskBySourcePath(_))
       .Times(AnyNumber());
-  EXPECT_CALL(*this, RequestMountInfoRefresh())
+  EXPECT_CALL(*this, EnsureMountInfoRefreshed(_))
       .Times(AnyNumber());
   EXPECT_CALL(*this, MountPath(_, _, _, _))
       .Times(AnyNumber());
@@ -234,6 +237,11 @@ MockDiskMountManager::FindDiskBySourcePathInternal(
   return disk_it == disks_.end() ? NULL : disk_it->second;
 }
 
+void MockDiskMountManager::EnsureMountInfoRefreshedInternal(
+    const EnsureMountInfoRefreshedCallback& callback) {
+  callback.Run(true);
+}
+
 void MockDiskMountManager::NotifyDiskChanged(
     DiskEvent event,
     const DiskMountManager::Disk* disk) {

chromeos/disks/mock_disk_mount_manager.h

@@ -32,7 +32,8 @@ class MockDiskMountManager : public DiskMountManager {
                      const DiskMountManager::Disk*(const std::string&));
   MOCK_CONST_METHOD0(mount_points,
                      const DiskMountManager::MountPointMap&(void));
-  MOCK_METHOD0(RequestMountInfoRefresh, void(void));
+  MOCK_METHOD1(EnsureMountInfoRefreshed,
+               void(const EnsureMountInfoRefreshedCallback&));
   MOCK_METHOD4(MountPath, void(const std::string&, const std::string&,
                                const std::string&, MountType));
   MOCK_METHOD3(UnmountPath, void(const std::string&,
@@ -88,6 +89,10 @@ class MockDiskMountManager : public DiskMountManager {
   const DiskMountManager::Disk* FindDiskBySourcePathInternal(
       const std::string& source_path) const;
 
+  // Is used to implement EnsureMountInfoRefreshed.
+  void EnsureMountInfoRefreshedInternal(
+      const EnsureMountInfoRefreshedCallback& callback);
+
   // Notifies observers about device status update.
   void NotifyDeviceChanged(DeviceEvent event,
                            const std::string& path);