Fixed shutdown concurrency issues in FilePathWatcher.

chrome/browser/file_path_watcher/file_path_watcher_win.cc

Index: chrome/browser/file_path_watcher/file_path_watcher_win.cc
diff --git a/chrome/browser/file_path_watcher/file_path_watcher_win.cc b/chrome/browser/file_path_watcher/file_path_watcher_win.cc
index c2f49b937ca55a352853e9b79e44a2ffcad28e2e..19668d40b453d86488309379efd5aa9a00ec460c 100644
--- a/chrome/browser/file_path_watcher/file_path_watcher_win.cc
+++ b/chrome/browser/file_path_watcher/file_path_watcher_win.cc
@@ -10,11 +10,13 @@
 #include "base/ref_counted.h"
 #include "base/time.h"
 #include "base/win/object_watcher.h"
+#include "content/browser/browser_thread.h"
 
 namespace {
 
 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) {}
 
@@ -24,6 +26,10 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate,
   // Callback from MessageLoopForIO.
   virtual void OnObjectSignaled(HANDLE object);
 
+  // MessageLoop::DestructionObserver overrides.
+  // This allows cleaning up when the File thread is shutting down.
+  virtual void WillDestroyCurrentMessageLoop();
+
  private:
   virtual ~FilePathWatcherImpl();
 
@@ -39,6 +45,9 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate,
   // Destroy the watch handle.
   void DestroyWatch();
 
+  // Start observing the destruction of the File thread.
+  void StartWatchingAndObserving();
+
   // Delegate to notify upon changes.
   scoped_refptr<FilePathWatcher::Delegate> delegate_;
 
@@ -71,11 +80,23 @@ bool FilePathWatcherImpl::Watch(const FilePath& path,
   if (!UpdateWatch())
     return false;
 
-  watcher_.StartWatching(handle_, this);
+  if (!BrowserThread::CurrentlyOn(BrowserThread::FILE)) {

[Mattias Nissler (ping if slow), 2011/03/16 09:41:14]

We are guaranteed to run on the file thread here. UpdateWatch also requires
this. See FilePathWatcher::Watch(). Putting a DCHECK in is probably still a good
idea, even if only for documentation purposes.

[Joao da Silva, 2011/03/18 16:09:17]

Done.

+    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+        NewRunnableMethod(this,
+                          &FilePathWatcherImpl::StartWatchingAndObserving));
+  } else {
+    StartWatchingAndObserving();
+  }
 
   return true;
 }
 
+void FilePathWatcherImpl::StartWatchingAndObserving() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  MessageLoop::current()->AddDestructionObserver(this);
+  watcher_.StartWatching(handle_, this);
+}
+
 void FilePathWatcherImpl::Cancel() {
   // Switch to the file thread if necessary so we can stop |watcher_|.
   if (!BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
@@ -88,6 +109,26 @@ void FilePathWatcherImpl::Cancel() {
     DestroyWatch();
 }
 
+void FilePathWatcherImpl::WillDestroyCurrentMessageLoop() {
+  if (handle_ != INVALID_HANDLE_VALUE)
+    DestroyWatch();
+}
+
+FilePathWatcherImpl::~FilePathWatcherImpl() {
+  // 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.
+  //
+  // 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.
+
+  if (handle_ != INVALID_HANDLE_VALUE)
+    DestroyWatch();

[Mattias Nissler (ping if slow), 2011/03/16 09:41:14]

Same comment as for Mac: Shouldn't we already have been shut down at this point
and can we DCHECK for that?

[Joao da Silva, 2011/03/18 16:09:17]

Same reply as for Mac: this might be required when Cancel() has to post a task
to the file thread, that is never executed during shutdown.

+}
+
 void FilePathWatcherImpl::OnObjectSignaled(HANDLE object) {
   DCHECK(object == handle_);
   // Make sure we stay alive through the body of this function.
@@ -137,11 +178,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) {
@@ -235,6 +271,9 @@ void FilePathWatcherImpl::DestroyWatch() {
   watcher_.StopWatching();
   FindCloseChangeNotification(handle_);
   handle_ = INVALID_HANDLE_VALUE;
+
+  MessageLoop::current()->RemoveDestructionObserver(this);

[Mattias Nissler (ping if slow), 2011/03/16 09:41:14]

Oh, so what happens when UpdateWatch is executed multiple times during
operation? Don't kill our destruction observer early then?

[Joao da Silva, 2011/03/18 16:09:17]

Same mistake as on the Mac version.

+  delegate_ = NULL;
 }
 
 }  // namespace