Remove ref-counting from FilePathWatcher.

base/files/file_path_watcher_linux.cc

 // Copyright (c) 2012 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.h"
 
 #include <errno.h>
 #include <stddef.h>
 #include <string.h>
 #include <sys/inotify.h>
@@ skipping 10 matching lines @@
 
 #include "base/bind.h"
 #include "base/containers/hash_tables.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/lazy_instance.h"
 #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"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/trace_event.h"
 
 namespace base {
 
@@ skipping 58 matching lines @@
   // |created| is true if the object appears.
   // |deleted| is true if the object disappears.
   // |is_dir| is true if the object is a directory.
   void OnFilePathChanged(InotifyReader::Watch fired_watch,
                          const FilePath::StringType& child,
                          bool created,
                          bool deleted,
                          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,
              bool recursive,
              const FilePathWatcher::Callback& callback) override;
 
   // 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:
   // - |watch|: the watch descriptor for a component.
   // - |subdir|: the subdirectory that identifies the next component.
   //   - For the last component, there is no next component, so it is empty.
   // - |linkname|: the target of the symlink.
   //   - Only if the target being watched is a symbolic link.
   struct WatchEntry {
     explicit WatchEntry(const FilePath::StringType& dirname)
@@ skipping 44 matching lines @@
   bool recursive_;
 
   // The vector of watches and next component names for all path components,
   // starting at the root directory. The last entry corresponds to the watch for
   // |target_| and always stores an empty next component name in |subdir|.
   WatchVector watches_;
 
   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);
 };
 
 void InotifyReaderCallback(InotifyReader* reader, int inotify_fd) {
   // Make sure the file descriptors are good for use with select().
   CHECK_LE(0, inotify_fd);
   CHECK_GT(FD_SETSIZE, inotify_fd);
 
   trace_event::TraceLog::GetInstance()->SetCurrentThreadBlocksMessageLoop();
@@ skipping 106 matching lines @@
        ++watcher) {
     (*watcher)->OnFilePathChanged(event->wd,
                                   child,
                                   event->mask & (IN_CREATE | IN_MOVED_TO),
                                   event->mask & (IN_DELETE | IN_MOVED_FROM),
                                   event->mask & IN_ISDIR);
   }
 }
 
 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;
-  }
+  // 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));
+}
 
-  // 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;
-  }
-
+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.
   bool did_update = false;
 
   // Find the entry in |watches_| that corresponds to |fired_watch|.
   for (size_t i = 0; i < watches_.size(); ++i) {
     const WatchEntry& watch_entry = watches_[i];
     if (fired_watch != watch_entry.watch)
       continue;
@@ skipping 81 matching lines @@
   target_.GetComponents(&comps);
   DCHECK(!comps.empty());
   for (size_t i = 1; i < comps.size(); ++i)
     watches_.push_back(WatchEntry(comps[i]));
   watches_.push_back(WatchEntry(FilePath::StringType()));
   UpdateWatches();
   return true;
 }
 
 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() {
   // Ensure this runs on the task_runner() exclusively in order to avoid
   // concurrency issues.
   DCHECK(task_runner()->BelongsToCurrentThread());
   DCHECK(HasValidWatchVector());
 
   // Walk the list of watches and update them as we go.
   FilePath path(FILE_PATH_LITERAL("/"));
@@ skipping 169 matching lines @@
 }
 
 }  // namespace
 
 FilePathWatcher::FilePathWatcher() {
   sequence_checker_.DetachFromSequence();
   impl_ = new FilePathWatcherImpl();
 }
 
 }  // namespace base