Fixed concurrency issue in FilePathWatcher

Review URL: http://codereview.chromium.org/6697020

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@79029 0039d316-1c4b-4281-b951-d872f2087c98

content/common/file_path_watcher/file_path_watcher.cc

@@ -21,19 +21,10 @@ bool FilePathWatcher::Watch(const FilePath& path,
   return impl_->Watch(path, delegate, loop);
 }
 
-FilePathWatcher::PlatformDelegate::PlatformDelegate() {
+FilePathWatcher::PlatformDelegate::PlatformDelegate(): cancelled_(false) {
 }
 
 FilePathWatcher::PlatformDelegate::~PlatformDelegate() {
+  DCHECK(is_cancelled());
 }
 
-void FilePathWatcher::DeletePlatformDelegate::Destruct(
-    const PlatformDelegate* delegate) {
-  scoped_refptr<base::MessageLoopProxy> loop = delegate->message_loop();
-  if (loop.get() == NULL || loop->BelongsToCurrentThread()) {
-    delete delegate;
-  } else {
-    loop->PostNonNestableTask(FROM_HERE,
-                              new DeleteTask<PlatformDelegate>(delegate));
-  }
-}

content/common/file_path_watcher/file_path_watcher.h

@@ -40,22 +40,35 @@ class FilePathWatcher {
   // by the Mac implementation right now, and must be backed by a CFRunLoop
   // based MessagePump. This is usually going to be a MessageLoop of type
   // TYPE_UI.
+  // OnFilePathChanged() will be called on the same thread as Watch() is called,
+  // which should have a MessageLoop of TYPE_IO.
   bool Watch(const FilePath& path,
              Delegate* delegate,
              base::MessageLoopProxy* loop) WARN_UNUSED_RESULT;
 
   class PlatformDelegate;
 
-  // Traits for PlatformDelegate, which must delete itself on the IO message
-  // loop that Watch was called from.
-  struct DeletePlatformDelegate {
-    static void Destruct(const PlatformDelegate* delegate);
+  // A custom Task that always cleans up the PlatformDelegate, either when
+  // executed or when deleted without having been executed at all, as can
+  // happen during shutdown.
+  class CancelTask : public Task {
+   public:
+    CancelTask(PlatformDelegate* delegate): delegate_(delegate) {}
+    virtual ~CancelTask() {
+      delegate_->CancelOnMessageLoopThread();
+    }
+
+    virtual void Run() {
+      delegate_->CancelOnMessageLoopThread();
+    }
+   private:
+    scoped_refptr<PlatformDelegate> delegate_;
+
+    DISALLOW_COPY_AND_ASSIGN(CancelTask);
   };
 
   // Used internally to encapsulate different members on different platforms.
-  class PlatformDelegate
-      : public base::RefCountedThreadSafe<PlatformDelegate,
-                                          DeletePlatformDelegate> {
+  class PlatformDelegate : public base::RefCountedThreadSafe<PlatformDelegate> {
    public:
     PlatformDelegate();
 
@@ -70,14 +83,17 @@ class FilePathWatcher {
 
     // Stop watching. This is called from FilePathWatcher's dtor in order to
     // allow to shut down properly while the object is still alive.
+    // It can be called from any thread.
     virtual void Cancel() = 0;
 
    protected:
-    friend class DeleteTask<PlatformDelegate>;
-    friend struct DeletePlatformDelegate;
-
     virtual ~PlatformDelegate();
 
+    // Stop watching. This is only called on the thread of the appropriate
+    // message loop. Since it can also be called more than once, it should
+    // check |is_cancelled()| to avoid duplicate work.
+    virtual void CancelOnMessageLoopThread() = 0;
+
     scoped_refptr<base::MessageLoopProxy> message_loop() const {
       return message_loop_;
     }
@@ -86,8 +102,21 @@ class FilePathWatcher {
       message_loop_ = loop;
     }
 
+    // Must be called before the PlatformDelegate is deleted.
+    void set_cancelled() {
+      cancelled_ = true;
+    }
+
+    bool is_cancelled() const {
+      return cancelled_;
+    }
+
    private:
+    friend class base::RefCountedThreadSafe<PlatformDelegate>;
+    friend class CancelTask;
+
     scoped_refptr<base::MessageLoopProxy> message_loop_;
+    bool cancelled_;
   };
 
  private:

content/common/file_path_watcher/file_path_watcher_inotify.cc

@@ -80,7 +80,8 @@ class InotifyReader {
   DISALLOW_COPY_AND_ASSIGN(InotifyReader);
 };
 
-class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate {
+class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate,
+                            public MessageLoop::DestructionObserver {
  public:
   FilePathWatcherImpl();
 
@@ -103,9 +104,17 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate {
   // Cancel the watch. This unregisters the instance with InotifyReader.
   virtual void Cancel() OVERRIDE;
 
+  // Deletion of the FilePathWatcher will call Cancel() to dispose of this
+  // object in the right thread. This also observes destruction of the required
+  // cleanup thread, in case it quits before Cancel() is called.
+  virtual void WillDestroyCurrentMessageLoop() OVERRIDE;
+
  private:
   virtual ~FilePathWatcherImpl() {}
 
+  // Cleans up and stops observing the |message_loop_| thread.
+  void CancelOnMessageLoopThread() OVERRIDE;
+
   // Inotify watches are installed for all directory components of |target_|. A
   // WatchEntry instance holds the watch descriptor for a component and the
   // subdirectory for that identifies the next component.
@@ -363,6 +372,8 @@ bool FilePathWatcherImpl::Watch(const FilePath& path,
   set_message_loop(base::MessageLoopProxy::CreateForCurrentThread());
   delegate_ = delegate;
   target_ = path;
+  MessageLoop::current()->AddDestructionObserver(this);
+
   std::vector<FilePath::StringType> comps;
   target_.GetComponents(&comps);
   DCHECK(!comps.empty());
@@ -376,26 +387,39 @@ bool FilePathWatcherImpl::Watch(const FilePath& path,
 }
 
 void FilePathWatcherImpl::Cancel() {
-  if (!message_loop().get()) {
-    // Watch was never called, so exit.
+  if (!delegate_) {
+    // Watch was never called, or the |message_loop_| thread is already gone.
+    set_cancelled();
     return;
   }
 
   // Switch to the message_loop_ if necessary so we can access |watches_|.
   if (!message_loop()->BelongsToCurrentThread()) {
     message_loop()->PostTask(FROM_HERE,
-        NewRunnableMethod(this, &FilePathWatcherImpl::Cancel));
-    return;
+                             new FilePathWatcher::CancelTask(this));
+  } else {
+    CancelOnMessageLoopThread();
   }
+}
 
-  for (WatchVector::iterator watch_entry(watches_.begin());
-       watch_entry != watches_.end(); ++watch_entry) {
-    if (watch_entry->watch_ != InotifyReader::kInvalidWatch)
-      g_inotify_reader.Get().RemoveWatch(watch_entry->watch_, this);
+void FilePathWatcherImpl::CancelOnMessageLoopThread() {
+  if (!is_cancelled()) {
+    set_cancelled();
+    MessageLoop::current()->RemoveDestructionObserver(this);
+
+    for (WatchVector::iterator watch_entry(watches_.begin());
+         watch_entry != watches_.end(); ++watch_entry) {
+      if (watch_entry->watch_ != InotifyReader::kInvalidWatch)
+        g_inotify_reader.Get().RemoveWatch(watch_entry->watch_, this);
+    }
+    watches_.clear();
+    delegate_ = NULL;
+    target_.clear();
   }
-  watches_.clear();
-  delegate_ = NULL;
-  target_.clear();
+}
+
+void FilePathWatcherImpl::WillDestroyCurrentMessageLoop() {
+  CancelOnMessageLoopThread();
 }
 
 bool FilePathWatcherImpl::UpdateWatches() {

content/common/file_path_watcher/file_path_watcher_mac.cc

@@ -32,7 +32,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();
 
@@ -49,6 +50,11 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate {
                      base::MessageLoopProxy* loop) OVERRIDE;
   virtual void Cancel() OVERRIDE;
 
+  // Deletion of the FilePathWatcher will call Cancel() to dispose of this
+  // object in the right thread. This also observes destruction of the required
+  // cleanup thread, in case it quits before Cancel() is called.
+  virtual void WillDestroyCurrentMessageLoop() OVERRIDE;
+
   scoped_refptr<base::MessageLoopProxy> run_loop_message_loop() {
     return run_loop_message_loop_;
   }
@@ -59,6 +65,13 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate {
   // 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 CancelOnMessageLoopThread() OVERRIDE;
+
   // Delegate to notify upon changes.
   scoped_refptr<FilePathWatcher::Delegate> delegate_;
 
@@ -79,9 +92,6 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate {
   // Run loop for FSEventStream to run on.
   scoped_refptr<base::MessageLoopProxy> run_loop_message_loop_;
 
-  // Used to detect early cancellation.
-  bool canceled_;
-
   DISALLOW_COPY_AND_ASSIGN(FilePathWatcherImpl);
 };
 
@@ -120,8 +130,7 @@ void FSEventsCallback(ConstFSEventStreamRef stream,
 // FilePathWatcherImpl implementation:
 
 FilePathWatcherImpl::FilePathWatcherImpl()
-    : fsevent_stream_(NULL),
-      canceled_(false) {
+    : fsevent_stream_(NULL) {
 }
 
 void FilePathWatcherImpl::OnFilePathChanged() {
@@ -191,29 +200,47 @@ 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);
+}
+
 void FilePathWatcherImpl::Cancel() {
   if (!run_loop_message_loop().get()) {
     // Watch was never called, so exit.
+    set_cancelled();
     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;
+                                      new FilePathWatcher::CancelTask(this));
+  } else {
+    CancelOnMessageLoopThread();
   }
+}
 
-  canceled_ = true;
-  if (fsevent_stream_)
+void FilePathWatcherImpl::CancelOnMessageLoopThread() {
+  set_cancelled();
+  if (fsevent_stream_) {
     DestroyEventStream();
+    MessageLoop::current()->RemoveDestructionObserver(this);
+    delegate_ = NULL;
+  }
+}
+
+void FilePathWatcherImpl::WillDestroyCurrentMessageLoop() {
+  CancelOnMessageLoopThread();
 }
 
 void FilePathWatcherImpl::UpdateEventStream(FSEventStreamEventId start_event) {
@@ -222,7 +249,7 @@ void FilePathWatcherImpl::UpdateEventStream(FSEventStreamEventId start_event) {
 
   // 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_)
+  if (is_cancelled())
     return;
 
   if (fsevent_stream_)
@@ -259,7 +286,6 @@ void FilePathWatcherImpl::UpdateEventStream(FSEventStreamEventId start_event) {
 }
 
 void FilePathWatcherImpl::DestroyEventStream() {
-  DCHECK(run_loop_message_loop()->BelongsToCurrentThread());
   FSEventStreamStop(fsevent_stream_);
   FSEventStreamUnscheduleFromRunLoop(fsevent_stream_, CFRunLoopGetCurrent(),
                                      kCFRunLoopDefaultMode);