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(callback_.is_null())

[Mattias Nissler (ping if slow), 2015/04/10 09:15:51]

Now, if the object gets destroyed on the g_task_runner thread, you can't be sure
that callback_ is up-to-date on that thread. Unless the reference counting
indeed provides a memory barrier on all relevant platforms... Please at least
add a comment here to explain why the member access from the wrong thread is OK
here.

An alternative thing you could do is checking either |callback_| or
|fsevent_stream_| depending on the thread you're on - but that's not pretty
either.

[Reilly Grant (use Gerrit), 2015/04/10 18:32:46]

Reference counting doesn't provide a sufficient memory barrier but the PostTask
in Cancel() does. I'll add a comment to this effect.

+      << "Cancel() must be called before FilePathWatcher is destroyed.";
 }
 
 void FilePathWatcherFSEvents::OnFilePathsChanged(
     const std::vector<FilePath>& paths) {

[Mattias Nissler (ping if slow), 2015/04/10 09:15:51]

nit: DCHECK(g_task_runner.Get().RunsTasksOnCurrentThread());

[Reilly Grant (use Gerrit), 2015/04/10 18:32:46]

Done.

-  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 10 matching lines @@
   context.copyDescription = NULL;
 
   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));
+  if (!FSEventStreamStart(fsevent_stream_)) {
+    message_loop()->PostTask(
+        FROM_HERE, Bind(&FilePathWatcherFSEvents::ReportError, this, target_));
+  }
 }
 
 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));
+  if (resolved_target_.empty()) {
+    message_loop()->PostTask(
+        FROM_HERE, Bind(&FilePathWatcherFSEvents::ReportError, this, target_));
+  }
   return changed;
 }
 
+void FilePathWatcherFSEvents::ReportError(const FilePath& target) {

[Mattias Nissler (ping if slow), 2015/04/10 09:15:51]

nit: DCHECK(message_loop()->RunsTasksOnCurrentThread());

[Reilly Grant (use Gerrit), 2015/04/10 18:32:46]

Done.

+  if (!callback_.is_null()) {
+    callback_.Run(target, true);
+  }
+}
+
 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