Fixed shutdown concurrency issues in FilePathWatcher.

content/common/file_path_watcher/file_path_watcher_inotify.cc

 // Copyright (c) 2011 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 "content/common/file_path_watcher/file_path_watcher.h"
 
 #include <errno.h>
 #include <string.h>
 #include <sys/inotify.h>
 #include <sys/ioctl.h>
@@ skipping 62 matching lines @@
 
   // Use self-pipe trick to unblock select during shutdown.
   int shutdown_pipe_[2];
 
   // Flag set to true when startup was successful.
   bool valid_;
 
   DISALLOW_COPY_AND_ASSIGN(InotifyReader);
 };
 
-class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate {
+// Deletion of the FilePathWatcher will call Cancel() to dispose of this
+// object in the right thread. In that case, |this| is already cleaned up when
+// the dtor enters.
+// However, it can happen that the file thread exits before Cancel() is called,
+// and by then Cancel()'s posted Task will never execute. Therefore we observe
+// the thread's MessageLoop and cleanup early, if necessary.
+class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate,
+                            public MessageLoop::DestructionObserver {
  public:
   FilePathWatcherImpl();
 
   // Called for each event coming from the watch. |fired_watch| identifies the
   // watch that fired, |child| indicates what has changed, and is relative to
   // the currently watched path for |fired_watch|. The flag |created| is true if
   // the object appears, and |is_directory| is set when the event refers to a
   // directory.
   void OnFilePathChanged(InotifyReader::Watch fired_watch,
                          const FilePath::StringType& child,
                          bool created,
                          bool is_directory);
 
   // 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,
                      FilePathWatcher::Delegate* delegate,
                      base::MessageLoopProxy* loop) OVERRIDE;
 
   // Cancel the watch. This unregisters the instance with InotifyReader.
   virtual void Cancel() OVERRIDE;
 
+  // MessageLoop::DestructionObserver overrides.
+  // This allows cleaning up when the |message_loop_| thread is
+  // shutting down.
+  virtual void WillDestroyCurrentMessageLoop();
+
  private:
-  virtual ~FilePathWatcherImpl() {}
+  virtual ~FilePathWatcherImpl();
+
+  // Cleans up and stops observing the |message_loop_| thread.
+  void CancelInternal();
 
   // 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.
   struct WatchEntry {
     WatchEntry(InotifyReader::Watch watch, const FilePath::StringType& subdir)
         : watch_(watch),
           subdir_(subdir) {}
 
     InotifyReader::Watch watch_;
@@ skipping 238 matching lines @@
 
 bool FilePathWatcherImpl::Watch(const FilePath& path,
                                 FilePathWatcher::Delegate* delegate,
                                 base::MessageLoopProxy*) {
   DCHECK(target_.empty());
   DCHECK(MessageLoopForIO::current());
 
   set_message_loop(base::MessageLoopProxy::CreateForCurrentThread());
   delegate_ = delegate;
   target_ = path;
+  MessageLoop::current()->AddDestructionObserver(this);
+
   std::vector<FilePath::StringType> comps;
   target_.GetComponents(&comps);
   DCHECK(!comps.empty());
   for (std::vector<FilePath::StringType>::const_iterator comp(++comps.begin());
        comp != comps.end(); ++comp) {
     watches_.push_back(WatchEntry(InotifyReader::kInvalidWatch, *comp));
   }
   watches_.push_back(WatchEntry(InotifyReader::kInvalidWatch,
                                 FilePath::StringType()));
   return UpdateWatches();
 }
 
+FilePathWatcherImpl::~FilePathWatcherImpl() {
+  // This covers the case when the Task is posted on Cancel(), but is not
+  // executed during shutdown, and |this| is just Released.

[Mattias Nissler (ping if slow), 2011/03/21 16:07:45]

But shouldn't we then see the WillDestroyCurrentMessageLoop() invocation? If we
call CancelInternal from here, how can we be sure that MessageLoop::current() is
non-NULL?

[Joao da Silva, 2011/03/22 10:10:08]

I saw a code path that actually triggers this, though it's an unlikely event.
First, ~FilePathWatcher() calls Cancel(), and a Task is posted. The Task holds a
reference to |this|, and the FilePathWatcher releases its reference. So the Task
is holding the object. But the message loop of the Task is already shutting
down, and the Task is never executed. And before notifying observers of the
destruction of the message loop, the message loop deletes pending tasks. At 
this point, |this| is destroyed and never cleaned up. It must clean up now,
otherwise the destruction notification will be triggered on a stale pointer.

+  if (delegate_)
+    CancelInternal();
+}
+
 void FilePathWatcherImpl::Cancel() {
-  if (!message_loop().get()) {
-    // Watch was never called, so exit.
+  if (!delegate_) {
+    // Watch was never called, or the |message_loop_| thread is already gone.
     return;
   }
 
   // Switch to the message_loop_ if necessary so we can access |watches_|.
   if (!message_loop()->BelongsToCurrentThread()) {
     message_loop()->PostTask(FROM_HERE,
-        NewRunnableMethod(this, &FilePathWatcherImpl::Cancel));
-    return;
+        NewRunnableMethod(this, &FilePathWatcherImpl::CancelInternal));
+  } else {
+    CancelInternal();
   }
+}
+
+void FilePathWatcherImpl::CancelInternal() {
+  MessageLoop::current()->RemoveDestructionObserver(this);
 
   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);
   }
   watches_.clear();
   delegate_ = NULL;
   target_.clear();
 }
 
+void FilePathWatcherImpl::WillDestroyCurrentMessageLoop() {
+  CancelInternal();
+}
+
 bool FilePathWatcherImpl::UpdateWatches() {
   // Ensure this runs on the message_loop_ exclusively in order to avoid
   // concurrency issues.
   DCHECK(message_loop()->BelongsToCurrentThread());
 
   // 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) {
@@ skipping 14 matching lines @@
   }
 
   return true;
 }
 
 }  // namespace
 
 FilePathWatcher::FilePathWatcher() {
   impl_ = new FilePathWatcherImpl();
 }