Clean up the Linux FileWatcher implementation.

base/files/file_path_watcher_linux.cc

Index: base/files/file_path_watcher_linux.cc
===================================================================
--- base/files/file_path_watcher_linux.cc	(revision 265970)
+++ base/files/file_path_watcher_linux.cc	(working copy)
@@ -49,8 +49,8 @@
   // change. Returns kInvalidWatch on failure.
   Watch AddWatch(const FilePath& path, FilePathWatcherImpl* watcher);
 
-  // Remove |watch|. Returns true on success.
-  bool RemoveWatch(Watch watch, FilePathWatcherImpl* watcher);

[Lei Zhang, 2014/04/25 06:45:23]

Nobody checks the return result.

+  // Remove |watch| if it's valid.
+  void RemoveWatch(Watch watch, FilePathWatcherImpl* watcher);
 
   // Callback for InotifyReaderTask.
   void OnInotifyEvent(const inotify_event* event);
@@ -97,6 +97,10 @@
                          const FilePath::StringType& child,
                          bool created);
 
+ protected:
+  virtual ~FilePathWatcherImpl() {}
+
+ private:
   // Start watching |path| for changes and notify |delegate| on each change.
   // Returns true if watch for |path| has been added successfully.
   virtual bool Watch(const FilePath& path,
@@ -106,25 +110,21 @@
   // Cancel the watch. This unregisters the instance with InotifyReader.
   virtual void Cancel() OVERRIDE;
 
+  // Cleans up and stops observing the message_loop() thread.
+  virtual void CancelOnMessageLoopThread() OVERRIDE;
+
   // Deletion of the FilePathWatcher will call Cancel() to dispose of this
   // object in the right thread. This also observes destruction of the required
   // cleanup thread, in case it quits before Cancel() is called.
   virtual void WillDestroyCurrentMessageLoop() OVERRIDE;
 
- protected:
-  virtual ~FilePathWatcherImpl() {}
-
- private:
-  // Cleans up and stops observing the |message_loop_| thread.
-  virtual void CancelOnMessageLoopThread() OVERRIDE;
-
   // Inotify watches are installed for all directory components of |target_|. A
   // WatchEntry instance holds the watch descriptor for a component and the
   // subdirectory for that identifies the next component. If a symbolic link
   // is being watched, the target of the link is also kept.
   struct WatchEntry {
-    WatchEntry(InotifyReader::Watch watch, const FilePath::StringType& subdir)

[Lei Zhang, 2014/04/25 06:45:23]

Always called with |watch| = -1

-        : watch_(watch),
+    explicit WatchEntry(const FilePath::StringType& subdir)
+        : watch_(InotifyReader::kInvalidWatch),
           subdir_(subdir) {}
 
     InotifyReader::Watch watch_;
@@ -137,6 +137,13 @@
   // that exists. Updates |watched_path_|. Returns true on success.
   bool UpdateWatches() WARN_UNUSED_RESULT;
 
+  // If |path| is a symlink to a non-existent target, attempt to add a watch to
+  // the link target's parent directory. Returns true and update watch_entry on
+  // success.
+  bool AddWatchForBrokenSymlink(const FilePath& path, WatchEntry* watch_entry);
+
+  bool HasValidWatchVector() const;
+
   // Callback to notify upon changes.
   FilePathWatcher::Callback callback_;
 
@@ -224,8 +231,8 @@
   shutdown_pipe_[1] = -1;
   if (inotify_fd_ >= 0 && pipe(shutdown_pipe_) == 0 && thread_.Start()) {
     thread_.message_loop()->PostTask(
-        FROM_HERE, Bind(&InotifyReaderCallback, this, inotify_fd_,
-                              shutdown_pipe_[0]));
+        FROM_HERE,
+        Bind(&InotifyReaderCallback, this, inotify_fd_, shutdown_pipe_[0]));
     valid_ = true;
   }
 }
@@ -267,10 +274,9 @@
   return watch;
 }
 
