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 a05851e3051bedcaa16660d6eac104c00ea711da..47cfbd2d4cd23239929b47c642ce1bd611fb214f 100644
--- a/chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc
+++ b/chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc
@@ -9,13 +9,12 @@
 #include "base/file_util.h"
 #include "base/sequenced_task_runner.h"
 #include "base/sequenced_task_runner_helpers.h"
+#include "base/synchronization/cancellation_flag.h"
 #include "base/string_util.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())
+      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,6 +369,8 @@ class ReadFileWorker
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                             Bind(&ReadFileWorker::DoWorkOnUIThread, this));
     on_task_completed_event_->Wait();
+    if (on_shutdown_event_->IsSignaled())
+      cancel_tasks_flag_.Set();
   }
 
   // Returns the media file contents received by ReadFileByPath() callback
@@ -365,6 +398,9 @@ class ReadFileWorker
   // contents.
   void DoWorkOnUIThread() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    if (cancel_tasks_flag_.IsSet())
+      return;
+
     GetMediaTransferProtocolManager()->ReadFileByPath(
       device_handle_, path_, Bind(&ReadFileWorker::OnDidWorkOnUIThread, this));
   }
@@ -379,7 +415,8 @@ class ReadFileWorker
       // pointer/ref rather than by value here to avoid an extra data copy.
       data_ = data;
     }
-    on_task_completed_event_->Signal();
+    if (!cancel_tasks_flag_.IsSet())
+      on_task_completed_event_->Signal();
   }
 
   // Stores the device unique identifier to query the device.
@@ -404,6 +441,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);
 };
 
@@ -459,6 +502,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.
@@ -488,6 +533,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(
@@ -508,7 +555,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.
@@ -536,6 +584,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);
 };
 
@@ -708,17 +762,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(
@@ -796,21 +839,33 @@ 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();
+  // This function is called on the IO thread but the member functions are
+  // accessed on |media_task_runner_|. Therefore, detach from the the current
+  // thread.
+  DetachFromThread();
+  return delegate;
+}
+
+MTPDeviceDelegateImplLinux::~MTPDeviceDelegateImplLinux() {
+  DCHECK(media_task_runner_->RunsTasksOnCurrentThread());
+  // Do all the clean up operations on DeleteDelegateOnTaskRunner().
 }
 
 bool MTPDeviceDelegateImplLinux::LazyInit() {
@@ -831,4 +886,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