Remove ref-counting from FilePathWatcher.

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 8e73b557aa291d7967ffb84d19183316cc619c1c..51a2c4044597ae296912a0f1eded41312dcc5ed6 100644
--- a/base/files/file_path_watcher_fsevents.cc
+++ b/base/files/file_path_watcher_fsevents.cc
@@ -13,7 +13,6 @@
 #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"
 
@@ -69,10 +68,16 @@ FilePath ResolvePath(const FilePath& path) {
 
 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,
@@ -105,11 +110,15 @@ 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();
+    }
   });
 }
 
@@ -140,31 +149,35 @@ void FilePathWatcherFSEvents::FSEventsCallback(
   // 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,
@@ -185,18 +198,6 @@ void FilePathWatcherFSEvents::DispatchEvents(const std::vector<FilePath>& paths,
   }
 }
 
-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
@@ -232,8 +233,9 @@ void FilePathWatcherFSEvents::UpdateEventStream(
   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_));
   }
 }
 
@@ -242,8 +244,9 @@ bool FilePathWatcherFSEvents::ResolveTargetPath() {
   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;
 }