Redesigned and refactored ScopedMTPDeviceMapEntry, MTPDeviceMapService & MTPDeviceDelegate classes.

chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc

Index: chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc
diff --git a/chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc b/chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc
index 5ae2ad28b0e0dedfe425a94b247855b84cd16ce0..3c8b19a3c2a585e2b75fc4c3995cf7f4f3353332 100644
--- a/chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc
+++ b/chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc
@@ -10,12 +10,11 @@
 #include "base/sequenced_task_runner.h"
 #include "base/sequenced_task_runner_helpers.h"
 #include "base/string_util.h"
+#include "base/synchronization/cancellation_flag.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "chrome/browser/media_transfer_protocol/media_transfer_protocol_manager.h"
 #include "chrome/browser/media_transfer_protocol/mtp_file_entry.pb.h"
-#include "chrome/common/chrome_notification_types.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/notification_service.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 using base::Bind;
@@ -68,6 +67,14 @@ MediaTransferProtocolManager* GetMediaTransferProtocolManager() {
 void DoNothing(bool error) {
 }
 
+// Closes the device storage on the UI thread.
+void CloseStorageOnUIThread(const std::string& device_handle) {
+  DCHECK(!device_handle.empty());
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  GetMediaTransferProtocolManager()->CloseStorage(device_handle,
+                                                  Bind(&DoNothing));
+}
+
 // Returns the device relative file path given |file_path|.
 // E.g.: If the |file_path| is "/usb:2,2:12345/DCIM" and |registered_dev_path|
 // is "/usb:2,2:12345", this function returns the device relative path which is
@@ -122,6 +129,9 @@ class OpenStorageWorker
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                             Bind(&OpenStorageWorker::DoWorkOnUIThread, this));
     on_task_completed_event_->Wait();
+
+    if (on_shutdown_event_->IsSignaled())
+      cancel_tasks_flag_.Set();
   }
 
   // Returns a device handle string if the OpenStorage() request was
@@ -150,6 +160,8 @@ class OpenStorageWorker
   // storage for communication. This is called on UI thread.
   void DoWorkOnUIThread() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    if (cancel_tasks_flag_.IsSet())
+      return;
 
     GetMediaTransferProtocolManager()->OpenStorage(
         storage_name_, mtpd::kReadOnlyMode,
@@ -163,7 +175,8 @@ class OpenStorageWorker
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     if (!error)
       device_handle_ = device_handle;
-    on_task_completed_event_->Signal();
+    if (!cancel_tasks_flag_.IsSet())

[Lei Zhang, 2012/11/27 00:13:01]

Can we check is the cancellation flag is set and return early - before
|device_handle_| gets set? That way, LazyInit() will fail rather than succeed
and go on to attempt to do more work.

[kmadhusu, 2012/11/27 01:44:03]

Done.

+      on_task_completed_event_->Signal();
   }
 
   // Stores the storage name to open the device.
@@ -185,6 +198,12 @@ class OpenStorageWorker
   // Stores the result of OpenStorage() request.
   std::string device_handle_;
 
+  // Set to ignore the request results. This will be set when
+  // MTPDeviceDelegateImplLinux object is about to be deleted.
+  // |on_task_completed_event_| and |on_shutdown_event_| should not be
+  // dereferenced when this is set.
+  base::CancellationFlag cancel_tasks_flag_;
+
   DISALLOW_COPY_AND_ASSIGN(OpenStorageWorker);
 };
 
@@ -220,6 +239,9 @@ class GetFileInfoWorker
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                             Bind(&GetFileInfoWorker::DoWorkOnUIThread, this));
     on_task_completed_event_->Wait();
+
+    if (on_shutdown_event_->IsSignaled())
+      cancel_tasks_flag_.Set();
   }
 
   // Returns GetFileInfo() result and fills in |file_info| with requested file
