Return error code from UnmountDeviceRecursively.

At the same time, fix up two outstanding TODOs:
1. A potential memory leak due to lack of callback data ownership.
2. Not correctly reporting unmount errors on a multi-parition device.

Also add some tests for UnmountDeviceRecursively.

BUG=873903

Change-Id: I3e99afa4e0185b18c1d7077a5c65fc225a5751b3
Reviewed-on: https://chromium-review.googlesource.com/1179421Reviewed-by: Noel Gordon <noel@chromium.org>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584627}

chrome/browser/chromeos/file_manager/fake_disk_mount_manager.cc

@@ -142,8 +142,7 @@ void FakeDiskMountManager::RenameMountedDevice(const std::string& mount_path,
 
 void FakeDiskMountManager::UnmountDeviceRecursively(
     const std::string& device_path,
-    const UnmountDeviceRecursivelyCallbackType& callback) {
-}
+    UnmountDeviceRecursivelyCallbackType callback) {}
 
 bool FakeDiskMountManager::AddDiskForTest(
     std::unique_ptr<chromeos::disks::Disk> disk) {

chrome/browser/chromeos/file_manager/fake_disk_mount_manager.h

@@ -98,7 +98,7 @@ class FakeDiskMountManager : public chromeos::disks::DiskMountManager {
                            const std::string& volume_name) override;
   void UnmountDeviceRecursively(
       const std::string& device_path,
-      const UnmountDeviceRecursivelyCallbackType& callback) override;
+      UnmountDeviceRecursivelyCallbackType callback) override;
 
   bool AddDiskForTest(std::unique_ptr<chromeos::disks::Disk> disk) override;
   bool AddMountPointForTest(const MountPointInfo& mount_point) override;

chrome/browser/extensions/api/image_writer_private/operation.h

@@ -22,6 +22,10 @@
 #include "chrome/common/extensions/api/image_writer_private.h"
 #include "extensions/common/extension_id.h"
 
