Remove ref-counting from FilePathWatcher.

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 <dispatch/dispatch.h>
 
 #include <list>
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/mac/scoped_cftyperef.h"
-#include "base/macros.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 
 namespace base {
 
 namespace {
 
 // The latency parameter passed to FSEventsStreamCreate().
 const CFAbsoluteTime kEventLatencySeconds = 0.3;
 
@@ skipping 35 matching lines @@
 
   if (resolve_count >= kMaxLinksToResolve)
     result.clear();
   return result;
 }
 
 }  // namespace
 
 FilePathWatcherFSEvents::FilePathWatcherFSEvents()
     : queue_(dispatch_queue_create(
-          base::StringPrintf(
-              "org.chromium.base.FilePathWatcher.%p", this).c_str(),
+          base::StringPrintf("org.chromium.base.FilePathWatcher.%p", this)
+              .c_str(),
           DISPATCH_QUEUE_SERIAL)),
-      fsevent_stream_(nullptr) {
+      fsevent_stream_(nullptr),
+      weak_factory_(this) {}
+
+FilePathWatcherFSEvents::~FilePathWatcherFSEvents() {
+  DCHECK(!task_runner() || task_runner()->RunsTasksOnCurrentThread());

[gab, 2017/01/05 21:22:53]

Is this a new requirement? Where is it documented?

[fdoray, 2017/01/05 23:04:11]

The DCHECK below is not new.

The DCHECK above documents an existing requirement:

A FilePathWatcherFSEvents can only be owned by a FilePathWatcherMac which can
only be owned by a FilePathWatcher. That means that the destructor of
FilePathWatcherFSEvents can only be invoked (indirectly) by the destructor of
FilePathWatcher. task_runner() is set in Watch(). From
https://cs.chromium.org/chromium/src/base/files/file_path_watcher.h?l=30:
"[FilePathWatcher] must be destroyed on the sequence that invokes Watch()."

+  DCHECK(callback_.is_null())
+      << "Cancel() must be called before FilePathWatcher is destroyed.";
 }
 
 bool FilePathWatcherFSEvents::Watch(const FilePath& path,
                                     bool recursive,
                                     const FilePathWatcher::Callback& callback) {
   DCHECK(!callback.is_null());
   DCHECK(callback_.is_null());
 
   // This class could support non-recursive watches, but that is currently
   // left to FilePathWatcherKQueue.
@@ skipping 12 matching lines @@
   dispatch_async(queue_, ^{
       StartEventStream(start_event, path_copy);
   });
   return true;
 }
 
 void FilePathWatcherFSEvents::Cancel() {
   set_cancelled();
   callback_.Reset();
 
-  // Switch to the dispatch queue to tear down the event stream. As the queue
-  // is owned by this object, and this method is called from the destructor,
-  // execute the block synchronously.
+  // Switch to the dispatch queue to tear down the event stream. As the queue is
+  // owned by |this|, and this method is called from the destructor, execute the
+  // block synchronously.
   dispatch_sync(queue_, ^{
-      CancelOnMessageLoopThread();
+    if (fsevent_stream_) {
+      DestroyEventStream();
+      target_.clear();
+      resolved_target_.clear();
+    }
   });
 }
 
 // static
 void FilePathWatcherFSEvents::FSEventsCallback(
     ConstFSEventStreamRef stream,
     void* event_watcher,
     size_t num_events,
     void* event_paths,
     const FSEventStreamEventFlags flags[],
@@ skipping 10 matching lines @@
       root_change_at = std::min(root_change_at, event_ids[i]);
     paths.push_back(FilePath(
         reinterpret_cast<char**>(event_paths)[i]).StripTrailingSeparators());
   }
 
   // Reinitialize the event stream if we find changes to the root. This is
   // necessary since FSEvents doesn't report any events for the subtree after
   // the directory to be watched gets created.
   if (root_changed) {
     // Resetting the event stream from within the callback fails (FSEvents spews
-    // bad file descriptor errors), so post a task to do the reset.
-    dispatch_async(watcher->queue_, ^{
-        watcher->UpdateEventStream(root_change_at);
-    });
+    // bad file descriptor errors). Therefore, post a task to task_runner() that
+    // will itself schedule a call to UpdateEventStream() on the dispatch queue.
+    // Bouncing on task_runner() and using a weak ptr ensures that
+    // UpdateEventStream() won't be called after |watcher| is destroyed  (i.e.
+    // after the watch is cancelled).
+    watcher->task_runner()->PostTask(
+        FROM_HERE, Bind(
+                       [](WeakPtr<FilePathWatcherFSEvents> weak_watcher,
+                          FSEventStreamEventId root_change_at) {
+                         if (!weak_watcher)
+                           return;
+                         FilePathWatcherFSEvents* watcher = weak_watcher.get();
+                         dispatch_async(watcher->queue_, ^{

[gab, 2017/01/05 21:22:53]

Is the async dispatch required here? i.e. it was required before to avoid
running from within the callback itself but this won't be the case from the
posted task, right?

PS: I don't understand ObjC :), can you have someone who does take a look as
well?

[fdoray, 2017/01/05 23:04:11]

Yes it's required. UpdateEventStream() must run on the same GCD sequence as
StartEventStream/DestroyEventStream/FSEventsCallback. I tried to clarify the
comment. Added mark@ to review this.

+                           watcher->UpdateEventStream(root_change_at);
+                         });
+                       },
+                       watcher->weak_factory_.GetWeakPtr(), root_change_at));
   }
 
   watcher->OnFilePathsChanged(paths);
 }
 
