Index: base/files/file_path_watcher_fsevents.cc |
diff --git a/base/files/file_path_watcher_fsevents.cc b/base/files/file_path_watcher_fsevents.cc |
index 8e73b557aa291d7967ffb84d19183316cc619c1c..51a2c4044597ae296912a0f1eded41312dcc5ed6 100644 |
--- a/base/files/file_path_watcher_fsevents.cc |
+++ b/base/files/file_path_watcher_fsevents.cc |
@@ -13,7 +13,6 @@ |
#include "base/lazy_instance.h" |
#include "base/logging.h" |
#include "base/mac/scoped_cftyperef.h" |
-#include "base/macros.h" |
#include "base/strings/stringprintf.h" |
#include "base/threading/sequenced_task_runner_handle.h" |
@@ -69,10 +68,16 @@ FilePath ResolvePath(const FilePath& path) { |
FilePathWatcherFSEvents::FilePathWatcherFSEvents() |
: queue_(dispatch_queue_create( |
- base::StringPrintf( |
- "org.chromium.base.FilePathWatcher.%p", this).c_str(), |
+ base::StringPrintf("org.chromium.base.FilePathWatcher.%p", this) |
+ .c_str(), |
DISPATCH_QUEUE_SERIAL)), |
- fsevent_stream_(nullptr) { |
+ fsevent_stream_(nullptr), |
+ weak_factory_(this) {} |
+ |
+FilePathWatcherFSEvents::~FilePathWatcherFSEvents() { |
+ DCHECK(!task_runner() || task_runner()->RunsTasksOnCurrentThread()); |
gab
2017/01/05 21:22:53
Is this a new requirement? Where is it documented?
fdoray
2017/01/05 23:04:11
The DCHECK below is not new.
The DCHECK above doc
|
+ DCHECK(callback_.is_null()) |
+ << "Cancel() must be called before FilePathWatcher is destroyed."; |
} |
bool FilePathWatcherFSEvents::Watch(const FilePath& path, |
@@ -105,11 +110,15 @@ void FilePathWatcherFSEvents::Cancel() { |
set_cancelled(); |
callback_.Reset(); |
- // Switch to the dispatch queue to tear down the event stream. As the queue |
- // is owned by this object, and this method is called from the destructor, |
- // execute the block synchronously. |
+ // Switch to the dispatch queue to tear down the event stream. As the queue is |
+ // owned by |this|, and this method is called from the destructor, execute the |
+ // block synchronously. |
dispatch_sync(queue_, ^{ |
- CancelOnMessageLoopThread(); |
+ if (fsevent_stream_) { |
+ DestroyEventStream(); |
+ target_.clear(); |
+ resolved_target_.clear(); |
+ } |
}); |
} |
@@ -140,31 +149,35 @@ void FilePathWatcherFSEvents::FSEventsCallback( |
// the directory to be watched gets created. |
if (root_changed) { |
// Resetting the event stream from within the callback fails (FSEvents spews |
- // bad file descriptor errors), so post a task to do the reset. |
- dispatch_async(watcher->queue_, ^{ |
- watcher->UpdateEventStream(root_change_at); |
- }); |
+ // bad file descriptor errors). Therefore, post a task to task_runner() that |
+ // will itself schedule a call to UpdateEventStream() on the dispatch queue. |
+ // Bouncing on task_runner() and using a weak ptr ensures that |
+ // UpdateEventStream() won't be called after |watcher| is destroyed (i.e. |
+ // after the watch is cancelled). |
+ watcher->task_runner()->PostTask( |
+ FROM_HERE, Bind( |
+ [](WeakPtr<FilePathWatcherFSEvents> weak_watcher, |
+ FSEventStreamEventId root_change_at) { |
+ if (!weak_watcher) |
+ return; |
+ FilePathWatcherFSEvents* watcher = weak_watcher.get(); |
+ dispatch_async(watcher->queue_, ^{ |
gab
2017/01/05 21:22:53
Is the async dispatch required here? i.e. it was r
fdoray
2017/01/05 23:04:11
Yes it's required. UpdateEventStream() must run on
|
+ watcher->UpdateEventStream(root_change_at); |
+ }); |
+ }, |
+ watcher->weak_factory_.GetWeakPtr(), root_change_at)); |
} |
watcher->OnFilePathsChanged(paths); |
} |
-FilePathWatcherFSEvents::~FilePathWatcherFSEvents() { |
- // This method may be called on either the libdispatch or task_runner() |
- // thread. Checking callback_ on the libdispatch thread here is safe because |
- // it is executing in a task posted by Cancel() which first reset callback_. |
- // PostTask forms a sufficient memory barrier to ensure that the value is |
- // consistent on the target thread. |
- DCHECK(callback_.is_null()) |
- << "Cancel() must be called before FilePathWatcher is destroyed."; |
-} |
- |
void FilePathWatcherFSEvents::OnFilePathsChanged( |
const std::vector<FilePath>& paths) { |
DCHECK(!resolved_target_.empty()); |
task_runner()->PostTask( |
- FROM_HERE, Bind(&FilePathWatcherFSEvents::DispatchEvents, this, paths, |
- target_, resolved_target_)); |
+ FROM_HERE, |
+ Bind(&FilePathWatcherFSEvents::DispatchEvents, weak_factory_.GetWeakPtr(), |
+ paths, target_, resolved_target_)); |
} |
void FilePathWatcherFSEvents::DispatchEvents(const std::vector<FilePath>& paths, |
@@ -185,18 +198,6 @@ void FilePathWatcherFSEvents::DispatchEvents(const std::vector<FilePath>& paths, |
} |
} |
-void FilePathWatcherFSEvents::CancelOnMessageLoopThread() { |
- // For all other implementations, the "message loop thread" is the IO thread, |
- // as returned by task_runner(). This implementation, however, needs to |
- // cancel pending work on the Dispatch Queue thread. |
- |
- if (fsevent_stream_) { |
- DestroyEventStream(); |
- target_.clear(); |
- resolved_target_.clear(); |
- } |
-} |
- |
void FilePathWatcherFSEvents::UpdateEventStream( |
FSEventStreamEventId start_event) { |
// It can happen that the watcher gets canceled while tasks that call this |
@@ -232,8 +233,9 @@ void FilePathWatcherFSEvents::UpdateEventStream( |
FSEventStreamSetDispatchQueue(fsevent_stream_, queue_); |
if (!FSEventStreamStart(fsevent_stream_)) { |
- task_runner()->PostTask( |
- FROM_HERE, Bind(&FilePathWatcherFSEvents::ReportError, this, target_)); |
+ task_runner()->PostTask(FROM_HERE, |
+ Bind(&FilePathWatcherFSEvents::ReportError, |
+ weak_factory_.GetWeakPtr(), target_)); |
} |
} |
@@ -242,8 +244,9 @@ bool FilePathWatcherFSEvents::ResolveTargetPath() { |
bool changed = resolved != resolved_target_; |
resolved_target_ = resolved; |
if (resolved_target_.empty()) { |
- task_runner()->PostTask( |
- FROM_HERE, Bind(&FilePathWatcherFSEvents::ReportError, this, target_)); |
+ task_runner()->PostTask(FROM_HERE, |
+ Bind(&FilePathWatcherFSEvents::ReportError, |
+ weak_factory_.GetWeakPtr(), target_)); |
} |
return changed; |
} |