+#if defined(OS_CHROMEOS)
+#include "chromeos/disks/disk_mount_manager.h"
+#endif
+
 namespace image_writer_api = extensions::api::image_writer_private;
 
 namespace base {
@@ -183,7 +187,8 @@ class Operation : public base::RefCountedThreadSafe<Operation> {
   // Unmounts all volumes on |device_path_|.
   void UnmountVolumes(const base::Closure& continuation);
   // Starts the write after unmounting.
-  void UnmountVolumesCallback(const base::Closure& continuation, bool success);
+  void UnmountVolumesCallback(const base::Closure& continuation,
+                              chromeos::MountError error_code);
   // Starts the ImageBurner write.  Note that target_path is the file path of
   // the device where device_path has been a system device path.
   void StartWriteOnUIThread(const std::string& target_path,

chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc

@@ -62,11 +62,11 @@ void Operation::UnmountVolumes(const base::Closure& continuation) {
 }
 
 void Operation::UnmountVolumesCallback(const base::Closure& continuation,
-                                       bool success) {
+                                       chromeos::MountError error_code) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  if (!success) {
-    LOG(ERROR) << "Volume unmounting failed.";
+  if (error_code != chromeos::MOUNT_ERROR_NONE) {
+    LOG(ERROR) << "Volume unmounting failed with error code " << error_code;
     PostTask(base::Bind(&Operation::Error, this, error::kUnmountVolumesError));
     return;
   }

chrome/browser/extensions/api/image_writer_private/test_utils.cc

@@ -74,9 +74,10 @@ FakeDiskMountManager::~FakeDiskMountManager() {}
 
 void FakeDiskMountManager::UnmountDeviceRecursively(
     const std::string& device_path,
-    const UnmountDeviceRecursivelyCallbackType& callback) {
-  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
-                                                base::BindOnce(callback, true));
+    UnmountDeviceRecursivelyCallbackType callback) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(std::move(callback), chromeos::MOUNT_ERROR_NONE));
 }
 #endif
 

chrome/browser/extensions/api/image_writer_private/test_utils.h

@@ -70,7 +70,7 @@ class FakeDiskMountManager : public chromeos::disks::MockDiskMountManager {
 
   void UnmountDeviceRecursively(
       const std::string& device_path,
-      const UnmountDeviceRecursivelyCallbackType& callback) override;
+      UnmountDeviceRecursivelyCallbackType callback) override;
 
  private:
   DiskMap disks_;

chromeos/disks/disk_mount_manager.cc

@@ -13,6 +13,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/barrier_closure.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/macros.h"
@@ -35,6 +36,20 @@ constexpr char kDefaultFormatVFAT[] = "vfat";
 constexpr char kDeviceNotFound[] = "Device could not be found";
 DiskMountManager* g_disk_mount_manager = NULL;
 
+struct UnmountDeviceRecursivelyCallbackData {
+  UnmountDeviceRecursivelyCallbackData(
+      DiskMountManager::UnmountDeviceRecursivelyCallbackType in_callback)
+      : callback(std::move(in_callback)) {}
+
+  DiskMountManager::UnmountDeviceRecursivelyCallbackType callback;
+  MountError error_code = MOUNT_ERROR_NONE;
+};
+
+void OnAllUnmountDeviceRecursively(
+    std::unique_ptr<UnmountDeviceRecursivelyCallbackData> cb_data) {
+  std::move(cb_data->callback).Run(cb_data->error_code);
+}
+
 // The DiskMountManager implementation.
 class DiskMountManagerImpl : public DiskMountManager,
                              public CrosDisksClient::Observer {
@@ -179,7 +194,7 @@ class DiskMountManagerImpl : public DiskMountManager,
   // DiskMountManager override.
   void UnmountDeviceRecursively(
       const std::string& device_path,
-      const UnmountDeviceRecursivelyCallbackType& callback) override {
+      UnmountDeviceRecursivelyCallbackType callback) override {
     std::vector<std::string> devices_to_unmount;
 
     // Get list of all devices to unmount.
@@ -197,38 +212,29 @@ class DiskMountManagerImpl : public DiskMountManager,
       if (disks_.find(device_path) == disks_.end()) {
         LOG(WARNING) << "Unmount recursive request failed for device "
                      << device_path << ", with error: " << kDeviceNotFound;
-        callback.Run(false);
+        std::move(callback).Run(MOUNT_ERROR_INVALID_DEVICE_PATH);
         return;
       }
 
       // Nothing to unmount.
-      callback.Run(true);
+      std::move(callback).Run(MOUNT_ERROR_NONE);
       return;
     }
 
-    // We will send the same callback data object to all Unmount calls and use
-    // 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.
-    // It is because the UnmountDeviceRecursivelyCallbackData keeps how
-    // many times OnUnmountDeviceRecursively callback is called and when
-    // all the callbacks are called, |cb_data| will be deleted in the method.
-    // However destructing the instance before all callback invocations will
-    // cancel all pending callbacks, so that the |cb_data| would never be
-    // deleted.
-    // Fortunately, in the real scenario, the instance will be destructed
-    // only for ShutDown. So, probably the memory would rarely be leaked.
-    // TODO(hidehiko): Fix the issue.
-    UnmountDeviceRecursivelyCallbackData* cb_data =
-        new UnmountDeviceRecursivelyCallbackData(
-            callback, devices_to_unmount.size());
+    std::unique_ptr<UnmountDeviceRecursivelyCallbackData> cb_data =
+        std::make_unique<UnmountDeviceRecursivelyCallbackData>(
+            std::move(callback));
+    UnmountDeviceRecursivelyCallbackData* raw_cb_data = cb_data.get();
+    base::RepeatingClosure done_callback = base::BarrierClosure(
+        devices_to_unmount.size(),
+        base::BindOnce(&OnAllUnmountDeviceRecursively, std::move(cb_data)));
+
     for (size_t i = 0; i < devices_to_unmount.size(); ++i) {
       cros_disks_client_->Unmount(
           devices_to_unmount[i], UNMOUNT_OPTIONS_NONE,
           base::BindOnce(&DiskMountManagerImpl::OnUnmountDeviceRecursively,
-                         weak_ptr_factory_.GetWeakPtr(), cb_data,
-                         devices_to_unmount[i]));
+                         weak_ptr_factory_.GetWeakPtr(), raw_cb_data,
+                         devices_to_unmount[i], done_callback));
     }
   }
 
