Remove ref-counting from FilePathWatcher.

base/files/file_path_watcher_linux.cc

Index: base/files/file_path_watcher_linux.cc
diff --git a/base/files/file_path_watcher_linux.cc b/base/files/file_path_watcher_linux.cc
index bf17cde45f7036aea326d33190b478eb3d1df8d2..a129bc3c6aa036e175f18f67859fad299caf3082 100644
--- a/base/files/file_path_watcher_linux.cc
+++ b/base/files/file_path_watcher_linux.cc
@@ -28,6 +28,7 @@
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/single_thread_task_runner.h"
 #include "base/stl_util.h"
@@ -106,12 +107,15 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate {
                          bool is_dir);
 
  protected:
-  ~FilePathWatcherImpl() override {
-    in_destructor_ = true;
-    CancelOnMessageLoopThreadOrInDestructor();
-  }
+  ~FilePathWatcherImpl() override = default;
 
  private:
+  void OnFilePathChangedOnOriginSequence(InotifyReader::Watch fired_watch,
+                                         const FilePath::StringType& child,
+                                         bool created,
+                                         bool deleted,
+                                         bool is_dir);
+
   // Start watching |path| for changes and notify |delegate| on each change.
   // Returns true if watch for |path| has been added successfully.
   bool Watch(const FilePath& path,
@@ -120,7 +124,6 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate {
 
   // Cancel the watch. This unregisters the instance with InotifyReader.
   void Cancel() override;
-  void CancelOnMessageLoopThreadOrInDestructor();
 
   // Inotify watches are installed for all directory components of |target_|.
   // A WatchEntry instance holds:
@@ -185,7 +188,7 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate {
   hash_map<InotifyReader::Watch, FilePath> recursive_paths_by_watch_;
   std::map<FilePath, InotifyReader::Watch> recursive_watches_by_path_;
 
-  bool in_destructor_ = false;
+  WeakPtrFactory<FilePathWatcherImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(FilePathWatcherImpl);
 };
@@ -312,30 +315,31 @@ void InotifyReader::OnInotifyEvent(const inotify_event* event) {
 }
 
 FilePathWatcherImpl::FilePathWatcherImpl()
-    : recursive_(false) {
-}
+    : recursive_(false), weak_factory_(this) {}
 
 void FilePathWatcherImpl::OnFilePathChanged(InotifyReader::Watch fired_watch,
                                             const FilePath::StringType& child,
                                             bool created,
                                             bool deleted,
                                             bool is_dir) {
-  if (!task_runner()->BelongsToCurrentThread()) {
-    // Switch to task_runner() to access |watches_| safely.
-    task_runner()->PostTask(FROM_HERE,
-                            Bind(&FilePathWatcherImpl::OnFilePathChanged, this,
-                                 fired_watch, child, created, deleted, is_dir));
-    return;
-  }
-
-  // Check to see if CancelOnMessageLoopThreadOrInDestructor() has already been
-  // called. May happen when code flow reaches here from the PostTask() above.
-  if (watches_.empty()) {
-    DCHECK(target_.empty());
-    return;
-  }
+  // This method is invoked on the Inotify thread. Switch to task_runner() to

[gab, 2016/12/23 16:05:06]

DCHECK not on TaskRunner thread?

[fdoray, 2016/12/23 21:16:07]

Done.

+  // access |watches_| safely. Use a WeakPtr to prevent the callback from
+  // running after the destructor is invoked (i.e. after the watch is
+  // cancelled).

[fdoray, 2016/12/23 15:34:45]

Cancel() is only called from ~FilePathWatcher
https://cs.chromium.org/chromium/src/base/files/file_path_watcher.cc?sq=packa...
With this CL, the only reference to |this| is owned by FilePathWatcher. This
means that |this| is destroyed and its weak ptrs are invalidated right after
Cancel() is called.

In separate CLs, I plan to:
- Get rid of ref-counting in file_path_watcher_*, to make it clear that a
FilePathWatcherImpl is owned by a FilePathWatcher (and does not outlive this
FilePathWatcher).
- Move code from FilePathWatcherImpl::Cancel() to
FilePathWatcherImpl::~FilePathWatcherImpl() and remove
FilePathWatcherImpl::Cancel(). 

[gab, 2016/12/23 16:05:06]

Ah cool now I see why you'd made all those changes. I think we should bring the
refcount change to this CL though as mixing WeakPtr and refcounting semantics is
weird.

[fdoray, 2016/12/23 21:16:07]

Done.

+  task_runner()->PostTask(
+      FROM_HERE, Bind(&FilePathWatcherImpl::OnFilePathChangedOnOriginSequence,
+                      weak_factory_.GetWeakPtr(), fired_watch, child, created,

[gab, 2016/12/23 16:05:06]

It's not safe to vend WeakPtr from one sequence and deref on another. You'll
need to store a WeakPtr as member in constructor I think (as you were doing
previously but I hadn't realized what was going on)

[fdoray, 2016/12/23 21:16:07]

Actually, the WeakPtrs of a given WeakPtrFactory are bound to the first sequence
on which one of them is dereferenced. There is nothing unsafe about getting a
WeakPtr from the factory on another sequence (as long as we know that the
factory won't be deleted during the call - which is the case because
OnFilePathChanged is called within the scope of InotifyReader::lock_ and
~FilePathWatcherImpl removes |this| from InotifyReader::watchers_ within the
scope of InotifyReader::lock_).

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

Good point, and it's implicit that FilePathWatcherImpl must not be deleted while
a member method call is in progress so it works, don't mind me :)!

+                      deleted, is_dir));
+}
 
+void FilePathWatcherImpl::OnFilePathChangedOnOriginSequence(
+    InotifyReader::Watch fired_watch,
+    const FilePath::StringType& child,
+    bool created,
+    bool deleted,
+    bool is_dir) {
   DCHECK(task_runner()->BelongsToCurrentThread());
+  DCHECK(!watches_.empty());
   DCHECK(HasValidWatchVector());
 
   // Used below to avoid multiple recursive updates.
@@ -437,41 +441,23 @@ bool FilePathWatcherImpl::Watch(const FilePath& path,
 }
 
 void FilePathWatcherImpl::Cancel() {
-  if (callback_.is_null()) {
-    // Watch was never called, or the message_loop() thread is already gone.
+  if (!callback_) {
+    // Watch() was never called.
     set_cancelled();
     return;
   }
 
-  // Switch to the task_runner() if necessary so we can access |watches_|.
-  if (!task_runner()->BelongsToCurrentThread()) {
-    task_runner()->PostTask(
-        FROM_HERE,
-        Bind(&FilePathWatcherImpl::CancelOnMessageLoopThreadOrInDestructor,
-             this));
-  } else {
-    CancelOnMessageLoopThreadOrInDestructor();
-  }
-}
-
-void FilePathWatcherImpl::CancelOnMessageLoopThreadOrInDestructor() {
-  DCHECK(in_destructor_ || task_runner()->BelongsToCurrentThread());
-
-  if (is_cancelled())
-    return;
+  DCHECK(task_runner()->BelongsToCurrentThread());
+  DCHECK(!is_cancelled());
 
   set_cancelled();
-
-  if (!callback_.is_null())
-    callback_.Reset();
+  callback_.Reset();
 
   for (size_t i = 0; i < watches_.size(); ++i)
     g_inotify_reader.Get().RemoveWatch(watches_[i].watch, this);
   watches_.clear();
   target_.clear();
-
-  if (recursive_)
-    RemoveRecursiveWatches();
+  RemoveRecursiveWatches();
 }
 
 void FilePathWatcherImpl::UpdateWatches() {