diff options
5 files changed, 147 insertions, 57 deletions
diff --git a/content/common/file_path_watcher/file_path_watcher.cc b/content/common/file_path_watcher/file_path_watcher.cc index 19cee5f..f40c8c8 100644 --- a/content/common/file_path_watcher/file_path_watcher.cc +++ b/content/common/file_path_watcher/file_path_watcher.cc @@ -21,19 +21,10 @@ bool FilePathWatcher::Watch(const FilePath& path, return impl_->Watch(path, delegate, loop); } -FilePathWatcher::PlatformDelegate::PlatformDelegate() { +FilePathWatcher::PlatformDelegate::PlatformDelegate(): cancelled_(false) { } FilePathWatcher::PlatformDelegate::~PlatformDelegate() { + DCHECK(is_cancelled()); } -void FilePathWatcher::DeletePlatformDelegate::Destruct( - const PlatformDelegate* delegate) { - scoped_refptr<base::MessageLoopProxy> loop = delegate->message_loop(); - if (loop.get() == NULL || loop->BelongsToCurrentThread()) { - delete delegate; - } else { - loop->PostNonNestableTask(FROM_HERE, - new DeleteTask<PlatformDelegate>(delegate)); - } -} diff --git a/content/common/file_path_watcher/file_path_watcher.h b/content/common/file_path_watcher/file_path_watcher.h index 7c784a8..1a8da91 100644 --- a/content/common/file_path_watcher/file_path_watcher.h +++ b/content/common/file_path_watcher/file_path_watcher.h @@ -40,22 +40,35 @@ class FilePathWatcher { // by the Mac implementation right now, and must be backed by a CFRunLoop // based MessagePump. This is usually going to be a MessageLoop of type // TYPE_UI. + // OnFilePathChanged() will be called on the same thread as Watch() is called, + // which should have a MessageLoop of TYPE_IO. bool Watch(const FilePath& path, Delegate* delegate, base::MessageLoopProxy* loop) WARN_UNUSED_RESULT; class PlatformDelegate; - // Traits for PlatformDelegate, which must delete itself on the IO message - // loop that Watch was called from. - struct DeletePlatformDelegate { - static void Destruct(const PlatformDelegate* delegate); + // A custom Task that always cleans up the PlatformDelegate, either when + // executed or when deleted without having been executed at all, as can + // happen during shutdown. + class CancelTask : public Task { + public: + CancelTask(PlatformDelegate* delegate): delegate_(delegate) {} + virtual ~CancelTask() { + delegate_->CancelOnMessageLoopThread(); + } + + virtual void Run() { + delegate_->CancelOnMessageLoopThread(); + } + private: + scoped_refptr<PlatformDelegate> delegate_; + + DISALLOW_COPY_AND_ASSIGN(CancelTask); }; // Used internally to encapsulate different members on different platforms. - class PlatformDelegate - : public base::RefCountedThreadSafe<PlatformDelegate, - DeletePlatformDelegate> { + class PlatformDelegate : public base::RefCountedThreadSafe<PlatformDelegate> { public: PlatformDelegate(); @@ -70,14 +83,17 @@ class FilePathWatcher { // Stop watching. This is called from FilePathWatcher's dtor in order to // allow to shut down properly while the object is still alive. + // It can be called from any thread. virtual void Cancel() = 0; protected: - friend class DeleteTask<PlatformDelegate>; - friend struct DeletePlatformDelegate; - virtual ~PlatformDelegate(); + // Stop watching. This is only called on the thread of the appropriate + // message loop. Since it can also be called more than once, it should + // check |is_cancelled()| to avoid duplicate work. + virtual void CancelOnMessageLoopThread() = 0; + scoped_refptr<base::MessageLoopProxy> message_loop() const { return message_loop_; } @@ -86,8 +102,21 @@ class FilePathWatcher { message_loop_ = loop; } + // Must be called before the PlatformDelegate is deleted. + void set_cancelled() { + cancelled_ = true; + } + + bool is_cancelled() const { + return cancelled_; + } + private: + friend class base::RefCountedThreadSafe<PlatformDelegate>; + friend class CancelTask; + scoped_refptr<base::MessageLoopProxy> message_loop_; + bool cancelled_; }; private: 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 ac9e5ce..e4017e9 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,8 @@ class InotifyReader { DISALLOW_COPY_AND_ASSIGN(InotifyReader); }; -class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate { +class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate, + public MessageLoop::DestructionObserver { public: FilePathWatcherImpl(); @@ -103,9 +104,17 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate { // Cancel the watch. This unregisters the instance with InotifyReader. virtual void Cancel() 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; + private: virtual ~FilePathWatcherImpl() {} + // Cleans up and stops observing the |message_loop_| thread. + 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. @@ -363,6 +372,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()); @@ -376,26 +387,39 @@ bool FilePathWatcherImpl::Watch(const FilePath& path, } 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. + set_cancelled(); 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; + new FilePathWatcher::CancelTask(this)); + } else { + CancelOnMessageLoopThread(); } +} - 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); +void FilePathWatcherImpl::CancelOnMessageLoopThread() { + if (!is_cancelled()) { + set_cancelled(); + 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(); } - watches_.clear(); - delegate_ = NULL; - target_.clear(); +} + +void FilePathWatcherImpl::WillDestroyCurrentMessageLoop() { + CancelOnMessageLoopThread(); } bool FilePathWatcherImpl::UpdateWatches() { diff --git a/content/common/file_path_watcher/file_path_watcher_mac.cc b/content/common/file_path_watcher/file_path_watcher_mac.cc index 6e89ad1..c8cfc6b 100644 --- a/content/common/file_path_watcher/file_path_watcher_mac.cc +++ b/content/common/file_path_watcher/file_path_watcher_mac.cc @@ -32,7 +32,8 @@ namespace { const CFAbsoluteTime kEventLatencySeconds = 0.3; // Mac-specific file watcher implementation based on the FSEvents API. -class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate { +class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate, + public MessageLoop::DestructionObserver { public: FilePathWatcherImpl(); @@ -49,6 +50,11 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate { base::MessageLoopProxy* loop) OVERRIDE; virtual void Cancel() 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; + scoped_refptr<base::MessageLoopProxy> run_loop_message_loop() { return run_loop_message_loop_; } @@ -59,6 +65,13 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate { // Destroy the event stream. void DestroyEventStream(); + // Start observing the destruction of the |run_loop_message_loop_| thread, + // and watching the FSEventStream. + void StartObserverAndEventStream(FSEventStreamEventId start_event); + + // Cleans up and stops observing the |run_loop_message_loop_| thread. + void CancelOnMessageLoopThread() OVERRIDE; + // Delegate to notify upon changes. scoped_refptr<FilePathWatcher::Delegate> delegate_; @@ -79,9 +92,6 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate { // Run loop for FSEventStream to run on. scoped_refptr<base::MessageLoopProxy> run_loop_message_loop_; - // Used to detect early cancellation. - bool canceled_; - DISALLOW_COPY_AND_ASSIGN(FilePathWatcherImpl); }; @@ -120,8 +130,7 @@ void FSEventsCallback(ConstFSEventStreamRef stream, // FilePathWatcherImpl implementation: FilePathWatcherImpl::FilePathWatcherImpl() - : fsevent_stream_(NULL), - canceled_(false) { + : fsevent_stream_(NULL) { } void FilePathWatcherImpl::OnFilePathChanged() { @@ -191,15 +200,23 @@ bool FilePathWatcherImpl::Watch(const FilePath& path, } run_loop_message_loop()->PostTask(FROM_HERE, - NewRunnableMethod(this, &FilePathWatcherImpl::UpdateEventStream, + NewRunnableMethod(this, &FilePathWatcherImpl::StartObserverAndEventStream, start_event)); return true; } +void FilePathWatcherImpl::StartObserverAndEventStream( + FSEventStreamEventId start_event) { + DCHECK(run_loop_message_loop()->BelongsToCurrentThread()); + MessageLoop::current()->AddDestructionObserver(this); + UpdateEventStream(start_event); +} + void FilePathWatcherImpl::Cancel() { if (!run_loop_message_loop().get()) { // Watch was never called, so exit. + set_cancelled(); return; } @@ -207,13 +224,23 @@ void FilePathWatcherImpl::Cancel() { // the event stream. if (!run_loop_message_loop()->BelongsToCurrentThread()) { run_loop_message_loop()->PostTask(FROM_HERE, - NewRunnableMethod(this, &FilePathWatcherImpl::Cancel)); - return; + new FilePathWatcher::CancelTask(this)); + } else { + CancelOnMessageLoopThread(); } +} - canceled_ = true; - if (fsevent_stream_) +void FilePathWatcherImpl::CancelOnMessageLoopThread() { + set_cancelled(); + if (fsevent_stream_) { DestroyEventStream(); + MessageLoop::current()->RemoveDestructionObserver(this); + delegate_ = NULL; + } +} + +void FilePathWatcherImpl::WillDestroyCurrentMessageLoop() { + CancelOnMessageLoopThread(); } void FilePathWatcherImpl::UpdateEventStream(FSEventStreamEventId start_event) { @@ -222,7 +249,7 @@ void FilePathWatcherImpl::UpdateEventStream(FSEventStreamEventId start_event) { // It can happen that the watcher gets canceled while tasks that call this // function are still in flight, so abort if this situation is detected. - if (canceled_) + if (is_cancelled()) return; if (fsevent_stream_) @@ -259,7 +286,6 @@ void FilePathWatcherImpl::UpdateEventStream(FSEventStreamEventId start_event) { } void FilePathWatcherImpl::DestroyEventStream() { - DCHECK(run_loop_message_loop()->BelongsToCurrentThread()); FSEventStreamStop(fsevent_stream_); FSEventStreamUnscheduleFromRunLoop(fsevent_stream_, CFRunLoopGetCurrent(), kCFRunLoopDefaultMode); diff --git a/content/common/file_path_watcher/file_path_watcher_win.cc b/content/common/file_path_watcher/file_path_watcher_win.cc index b6b4333..dc36958 100644 --- a/content/common/file_path_watcher/file_path_watcher_win.cc +++ b/content/common/file_path_watcher/file_path_watcher_win.cc @@ -15,7 +15,8 @@ namespace { class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate, - public base::win::ObjectWatcher::Delegate { + public base::win::ObjectWatcher::Delegate, + public MessageLoop::DestructionObserver { public: FilePathWatcherImpl() : delegate_(NULL), handle_(INVALID_HANDLE_VALUE) {} @@ -25,11 +26,16 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate, base::MessageLoopProxy* loop) OVERRIDE; virtual void Cancel() 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; + // Callback from MessageLoopForIO. virtual void OnObjectSignaled(HANDLE object); private: - virtual ~FilePathWatcherImpl(); + virtual ~FilePathWatcherImpl() {} // Setup a watch handle for directory |dir|. Returns true if no fatal error // occurs. |handle| will receive the handle value if |dir| is watchable, @@ -43,6 +49,9 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate, // Destroy the watch handle. void DestroyWatch(); + // Cleans up and stops observing the |message_loop_| thread. + void CancelOnMessageLoopThread() OVERRIDE; + // Delegate to notify upon changes. scoped_refptr<FilePathWatcher::Delegate> delegate_; @@ -74,6 +83,7 @@ bool FilePathWatcherImpl::Watch(const FilePath& path, set_message_loop(base::MessageLoopProxy::CreateForCurrentThread()); delegate_ = delegate; target_ = path; + MessageLoop::current()->AddDestructionObserver(this); if (!UpdateWatch()) return false; @@ -84,20 +94,35 @@ bool FilePathWatcherImpl::Watch(const FilePath& path, } void FilePathWatcherImpl::Cancel() { - if (!message_loop().get()) { - // Watch was never called, so exit. + if (!delegate_) { + // Watch was never called, or the |message_loop_| has already quit. + set_cancelled(); return; } // Switch to the file thread if necessary so we can stop |watcher_|. if (!message_loop()->BelongsToCurrentThread()) { message_loop()->PostTask(FROM_HERE, - NewRunnableMethod(this, &FilePathWatcherImpl::Cancel)); - return; + new FilePathWatcher::CancelTask(this)); + } else { + CancelOnMessageLoopThread(); } +} + +void FilePathWatcherImpl::CancelOnMessageLoopThread() { + set_cancelled(); if (handle_ != INVALID_HANDLE_VALUE) DestroyWatch(); + + if (delegate_) { + MessageLoop::current()->RemoveDestructionObserver(this); + delegate_ = NULL; + } +} + +void FilePathWatcherImpl::WillDestroyCurrentMessageLoop() { + CancelOnMessageLoopThread(); } void FilePathWatcherImpl::OnObjectSignaled(HANDLE object) { @@ -149,11 +174,6 @@ void FilePathWatcherImpl::OnObjectSignaled(HANDLE object) { watcher_.StartWatching(handle_, this); } -FilePathWatcherImpl::~FilePathWatcherImpl() { - if (handle_ != INVALID_HANDLE_VALUE) - DestroyWatch(); -} - // static bool FilePathWatcherImpl::SetupWatchHandle(const FilePath& dir, HANDLE* handle) { |