Fix two races in DirectoryWatcherInotify:

1. In DirectoryWatcherImpl::Watch we have to initialize delegate_, root_path_
   and other members before calling InotifyReader::AddWatch, because otherwise
   the following race is possible:

   1. InotifyReader::AddWatch
   2. DirectoryWatcherImpl::OnInotifyEvent, where we want to use delegate_
      and root_path_
   3. Now-late initialization of delegate_ and root_path_ in DirectoryWatcherImpl::Watch

2. In InotifyReader::OnInotifyEvent we can't just copy things under a lock
   and notify without a lock. Otherwise the following race is possible:

   1. Things get copied in InotifyReader::OnInotifyEvent, lock is released
   2. DirectoryWatcherImpl is destroyed, removing its watch
   3. But it's still in the copied set of watchers to be notified. When we get
      to notifying it, it's invalid, and we have a crash.

To fix race #2, I decided to just do everything under the lock. Notifications
shouldn't be a bottleneck, and I don't want to prematurely add more complexity.

TEST=Manually with RaceChecker, see bug.
http://crbug.com/15473

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

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

1 files changed, 13 insertions, 18 deletions

diff --git a/base/directory_watcher_inotify.cc b/base/directory_watcher_inotify.cc
index 9847569..44e4dcb 100644
--- a/base/directory_watcher_inotify.cc
+++ b/base/directory_watcher_inotify.cc
@@ -331,18 +331,17 @@ void InotifyReader::OnInotifyEvent(const inotify_event* event) {
   if (event->mask & IN_IGNORED)
     return;
 
-  WatcherSet watchers_to_notify;
-  FilePath changed_path;
-
-  {
-    AutoLock auto_lock(lock_);
-    changed_path = paths_[event->wd];
-    watchers_to_notify.insert(watchers_[event->wd].begin(),
-                              watchers_[event->wd].end());
-  }
+  // In case you want to limit the scope of this lock, it's not sufficient
+  // to just copy things under the lock, and then run the notifications
+  // without holding the lock. DirectoryWatcherImpl's dtor removes its watches,
+  // and to do that obtains the lock. After it finishes removing watches,
+  // it's destroyed. So, if you copy under the lock and notify without the lock,
+  // it's possible you'll copy the DirectoryWatcherImpl which is being
+  // destroyed, then it will destroy itself, and then you'll try to notify it.
+  AutoLock auto_lock(lock_);
 
-  for (WatcherSet::iterator watcher = watchers_to_notify.begin();
-       watcher != watchers_to_notify.end();
+  for (WatcherSet::iterator watcher = watchers_[event->wd].begin();
+       watcher != watchers_[event->wd].end();
        ++watcher) {
     (*watcher)->OnInotifyEvent(event);
   }
@@ -438,16 +437,13 @@ bool DirectoryWatcherImpl::Watch(const FilePath& path,
   if (!file_util::GetInode(path, &inode))
     return false;
 
-  InotifyReader::Watch watch =
-      Singleton<InotifyReader>::get()->AddWatch(path, this);
-  if (watch == InotifyReader::kInvalidWatch)
-    return false;
-
   delegate_ = delegate;
   recursive_ = recursive;
   root_path_ = path;
-  watch_ = watch;
   loop_ = MessageLoop::current();
+  watch_ = Singleton<InotifyReader>::get()->AddWatch(path, this);
+  if (watch_ == InotifyReader::kInvalidWatch)
+    return false;
 
   {
     AutoLock auto_lock(lock_);
@@ -473,4 +469,3 @@ bool DirectoryWatcherImpl::Watch(const FilePath& path,
 DirectoryWatcher::DirectoryWatcher() {
   impl_ = new DirectoryWatcherImpl();
 }
-