-FilePathWatcherFSEvents::~FilePathWatcherFSEvents() {
-  // This method may be called on either the libdispatch or task_runner()
-  // thread. Checking callback_ on the libdispatch thread here is safe because
-  // it is executing in a task posted by Cancel() which first reset callback_.
-  // PostTask forms a sufficient memory barrier to ensure that the value is
-  // consistent on the target thread.
-  DCHECK(callback_.is_null())
-      << "Cancel() must be called before FilePathWatcher is destroyed.";
-}
-
 void FilePathWatcherFSEvents::OnFilePathsChanged(
     const std::vector<FilePath>& paths) {
   DCHECK(!resolved_target_.empty());
   task_runner()->PostTask(
-      FROM_HERE, Bind(&FilePathWatcherFSEvents::DispatchEvents, this, paths,
-                      target_, resolved_target_));
+      FROM_HERE,
+      Bind(&FilePathWatcherFSEvents::DispatchEvents, weak_factory_.GetWeakPtr(),
+           paths, target_, resolved_target_));
 }
 
 void FilePathWatcherFSEvents::DispatchEvents(const std::vector<FilePath>& paths,
                                              const FilePath& target,
                                              const FilePath& resolved_target) {
   DCHECK(task_runner()->RunsTasksOnCurrentThread());
 
   // Don't issue callbacks after Cancel() has been called.
   if (is_cancelled() || callback_.is_null()) {
     return;
   }
 
   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 task_runner(). This implementation, however, needs to
-  // cancel pending work on the Dispatch Queue thread.
-
-  if (fsevent_stream_) {
-    DestroyEventStream();
-    target_.clear();
-    resolved_target_.clear();
-  }
-}
-
 void FilePathWatcherFSEvents::UpdateEventStream(
     FSEventStreamEventId start_event) {
   // 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 (resolved_target_.empty())
     return;
 
   if (fsevent_stream_)
     DestroyEventStream();
 
@@ skipping 15 matching lines @@
   context.copyDescription = NULL;
 
   fsevent_stream_ = FSEventStreamCreate(NULL, &FSEventsCallback, &context,
                                         watched_paths,
                                         start_event,
                                         kEventLatencySeconds,
                                         kFSEventStreamCreateFlagWatchRoot);
   FSEventStreamSetDispatchQueue(fsevent_stream_, queue_);
 
   if (!FSEventStreamStart(fsevent_stream_)) {
-    task_runner()->PostTask(
-        FROM_HERE, Bind(&FilePathWatcherFSEvents::ReportError, this, target_));
+    task_runner()->PostTask(FROM_HERE,
+                            Bind(&FilePathWatcherFSEvents::ReportError,
+                                 weak_factory_.GetWeakPtr(), target_));
   }
 }
 
 bool FilePathWatcherFSEvents::ResolveTargetPath() {
   FilePath resolved = ResolvePath(target_).StripTrailingSeparators();
   bool changed = resolved != resolved_target_;
   resolved_target_ = resolved;
   if (resolved_target_.empty()) {
-    task_runner()->PostTask(
-        FROM_HERE, Bind(&FilePathWatcherFSEvents::ReportError, this, target_));
+    task_runner()->PostTask(FROM_HERE,
+                            Bind(&FilePathWatcherFSEvents::ReportError,
+                                 weak_factory_.GetWeakPtr(), target_));
   }
   return changed;
 }
 
 void FilePathWatcherFSEvents::ReportError(const FilePath& target) {
   DCHECK(task_runner()->RunsTasksOnCurrentThread());
   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,
                                                const FilePath& path) {
   DCHECK(resolved_target_.empty());
 
   target_ = path;
   ResolveTargetPath();
   UpdateEventStream(start_event);
 }
 
 }  // namespace base