Fixed shutdown concurrency issues in FilePathWatcher.

content/common/file_path_watcher/file_path_watcher_inotify.cc

Index: content/common/file_path_watcher/file_path_watcher_inotify.cc
diff --git a/content/common/file_path_watcher/file_path_watcher_inotify.cc b/content/common/file_path_watcher/file_path_watcher_inotify.cc
index ac9e5ce26d19ce77e4e171baf5b2ac6cad47412b..fe574ebcc89102130ca1df150166db8944fc5c80 100644
--- a/content/common/file_path_watcher/file_path_watcher_inotify.cc
+++ b/content/common/file_path_watcher/file_path_watcher_inotify.cc
@@ -80,7 +80,14 @@ class InotifyReader {
   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();
 
@@ -103,8 +110,16 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate {
   // 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
@@ -363,6 +378,8 @@ bool FilePathWatcherImpl::Watch(const FilePath& path,
   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());
@@ -375,18 +392,30 @@ bool FilePathWatcherImpl::Watch(const FilePath& path,
   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) {
@@ -398,6 +427,10 @@ void FilePathWatcherImpl::Cancel() {
   target_.clear();
 }
 
+void FilePathWatcherImpl::WillDestroyCurrentMessageLoop() {
+  CancelInternal();
+}
+
 bool FilePathWatcherImpl::UpdateWatches() {
   // Ensure this runs on the message_loop_ exclusively in order to avoid
   // concurrency issues.