Fix a bug that unmounting devices on the policy update can be a busy loop.

TEST=unit_tests passed
BUG=643085

Review-Url: https://codereview.chromium.org/2348813002
Cr-Commit-Position: refs/heads/master@{#421163}

chrome/browser/chromeos/file_manager/fake_disk_mount_manager.cc

@@ -105,7 +105,19 @@ void FakeDiskMountManager::UnmountPath(const std::string& mount_path,
                     OnMountEvent(DiskMountManager::UNMOUNTING,
                                  chromeos::MOUNT_ERROR_NONE,
                                  mount_point));
-  // Currently |callback| is just ignored.
+  // Enqueue callback so that |FakeDiskMountManager::FinishAllUnmountRequest()|
+  // can call them.
+  pending_unmount_callbacks_.push(callback);
+}
+
+bool FakeDiskMountManager::FinishAllUnmountPathRequests() {
+  if (pending_unmount_callbacks_.empty())
+    return false;
+  while (!pending_unmount_callbacks_.empty()) {
+    pending_unmount_callbacks_.front().Run(chromeos::MOUNT_ERROR_NONE);
+    pending_unmount_callbacks_.pop();
+  }
+  return true;
 }
 
 void FakeDiskMountManager::FormatMountedDevice(const std::string& mount_path) {

chrome/browser/chromeos/file_manager/fake_disk_mount_manager.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_CHROMEOS_FILE_MANAGER_FAKE_DISK_MOUNT_MANAGER_H_
 #define CHROME_BROWSER_CHROMEOS_FILE_MANAGER_FAKE_DISK_MOUNT_MANAGER_H_
 
+#include <queue>
 #include <string>
 #include <vector>
 
@@ -51,6 +52,11 @@ class FakeDiskMountManager : public chromeos::disks::DiskMountManager {
     return unmount_requests_;
   }
 
+  // Emulates that all mount request finished.
+  // Return true if there was one or more mount request enqueued, or false
+  // otherwise.
+  bool FinishAllUnmountPathRequests();
+
   // DiskMountManager overrides.
   void AddObserver(Observer* observer) override;
   void RemoveObserver(Observer* observer) override;
@@ -66,6 +72,9 @@ class FakeDiskMountManager : public chromeos::disks::DiskMountManager {
                  const std::string& mount_label,
                  chromeos::MountType type,
                  chromeos::MountAccessMode access_mode) override;
+  // In order to simulate asynchronous invocation of callbacks after unmount
+  // is finished, |callback| will be invoked only when
+  // |FinishAllUnmountRequest()| is called.
   void UnmountPath(const std::string& mount_path,
                    chromeos::UnmountOptions options,
                    const UnmountPathCallback& callback) override;
@@ -80,6 +89,7 @@ class FakeDiskMountManager : public chromeos::disks::DiskMountManager {
 
  private:
   base::ObserverList<Observer> observers_;
+  std::queue<UnmountPathCallback> pending_unmount_callbacks_;
 
   DiskMap disks_;
   MountPointMap mount_points_;

chrome/browser/chromeos/file_manager/volume_manager.cc

@@ -682,6 +682,20 @@ void VolumeManager::OnProvidedFileSystemUnmount(
   DoUnmountEvent(mount_error, volume);
 }
 
+void VolumeManager::OnExternalStorageDisabledChangedUnmountCallback(
+    chromeos::MountError error_code) {
+  if (disk_mount_manager_->mount_points().empty())
+    return;
+  // Repeat until unmount all paths
+  const std::string& mount_path =
+      disk_mount_manager_->mount_points().begin()->second.mount_path;
+  disk_mount_manager_->UnmountPath(
+      mount_path, chromeos::UNMOUNT_OPTIONS_NONE,
+      base::Bind(
+          &VolumeManager::OnExternalStorageDisabledChangedUnmountCallback,
+          weak_ptr_factory_.GetWeakPtr()));
+}
+
 void VolumeManager::OnExternalStorageDisabledChanged() {
   // If the policy just got disabled we have to unmount every device currently
   // mounted. The opposite is fine - we can let the user re-plug their device to
@@ -690,14 +704,15 @@ void VolumeManager::OnExternalStorageDisabledChanged() {
     // We do not iterate on mount_points directly, because mount_points can
     // be changed by UnmountPath().
     // TODO(hidehiko): Is it necessary to unmount mounted archives, too, here?
-    while (!disk_mount_manager_->mount_points().empty()) {
-      std::string mount_path =
-          disk_mount_manager_->mount_points().begin()->second.mount_path;
-      disk_mount_manager_->UnmountPath(
-          mount_path,
-          chromeos::UNMOUNT_OPTIONS_NONE,
-          chromeos::disks::DiskMountManager::UnmountPathCallback());
-    }
+    if (disk_mount_manager_->mount_points().empty())
+      return;
+    const std::string& mount_path =
+        disk_mount_manager_->mount_points().begin()->second.mount_path;
+    disk_mount_manager_->UnmountPath(
+        mount_path, chromeos::UNMOUNT_OPTIONS_NONE,
+        base::Bind(
+            &VolumeManager::OnExternalStorageDisabledChangedUnmountCallback,
+            weak_ptr_factory_.GetWeakPtr()));
   }
 }
 

chrome/browser/chromeos/file_manager/volume_manager.h

@@ -301,6 +301,8 @@ class VolumeManager : public KeyedService,
                     const linked_ptr<Volume>& volume);
   void DoUnmountEvent(chromeos::MountError error_code,
                       const linked_ptr<Volume>& volume);
+  void OnExternalStorageDisabledChangedUnmountCallback(
+      chromeos::MountError error_code);
 
   Profile* profile_;
   drive::DriveIntegrationService* drive_integration_service_;  // Not owned.

chrome/browser/chromeos/file_manager/volume_manager_unittest.cc

@@ -722,6 +722,10 @@ TEST_F(VolumeManagerTest, OnExternalStorageDisabledChanged) {
   profile()->GetPrefs()->SetBoolean(prefs::kExternalStorageDisabled, true);
   volume_manager()->OnExternalStorageDisabledChanged();
 
+  // Wait until all unmount request finishes, so that callback chain to unmount
+  // all the mount points will be invoked.
+  disk_mount_manager_->FinishAllUnmountPathRequests();
+
   // The all mount points should be unmounted.
   EXPECT_EQ(0U, disk_mount_manager_->mount_points().size());