Access fields from the appropriate threads in FilePathWatcherFSEvents.

base/files/file_path_watcher_fsevents.cc

 // Copyright 2014 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 "base/files/file_path_watcher_fsevents.h"
 
 #include <list>
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
@@ skipping 93 matching lines @@
     // bad file descriptor errors), so post a task to do the reset.
     g_task_runner.Get().PostTask(
         FROM_HERE,
         Bind(&FilePathWatcherFSEvents::UpdateEventStream, watcher,
              root_change_at));
   }
 
   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) {
 }
 
 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());
 
   // 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));
+      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();
   if (fsevent_stream_) {
     DestroyEventStream();
     callback_.Reset();
     target_.clear();
     resolved_target_.clear();
   }
 }
 
 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();
 
   ScopedCFTypeRef<CFStringRef> cf_path(CFStringCreateWithCString(
       NULL, resolved_target_.value().c_str(), kCFStringEncodingMacHFS));
   ScopedCFTypeRef<CFStringRef> cf_dir_path(CFStringCreateWithCString(
       NULL, resolved_target_.DirName().value().c_str(),
       kCFStringEncodingMacHFS));
   CFStringRef paths_array[] = { cf_path.get(), cf_dir_path.get() };
   ScopedCFTypeRef<CFArrayRef> watched_paths(CFArrayCreate(
@@ skipping 30 matching lines @@
 }
 
 void FilePathWatcherFSEvents::DestroyEventStream() {
   FSEventStreamStop(fsevent_stream_);
   FSEventStreamInvalidate(fsevent_stream_);
   FSEventStreamRelease(fsevent_stream_);
   fsevent_stream_ = NULL;
 }
 
 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);
 }
 
 FilePathWatcherFSEvents::~FilePathWatcherFSEvents() {
   DCHECK(!fsevent_stream_)
       << "File path watcher destroyed before event stream.";
 }
 
 }  // namespace base