Fixed shutdown concurrency issues in FilePathWatcher.

chrome/browser/file_path_watcher/file_path_watcher_mac.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/file_path_watcher/file_path_watcher.h"
 
 #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 {
 
 // The latency parameter passed to FSEventsStreamCreate().
 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();
 
   // Called from the FSEvents callback whenever there is a change to the paths
   void OnFilePathChanged();
 
   // (Re-)Initialize the event stream to start reporting events from
   // |start_event|.
   void UpdateEventStream(FSEventStreamEventId start_event);
 
   // FilePathWatcher::PlatformDelegate overrides.
   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_;
 
   // Target path to watch (passed to delegate).
   FilePath target_;
 
   // Keep track of the last modified time of the file.  We use nulltime
   // to represent the file not existing.
   base::Time last_modified_;
 
@@ skipping 43 matching lines @@
       NewRunnableMethod(watcher, &FilePathWatcherImpl::OnFilePathChanged));
 }
 
 // FilePathWatcherImpl implementation:
 
 FilePathWatcherImpl::FilePathWatcherImpl()
     : fsevent_stream_(NULL),
       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());
 
   base::PlatformFileInfo file_info;
   bool file_exists = file_util::GetFileInfo(target_, &file_info);
   if (file_exists && (last_modified_.is_null() ||
       last_modified_ != file_info.last_modified)) {
     last_modified_ = file_info.last_modified;
     first_notification_ = base::Time::Now();
@@ skipping 33 matching lines @@
   delegate_ = delegate;
 
   FSEventStreamEventId start_event = FSEventsGetCurrentEventId();
 
   base::PlatformFileInfo file_info;
   if (file_util::GetFileInfo(target_, &file_info)) {
     last_modified_ = file_info.last_modified;
     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));
 
   return true;
 }
 
 void FilePathWatcherImpl::Cancel() {
   // Switch to the UI thread if necessary, so we can tear down the event stream.
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
         NewRunnableMethod(this, &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) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // It can happen that the watcher gets canceled while tasks that call this
   // function are still in flight, so abort if this situation is detected.
   if (canceled_)
     return;
 
@@ skipping 24 matching lines @@
   FSEventStreamScheduleWithRunLoop(fsevent_stream_, CFRunLoopGetCurrent(),
                                    kCFRunLoopDefaultMode);
   if (!FSEventStreamStart(fsevent_stream_)) {
     BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
         NewRunnableMethod(delegate_.get(),
                           &FilePathWatcher::Delegate::OnError));
   }
 }
 
 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
 
 FilePathWatcher::FilePathWatcher() {
   impl_ = new FilePathWatcherImpl();
 }