@@ -252,6 +274,8 @@ class GetFileInfoWorker
   // information.
   void DoWorkOnUIThread() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    if (cancel_tasks_flag_.IsSet())
+      return;
 
     GetMediaTransferProtocolManager()->GetFileInfoByPath(
         device_handle_, path_,
@@ -276,7 +300,8 @@ class GetFileInfoWorker
           base::Time::FromTimeT(file_entry.modification_time());
       file_entry_info_.creation_time = base::Time();
     }
-    on_task_completed_event_->Signal();
+    if (!cancel_tasks_flag_.IsSet())
+      on_task_completed_event_->Signal();
   }
 
   // Stores the device handle to query the device.
@@ -304,6 +329,12 @@ class GetFileInfoWorker
   // Stores a reference to waitable event associated with the shut down message.
   WaitableEvent* on_shutdown_event_;
 
+  // Set to ignore the request results. This will be set when
+  // MTPDeviceDelegateImplLinux object is about to be deleted.
+  // |on_task_completed_event_| and |on_shutdown_event_| should not be
+  // dereferenced when this is set.
+  base::CancellationFlag cancel_tasks_flag_;
+
   DISALLOW_COPY_AND_ASSIGN(GetFileInfoWorker);
 };
 
@@ -338,10 +369,13 @@ class ReadFileWorker
       return;
     }
 
-    while (!error_occurred_ && (data_.size() < total_bytes_)) {
+    while (!error_occurred_ && (data_.size() < total_bytes_) &&
+           !cancel_tasks_flag_.IsSet()) {
       BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                               Bind(&ReadFileWorker::DoWorkOnUIThread, this));
       on_task_completed_event_->Wait();
+      if (on_shutdown_event_->IsSignaled())
+        cancel_tasks_flag_.Set();
     }
   }
 
@@ -369,6 +403,9 @@ class ReadFileWorker
   // contents.
   void DoWorkOnUIThread() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    if (cancel_tasks_flag_.IsSet())
+      return;
+
     GetMediaTransferProtocolManager()->ReadFileChunkByPath(
         device_handle_, path_, data_.size(), BytesToRead(),
         Bind(&ReadFileWorker::OnDidWorkOnUIThread, this));
@@ -390,7 +427,8 @@ class ReadFileWorker
         error_occurred_ = true;
       }
     }
-    on_task_completed_event_->Signal();
+    if (!cancel_tasks_flag_.IsSet())
+      on_task_completed_event_->Signal();
   }
 
   uint32 BytesToRead() const {
@@ -428,6 +466,12 @@ class ReadFileWorker
   // Stores a reference to waitable event associated with the shut down message.
   WaitableEvent* on_shutdown_event_;
 
+  // Set to ignore the request results. This will be set when
+  // MTPDeviceDelegateImplLinux object is about to be deleted.
+  // |on_task_completed_event_| and |on_shutdown_event_| should not be
+  // dereferenced when this is set.
+  base::CancellationFlag cancel_tasks_flag_;
+
   DISALLOW_COPY_AND_ASSIGN(ReadFileWorker);
 };
 
@@ -483,6 +527,8 @@ class ReadDirectoryWorker
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                             Bind(&ReadDirectoryWorker::DoWorkOnUIThread, this));
     on_task_completed_event_->Wait();
+    if (on_shutdown_event_->IsSignaled())
+      cancel_tasks_flag_.Set();
   }
 
   // Returns the directory entries for the given directory path.
@@ -512,6 +558,8 @@ class ReadDirectoryWorker
   // entries. This is called on UI thread.
   void DoWorkOnUIThread() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    if (cancel_tasks_flag_.IsSet())