-bool InotifyReader::RemoveWatch(Watch watch,
-                                FilePathWatcherImpl* watcher) {
-  if (!valid_)
-    return false;
+void InotifyReader::RemoveWatch(Watch watch, FilePathWatcherImpl* watcher) {
+  if (!valid_ || (watch == kInvalidWatch))

[Lei Zhang, 2014/04/25 06:45:23]

Slightly easier to check for invalid |watch| here than in all the callers.

+    return;
 
   AutoLock auto_lock(lock_);
 
@@ -278,17 +284,15 @@
 
   if (watchers_[watch].empty()) {
     watchers_.erase(watch);
-    return (inotify_rm_watch(inotify_fd_, watch) == 0);
+    inotify_rm_watch(inotify_fd_, watch);
   }
-
-  return true;
 }
 
 void InotifyReader::OnInotifyEvent(const inotify_event* event) {
   if (event->mask & IN_IGNORED)
     return;
 
-  FilePath::StringType child(event->len ? event->name : FILE_PATH_LITERAL(""));
+  FilePath::StringType child(event->len ? event->name : "");
   AutoLock auto_lock(lock_);
 
   for (WatcherSet::iterator watcher = watchers_[event->wd].begin();
@@ -307,61 +311,68 @@
                                             const FilePath::StringType& child,
                                             bool created) {
   if (!message_loop()->BelongsToCurrentThread()) {
-    // Switch to message_loop_ to access watches_ safely.
-    message_loop()->PostTask(FROM_HERE,
-        Bind(&FilePathWatcherImpl::OnFilePathChanged,
-                   this,
-                   fired_watch,
-                   child,
-                   created));
+    // Switch to message_loop() to access |watches_| safely.
+    message_loop()->PostTask(
+        FROM_HERE,
+        Bind(&FilePathWatcherImpl::OnFilePathChanged, this,
+             fired_watch, child, created));
     return;
   }
 
+  // Check to see if CancelOnMessageLoopThread() has already been called.
+  // Happens when code flow reaches here from the PostTask() above.
+  if (watches_.empty()) {

[Lei Zhang, 2014/04/25 06:45:23]

Only needed because we call DCHECK(HasValidWatchVector()); below. I added
HasValidWatchVector() so when we operate on |watches_|, we can check and make
sure the variants have been satisfied.

+    DCHECK(target_.empty());
+    return;
+  }
+
   DCHECK(MessageLoopForIO::current());
+  DCHECK(HasValidWatchVector());
 
   // Find the entry in |watches_| that corresponds to |fired_watch|.
-  WatchVector::const_iterator watch_entry(watches_.begin());
-  for ( ; watch_entry != watches_.end(); ++watch_entry) {
-    if (fired_watch == watch_entry->watch_) {
-      // Check whether a path component of |target_| changed.
-      bool change_on_target_path = child.empty() ||
-          ((child == watch_entry->subdir_) && watch_entry->linkname_.empty()) ||
-          (child == watch_entry->linkname_);
+  for (size_t i = 0; i < watches_.size(); ++i) {
+    const WatchEntry& watch_entry = watches_[i];
+    if (fired_watch != watch_entry.watch_)
+      continue;
 
-      // Check whether the change references |target_| or a direct child.
-      DCHECK(watch_entry->subdir_.empty() ||

[Lei Zhang, 2014/04/25 06:45:23]

No longer needed since we already validated |watches_| before the loop.

-          (watch_entry + 1) != watches_.end());
-      bool target_changed =
-          (watch_entry->subdir_.empty() && (child == watch_entry->linkname_)) ||
-          (watch_entry->subdir_.empty() && watch_entry->linkname_.empty()) ||
-          (watch_entry->subdir_ == child && (watch_entry + 1)->subdir_.empty());
+    // Check whether a path component of |target_| changed.
+    bool change_on_target_path =
+        child.empty() ||
+        (child == watch_entry.linkname_) ||

[Lei Zhang, 2014/04/25 06:45:23]

I reordered the checks to avoid checking for linkname_.empty(). |child| has to
be non-empty due to the prev line, so if we pass this check, on the next line,
linkname_.empty() is implied.

+        (child == watch_entry.subdir_);
 
-      // Update watches if a directory component of the |target_| path
-      // (dis)appears. Note that we don't add the additional restriction
-      // of checking the event mask to see if it is for a directory here
-      // as changes to symlinks on the target path will not have
-      // IN_ISDIR set in the event masks. As a result we may sometimes
-      // call UpdateWatches() unnecessarily.
-      if (change_on_target_path && !UpdateWatches()) {
-        callback_.Run(target_, true /* error */);
-        return;
-      }
+    // Check if the change references |target_| or a direct child of |target_|.
+    bool is_direct_child = watch_entry.subdir_.empty();
+    bool target_changed =
+        (is_direct_child && (child == watch_entry.linkname_)) ||
+        (is_direct_child && watch_entry.linkname_.empty()) ||
+        (watch_entry.subdir_ == child && watches_[i + 1].subdir_.empty());
 
-      // Report the following events:
-      //  - The target or a direct child of the target got changed (in case the
-      //    watched path refers to a directory).
-      //  - One of the parent directories got moved or deleted, since the target
-      //    disappears in this case.
-      //  - One of the parent directories appears. The event corresponding to
-      //    the target appearing might have been missed in this case, so
-      //    recheck.
-      if (target_changed ||
-          (change_on_target_path && !created) ||
-          (change_on_target_path && PathExists(target_))) {
-        callback_.Run(target_, false);
-        return;
-      }
+    // Update watches if a directory component of the |target_| path
+    // (dis)appears. Note that we don't add the additional restriction
+    // of checking the event mask to see if it is for a directory here
+    // as changes to symlinks on the target path will not have
+    // IN_ISDIR set in the event masks. As a result we may sometimes
+    // call UpdateWatches() unnecessarily.
+    if (change_on_target_path && !UpdateWatches()) {
+      callback_.Run(target_, true /* error */);
+      return;
     }
+
+    // Report the following events:
+    //  - The target or a direct child of the target got changed (in case the
+    //    watched path refers to a directory).
+    //  - One of the parent directories got moved or deleted, since the target
+    //    disappears in this case.
+    //  - One of the parent directories appears. The event corresponding to
+    //    the target appearing might have been missed in this case, so
+    //    recheck.
+    if (target_changed ||
+        (change_on_target_path && !created) ||
+        (change_on_target_path && PathExists(target_))) {
+      callback_.Run(target_, false /* error */);
+      return;
+    }
   }
 }
 
@@ -384,46 +395,39 @@
   std::vector<FilePath::StringType> comps;
   target_.GetComponents(&comps);
   DCHECK(!comps.empty());
-  std::vector<FilePath::StringType>::const_iterator comp = comps.begin();
-  for (++comp; comp != comps.end(); ++comp)
-    watches_.push_back(WatchEntry(InotifyReader::kInvalidWatch, *comp));
-
-  watches_.push_back(WatchEntry(InotifyReader::kInvalidWatch,
-                                FilePath::StringType()));
+  for (size_t i = 1; i < comps.size(); ++i)
+    watches_.push_back(WatchEntry(comps[i]));
+  watches_.push_back(WatchEntry(FilePath::StringType()));
   return UpdateWatches();
 }
 
 void FilePathWatcherImpl::Cancel() {
   if (callback_.is_null()) {
-    // Watch was never called, or the |message_loop_| thread is already gone.
+    // Watch was never called, or the message_loop() thread is already gone.
     set_cancelled();
     return;
   }
 
-  // Switch to the message_loop_ if necessary so we can access |watches_|.
+  // Switch to the message_loop() if necessary so we can access |watches_|.
   if (!message_loop()->BelongsToCurrentThread()) {
     message_loop()->PostTask(FROM_HERE,
                              Bind(&FilePathWatcher::CancelWatch,
-                                        make_scoped_refptr(this)));
+                                  make_scoped_refptr(this)));
   } else {
     CancelOnMessageLoopThread();
   }
 }
 
 void FilePathWatcherImpl::CancelOnMessageLoopThread() {
-  if (!is_cancelled())
-    set_cancelled();
+  set_cancelled();
 
   if (!callback_.is_null()) {
     MessageLoop::current()->RemoveDestructionObserver(this);
     callback_.Reset();
   }
 
-  for (WatchVector::iterator watch_entry(watches_.begin());
-       watch_entry != watches_.end(); ++watch_entry) {
-    if (watch_entry->watch_ != InotifyReader::kInvalidWatch)
-      g_inotify_reader.Get().RemoveWatch(watch_entry->watch_, this);
-  }
+  for (size_t i = 0; i < watches_.size(); ++i)
+    g_inotify_reader.Get().RemoveWatch(watches_[i].watch_, this);
   watches_.clear();
   target_.clear();
 }
@@ -433,57 +437,75 @@
 }
 
 bool FilePathWatcherImpl::UpdateWatches() {
-  // Ensure this runs on the |message_loop_| exclusively in order to avoid
+  // Ensure this runs on the message_loop() exclusively in order to avoid
   // concurrency issues.
   DCHECK(message_loop()->BelongsToCurrentThread());
+  DCHECK(HasValidWatchVector());
 
   // Walk the list of watches and update them as we go.
-  FilePath path(FILE_PATH_LITERAL("/"));
+  FilePath path("/");
   bool path_valid = true;
-  for (WatchVector::iterator watch_entry(watches_.begin());
-       watch_entry != watches_.end(); ++watch_entry) {
-    InotifyReader::Watch old_watch = watch_entry->watch_;
+  for (size_t i = 0; i < watches_.size(); ++i) {
+    WatchEntry& watch_entry = watches_[i];
+    InotifyReader::Watch old_watch = watch_entry.watch_;
+    watch_entry.watch_ = InotifyReader::kInvalidWatch;
+    watch_entry.linkname_.clear();

[Lei Zhang, 2014/04/25 06:45:23]

This is one actual change to the functionality. When we are updating, part of
the path that used to be a symlink may no longer be a symlink.

     if (path_valid) {
-      watch_entry->watch_ = g_inotify_reader.Get().AddWatch(path, this);
-      if ((watch_entry->watch_ == InotifyReader::kInvalidWatch) &&
-          base::IsLink(path)) {
-        FilePath link;
-        if (ReadSymbolicLink(path, &link)) {
-          if (!link.IsAbsolute())
-            link = path.DirName().Append(link);
-          // Try watching symlink target directory. If the link target is "/",
-          // then we shouldn't get here in normal situations and if we do, we'd
-          // watch "/" for changes to a component "/" which is harmless so no
-          // special treatment of this case is required.
-          watch_entry->watch_ =
-              g_inotify_reader.Get().AddWatch(link.DirName(), this);
-          if (watch_entry->watch_ != InotifyReader::kInvalidWatch) {
-            watch_entry->linkname_ = link.BaseName().value();
-          } else {
-            DPLOG(WARNING) << "Watch failed for "  << link.DirName().value();
-            // TODO(craig) Symlinks only work if the parent directory
-            // for the target exist. Ideally we should make sure we've
-            // watched all the components of the symlink path for
-            // changes. See crbug.com/91561 for details.
-          }
-        }
+      watch_entry.watch_ = g_inotify_reader.Get().AddWatch(path, this);
+      if (watch_entry.watch_ == InotifyReader::kInvalidWatch) {
+        path_valid = AddWatchForBrokenSymlink(path, &watch_entry);
       }
-      if (watch_entry->watch_ == InotifyReader::kInvalidWatch) {
-        path_valid = false;
-      }
-    } else {
-      watch_entry->watch_ = InotifyReader::kInvalidWatch;
     }
-    if (old_watch != InotifyReader::kInvalidWatch &&
-        old_watch != watch_entry->watch_) {
+    if (old_watch != watch_entry.watch_)
       g_inotify_reader.Get().RemoveWatch(old_watch, this);
-    }
-    path = path.Append(watch_entry->subdir_);
+    path = path.Append(watch_entry.subdir_);
   }
 
   return true;
 }
 
+bool FilePathWatcherImpl::AddWatchForBrokenSymlink(const FilePath& path,
+                                                   WatchEntry* watch_entry) {
+  DCHECK_EQ(InotifyReader::kInvalidWatch, watch_entry->watch_);
+  if (!IsLink(path))
+    return false;
+
+  FilePath link;
+  if (!ReadSymbolicLink(path, &link))
+    return false;
+
+  if (!link.IsAbsolute())
+    link = path.DirName().Append(link);
+
+  // Try watching symlink target directory. If the link target is "/",
+  // then we shouldn't get here in normal situations and if we do, we'd
+  // watch "/" for changes to a component "/" which is harmless so no
+  // special treatment of this case is required.
+  InotifyReader::Watch watch =
+      g_inotify_reader.Get().AddWatch(link.DirName(), this);
+  if (watch == InotifyReader::kInvalidWatch) {
+    // TODO(craig) Symlinks only work if the parent directory
+    // for the target exist. Ideally we should make sure we've
+    // watched all the components of the symlink path for
+    // changes. See crbug.com/91561 for details.
+    DPLOG(WARNING) << "Watch failed for "  << link.DirName().value();
+    return false;
+  }
+  watch_entry->watch_ = watch;
+  watch_entry->linkname_ = link.BaseName().value();
+  return true;
+}
+
+bool FilePathWatcherImpl::HasValidWatchVector() const {
+  if (watches_.empty())
+    return false;
+  for (size_t i = 0; i < watches_.size() - 1; ++i) {
+    if (watches_[i].subdir_.empty())
+      return false;
+  }
+  return watches_[watches_.size() - 1].subdir_.empty();
+}
+
 }  // namespace
 
 FilePathWatcher::FilePathWatcher() {