Redesigned and refactored MTPDeviceMapService and MTPDeviceDelegate.

Previously, MTPDeviceDelegate classes were ref-counted. Since the MTPDeviceDelegate was ref-counted, the design was complicated and the ownership of the MTPDeviceDelegateImpl object was not clearly defined.

In this CL, 
(1) MTPDeviceDelegate supports weak pointers and is not ref-counted.
(2) MediaFileSystemRegistry manages ScopedMTPDeviceMapEntry object.
(3) ScopedMTPDeviceMapEntry manages the lifetime of MTPDeviceDelegateImpl object via MTPDeviceMapService class.
(4) ScopedMTPDeviceMapEntry adds an entry in MTPDeviceMapService when the MTP device is registered as a media gallery file system by an extension.
(5) ScopedMTPDeviceMapEntry removes an entry from MTPDeviceMapService, when the browser is in shutdown mode or when the last extension using the device delegate is destroyed or when the device is detached from the system or when the user revoke the device gallery permissions.
(7) FileSystemOperationContext stores a weak pointer of MTPDeviceDelegate object.
(8) DeviceMediaFileUtil validates the weak pointer before forwarding any file operation request to the MTPDeviceDelegateImpl.
(9) When an entry is removed from MTPDeviceMapService, the ownership of the |MTPDeviceDelegateImpl| is handed off to the |MTPDeviceDelegateImpl| object which takes care of deleting itself on the right thread.

BUG=144527
TEST=none

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

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

6 files changed, 67 insertions, 52 deletions

diff --git a/webkit/fileapi/file_system_operation_context.h b/webkit/fileapi/file_system_operation_context.h
index 067e0d8..9c9c59a 100644
--- a/webkit/fileapi/file_system_operation_context.h
+++ b/webkit/fileapi/file_system_operation_context.h
@@ -6,6 +6,7 @@
 #define WEBKIT_FILEAPI_FILE_SYSTEM_OPERATION_CONTEXT_H_
 
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "webkit/fileapi/file_system_context.h"
 #include "webkit/fileapi/media/mtp_device_file_system_config.h"
 #include "webkit/fileapi/task_runner_bound_observer_list.h"