@@ -312,18 +318,6 @@ class DiskMountManagerImpl : public DiskMountManager,
   // device_path and the value is new volume_name.
   std::map<std::string, std::string> pending_rename_changes_;
 
-  struct UnmountDeviceRecursivelyCallbackData {
-    UnmountDeviceRecursivelyCallbackData(
-        const UnmountDeviceRecursivelyCallbackType& in_callback,
-        int in_num_pending_callbacks)
-        : callback(in_callback),
-          num_pending_callbacks(in_num_pending_callbacks) {
-    }
-
-    const UnmountDeviceRecursivelyCallbackType callback;
-    size_t num_pending_callbacks;
-  };
-
   // Called on D-Bus CrosDisksClient::Mount() is done.
   void OnMount(const std::string& source_path, MountType type, bool result) {
     // When succeeds, OnMountCompleted will be called by "MountCompleted",
@@ -388,24 +382,20 @@ class DiskMountManagerImpl : public DiskMountManager,
   // Callback for UnmountDeviceRecursively.
   void OnUnmountDeviceRecursively(UnmountDeviceRecursivelyCallbackData* cb_data,
                                   const std::string& mount_path,
+                                  base::OnceClosure done_callback,
                                   MountError error_code) {
-    bool success = error_code == MOUNT_ERROR_NONE;
-    if (success) {
+    if (error_code == MOUNT_ERROR_NONE) {
       // Do standard processing for Unmount event.
       OnUnmountPath(UnmountPathCallback(), mount_path, MOUNT_ERROR_NONE);
       VLOG(1) << mount_path <<  " unmounted.";
+    } else {
+      // This causes the last non-success error to be reported.
+      // TODO(amistry): We could ignore certain errors such as
+      // MOUNT_ERROR_PATH_NOT_MOUNTED, or prioritise more important ones.
+      cb_data->error_code = error_code;
     }
-    // This is safe as long as all callbacks are called on the same thread as
-    // UnmountDeviceRecursively.
-    cb_data->num_pending_callbacks--;
 
-    if (cb_data->num_pending_callbacks == 0) {
-      // This code has a problem that the |success| status used here is for the
-      // last "unmount" callback, but not whether all unmounting is succeeded.
-      // TODO(hidehiko): Fix the issue.
-      cb_data->callback.Run(success);
-      delete cb_data;
-    }
+    std::move(done_callback).Run();
   }
 
   // CrosDisksClient::Observer override.

chromeos/disks/disk_mount_manager.h

@@ -85,7 +85,8 @@ class CHROMEOS_EXPORT DiskMountManager {
 
   // A callback function type which is called after UnmountDeviceRecursively
   // finishes.
-  typedef base::Callback<void(bool)> UnmountDeviceRecursivelyCallbackType;
+  typedef base::OnceCallback<void(MountError error_code)>
+      UnmountDeviceRecursivelyCallbackType;
 
   // A callback type for UnmountPath method.
   typedef base::Callback<void(MountError error_code)> UnmountPathCallback;
@@ -186,7 +187,7 @@ class CHROMEOS_EXPORT DiskMountManager {
   // Unmounts device_path and all of its known children.
   virtual void UnmountDeviceRecursively(
       const std::string& device_path,
-      const UnmountDeviceRecursivelyCallbackType& callback) = 0;
+      UnmountDeviceRecursivelyCallbackType callback) = 0;
 
   // Used in tests to initialize the manager's disk and mount point sets.
   // Default implementation does noting. It just fails.

chromeos/disks/disk_mount_manager_unittest.cc

@@ -26,6 +26,7 @@ using chromeos::disks::DiskMountManager;
 using chromeos::CrosDisksClient;
 using chromeos::DBusThreadManager;
 using chromeos::FakeCrosDisksClient;
+using chromeos::MountError;
 using chromeos::MountType;
 using chromeos::disks::MountCondition;
 
@@ -554,7 +555,7 @@ class DiskMountManagerTest : public testing::Test {
   }
 
   // Adds a new mount point to the disk mount manager.
-  // If the moutn point is a device mount point, disk with its source path
+  // If the mount point is a device mount point, disk with its source path
   // should already be added to the disk mount manager.
   void AddTestMountPoint(const TestMountPointInfo& mount_point) {
     EXPECT_TRUE(DiskMountManager::GetInstance()->AddMountPointForTest(
@@ -1392,4 +1393,139 @@ TEST_F(DiskMountManagerTest, Rename_ConsecutiveRenameCalls) {
                                             kDevice1MountPath));
 }
 
+void SaveUnmountResult(MountError* save_error,
+                       base::OnceClosure done_callback,
+                       MountError error_code) {
+  *save_error = error_code;
+  std::move(done_callback).Run();
+}
+
+TEST_F(DiskMountManagerTest, UnmountDeviceRecursively) {
+  base::RunLoop run_loop;
+
+  auto disk_sda =
+      Disk::Builder().SetDevicePath("/dev/sda").SetIsParent(true).Build();
+  EXPECT_TRUE(
+      DiskMountManager::GetInstance()->AddDiskForTest(std::move(disk_sda)));
+
+  auto disk_sda1 = Disk::Builder()
+                       .SetDevicePath("/dev/sda1")
+                       .SetMountPath("/mount/path1")
+                       .Build();
+  EXPECT_TRUE(
+      DiskMountManager::GetInstance()->AddDiskForTest(std::move(disk_sda1)));
+
+  auto disk_sda2 = Disk::Builder()
+                       .SetDevicePath("/dev/sda2")
+                       .SetMountPath("/mount/path2")
+                       .Build();
+  EXPECT_TRUE(
+      DiskMountManager::GetInstance()->AddDiskForTest(std::move(disk_sda2)));
+
+  MountError error_code = chromeos::MOUNT_ERROR_UNKNOWN;
+  DiskMountManager::GetInstance()->UnmountDeviceRecursively(
+      "/dev/sda",
+      base::BindOnce(&SaveUnmountResult, base::Unretained(&error_code),
+                     run_loop.QuitClosure()));
+  run_loop.Run();
+
+  EXPECT_EQ(2, fake_cros_disks_client_->unmount_call_count());
+  EXPECT_EQ(chromeos::UNMOUNT_OPTIONS_NONE,
+            fake_cros_disks_client_->last_unmount_options());
+  EXPECT_EQ(chromeos::MOUNT_ERROR_NONE, error_code);
+}
+
+TEST_F(DiskMountManagerTest, UnmountDeviceRecursively_NoMounted) {
+  base::RunLoop run_loop;
+
+  auto disk_sda =
+      Disk::Builder().SetDevicePath("/dev/sda").SetIsParent(true).Build();
+  EXPECT_TRUE(
+      DiskMountManager::GetInstance()->AddDiskForTest(std::move(disk_sda)));
+
+  auto disk_sda1 = Disk::Builder().SetDevicePath("/dev/sda1").Build();
+  EXPECT_TRUE(
+      DiskMountManager::GetInstance()->AddDiskForTest(std::move(disk_sda1)));
+
+  MountError error_code = chromeos::MOUNT_ERROR_UNKNOWN;
+  DiskMountManager::GetInstance()->UnmountDeviceRecursively(
+      "/dev/sda",
+      base::BindOnce(&SaveUnmountResult, base::Unretained(&error_code),
+                     run_loop.QuitClosure()));
+  run_loop.Run();
+
+  EXPECT_EQ(0, fake_cros_disks_client_->unmount_call_count());
+  EXPECT_EQ(chromeos::MOUNT_ERROR_NONE, error_code);
+}
+
+TEST_F(DiskMountManagerTest, UnmountDeviceRecursively_NoDisk) {
+  base::RunLoop run_loop;
+
+  auto disk_sda =
+      Disk::Builder().SetDevicePath("/dev/sda").SetIsParent(true).Build();
+  EXPECT_TRUE(
+      DiskMountManager::GetInstance()->AddDiskForTest(std::move(disk_sda)));
+
+  auto disk_sda1 = Disk::Builder().SetDevicePath("/dev/sda1").Build();
+  EXPECT_TRUE(
+      DiskMountManager::GetInstance()->AddDiskForTest(std::move(disk_sda1)));
+
+  MountError error_code = chromeos::MOUNT_ERROR_UNKNOWN;
+  // Unmount sdB instead of sdA.
+  DiskMountManager::GetInstance()->UnmountDeviceRecursively(
+      "/dev/sdb",
+      base::BindOnce(&SaveUnmountResult, base::Unretained(&error_code),
+                     run_loop.QuitClosure()));
+  run_loop.Run();
+
+  EXPECT_EQ(0, fake_cros_disks_client_->unmount_call_count());
+  EXPECT_EQ(chromeos::MOUNT_ERROR_INVALID_DEVICE_PATH, error_code);
+}
+
+void SetUnmountError(FakeCrosDisksClient* client, MountError error_code) {
+  client->MakeUnmountFail(error_code);
+}
+
+TEST_F(DiskMountManagerTest, UnmountDeviceRecursively_FailFirst) {
+  base::RunLoop run_loop;
+
+  auto disk_sda =
+      Disk::Builder().SetDevicePath("/dev/sda").SetIsParent(true).Build();
+  EXPECT_TRUE(
+      DiskMountManager::GetInstance()->AddDiskForTest(std::move(disk_sda)));
+
+  auto disk_sda1 = Disk::Builder()
+                       .SetDevicePath("/dev/sda1")
+                       .SetMountPath("/mount/path1")
+                       .Build();
+  EXPECT_TRUE(
+      DiskMountManager::GetInstance()->AddDiskForTest(std::move(disk_sda1)));
+
+  auto disk_sda2 = Disk::Builder()
+                       .SetDevicePath("/dev/sda2")
+                       .SetMountPath("/mount/path2")
+                       .Build();
+  EXPECT_TRUE(
+      DiskMountManager::GetInstance()->AddDiskForTest(std::move(disk_sda2)));
+
+  // Fail the first unmount, but make the second succeed.
+  fake_cros_disks_client_->MakeUnmountFail(
+      chromeos::MOUNT_ERROR_INVALID_UNMOUNT_OPTIONS);
+  fake_cros_disks_client_->set_unmount_listener(
+      base::Bind(&SetUnmountError, base::Unretained(fake_cros_disks_client_),
+                 chromeos::MOUNT_ERROR_NONE));
+
+  MountError error_code = chromeos::MOUNT_ERROR_UNKNOWN;
+  DiskMountManager::GetInstance()->UnmountDeviceRecursively(
+      "/dev/sda",
+      base::BindOnce(&SaveUnmountResult, base::Unretained(&error_code),
+                     run_loop.QuitClosure()));
+  run_loop.Run();
+
+  EXPECT_EQ(2, fake_cros_disks_client_->unmount_call_count());
+  EXPECT_EQ(chromeos::UNMOUNT_OPTIONS_NONE,
+            fake_cros_disks_client_->last_unmount_options());
+  EXPECT_EQ(chromeos::MOUNT_ERROR_INVALID_UNMOUNT_OPTIONS, error_code);
+}
+
 }  // namespace

chromeos/disks/mock_disk_mount_manager.h

@@ -50,10 +50,9 @@ class MockDiskMountManager : public DiskMountManager {
   MOCK_METHOD1(FormatMountedDevice, void(const std::string&));
   MOCK_METHOD2(RenameMountedDevice,
                void(const std::string&, const std::string&));
-  MOCK_METHOD2(
-      UnmountDeviceRecursively,
+  MOCK_METHOD2(UnmountDeviceRecursively,
                void(const std::string&,
-           const DiskMountManager::UnmountDeviceRecursivelyCallbackType&));
+                    DiskMountManager::UnmountDeviceRecursivelyCallbackType));
 
   // Invokes fake device insert events.
   void NotifyDeviceInsertEvents();