Access fields from the appropriate threads in FilePathWatcherFSEvents.

base/files/file_path_watcher_fsevents.cc

Index: base/files/file_path_watcher_fsevents.cc
diff --git a/base/files/file_path_watcher_fsevents.cc b/base/files/file_path_watcher_fsevents.cc
index 6e1a927a8def71d8e5841cdde84589ea0aff9c9e..5e39b763b5771ed047283ad445fd2722e7c5c661 100644
--- a/base/files/file_path_watcher_fsevents.cc
+++ b/base/files/file_path_watcher_fsevents.cc
@@ -75,11 +75,51 @@ FilePath ResolvePath(const FilePath& path) {
   return result;
 }
 
-// The callback passed to FSEventStreamCreate().
-void FSEventsCallback(ConstFSEventStreamRef stream,
-                      void* event_watcher, size_t num_events,
-                      void* event_paths, const FSEventStreamEventFlags flags[],
-                      const FSEventStreamEventId event_ids[]) {
+}  // namespace
+
+FilePathWatcherFSEvents::FilePathWatcherFSEvents() : fsevent_stream_(NULL) {
+}
+
+bool FilePathWatcherFSEvents::Watch(const FilePath& path,
+                                    bool recursive,
+                                    const FilePathWatcher::Callback& callback) {
+  DCHECK(MessageLoopForIO::current());
+  DCHECK(!callback.is_null());
+  DCHECK(callback_.is_null());
+
+  // This class could support non-recursive watches, but that is currently
+  // left to FilePathWatcherKQueue.
+  if (!recursive)
+    return false;
+
+  set_message_loop(MessageLoopProxy::current());
+  callback_ = callback;
+
+  FSEventStreamEventId start_event = FSEventsGetCurrentEventId();
+  g_task_runner.Get().PostTask(
+      FROM_HERE, Bind(&FilePathWatcherFSEvents::StartEventStream, this,
+                      start_event, path));
+  return true;
+}
+
+void FilePathWatcherFSEvents::Cancel() {
+  set_cancelled();
+  callback_.Reset();
+
+  // Switch to the dispatch queue thread to tear down the event stream.
+  g_task_runner.Get().PostTask(
+      FROM_HERE,
+      Bind(&FilePathWatcherFSEvents::CancelOnMessageLoopThread, this));
+}
+
+// static
+void FilePathWatcherFSEvents::FSEventsCallback(
+    ConstFSEventStreamRef stream,
+    void* event_watcher,
+    size_t num_events,
+    void* event_paths,
+    const FSEventStreamEventFlags flags[],
+    const FSEventStreamEventId event_ids[]) {
   FilePathWatcherFSEvents* watcher =
       reinterpret_cast<FilePathWatcherFSEvents*>(event_watcher);
   DCHECK(g_task_runner.Get().RunsTasksOnCurrentThread());
@@ -111,83 +151,45 @@ void FSEventsCallback(ConstFSEventStreamRef stream,
   watcher->OnFilePathsChanged(paths);
 }
 
-}  // namespace
-
-FilePathWatcherFSEvents::FilePathWatcherFSEvents() : fsevent_stream_(NULL) {
+FilePathWatcherFSEvents::~FilePathWatcherFSEvents() {
+  DCHECK(!fsevent_stream_)

[Mattias Nissler (ping if slow), 2015/04/07 11:36:20]

Is the dtor guaranteed to run on the dispatch queue thread? Otherwise the
reference to fsevent_stream_ here may be from the wrong thread.

[Reilly Grant (use Gerrit), 2015/04/07 20:42:14]

There are a couple of cases:

1) If Cancel has not been called then the last reference is released on the
message_loop thread then this will be called here and the assertion will
trigger. We can guarantee that Cancel won't be called after the last reference
is released so checking fsevent_stream_ is reasonably safe and we're about to
hit a DCHECK anyways.
2) If Cancel has been called then there are at least two references. One on the
message_loop thread and one held by the callback posted by Cancel to the
libdispatch thread. If the former is released first then this path isn't hit. If
the later is released first then when DestroyEventStream completes the
destructor is called on the libdispatch thread and fsevent_stream_ is guaranteed
to be nullptr.

[Mattias Nissler (ping if slow), 2015/04/08 07:57:09]

I don't see why you would be guaranteed that fsevent_stream_ is nullptr on the
message_loop() thread. Say the reference on the libdispatch thread gets dropped
first and |fsevent_stream_| reset to nullptr there. If the reference on the
message_loop() thread gets dropped shortly after, the |fsevent_stream_| pointer
as observed on the message_loop() thread may still not have picked up the
nullptr value (unless there's a synchronization point I'm missing).

