Fixed shutdown concurrency issues in FilePathWatcher.

content/common/file_path_watcher/file_path_watcher_mac.cc

Index: content/common/file_path_watcher/file_path_watcher_mac.cc
diff --git a/content/common/file_path_watcher/file_path_watcher_mac.cc b/content/common/file_path_watcher/file_path_watcher_mac.cc
index 6e89ad1a6b588d792fdd1e8132c6960e4bc4e3d4..ea201457c2081abe4d2062cce9d64701de5b18cd 100644
--- a/content/common/file_path_watcher/file_path_watcher_mac.cc
+++ b/content/common/file_path_watcher/file_path_watcher_mac.cc
@@ -32,7 +32,18 @@ namespace {
 const CFAbsoluteTime kEventLatencySeconds = 0.3;
 
 // Mac-specific file watcher implementation based on the FSEvents API.
-class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate {
+//
+// Deletion of the FilePathWatcher will call Cancel() to dispose of this
+// object in the right thread. In that case, |this| is already cleaned up 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, DestroyEventStream() is called on the right thread.

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

Does the second case actually happen? I would expect that before all tasks get
deleted, the WillDestroyCurrentMessageLoop() gets called. So why would we need
code in the dtor?

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

It does happen, see previous comment.

+class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate,
+                            public MessageLoop::DestructionObserver {
  public:
   FilePathWatcherImpl();
 
@@ -49,16 +60,28 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate {
                      base::MessageLoopProxy* loop) OVERRIDE;
   virtual void Cancel() OVERRIDE;
 
+  // MessageLoop::DestructionObserver overrides.
+  // This allows cleaning up when the |run_loop_message_loop_| thread is
+  // shutting down.
+  virtual void WillDestroyCurrentMessageLoop();
+
   scoped_refptr<base::MessageLoopProxy> run_loop_message_loop() {
     return run_loop_message_loop_;
   }
 
  private:
-  virtual ~FilePathWatcherImpl() {}
+  virtual ~FilePathWatcherImpl();
 
   // Destroy the event stream.
   void DestroyEventStream();
 
+  // Start observing the destruction of the |run_loop_message_loop_| thread,
+  // and watching the FSEventStream.
+  void StartObserverAndEventStream(FSEventStreamEventId start_event);
+
+  // Cleans up and stops observing the |run_loop_message_loop_| thread.
+  void CancelInternal();
+
   // Delegate to notify upon changes.
   scoped_refptr<FilePathWatcher::Delegate> delegate_;
 
@@ -191,12 +214,24 @@ bool FilePathWatcherImpl::Watch(const FilePath& path,
   }
 
   run_loop_message_loop()->PostTask(FROM_HERE,
-      NewRunnableMethod(this, &FilePathWatcherImpl::UpdateEventStream,
+      NewRunnableMethod(this, &FilePathWatcherImpl::StartObserverAndEventStream,
                         start_event));
 
   return true;
 }
 
+void FilePathWatcherImpl::StartObserverAndEventStream(
+    FSEventStreamEventId start_event) {
+  DCHECK(run_loop_message_loop()->BelongsToCurrentThread());
+  MessageLoop::current()->AddDestructionObserver(this);
+  UpdateEventStream(start_event);
+}
+
+FilePathWatcherImpl::~FilePathWatcherImpl() {
+  if (!canceled_)
+    CancelInternal();
+}
+
 void FilePathWatcherImpl::Cancel() {
   if (!run_loop_message_loop().get()) {
     // Watch was never called, so exit.
@@ -207,13 +242,28 @@ void FilePathWatcherImpl::Cancel() {
   // the event stream.
   if (!run_loop_message_loop()->BelongsToCurrentThread()) {
     run_loop_message_loop()->PostTask(FROM_HERE,
-        NewRunnableMethod(this, &FilePathWatcherImpl::Cancel));
-    return;
+        NewRunnableMethod(this, &FilePathWatcherImpl::CancelInternal));
+  } else {
+    CancelInternal();
   }
+}
 
+void FilePathWatcherImpl::CancelInternal() {
   canceled_ = true;
-  if (fsevent_stream_)
+  if (fsevent_stream_) {
     DestroyEventStream();
+    MessageLoop::current()->RemoveDestructionObserver(this);
+    delegate_ = NULL;
+  }
+}
+
+void FilePathWatcherImpl::WillDestroyCurrentMessageLoop() {
+  // This DCHECK will fail because the |run_loop_message_loop_| has been
+  // notified that the thread is quitting too.
+  //
+  // DCHECK(run_loop_message_loop()->BelongsToCurrentThread());

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

So just remove this block?

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

Done.

+
+  CancelInternal();
 }
 
 void FilePathWatcherImpl::UpdateEventStream(FSEventStreamEventId start_event) {
@@ -259,7 +309,6 @@ void FilePathWatcherImpl::UpdateEventStream(FSEventStreamEventId start_event) {
 }
 
 void FilePathWatcherImpl::DestroyEventStream() {
-  DCHECK(run_loop_message_loop()->BelongsToCurrentThread());
   FSEventStreamStop(fsevent_stream_);
   FSEventStreamUnscheduleFromRunLoop(fsevent_stream_, CFRunLoopGetCurrent(),
                                      kCFRunLoopDefaultMode);