Fixed shutdown concurrency issues in FilePathWatcher.

chrome/browser/file_path_watcher/file_path_watcher_mac.cc

Index: chrome/browser/file_path_watcher/file_path_watcher_mac.cc
diff --git a/chrome/browser/file_path_watcher/file_path_watcher_mac.cc b/chrome/browser/file_path_watcher/file_path_watcher_mac.cc
index e89cf872de3a4f77e3145095bd93f6f75968c8e1..1a4642e7c9a7d9270f00490d3448eda7df51cf0e 100644
--- a/chrome/browser/file_path_watcher/file_path_watcher_mac.cc
+++ b/chrome/browser/file_path_watcher/file_path_watcher_mac.cc
@@ -7,12 +7,12 @@
 #include <CoreServices/CoreServices.h>
 #include <set>
 
-#include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/mac/scoped_cftyperef.h"
 #include "base/singleton.h"
 #include "base/time.h"
+#include "content/browser/browser_thread.h"
 
 namespace {
 
@@ -20,7 +20,8 @@ namespace {
 const CFAbsoluteTime kEventLatencySeconds = 0.3;
 
 // Mac-specific file watcher implementation based on the FSEvents API.
-class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate {
+class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate,
+                            public MessageLoop::DestructionObserver {
  public:
   FilePathWatcherImpl();
 
@@ -35,12 +36,19 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate {
   virtual bool Watch(const FilePath& path, FilePathWatcher::Delegate* delegate);
   virtual void Cancel();
 
+  // MessageLoop::DestructionObserver overrides.
+  // This allows cleaning up when the UI thread is shutting down.
+  virtual void WillDestroyCurrentMessageLoop();
+
  private:
-  virtual ~FilePathWatcherImpl() {}
+  virtual ~FilePathWatcherImpl();
 
   // Destroy the event stream.
   void DestroyEventStream();
 
+  // Start observing the destruction of the UI thread.
+  void StartObservingUIThread();
+
   // Delegate to notify upon changes.
   scoped_refptr<FilePathWatcher::Delegate> delegate_;
 
@@ -104,6 +112,19 @@ FilePathWatcherImpl::FilePathWatcherImpl()
       canceled_(false) {
 }
 
+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.
+  //
+  // 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)

[Bernhard Bauer, 2011/03/15 22:00:07]

I guess the first case refers to WillDestroyCurrentMessageLoop? In that case, it
might be better to put that part of the comment there, or the whole comment at
the beginning of the class.

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

Done.

+  // is deleted and releases a handle on |this|, triggering the dtor.
+  // In either case, we are on the right thread.
+
+  DestroyEventStream();

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

If I understand correctly, the DestroyEventStream() should be a noop here since
we have either been canceled through the reposted Cancel() task or the message
loop observer. So shouldn't we rather put a DCHECK here that the event stream is
gone and/or canceled_ == true?

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

This covers the case when Cancel() tries to switch to the UI thread by posting a
task, but that task is never executed (because the MessageLoop is shutting
down).
In that case, either
1) the Task is deleted, and we enter this dtor and have to cancel now (notice
that calling Cancel() from the dtor will only perform any work in this case, and
it's the right thread), or
2) the Task is NOT deleted (cause we're not running under Valgrind or something)
and was not executed. In this case, we Cancel() when the thread is about to
terminate, on WillDestroyCurrentMessageLoop.

+}
+
 void FilePathWatcherImpl::OnFilePathChanged() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   DCHECK(!target_.empty());
@@ -157,6 +178,11 @@ bool FilePathWatcherImpl::Watch(const FilePath& path,
     first_notification_ = base::Time::Now();
   }
 
+  // Start observing for destruction of the UI thread before starting to
+  // receive FSEventStream events.
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+      NewRunnableMethod(this, &FilePathWatcherImpl::StartObservingUIThread));
+
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(this, &FilePathWatcherImpl::UpdateEventStream,
                         start_event));
@@ -172,9 +198,19 @@ void FilePathWatcherImpl::Cancel() {
     return;
   }
 
-  canceled_ = true;
-  if (fsevent_stream_)
-    DestroyEventStream();
+  DestroyEventStream();
+}
+
+void FilePathWatcherImpl::WillDestroyCurrentMessageLoop() {
+  // This should only execute on the UI thread, but DCHECK'ing
+  // BrowserThread::CurrentlyOn(BrowserThread::UI) won't work because
+  // BrowserThread::message_loop() might be returning NULL during shutdown.
+  DestroyEventStream();
+}
+
+void FilePathWatcherImpl::StartObservingUIThread() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  MessageLoop::current()->AddDestructionObserver(this);
 }
 
 void FilePathWatcherImpl::UpdateEventStream(FSEventStreamEventId start_event) {
@@ -219,12 +255,21 @@ void FilePathWatcherImpl::UpdateEventStream(FSEventStreamEventId start_event) {
 }
 
 void FilePathWatcherImpl::DestroyEventStream() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  FSEventStreamStop(fsevent_stream_);
-  FSEventStreamUnscheduleFromRunLoop(fsevent_stream_, CFRunLoopGetCurrent(),
-                                     kCFRunLoopDefaultMode);
-  FSEventStreamRelease(fsevent_stream_);
-  fsevent_stream_ = NULL;
+  if (canceled_)
+    return;
+
+  canceled_ = true;

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

Hm, I think we have a problem here. DestroyEventStream() is also called from
UpdateEventStream(), which might get called multiple times during the lifetime
of FilePathWatcher. However, if you set canceled_ to true here, all following
UpdateEventStream calls will bail out early, which is not what we want.

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

Horrible mistake, thanks for spotting. The try bots agree with you :-)

+
+  if (fsevent_stream_) {
+    FSEventStreamStop(fsevent_stream_);
+    FSEventStreamUnscheduleFromRunLoop(fsevent_stream_, CFRunLoopGetCurrent(),
+                                       kCFRunLoopDefaultMode);
+    FSEventStreamRelease(fsevent_stream_);
+    fsevent_stream_ = NULL;
+
+    MessageLoop::current()->RemoveDestructionObserver(this);

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

Same problem as above, this will kill our destruction observer early, even if
we're only reinitializing the watch.

+    delegate_ = NULL;
+  }
 }
 
 }  // namespace