Fixed shutdown concurrency issues in FilePathWatcher.

content/common/file_path_watcher/file_path_watcher_mac.cc

 // Copyright (c) 2011 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 "content/common/file_path_watcher/file_path_watcher.h"
 
 #include <CoreServices/CoreServices.h>
 #include <set>
 
 #include "base/file_path.h"
@@ skipping 14 matching lines @@
 // resource. If you have a good idea on how to get around this, the source for a
 // reasonable implementation of this class using kqueues is attached here:
 // http://code.google.com/p/chromium/issues/detail?id=54822#c13
 
 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 {
+//
+// 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();
 
   // 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,
                      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_;
 
   // 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 112 matching lines @@
 
   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();
   }
 
   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.
     return;
   }
 
   // Switch to the CFRunLoop based thread if necessary, so we can tear down
   // 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) {
   DCHECK(run_loop_message_loop()->BelongsToCurrentThread());
   DCHECK(MessageLoopForUI::current());
 
   // 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 25 matching lines @@
   FSEventStreamScheduleWithRunLoop(fsevent_stream_, CFRunLoopGetCurrent(),
                                    kCFRunLoopDefaultMode);
   if (!FSEventStreamStart(fsevent_stream_)) {
     message_loop()->PostTask(FROM_HERE,
         NewRunnableMethod(delegate_.get(),
                           &FilePathWatcher::Delegate::OnError));
   }
 }
 
 void FilePathWatcherImpl::DestroyEventStream() {
-  DCHECK(run_loop_message_loop()->BelongsToCurrentThread());
   FSEventStreamStop(fsevent_stream_);
   FSEventStreamUnscheduleFromRunLoop(fsevent_stream_, CFRunLoopGetCurrent(),
                                      kCFRunLoopDefaultMode);
   FSEventStreamRelease(fsevent_stream_);
   fsevent_stream_ = NULL;
 }
 
 }  // namespace
 
 FilePathWatcher::FilePathWatcher() {
   impl_ = new FilePathWatcherImpl();
 }