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 57 matching lines @@
     } else {
       result = current;
     }
   }
 
   if (resolve_count >= kMaxLinksToResolve)
     result.clear();
   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());
 
   bool root_changed = watcher->ResolveTargetPath();
   std::vector<FilePath> paths;
   FSEventStreamEventId root_change_at = FSEventStreamGetLatestEventId(stream);
   for (size_t i = 0; i < num_events; i++) {
     if (flags[i] & kFSEventStreamEventFlagRootChanged)
       root_changed = true;
@@ skipping 11 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);
 }
 
-}  // 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));
+  DCHECK(!resolved_target_.empty());
+  message_loop()->PostTask(
+      FROM_HERE, Bind(&FilePathWatcherFSEvents::DispatchEvents, this, paths,
+                      target_, resolved_target_));
+}
+
+void FilePathWatcherFSEvents::DispatchEvents(const std::vector<FilePath>& paths,
+                                             const FilePath& target,
+                                             const FilePath& resolved_target) {
+  DCHECK(message_loop()->RunsTasksOnCurrentThread());
+
+  // Don't issue callbacks after Cancel() has been called.
+  if (is_cancelled() || callback_.is_null()) {
     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);
+  for (const FilePath& path : paths) {
+    if (resolved_target.IsParent(path) || resolved_target == path) {
+      callback_.Run(target, false);
       return;
     }
   }
 }
 
-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::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();
-  }
-}
-
 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
   // 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_)
     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));
@@ skipping 11 matching lines @@
 
   fsevent_stream_ = FSEventStreamCreate(NULL, &FSEventsCallback, &context,
                                         watched_paths,
                                         start_event,
                                         kEventLatencySeconds,
                                         kFSEventStreamCreateFlagWatchRoot);
   FSEventStreamSetDispatchQueue(fsevent_stream_,
                                 g_task_runner.Get().GetDispatchQueue());
 
   if (!FSEventStreamStart(fsevent_stream_))
     message_loop()->PostTask(FROM_HERE, Bind(callback_, target_, true));

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

Strictly speaking, this is still referencing callback_ from the wrong thread
(same in line 245 below). Probably makes sense to bump this through a helper on
the message_loop() thread which can safely fill in callback_. Might just call it
ReportError() for clarity.

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

Done.

 }
 
 bool FilePathWatcherFSEvents::ResolveTargetPath() {
   DCHECK(g_task_runner.Get().RunsTasksOnCurrentThread());
   FilePath resolved = ResolvePath(target_).StripTrailingSeparators();
   bool changed = resolved != resolved_target_;
   resolved_target_ = resolved;
   if (resolved_target_.empty())
     message_loop()->PostTask(FROM_HERE, Bind(callback_, target_, true));
   return changed;
 }
 
 void FilePathWatcherFSEvents::DestroyEventStream() {
   FSEventStreamStop(fsevent_stream_);
   FSEventStreamInvalidate(fsevent_stream_);
   FSEventStreamRelease(fsevent_stream_);
   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