[Linux] Make FilePathWatcher somewhat more symlink aware.

This is still broken in a variety of ways.

BUG=91561
TEST=updated FilePathWatcher tests and manual testing

Review URL: http://codereview.chromium.org/7583034

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@99657 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 227 insertions, 33 deletions

diff --git a/base/files/file_path_watcher_browsertest.cc b/base/files/file_path_watcher_browsertest.cc
index 8e5f361..e59f702 100644
--- a/base/files/file_path_watcher_browsertest.cc
+++ b/base/files/file_path_watcher_browsertest.cc
@@ -166,6 +166,10 @@ class FilePathWatcherTest : public testing::Test {
     return temp_dir_.path().AppendASCII("FilePathWatcherTest");
   }
 
+  FilePath test_link() {
+    return temp_dir_.path().AppendASCII("FilePathWatcherTest.lnk");
+  }
+
   // Write |content| to |file|. Returns true on success.
   bool WriteFile(const FilePath& file, const std::string& content) {
     int write_size = file_util::WriteFile(file, content.c_str(),
@@ -495,6 +499,166 @@ TEST_F(FilePathWatcherTest, FileAttributesChanged) {
 
 #endif  // !OS_LINUX
 
+#if defined(OS_LINUX)
+
+// Verify that creating a symlink is caught.
+TEST_F(FilePathWatcherTest, CreateLink) {
+  FilePathWatcher watcher;
+  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
+  // Note that we are watching the symlink
+  ASSERT_TRUE(SetupWatch(test_link(), &watcher, delegate.get()));
+
+  // Now make sure we get notified if the link is created.
+  // Note that test_file() doesn't have to exist.
+  ASSERT_TRUE(file_util::CreateSymbolicLink(test_file(), test_link()));
+  ASSERT_TRUE(WaitForEvents());
+}
+
+// Verify that deleting a symlink is caught.
+TEST_F(FilePathWatcherTest, DeleteLink) {
+  // Unfortunately this test case only works if the link target exists.
+  // TODO(craig) fix this as part of crbug.com/91561.
+  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  ASSERT_TRUE(file_util::CreateSymbolicLink(test_file(), test_link()));
+  FilePathWatcher watcher;
+  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
+  ASSERT_TRUE(SetupWatch(test_link(), &watcher, delegate.get()));
+
+  // Now make sure we get notified if the link is deleted.
+  ASSERT_TRUE(file_util::Delete(test_link(), false));
+  ASSERT_TRUE(WaitForEvents());
+}
+
+// Verify that modifying a target file that a link is pointing to
+// when we are watching the link is caught.
+TEST_F(FilePathWatcherTest, ModifiedLinkedFile) {
+  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  ASSERT_TRUE(file_util::CreateSymbolicLink(test_file(), test_link()));
+  FilePathWatcher watcher;
+  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
+  // Note that we are watching the symlink.
+  ASSERT_TRUE(SetupWatch(test_link(), &watcher, delegate.get()));
+
+  // Now make sure we get notified if the file is modified.
+  ASSERT_TRUE(WriteFile(test_file(), "new content"));
+  ASSERT_TRUE(WaitForEvents());
+}
+
+// Verify that creating a target file that a link is pointing to
+// when we are watching the link is caught.
+TEST_F(FilePathWatcherTest, CreateTargetLinkedFile) {
+  ASSERT_TRUE(file_util::CreateSymbolicLink(test_file(), test_link()));
+  FilePathWatcher watcher;
+  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
+  // Note that we are watching the symlink.
+  ASSERT_TRUE(SetupWatch(test_link(), &watcher, delegate.get()));
+
+  // Now make sure we get notified if the target file is created.
+  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  ASSERT_TRUE(WaitForEvents());
+}
+
+// Verify that deleting a target file that a link is pointing to
+// when we are watching the link is caught.
+TEST_F(FilePathWatcherTest, DeleteTargetLinkedFile) {
+  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  ASSERT_TRUE(file_util::CreateSymbolicLink(test_file(), test_link()));
+  FilePathWatcher watcher;
+  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
+  // Note that we are watching the symlink.
+  ASSERT_TRUE(SetupWatch(test_link(), &watcher, delegate.get()));
+
+  // Now make sure we get notified if the target file is deleted.
+  ASSERT_TRUE(file_util::Delete(test_file(), false));
+  ASSERT_TRUE(WaitForEvents());
+}
+
+// Verify that watching a file whose parent directory is a link that
+// doesn't exist yet works if the symlink is created eventually.
+TEST_F(FilePathWatcherTest, LinkedDirectoryPart1) {
+  FilePathWatcher watcher;
+  FilePath dir(temp_dir_.path().AppendASCII("dir"));
+  FilePath link_dir(temp_dir_.path().AppendASCII("dir.lnk"));
+  FilePath file(dir.AppendASCII("file"));
+  FilePath linkfile(link_dir.AppendASCII("file"));
+  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
+  // dir/file should exist.
+  ASSERT_TRUE(file_util::CreateDirectory(dir));
+  ASSERT_TRUE(WriteFile(file, "content"));
+  // Note that we are watching dir.lnk/file which doesn't exist yet.
+  ASSERT_TRUE(SetupWatch(linkfile, &watcher, delegate.get()));
+
+  ASSERT_TRUE(file_util::CreateSymbolicLink(dir, link_dir));
+  VLOG(1) << "Waiting for link creation";
+  ASSERT_TRUE(WaitForEvents());
+
+  ASSERT_TRUE(WriteFile(file, "content v2"));
+  VLOG(1) << "Waiting for file change";
+  ASSERT_TRUE(WaitForEvents());
+
+  ASSERT_TRUE(file_util::Delete(file, false));
+  VLOG(1) << "Waiting for file deletion";
+  ASSERT_TRUE(WaitForEvents());
+}
+
+// Verify that watching a file whose parent directory is a
+// dangling symlink works if the directory is created eventually.
+TEST_F(FilePathWatcherTest, LinkedDirectoryPart2) {
+  FilePathWatcher watcher;
+  FilePath dir(temp_dir_.path().AppendASCII("dir"));
+  FilePath link_dir(temp_dir_.path().AppendASCII("dir.lnk"));
+  FilePath file(dir.AppendASCII("file"));
+  FilePath linkfile(link_dir.AppendASCII("file"));
+  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
+  // Now create the link from dir.lnk pointing to dir but
+  // neither dir nor dir/file exist yet.
+  ASSERT_TRUE(file_util::CreateSymbolicLink(dir, link_dir));
+  // Note that we are watching dir.lnk/file.
+  ASSERT_TRUE(SetupWatch(linkfile, &watcher, delegate.get()));
+
+  ASSERT_TRUE(file_util::CreateDirectory(dir));
+  ASSERT_TRUE(WriteFile(file, "content"));
+  VLOG(1) << "Waiting for dir/file creation";
+  ASSERT_TRUE(WaitForEvents());
+
+  ASSERT_TRUE(WriteFile(file, "content v2"));
+  VLOG(1) << "Waiting for file change";
+  ASSERT_TRUE(WaitForEvents());
+
+  ASSERT_TRUE(file_util::Delete(file, false));
+  VLOG(1) << "Waiting for file deletion";
+  ASSERT_TRUE(WaitForEvents());
+}
+
+// Verify that watching a file with a symlink on the path
+// to the file works.
+TEST_F(FilePathWatcherTest, LinkedDirectoryPart3) {
+  FilePathWatcher watcher;
+  FilePath dir(temp_dir_.path().AppendASCII("dir"));
+  FilePath link_dir(temp_dir_.path().AppendASCII("dir.lnk"));
+  FilePath file(dir.AppendASCII("file"));
+  FilePath linkfile(link_dir.AppendASCII("file"));
+  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
+  ASSERT_TRUE(file_util::CreateDirectory(dir));
+  ASSERT_TRUE(file_util::CreateSymbolicLink(dir, link_dir));
+  // Note that we are watching dir.lnk/file but the file doesn't exist yet.
+  ASSERT_TRUE(SetupWatch(linkfile, &watcher, delegate.get()));
+
+  ASSERT_TRUE(WriteFile(file, "content"));
+  VLOG(1) << "Waiting for file creation";
+  ASSERT_TRUE(WaitForEvents());
+
+  ASSERT_TRUE(WriteFile(file, "content v2"));
+  VLOG(1) << "Waiting for file change";
+  ASSERT_TRUE(WaitForEvents());
+
+  ASSERT_TRUE(file_util::Delete(file, false));
+  VLOG(1) << "Waiting for file deletion";
+  ASSERT_TRUE(WaitForEvents());
+}
+
+#endif  // OS_LINUX
+
 enum Permission {
   Read,
   Write,
diff --git a/base/files/file_path_watcher_linux.cc b/base/files/file_path_watcher_linux.cc
index 265366b..2dfd54bd 100644
--- a/base/files/file_path_watcher_linux.cc
+++ b/base/files/file_path_watcher_linux.cc
@@ -119,7 +119,8 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate,
 
   // 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.
+  // 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),
@@ -127,6 +128,7 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate,
 
     InotifyReader::Watch watch_;
     FilePath::StringType subdir_;
+    FilePath::StringType linkname_;
   };
   typedef std::vector<WatchEntry> WatchVector;
 
@@ -328,41 +330,47 @@ void FilePathWatcherImpl::OnFilePathChanged(
   // 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_)
-      break;
-  }
-
-  // If this notification is from a previous generation of watches or the watch
-  // has been cancelled (|watches_| is empty then), bail out.
-  if (watch_entry == watches_.end())
-    return;
-
-  // Check whether a path component of |target_| changed.
-  bool change_on_target_path = child.empty() || child == watch_entry->subdir_;
-
-  // 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() ||
-      (watch_entry->subdir_ == child && (++watch_entry)->subdir_.empty());
+    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_);
+
+      // 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());
+
+      // Update watches if a directory component of the |target_| path
+      // (dis)appears. Note that we don't add the additional restriction
+      // of checking is_directory here as changes to symlinks on the
+      // target path will not have is_directory set but as a result we
+      // may sometimes call UpdateWatches unnecessarily.
+      if (change_on_target_path && !UpdateWatches()) {
+        delegate_->OnFilePathError(target_);
+        return;
+      }
 
-  // Update watches if a directory component of the |target_| path (dis)appears.
-  if (is_directory && change_on_target_path && !UpdateWatches()) {
-    delegate_->OnFilePathError(target_);
-    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 && file_util::PathExists(target_))) {
+        delegate_->OnFilePathChanged(target_);
+        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 && file_util::PathExists(target_))) {
-    delegate_->OnFilePathChanged(target_);
-  }
 }
 
 bool FilePathWatcherImpl::Watch(const FilePath& path,
@@ -438,6 +446,28 @@ bool FilePathWatcherImpl::UpdateWatches() {
     InotifyReader::Watch old_watch = watch_entry->watch_;
     if (path_valid) {
       watch_entry->watch_ = g_inotify_reader.Get().AddWatch(path, this);
+      if ((watch_entry->watch_ == InotifyReader::kInvalidWatch) &&
+          file_util::IsLink(path)) {
+        FilePath link;
+        file_util::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.
+        }
+      }
       if (watch_entry->watch_ == InotifyReader::kInvalidWatch) {
         path_valid = false;
       }