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..2ab78dd740416cacc6bb924e348c92994cd980fd 100644
--- a/base/files/file_path_watcher_fsevents.cc
+++ b/base/files/file_path_watcher_fsevents.cc
@@ -111,6 +111,18 @@ void FSEventsCallback(ConstFSEventStreamRef stream,
   watcher->OnFilePathsChanged(paths);
 }
 
+void FilterAndDispatchEvents(const std::vector<FilePath>& paths,
+                             const FilePath& target,
+                             const FilePath& resolved_target,
+                             const FilePathWatcher::Callback& callback) {
+  for (const FilePath& path : paths) {
+    if (resolved_target.IsParent(path) || resolved_target == path) {
+      callback.Run(target, false);

[Mattias Nissler (ping if slow), 2015/04/02 17:35:45]

I think this may result in callbacks firing even after
FilePathWatcherFSEvents::Cancel() was invoked, which is against the contract. Or
am I missing something? Btw., do we have a test case for this?

[Reilly Grant (use Gerrit), 2015/04/02 17:53:30]

That's true. If it's a contract violation then I'll come up with a way to safely
invalidate the callback across threads.

+      return;
+    }
+  }
+}
+
 }  // namespace
 
 FilePathWatcherFSEvents::FilePathWatcherFSEvents() : fsevent_stream_(NULL) {
@@ -118,29 +130,15 @@ FilePathWatcherFSEvents::FilePathWatcherFSEvents() : fsevent_stream_(NULL) {
 
 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(&FilterAndDispatchEvents, paths, target_,
+                                resolved_target_, callback_));
 }
 
 bool FilePathWatcherFSEvents::Watch(const FilePath& path,
                                     bool recursive,
                                     const FilePathWatcher::Callback& callback) {
-  DCHECK(resolved_target_.empty());
   DCHECK(MessageLoopForIO::current());
   DCHECK(!callback.is_null());
 
@@ -150,38 +148,25 @@ bool FilePathWatcherFSEvents::Watch(const FilePath& path,
     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));
+      FROM_HERE, Bind(&FilePathWatcherFSEvents::StartEventStream, this,
+                      start_event, path, callback));
   return true;
 }
 
 void FilePathWatcherFSEvents::Cancel() {
-  if (callback_.is_null()) {
-    // Watch was never called, so exit.
-    set_cancelled();
-    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();
-  }
+  // Switch to the dispatch queue thread to tear down the event stream.
+  g_task_runner.Get().PostTask(
+      FROM_HERE,
+      Bind(&FilePathWatcherFSEvents::CancelOnMessageLoopThread, this));
 }
 
 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();
@@ -197,11 +182,6 @@ void FilePathWatcherFSEvents::UpdateEventStream(
     FSEventStreamEventId start_event) {
   DCHECK(g_task_runner.Get().RunsTasksOnCurrentThread());
 
-  // It can happen that the watcher gets canceled while tasks that call this

[Mattias Nissler (ping if slow), 2015/04/02 17:35:45]

I don't see why we no longer need a safeguard here, can you explain? I think
FSEventsCallback can still post a task to this function and in the mean time we
might execute a cancel task. And then, we'd accidentally recreate the event
stream when the UpdateEventStream task runs. You might be able to use |target_|
as a sentinel.

[Reilly Grant (use Gerrit), 2015/04/02 17:53:30]

With the cancel operation serialized onto the libdispatch thread it will either
run before FSEventsCallback is delivered, preventing it from being called or
after. The issue is the callbacks after cancel mentioned above.

-  // function are still in flight, so abort if this situation is detected.
-  if (is_cancelled() || resolved_target_.empty())
-    return;
-
   if (fsevent_stream_)
     DestroyEventStream();
 
@@ -252,8 +232,14 @@ void FilePathWatcherFSEvents::DestroyEventStream() {
 }
 
 void FilePathWatcherFSEvents::StartEventStream(
-    FSEventStreamEventId start_event) {
+    FSEventStreamEventId start_event,
+    const FilePath& path,
+    const FilePathWatcher::Callback& callback) {
   DCHECK(g_task_runner.Get().RunsTasksOnCurrentThread());
+  DCHECK(resolved_target_.empty());
+
+  callback_ = callback;

[Mattias Nissler (ping if slow), 2015/04/02 17:35:45]

I think you want to keep callback_ on the message_loop() thread, so you can nuke
it on Cancel(), so you can reliably stop running callbacks after Cancel() gets
called on the message_loop() thread.

[Reilly Grant (use Gerrit), 2015/04/02 17:53:31]

I don't think base::Callback is thread safe. I'll find something that is to use
as a sentinel.

+  target_ = path;
   ResolveTargetPath();
   UpdateEventStream(start_event);
 }