Files: Only unmount external media when enabling ExternalStorageDisabled

The policy is intended to unmount external media such as USB drives and
SD cards. However, cros-disks is now used to mount other types of file
systems such as DriveFS and sshfs for crostini. These don't count as
"external" and are covered by other policies.

Also, if an unmount fails, Chrome will keep trying to unmount the
device. Fix this by generating a list of mount points and iterating
through that list.

BUG=947375

Change-Id: I8a0813daa863e9f88e8cb1fc07cf8b7fc157a25d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1545956
Commit-Queue: Anand Mistry <amistry@chromium.org>
Reviewed-by: Tatsuhisa Yamaguchi <yamaguchi@chromium.org>
Reviewed-by: Sergei Datsenko <dats@chromium.org>
Cr-Commit-Position: refs/heads/master@{#647127}

chrome/browser/chromeos/file_manager/fake_disk_mount_manager.cc

@@ -6,6 +6,7 @@
 
 #include <utility>
 
+#include "base/bind.h"
 #include "chromeos/disks/disk.h"
 
 namespace file_manager {
@@ -103,20 +104,30 @@ void FakeDiskMountManager::UnmountPath(const std::string& mount_path,
                                        UnmountPathCallback callback) {
   unmount_requests_.emplace_back(mount_path, options);
 
-  MountPointMap::iterator iter = mount_points_.find(mount_path);
-  if (iter == mount_points_.end())
-    return;
-
-  const MountPointInfo mount_point = iter->second;
-  mount_points_.erase(iter);
-  for (auto& observer : observers_) {
-    observer.OnMountEvent(DiskMountManager::UNMOUNTING,
-                          chromeos::MOUNT_ERROR_NONE, mount_point);
+  chromeos::MountError error = chromeos::MOUNT_ERROR_NONE;
+  auto unmount_iter = unmount_errors_.find(mount_path);
+  if (unmount_iter != unmount_errors_.end()) {
+    error = unmount_iter->second;
+    unmount_errors_.erase(unmount_iter);
+  } else {
+    MountPointMap::iterator iter = mount_points_.find(mount_path);
+    if (iter == mount_points_.end())
+      return;
+
+    const MountPointInfo mount_point = iter->second;
+    mount_points_.erase(iter);
+    for (auto& observer : observers_) {
+      observer.OnMountEvent(DiskMountManager::UNMOUNTING,
+                            chromeos::MOUNT_ERROR_NONE, mount_point);
+    }
   }
 
   // Enqueue callback so that |FakeDiskMountManager::FinishAllUnmountRequest()|
   // can call them.
-  pending_unmount_callbacks_.push(std::move(callback));
+  if (callback) {
+    // Some tests pass a null |callback|.
+    pending_unmount_callbacks_.push(base::BindOnce(std::move(callback), error));
+  }
 }
 
 void FakeDiskMountManager::RemountAllRemovableDrives(
@@ -129,13 +140,17 @@ bool FakeDiskMountManager::FinishAllUnmountPathRequests() {
     return false;
 
   while (!pending_unmount_callbacks_.empty()) {
-    std::move(pending_unmount_callbacks_.front())
-        .Run(chromeos::MOUNT_ERROR_NONE);
+    std::move(pending_unmount_callbacks_.front()).Run();
     pending_unmount_callbacks_.pop();
   }
   return true;
 }
 
+void FakeDiskMountManager::FailUnmountRequest(const std::string& mount_path,
+                                              chromeos::MountError error_code) {
+  unmount_errors_[mount_path] = error_code;
+}
+
 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 <map>
 #include <memory>
 #include <string>
 #include <vector>
@@ -70,6 +71,10 @@ class FakeDiskMountManager : public chromeos::disks::DiskMountManager {
   // otherwise.
   bool FinishAllUnmountPathRequests();
 
+  // Fails a future unmount request for |mount_path| with |error_code|.
+  void FailUnmountRequest(const std::string& mount_path,
+                          chromeos::MountError error_code);
+
   // DiskMountManager overrides.
   void AddObserver(Observer* observer) override;
   void RemoveObserver(Observer* observer) override;
@@ -107,7 +112,7 @@ class FakeDiskMountManager : public chromeos::disks::DiskMountManager {
 
  private:
   base::ObserverList<Observer>::Unchecked observers_;
-  base::queue<UnmountPathCallback> pending_unmount_callbacks_;
+  base::queue<base::OnceClosure> pending_unmount_callbacks_;
 
   DiskMap disks_;
   MountPointMap mount_points_;
@@ -115,6 +120,7 @@ class FakeDiskMountManager : public chromeos::disks::DiskMountManager {
   std::vector<MountRequest> mount_requests_;
   std::vector<UnmountRequest> unmount_requests_;
   std::vector<RemountAllRequest> remount_all_requests_;
+  std::map<std::string, chromeos::MountError> unmount_errors_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeDiskMountManager);
 };

chrome/browser/chromeos/file_manager/volume_manager.cc

@@ -18,6 +18,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -1050,17 +1051,28 @@ void VolumeManager::OnProvidedFileSystemUnmount(
 }
 
 void VolumeManager::OnExternalStorageDisabledChangedUnmountCallback(
+    std::vector<std::string> remaining_mount_paths,
     chromeos::MountError error_code) {
-  if (disk_mount_manager_->mount_points().empty())
+  LOG_IF(ERROR, error_code != chromeos::MOUNT_ERROR_NONE)
+      << "Unmount on ExternalStorageDisabled policy change failed: "
+      << error_code;
+
+  while (!remaining_mount_paths.empty()) {
+    std::string mount_path = remaining_mount_paths.back();
+    remaining_mount_paths.pop_back();
+    if (!base::ContainsKey(disk_mount_manager_->mount_points(), mount_path)) {
+      // The mount point could have already been removed for another reason
+      // (i.e. the disk was removed by the user).
+      continue;
+    }
+
+    disk_mount_manager_->UnmountPath(
+        mount_path, chromeos::UNMOUNT_OPTIONS_NONE,
+        base::BindOnce(
+            &VolumeManager::OnExternalStorageDisabledChangedUnmountCallback,
+            weak_ptr_factory_.GetWeakPtr(), std::move(remaining_mount_paths)));
     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::BindOnce(
-          &VolumeManager::OnExternalStorageDisabledChangedUnmountCallback,
-          weak_ptr_factory_.GetWeakPtr()));
+  }
 }
 
 void VolumeManager::OnArcPlayStoreEnabledChanged(bool enabled) {
@@ -1107,17 +1119,26 @@ void VolumeManager::OnExternalStorageDisabledChanged() {
   // make it available.
   if (profile_->GetPrefs()->GetBoolean(prefs::kExternalStorageDisabled)) {
     // 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?
-    if (disk_mount_manager_->mount_points().empty())
+    // be changed by UnmountPath(). Also, a failing unmount shouldn't be retried
+    // indefinitely. So make a set of all the mount points that should be
+    // unmounted (all external media mounts), and iterate through them.
+    std::vector<std::string> remaining_mount_paths;
+    for (auto& mount_point : disk_mount_manager_->mount_points()) {
+      if (mount_point.second.mount_type == chromeos::MOUNT_TYPE_DEVICE) {
+        remaining_mount_paths.push_back(mount_point.first);
+      }
+    }
+    if (remaining_mount_paths.empty()) {
       return;
-    const std::string& mount_path =
-        disk_mount_manager_->mount_points().begin()->second.mount_path;
+    }
+
+    std::string mount_path = remaining_mount_paths.back();
+    remaining_mount_paths.pop_back();
     disk_mount_manager_->UnmountPath(
         mount_path, chromeos::UNMOUNT_OPTIONS_NONE,
         base::BindOnce(
             &VolumeManager::OnExternalStorageDisabledChangedUnmountCallback,
-            weak_ptr_factory_.GetWeakPtr()));
+            weak_ptr_factory_.GetWeakPtr(), std::move(remaining_mount_paths)));
   }
 }
 

chrome/browser/chromeos/file_manager/volume_manager.h

@@ -433,6 +433,7 @@ class VolumeManager : public KeyedService,
                     std::unique_ptr<Volume> volume);
   void DoUnmountEvent(chromeos::MountError error_code, const Volume& volume);
   void OnExternalStorageDisabledChangedUnmountCallback(
+      std::vector<std::string> remaining_mount_paths,
       chromeos::MountError error_code);
 
   // Returns the path of the mount point for drive.

chrome/browser/chromeos/file_manager/volume_manager_unittest.cc

@@ -7,6 +7,7 @@
 #include <stddef.h>
 
 #include <memory>
+#include <set>
 #include <string>
 #include <utility>
 #include <vector>
@@ -14,6 +15,7 @@
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/chromeos/drive/file_system_util.h"
@@ -746,16 +748,24 @@ TEST_F(VolumeManagerTest, OnFormatEvent_CompletedFailed) {
 }
 
 TEST_F(VolumeManagerTest, OnExternalStorageDisabledChanged) {
-  // Here create two mount points.
+  // Here create four mount points.
   disk_mount_manager_->MountPath("mount1", "", "", {},
                                  chromeos::MOUNT_TYPE_DEVICE,
                                  chromeos::MOUNT_ACCESS_MODE_READ_WRITE);
   disk_mount_manager_->MountPath("mount2", "", "", {},
                                  chromeos::MOUNT_TYPE_DEVICE,
                                  chromeos::MOUNT_ACCESS_MODE_READ_ONLY);
+  disk_mount_manager_->MountPath("mount3", "", "", {},
+                                 chromeos::MOUNT_TYPE_NETWORK_STORAGE,
+                                 chromeos::MOUNT_ACCESS_MODE_READ_ONLY);
+  disk_mount_manager_->MountPath("failed_unmount", "", "", {},
+                                 chromeos::MOUNT_TYPE_DEVICE,
+                                 chromeos::MOUNT_ACCESS_MODE_READ_WRITE);
+  disk_mount_manager_->FailUnmountRequest("failed_unmount",
+                                          chromeos::MOUNT_ERROR_UNKNOWN);
 
-  // Initially, there are two mount points.
-  ASSERT_EQ(2U, disk_mount_manager_->mount_points().size());
+  // Initially, there are four mount points.
+  ASSERT_EQ(4U, disk_mount_manager_->mount_points().size());
   ASSERT_EQ(0U, disk_mount_manager_->unmount_requests().size());
 
   // Emulate to set kExternalStorageDisabled to false.
@@ -763,7 +773,7 @@ TEST_F(VolumeManagerTest, OnExternalStorageDisabledChanged) {
   volume_manager()->OnExternalStorageDisabledChanged();
 
   // Expect no effects.
-  EXPECT_EQ(2U, disk_mount_manager_->mount_points().size());
+  EXPECT_EQ(4U, disk_mount_manager_->mount_points().size());
   EXPECT_EQ(0U, disk_mount_manager_->unmount_requests().size());
 
   // Emulate to set kExternalStorageDisabled to true.
@@ -774,17 +784,21 @@ TEST_F(VolumeManagerTest, OnExternalStorageDisabledChanged) {
   // 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());
-
-  EXPECT_EQ(2U, disk_mount_manager_->unmount_requests().size());
-  const FakeDiskMountManager::UnmountRequest& unmount_request1 =
-      disk_mount_manager_->unmount_requests()[0];
-  EXPECT_EQ("mount1", unmount_request1.mount_path);
+  // The external media mount points should be unmounted. Other mount point
+  // types should remain. The failing unmount should also remain.
+  EXPECT_EQ(2U, disk_mount_manager_->mount_points().size());
 
-  const FakeDiskMountManager::UnmountRequest& unmount_request2 =
-      disk_mount_manager_->unmount_requests()[1];
-  EXPECT_EQ("mount2", unmount_request2.mount_path);
+  std::set<std::string> expected_unmount_requests = {
+      "mount1",
+      "mount2",
+      "failed_unmount",
+  };
+  for (const auto& request : disk_mount_manager_->unmount_requests()) {
+    EXPECT_TRUE(
+        base::ContainsKey(expected_unmount_requests, request.mount_path));
+    expected_unmount_requests.erase(request.mount_path);
+  }
+  EXPECT_TRUE(expected_unmount_requests.empty());
 }
 
 TEST_F(VolumeManagerTest, ExternalStorageDisabledPolicyMultiProfile) {