Use base::Callback for UnmountDeviceRecursivelyCallbackType.

There are two main goals of this refactoring.
1) Standardize the async implementation in BurnLibrary.
This is for the preparation of migration between BurnLibrary and BurnManager.
2) Fix potential (but probably quite rare) crash issue.
The crash would happen if the instance of BurnLibrary is deleted during
UnmountDeviceRecursively.
This CL is a kind of small refactoring, so the method body of
UnmountDeviceRecursive and its name are cleaned a bit,
as well as comments about potential issues with TODO related to the target
method are added.

BUG=126979
TEST=Ran try server.


Review URL: https://chromiumcodereview.appspot.com/12255072

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

4 files changed, 68 insertions, 57 deletions

diff --git a/chromeos/disks/disk_mount_manager.cc b/chromeos/disks/disk_mount_manager.cc
index 448bcb7..1d018b5 100644
--- a/chromeos/disks/disk_mount_manager.cc
+++ b/chromeos/disks/disk_mount_manager.cc
@@ -123,12 +123,9 @@ class DiskMountManagerImpl : public DiskMountManager {
   }
 
   // DiskMountManager override.
-  virtual void UnmountDeviceRecursive(
+  virtual void UnmountDeviceRecursively(
       const std::string& device_path,
-      UnmountDeviceRecursiveCallbackType callback,
-      void* user_data) OVERRIDE {
-    bool success = true;
-    std::string error_message;
+      const UnmountDeviceRecursivelyCallbackType& callback) OVERRIDE {
     std::vector<std::string> devices_to_unmount;
 
     // Get list of all devices to unmount.
@@ -140,36 +137,46 @@ class DiskMountManagerImpl : public DiskMountManager {
         devices_to_unmount.push_back(it->second->mount_path());
       }
     }
+
     // We should detect at least original device.
     if (devices_to_unmount.empty()) {
       if (disks_.find(device_path) == disks_.end()) {
-        success = false;
-        error_message = kDeviceNotFound;
-      } else {
-        // Nothing to unmount.
-        callback(user_data, true);
+        LOG(WARNING) << "Unmount recursive request failed for device "
+                     << device_path << ", with error: " << kDeviceNotFound;
+        callback.Run(false);
         return;
       }
+
+      // Nothing to unmount.
+      callback.Run(true);
+      return;
     }
-    if (success) {
-      // We will send the same callback data object to all Unmount calls and use
-      // it to syncronize callbacks.
-      UnmountDeviceRecursiveCallbackData* cb_data =
-          new UnmountDeviceRecursiveCallbackData(user_data, callback,
-                                                 devices_to_unmount.size());
-      for (size_t i = 0; i < devices_to_unmount.size(); ++i) {
-        cros_disks_client_->Unmount(
-            devices_to_unmount[i],
-            UNMOUNT_OPTIONS_NONE,
-            base::Bind(&DiskMountManagerImpl::OnUnmountDeviceRecursive,
-                       weak_ptr_factory_.GetWeakPtr(), cb_data, true),
-            base::Bind(&DiskMountManagerImpl::OnUnmountDeviceRecursive,
-                       weak_ptr_factory_.GetWeakPtr(), cb_data, false));
-      }
-    } else {
-      LOG(WARNING) << "Unmount recursive request failed for device "
-                   << device_path << ", with error: " << error_message;
-      callback(user_data, false);
+
+    // We will send the same callback data object to all Unmount calls and use
+    // it to syncronize 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());
+    for (size_t i = 0; i < devices_to_unmount.size(); ++i) {
+      cros_disks_client_->Unmount(
+          devices_to_unmount[i],
+          UNMOUNT_OPTIONS_NONE,
+          base::Bind(&DiskMountManagerImpl::OnUnmountDeviceRecursively,
+                     weak_ptr_factory_.GetWeakPtr(), cb_data, true),
+          base::Bind(&DiskMountManagerImpl::OnUnmountDeviceRecursively,
+                     weak_ptr_factory_.GetWeakPtr(), cb_data, false));
     }
   }
 
@@ -226,18 +233,16 @@ class DiskMountManagerImpl : public DiskMountManager {
   }
 
  private:
-  struct UnmountDeviceRecursiveCallbackData {
-    void* user_data;
-    UnmountDeviceRecursiveCallbackType callback;
-    size_t pending_callbacks_count;
-
-    UnmountDeviceRecursiveCallbackData(void* ud,
-                                       UnmountDeviceRecursiveCallbackType cb,
-                                       int count)
-        : user_data(ud),
-          callback(cb),
-          pending_callbacks_count(count) {
+  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;
   };
 
   // Unmounts all mount points whose source path is transitively parented by
@@ -258,21 +263,25 @@ class DiskMountManagerImpl : public DiskMountManager {
     }
   }
 
-  // Callback for UnmountDeviceRecursive.
-  void OnUnmountDeviceRecursive(UnmountDeviceRecursiveCallbackData* cb_data,
-                                bool success,
-                                const std::string& mount_path) {
+  // Callback for UnmountDeviceRecursively.
+  void OnUnmountDeviceRecursively(
+      UnmountDeviceRecursivelyCallbackData* cb_data,
+      bool success,
+      const std::string& mount_path) {
     if (success) {
       // Do standard processing for Unmount event.
       OnUnmountPath(true, mount_path);
       LOG(INFO) << mount_path <<  " unmounted.";
     }
     // This is safe as long as all callbacks are called on the same thread as
-    // UnmountDeviceRecursive.
-    cb_data->pending_callbacks_count--;
-
-    if (cb_data->pending_callbacks_count == 0) {
-      cb_data->callback(cb_data->user_data, success);
+    // 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;
     }
   }
diff --git a/chromeos/disks/disk_mount_manager.h b/chromeos/disks/disk_mount_manager.h
index 895c607..842b6f5 100644
--- a/chromeos/disks/disk_mount_manager.h
+++ b/chromeos/disks/disk_mount_manager.h
@@ -7,6 +7,7 @@
 
 #include <map>
 
+#include "base/callback_forward.h"
 #include "chromeos/chromeos_export.h"
 #include "chromeos/dbus/cros_disks_client.h"
 
@@ -190,9 +191,9 @@ class CHROMEOS_EXPORT DiskMountManager {
   // MountPointMap key is mount_path.
   typedef std::map<std::string, MountPointInfo> MountPointMap;
 
-  // A callback function type which is called after UnmountDeviceRecursive
+  // A callback function type which is called after UnmountDeviceRecursively
   // finishes.
-  typedef void(*UnmountDeviceRecursiveCallbackType)(void*, bool);
+  typedef base::Callback<void(bool)> UnmountDeviceRecursivelyCallbackType;
 
   // Implement this interface to be notified about disk/mount related events.
   class Observer {
@@ -251,10 +252,9 @@ class CHROMEOS_EXPORT DiskMountManager {
   virtual void FormatMountedDevice(const std::string& mount_path) = 0;
 
   // Unmounts device_path and all of its known children.
-  virtual void UnmountDeviceRecursive(
+  virtual void UnmountDeviceRecursively(
       const std::string& device_path,
-      UnmountDeviceRecursiveCallbackType callback,
-      void* user_data) = 0;
+      const UnmountDeviceRecursivelyCallbackType& callback) = 0;
 
   // Used in tests to initialize the manager's disk and mount point sets.
   // Default implementation does noting. It just fails.
diff --git a/chromeos/disks/mock_disk_mount_manager.cc b/chromeos/disks/mock_disk_mount_manager.cc
index 6657368..f8fc5e2 100644
--- a/chromeos/disks/mock_disk_mount_manager.cc
+++ b/chromeos/disks/mock_disk_mount_manager.cc
@@ -170,7 +170,7 @@ void MockDiskMountManager::SetupDefaultReplies() {
       .Times(AnyNumber());
   EXPECT_CALL(*this, FormatMountedDevice(_))
       .Times(AnyNumber());
-  EXPECT_CALL(*this, UnmountDeviceRecursive(_, _, _))
+  EXPECT_CALL(*this, UnmountDeviceRecursively(_, _))
       .Times(AnyNumber());
 }
 
diff --git a/chromeos/disks/mock_disk_mount_manager.h b/chromeos/disks/mock_disk_mount_manager.h
index aff88b3..1b13c47 100644
--- a/chromeos/disks/mock_disk_mount_manager.h
+++ b/chromeos/disks/mock_disk_mount_manager.h
@@ -35,8 +35,10 @@ class MockDiskMountManager : public DiskMountManager {
                                const std::string&, MountType));
   MOCK_METHOD2(UnmountPath, void(const std::string&, UnmountOptions));
   MOCK_METHOD1(FormatMountedDevice, void(const std::string&));
-  MOCK_METHOD3(UnmountDeviceRecursive, void(const std::string&,
-      DiskMountManager::UnmountDeviceRecursiveCallbackType, void*));
+  MOCK_METHOD2(
+      UnmountDeviceRecursively,
+      void(const std::string&,
+           const DiskMountManager::UnmountDeviceRecursivelyCallbackType&));
 
   // Invokes fake device insert events.
   void NotifyDeviceInsertEvents();