Fixed shutdown concurrency issues in FilePathWatcher.

content/common/file_path_watcher/file_path_watcher_win.cc

Index: content/common/file_path_watcher/file_path_watcher_win.cc
diff --git a/content/common/file_path_watcher/file_path_watcher_win.cc b/content/common/file_path_watcher/file_path_watcher_win.cc
index b6b4333bf32c2befc3d2bc3bda109132807a1405..26be2121afd76113768df13463eaa611adf0c005 100644
--- a/content/common/file_path_watcher/file_path_watcher_win.cc
+++ b/content/common/file_path_watcher/file_path_watcher_win.cc
@@ -14,8 +14,18 @@
 
 namespace {
 
+// Deletion of the FilePathWatcher will call Cancel() to dispose of this
+// object in the right thread. In that case, |this| is already cleaned up
+// and |handle_| is INVALID_HANDLE_VALUE when the dtor enters.
+//
+// However, Cancel() might have to switch threads and its Task might never be
+// called during shutdown. In that case, either |this| gets notified that the
+// thread is shutting down and cleans up, or the Task (that wasn't executed)
+// is deleted and releases a handle on |this|, triggering the dtor.
+// In either case, we are on the right thread when cleaning up.

[Mattias Nissler (ping if slow), 2011/03/21 16:07:45]

same question as for the others

[Joao da Silva, 2011/03/22 10:10:08]

Same reply :-)

 class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate,
-                            public base::win::ObjectWatcher::Delegate {
+                            public base::win::ObjectWatcher::Delegate,
+                            public MessageLoop::DestructionObserver {
  public:
   FilePathWatcherImpl() : delegate_(NULL), handle_(INVALID_HANDLE_VALUE) {}
 
@@ -25,6 +35,10 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate,
                      base::MessageLoopProxy* loop) OVERRIDE;
   virtual void Cancel() OVERRIDE;
 
+  // MessageLoop::DestructionObserver overrides.
+  // This allows cleaning up when the |message_loop_| thread is shutting down.
+  virtual void WillDestroyCurrentMessageLoop();
+
   // Callback from MessageLoopForIO.
   virtual void OnObjectSignaled(HANDLE object);
 
@@ -43,6 +57,9 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate,
   // Destroy the watch handle.
   void DestroyWatch();
 
+  // Cleans up and stops observing the |message_loop_| thread.
+  void CancelInternal();
+
   // Delegate to notify upon changes.
   scoped_refptr<FilePathWatcher::Delegate> delegate_;
 
@@ -74,6 +91,7 @@ bool FilePathWatcherImpl::Watch(const FilePath& path,
   set_message_loop(base::MessageLoopProxy::CreateForCurrentThread());
   delegate_ = delegate;
   target_ = path;
+  MessageLoop::current()->AddDestructionObserver(this);
 
   if (!UpdateWatch())
     return false;
@@ -84,20 +102,36 @@ bool FilePathWatcherImpl::Watch(const FilePath& path,
 }
 
 void FilePathWatcherImpl::Cancel() {
-  if (!message_loop().get()) {
-    // Watch was never called, so exit.
+  if (!delegate_) {
+    // Watch was never called, or the |message_loop_| has already quit.
     return;
   }
 
   // Switch to the file thread if necessary so we can stop |watcher_|.
   if (!message_loop()->BelongsToCurrentThread()) {
     message_loop()->PostTask(FROM_HERE,
-        NewRunnableMethod(this, &FilePathWatcherImpl::Cancel));
-    return;
+        NewRunnableMethod(this, &FilePathWatcherImpl::CancelInternal));
+  } else {
+    CancelInternal();
   }
+}
 
+void FilePathWatcherImpl::CancelInternal() {
   if (handle_ != INVALID_HANDLE_VALUE)
     DestroyWatch();
+
+  if (delegate_) {
+    MessageLoop::current()->RemoveDestructionObserver(this);
+    delegate_ = NULL;
+  }
+}
+
+void FilePathWatcherImpl::WillDestroyCurrentMessageLoop() {
+  CancelInternal();
+}
+
+FilePathWatcherImpl::~FilePathWatcherImpl() {
+  CancelInternal();
 }
 
 void FilePathWatcherImpl::OnObjectSignaled(HANDLE object) {
@@ -149,11 +183,6 @@ void FilePathWatcherImpl::OnObjectSignaled(HANDLE object) {
     watcher_.StartWatching(handle_, this);
 }
 
-FilePathWatcherImpl::~FilePathWatcherImpl() {
-  if (handle_ != INVALID_HANDLE_VALUE)
-    DestroyWatch();
-}
-
 // static
 bool FilePathWatcherImpl::SetupWatchHandle(const FilePath& dir,
                                            HANDLE* handle) {