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);
+  // 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,30 +110,26 @@
   // 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)
-        : watch_(watch),
-          subdir_(subdir) {}
+    explicit WatchEntry(const FilePath::StringType& dirname)
+        : watch(InotifyReader::kInvalidWatch),
+          subdir(dirname) {}
 
-    InotifyReader::Watch watch_;
-    FilePath::StringType subdir_;
-    FilePath::StringType linkname_;
+    InotifyReader::Watch watch;
+    FilePath::StringType subdir;
+    FilePath::StringType linkname;
   };
   typedef std::vector<WatchEntry> WatchVector;
 
@@ -137,6 +137,13 @@
   // that exists. Updates |watched_path_|. Returns true on success.
   bool UpdateWatches() WARN_UNUSED_RESULT;
 
+  // |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_;
 
@@ -145,7 +152,7 @@
 
   // 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_|.
+  // |target_| and always stores an empty next component name in |subdir|.
   WatchVector watches_;
 
   DISALLOW_COPY_AND_ASSIGN(FilePathWatcherImpl);
@@ -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))
+    return;
 
   AutoLock auto_lock(lock_);
 
@@ -278,10 +284,8 @@
 
   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) {
@@ -307,61 +311,66 @@
                                             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.
+  // May happen when code flow reaches here from the PostTask() above.
+  if (watches_.empty()) {
+    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() ||
-          (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) ||
+        (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_watch_for_target = watch_entry.subdir.empty();
+    bool target_changed =
+        (is_watch_for_target && (child == watch_entry.linkname)) ||
+        (is_watch_for_target && 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 +393,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 +435,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("/"));
   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();
     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) {
+        if (IsLink(path)) {
+          path_valid = AddWatchForBrokenSymlink(path, &watch_entry);
+        } else {
+          path_valid = false;
         }
       }
-      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);
+  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() {