+      return;
 
     if (!dir_path_.empty()) {
       GetMediaTransferProtocolManager()->ReadDirectoryByPath(
@@ -532,7 +580,8 @@ class ReadDirectoryWorker
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     if (!error)
       file_entries_ = file_entries;
-    on_task_completed_event_->Signal();
+    if (!cancel_tasks_flag_.IsSet())
+      on_task_completed_event_->Signal();
   }
 
   // Stores the device handle to communicate with storage device.
@@ -560,6 +609,12 @@ class ReadDirectoryWorker
   // Stores the result of read directory request.
   std::vector<MtpFileEntry> file_entries_;
 
+  // Set to ignore the request results. This will be set when
+  // MTPDeviceDelegateImplLinux object is about to be deleted.
+  // |on_task_completed_event_| and |on_shutdown_event_| should not be
+  // dereferenced when this is set.
+  base::CancellationFlag cancel_tasks_flag_;
+
   DISALLOW_COPY_AND_ASSIGN(ReadDirectoryWorker);
 };
 
@@ -732,17 +787,6 @@ MTPDeviceDelegateImplLinux::MTPDeviceDelegateImplLinux(
   base::SequencedWorkerPool::SequenceToken media_sequence_token =
       pool->GetNamedSequenceToken("media-task-runner");
   media_task_runner_ = pool->GetSequencedTaskRunner(media_sequence_token);
-  registrar_.Add(this, chrome::NOTIFICATION_APP_TERMINATING,
-                 content::NotificationService::AllSources());
-
-  DCHECK(media_task_runner_);
-}
-
-MTPDeviceDelegateImplLinux::~MTPDeviceDelegateImplLinux() {
-  registrar_.RemoveAll();
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  GetMediaTransferProtocolManager()->CloseStorage(device_handle_,
-                                                  Bind(&DoNothing));
 }
 
 PlatformFileError MTPDeviceDelegateImplLinux::GetFileInfo(
@@ -826,21 +870,32 @@ SequencedTaskRunner* MTPDeviceDelegateImplLinux::GetMediaTaskRunner() {
   return media_task_runner_.get();
 }
 
-void MTPDeviceDelegateImplLinux::DeleteOnCorrectThread() const {
-  if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
-    BrowserThread::DeleteSoon(BrowserThread::UI, FROM_HERE, this);
-    return;
-  }
-  delete this;
-}
-
-void MTPDeviceDelegateImplLinux::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  DCHECK_EQ(chrome::NOTIFICATION_APP_TERMINATING, type);
+void MTPDeviceDelegateImplLinux::CancelPendingTasksAndDeleteDelegate() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  // Caution: This function is called on the IO thread. Access only the thread
+  // safe member variables in this function. Do all the clean up operations in
+  // DeleteDelegateOnTaskRunner().
   on_shutdown_event_.Signal();
   on_task_completed_event_.Signal();
+  media_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&MTPDeviceDelegateImplLinux::DeleteDelegateOnTaskRunner,
+                 base::Unretained(this)));
+}
+
+base::WeakPtr<fileapi::MTPDeviceDelegate> MTPDeviceDelegateImplLinux::
+    GetAsWeakPtrOnIOThread() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  base::WeakPtr<fileapi::MTPDeviceDelegate> delegate = AsWeakPtr();
+  // The weak pointer is instantiated on the IO thread, but only accessed on
+  // |media_task_runner_|. Therefore, detach from the current thread.
+  DetachFromThread();
+  return delegate;
+}
+
+MTPDeviceDelegateImplLinux::~MTPDeviceDelegateImplLinux() {
+  DCHECK(media_task_runner_->RunsTasksOnCurrentThread());
+  // Do all the clean up operations on DeleteDelegateOnTaskRunner().
 }
 
 bool MTPDeviceDelegateImplLinux::LazyInit() {
@@ -861,4 +916,11 @@ bool MTPDeviceDelegateImplLinux::LazyInit() {
   return !device_handle_.empty();
 }
 
+void MTPDeviceDelegateImplLinux::DeleteDelegateOnTaskRunner() {
+  DCHECK(media_task_runner_->RunsTasksOnCurrentThread());
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                          Bind(&CloseStorageOnUIThread, device_handle_));
+  delete this;
+}
+
 }  // namespace chrome