+      << "File path watcher destroyed before event stream.";
 }
 
 void FilePathWatcherFSEvents::OnFilePathsChanged(
     const std::vector<FilePath>& paths) {
-  if (!message_loop()->BelongsToCurrentThread()) {
-    message_loop()->PostTask(
-        FROM_HERE,
-        Bind(&FilePathWatcherFSEvents::OnFilePathsChanged, this, paths));
-    return;
-  }
-
-  DCHECK(message_loop()->BelongsToCurrentThread());
-  if (resolved_target_.empty())
-    return;
-
-  for (size_t i = 0; i < paths.size(); i++) {
-    if (resolved_target_.IsParent(paths[i]) || resolved_target_ == paths[i]) {
-      callback_.Run(target_, false);
-      return;
-    }
-  }
+  DCHECK(!resolved_target_.empty());
+  message_loop()->PostTask(
+      FROM_HERE, Bind(&FilePathWatcherFSEvents::DispatchEvents, this, paths,
+                      target_, resolved_target_));
 }
 
-bool FilePathWatcherFSEvents::Watch(const FilePath& path,
-                                    bool recursive,
-                                    const FilePathWatcher::Callback& callback) {
-  DCHECK(resolved_target_.empty());
-  DCHECK(MessageLoopForIO::current());
-  DCHECK(!callback.is_null());
-
-  // This class could support non-recursive watches, but that is currently
-  // left to FilePathWatcherKQueue.
-  if (!recursive)
-    return false;
-
-  set_message_loop(MessageLoopProxy::current());
-  callback_ = callback;
-  target_ = path;
-
-  FSEventStreamEventId start_event = FSEventsGetCurrentEventId();
-  g_task_runner.Get().PostTask(
-      FROM_HERE,
-      Bind(&FilePathWatcherFSEvents::StartEventStream, this, start_event));
-  return true;
-}
+void FilePathWatcherFSEvents::DispatchEvents(const std::vector<FilePath>& paths,
+                                             const FilePath& target,
+                                             const FilePath& resolved_target) {
+  DCHECK(message_loop()->RunsTasksOnCurrentThread());
 
-void FilePathWatcherFSEvents::Cancel() {
-  if (callback_.is_null()) {
-    // Watch was never called, so exit.
-    set_cancelled();
+  // Don't issue callbacks after Cancel() has been called.
+  if (is_cancelled() || callback_.is_null()) {
     return;
   }
 
-  // Switch to the dispatch queue thread if necessary, so we can tear down
-  // the event stream.
-  if (!g_task_runner.Get().RunsTasksOnCurrentThread()) {
-    g_task_runner.Get().PostTask(
-        FROM_HERE,
-        Bind(&FilePathWatcherFSEvents::CancelOnMessageLoopThread, this));
-  } else {
-    CancelOnMessageLoopThread();
+  for (const FilePath& path : paths) {
+    if (resolved_target.IsParent(path) || resolved_target == path) {
+      callback_.Run(target, false);
+      return;
+    }
   }
 }
 
 void FilePathWatcherFSEvents::CancelOnMessageLoopThread() {
   // For all other implementations, the "message loop thread" is the IO thread,
   // as returned by message_loop(). This implementation, however, needs to
-  // cancel pending work on the Dipatch Queue thread.
+  // cancel pending work on the Dispatch Queue thread.
   DCHECK(g_task_runner.Get().RunsTasksOnCurrentThread());
 
-  set_cancelled();
   if (fsevent_stream_) {
     DestroyEventStream();
-    callback_.Reset();
     target_.clear();
     resolved_target_.clear();
   }
@@ -199,7 +201,7 @@ void FilePathWatcherFSEvents::UpdateEventStream(
 
   // 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 (is_cancelled() || resolved_target_.empty())
+  if (resolved_target_.empty())
     return;
 
   if (fsevent_stream_)
@@ -251,16 +253,14 @@ void FilePathWatcherFSEvents::DestroyEventStream() {
   fsevent_stream_ = NULL;
 }
 
-void FilePathWatcherFSEvents::StartEventStream(
-    FSEventStreamEventId start_event) {
+void FilePathWatcherFSEvents::StartEventStream(FSEventStreamEventId start_event,
+                                               const FilePath& path) {
   DCHECK(g_task_runner.Get().RunsTasksOnCurrentThread());
+  DCHECK(resolved_target_.empty());
+
+  target_ = path;
   ResolveTargetPath();
   UpdateEventStream(start_event);
 }
 
-FilePathWatcherFSEvents::~FilePathWatcherFSEvents() {
-  DCHECK(!fsevent_stream_)
-      << "File path watcher destroyed before event stream.";
-}
-
 }  // namespace base