@@ -38,12 +39,16 @@ class WEBKIT_STORAGE_EXPORT_PRIVATE FileSystemOperationContext {
   int64 allowed_bytes_growth() const { return allowed_bytes_growth_; }
 
 #if defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
-  void set_mtp_device_delegate(MTPDeviceDelegate* delegate) {
+  // Called on IO thread.
+  void set_mtp_device_delegate(
+      const base::WeakPtr<MTPDeviceDelegate>& delegate) {
     mtp_device_delegate_ = delegate;
   }
 
-  MTPDeviceDelegate* mtp_device_delegate() const {
-    return mtp_device_delegate_.get();
+  // Caller of this function should dereference the delegate only on media
+  // sequenced task runner thread.
+  base::WeakPtr<MTPDeviceDelegate> mtp_device_delegate() const {
+    return mtp_device_delegate_;
   }
 #endif
 
@@ -91,8 +96,9 @@ class WEBKIT_STORAGE_EXPORT_PRIVATE FileSystemOperationContext {
   UpdateObserverList update_observers_;
 
 #if defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
-  // Store the current mtp device delegate.
-  scoped_refptr<MTPDeviceDelegate> mtp_device_delegate_;
+  // The media transfer protocol (MTP) device delegate.
+  // Set on IO thread and dereferenced on media sequenced task runner thread.
+  base::WeakPtr<MTPDeviceDelegate> mtp_device_delegate_;
 #endif
 };
 
diff --git a/webkit/fileapi/isolated_mount_point_provider.cc b/webkit/fileapi/isolated_mount_point_provider.cc
index a11731d..62135a8 100644
--- a/webkit/fileapi/isolated_mount_point_provider.cc
+++ b/webkit/fileapi/isolated_mount_point_provider.cc
@@ -131,7 +131,8 @@ FileSystemOperation* IsolatedMountPointProvider::CreateFileSystemOperation(
         *error_code = base::PLATFORM_FILE_ERROR_NOT_FOUND;
       return NULL;
     }
-    operation_context->set_mtp_device_delegate(device_delegate);
+    operation_context->set_mtp_device_delegate(
+        device_delegate->GetAsWeakPtrOnIOThread());
     operation_context->set_task_runner(device_delegate->GetMediaTaskRunner());
     operation_context->set_media_path_filter(media_path_filter_.get());
   }
diff --git a/webkit/fileapi/media/device_media_file_util.cc b/webkit/fileapi/media/device_media_file_util.cc
index 6283bff..bf7cd40 100644
--- a/webkit/fileapi/media/device_media_file_util.cc
+++ b/webkit/fileapi/media/device_media_file_util.cc
@@ -63,7 +63,9 @@ PlatformFileError DeviceMediaFileUtil::GetFileInfo(
     const FileSystemURL& url,
     PlatformFileInfo* file_info,
     FilePath* platform_path) {
-  DCHECK(context->mtp_device_delegate());
+  if (!context->mtp_device_delegate().get())
+    return base::PLATFORM_FILE_ERROR_NOT_FOUND;
+
   PlatformFileError error =
       context->mtp_device_delegate()->GetFileInfo(url.path(), file_info);
   if (error != base::PLATFORM_FILE_OK)
@@ -80,7 +82,10 @@ scoped_ptr<FileSystemFileUtil::AbstractFileEnumerator>
         FileSystemOperationContext* context,
         const FileSystemURL& url,
         bool recursive) {
-  DCHECK(context->mtp_device_delegate());
+  if (!context->mtp_device_delegate().get()) {
+    return scoped_ptr<FileSystemFileUtil::AbstractFileEnumerator>(
+        new FileSystemFileUtil::EmptyFileEnumerator());
+  }
   return make_scoped_ptr(new FilteringFileEnumerator(
       context->mtp_device_delegate()->CreateFileEnumerator(url.path(),
                                                            recursive),
@@ -113,7 +118,9 @@ PlatformFileError DeviceMediaFileUtil::Truncate(
 bool DeviceMediaFileUtil::IsDirectoryEmpty(
     FileSystemOperationContext* context,
     const FileSystemURL& url) {
-  DCHECK(context->mtp_device_delegate());
+  if (!context->mtp_device_delegate().get())
+    return false;
+
   scoped_ptr<AbstractFileEnumerator> enumerator(
       CreateFileEnumerator(context, url, false));
   FilePath path;
@@ -161,7 +168,8 @@ base::PlatformFileError DeviceMediaFileUtil::CreateSnapshotFile(
   DCHECK(file_info);
   DCHECK(local_path);
   DCHECK(snapshot_policy);
-  DCHECK(context->mtp_device_delegate());
+  if (!context->mtp_device_delegate().get())
+    return base::PLATFORM_FILE_ERROR_NOT_FOUND;
 
   // We return a temporary file as a snapshot.
   *snapshot_policy = FileSystemFileUtil::kSnapshotFileTemporary;
diff --git a/webkit/fileapi/media/mtp_device_delegate.h b/webkit/fileapi/media/mtp_device_delegate.h
index 0fc2d77..a9bc764 100644
--- a/webkit/fileapi/media/mtp_device_delegate.h
+++ b/webkit/fileapi/media/mtp_device_delegate.h
@@ -6,8 +6,8 @@
 #define WEBKIT_FILEAPI_MEDIA_MTP_DEVICE_DELEGATE_H_
 
 #include "base/file_path.h"
-#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/platform_file.h"
 #include "base/sequenced_task_runner_helpers.h"
 #include "webkit/fileapi/file_system_file_util.h"
@@ -20,14 +20,11 @@ class Time;
 
 namespace fileapi {
 
-struct MTPDeviceDelegateDeleter;
-
-// Delegate for media transfer protocol (MTP_ device to perform media device
+// Delegate for media transfer protocol (MTP) device to perform media device
 // isolated file system operations. Class that implements this delegate does
-// the actual communication with the MTP device.
-class MTPDeviceDelegate
-    : public base::RefCountedThreadSafe<MTPDeviceDelegate,
-                                        MTPDeviceDelegateDeleter> {
+// the actual communication with the MTP device. ScopedMTPDeviceMapEntry class
+// manages the lifetime of the delegate via MTPDeviceMapService class.
+class MTPDeviceDelegate : public base::SupportsWeakPtr<MTPDeviceDelegate> {
  public:
   // Returns information about the given file path.
   virtual base::PlatformFileError GetFileInfo(
@@ -53,23 +50,24 @@ class MTPDeviceDelegate
   // Returns TaskRunner on which the operation is performed.
   virtual base::SequencedTaskRunner* GetMediaTaskRunner() = 0;
 
-  // Helper function to destruct the delegate object on UI thread.
-  virtual void DeleteOnCorrectThread() const = 0;
+  // Called when the
+  // (1) Browser application is in shutdown mode (or)
+  // (2) Last extension using this MTP device is destroyed (or)
+  // (3) Attached MTP device is removed (or)
+  // (4) User revoked the MTP device gallery permission.
+  // Ownership of |MTPDeviceDelegate| is handed off to the delegate
+  // implementation class by this call. This function should take care of
+  // deleting itself on the right thread. This function should cancel all the
+  // pending requests before posting any message to delete itself.
+  // Called on the IO thread.
+  virtual void CancelPendingTasksAndDeleteDelegate() = 0;
+
+  // Called on the IO thread.
+  virtual base::WeakPtr<MTPDeviceDelegate> GetAsWeakPtrOnIOThread() = 0;
 
  protected:
+  // Always destruct this object via CancelPendingTasksAndDeleteDelegate().
   virtual ~MTPDeviceDelegate() {}
-
- private:
-  friend struct MTPDeviceDelegateDeleter;
-  friend class base::DeleteHelper<MTPDeviceDelegate>;
-  friend class base::RefCountedThreadSafe<MTPDeviceDelegate,
-                                          MTPDeviceDelegateDeleter>;
-};
-
-struct MTPDeviceDelegateDeleter {
-  static void Destruct(const MTPDeviceDelegate* delegate) {
-    delegate->DeleteOnCorrectThread();
-  }
 };
 
 }  // namespace fileapi
diff --git a/webkit/fileapi/media/mtp_device_map_service.cc b/webkit/fileapi/media/mtp_device_map_service.cc
index 68bf3d4..e4284e1 100644
--- a/webkit/fileapi/media/mtp_device_map_service.cc
+++ b/webkit/fileapi/media/mtp_device_map_service.cc
@@ -20,11 +20,10 @@ MTPDeviceMapService* MTPDeviceMapService::GetInstance() {
 
 void MTPDeviceMapService::AddDelegate(
     const FilePath::StringType& device_location,
-    scoped_refptr<MTPDeviceDelegate> delegate) {
+    MTPDeviceDelegate* delegate) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(delegate.get());
+  DCHECK(delegate);
   DCHECK(!device_location.empty());
-
   if (ContainsKey(delegate_map_, device_location))
     return;
 
@@ -36,6 +35,7 @@ void MTPDeviceMapService::RemoveDelegate(
   DCHECK(thread_checker_.CalledOnValidThread());
   DelegateMap::iterator it = delegate_map_.find(device_location);
   DCHECK(it != delegate_map_.end());
+  it->second->CancelPendingTasksAndDeleteDelegate();
   delegate_map_.erase(it);
 }
 
@@ -53,7 +53,7 @@ MTPDeviceDelegate* MTPDeviceMapService::GetMTPDeviceDelegate(
 
   DelegateMap::const_iterator it = delegate_map_.find(device_location);
   DCHECK(it != delegate_map_.end());
-  return it->second.get();
+  return it->second;
 }
 
 MTPDeviceMapService::MTPDeviceMapService() {
diff --git a/webkit/fileapi/media/mtp_device_map_service.h b/webkit/fileapi/media/mtp_device_map_service.h
index 3002097..6849976 100644
--- a/webkit/fileapi/media/mtp_device_map_service.h
+++ b/webkit/fileapi/media/mtp_device_map_service.h
@@ -8,8 +8,8 @@
 #include <map>
 
 #include "base/file_path.h"
-#include "base/memory/ref_counted.h"
 #include "base/memory/singleton.h"
+#include "base/memory/weak_ptr.h"
 #include "base/threading/thread_checker.h"
 #include "webkit/storage/webkit_storage_export.h"
 
@@ -17,43 +17,45 @@ namespace fileapi {
 
 class MTPDeviceDelegate;
 
-// Helper class to manage media device delegates which can communicate with mtp
-// devices to complete media file system operations.
+// This class provides media transfer protocol (MTP) device delegate to
+// complete media file system operations. ScopedMTPDeviceMapEntry class
+// manages the device map entries. This class operates on the IO thread.
 class WEBKIT_STORAGE_EXPORT MTPDeviceMapService {
  public:
   static MTPDeviceMapService* GetInstance();
 
-  // Adds the media device delegate for the given |device_location|. Called on
-  // IO thread.
+  // Adds the MTP device delegate to the map service. |device_location|
+  // specifies the mount location of the MTP device.
+  // Called on the IO thread.
   void AddDelegate(const FilePath::StringType& device_location,
-                   scoped_refptr<MTPDeviceDelegate> delegate);
+                   MTPDeviceDelegate* delegate);
 
-  // Removes the media device delegate for the given |device_location| if
-  // exists. Called on IO thread.
+  // Removes the MTP device delegate from the map service. |device_location|
+  // specifies the mount location of the MTP device.
+  // Called on the IO thread.
   void RemoveDelegate(const FilePath::StringType& device_location);
 
   // Gets the media device delegate associated with |filesystem_id|.
   // Return NULL if the |filesystem_id| is no longer valid (e.g. because the
-  // corresponding device is detached etc). Called on IO thread.
+  // corresponding device is detached etc).
+  // Called on the IO thread.
   MTPDeviceDelegate* GetMTPDeviceDelegate(const std::string& filesystem_id);
 
  private:
   friend struct DefaultSingletonTraits<MTPDeviceMapService>;
 
-  typedef scoped_refptr<MTPDeviceDelegate> MTPDeviceDelegateObj;
-
-  // Mapping of device_location and MTPDeviceDelegate object.
-  typedef std::map<FilePath::StringType, MTPDeviceDelegateObj> DelegateMap;
+  // Mapping of device_location and MTPDeviceDelegate* object. It is safe to
+  // store and access the raw pointer. This class operates on the IO thread.
+  typedef std::map<FilePath::StringType, MTPDeviceDelegate*> DelegateMap;
 
   // Get access to this class using GetInstance() method.
   MTPDeviceMapService();
   ~MTPDeviceMapService();
 
-  // Stores a map of attached mtp device delegates.
+  // Map of attached mtp device delegates.
   DelegateMap delegate_map_;
 
-  // Stores a |thread_checker_| object to verify all methods of this class are
-  // called on same thread.
+  // Object to verify all methods of this class are called on the same thread.
   base::ThreadChecker thread_checker_;
 
   DISALLOW_COPY_AND_ASSIGN(MTPDeviceMapService);