Chrome Version: master @ 93dcec8efd45173b0e2e43b29133354a6ee6ba0f
OS: macOS 10.14.2

I'm experimenting with some changes to the Mac message pump, and I've come across two threading/sequence safety issues in FileDescriptorWatcher. For the purposes of this bug, I'm looking at it specifically in relation to FilePathWatcherKQueue and FilePathWatcherTest.NewFile, but the issues occurs with FileDescriptorWatcher and its other tests.

A brief recap: a FileDescriptorWatcher has a Controller and a Watcher. The Controller is bound to the sequence on which it was created and is the client interface. The Watcher is bound to the IO task runner that was passed when creating the Controller and interacts with the MessagePumpForIO.

The first issue is that Watcher::StartWatching() can be called for a deleted Controller, effectively starting a watch on an FD that could already have been closed. This happens because when ~FilePathWatcher runs, it Cancel()s its PlatformDelegate, which then deletes FileDescriptorWatcher::Controller immediately. In ~Controller, a task is then posted to the IO task runner to delete the corresponding Watcher. The issue occurs because in Controller::RunCallback(), if the Controller's weak pointer is still valid, it posts another task to the IO task runner to call Watcher::StartWatching(). However, if the FilePathWatcher (and its Controller) is deleted after the callback runs but outside the callback task itself, that restart-watching task is still queued on the IO thread. This can occur, like in the test, when the callback is a QuitClosure for the RunLoop, and then the FilePathWatcher is deleted after the loop stops. If the run loop is spun again, then Watcher::StartWatching() will be called for the now-deleted Controller. The posted delete-Watcher task will run next, stopping the watch. The WeakPtr liveness check in Controller::StartWatching is insufficient; there needs to be some other liveness check in Watcher::StartWatching(). The crux of the issue is that the MessagePump will be operating on a closed (and potentially reused) FD.

The second issue is distinct, but related. As noted above, in FilePathWatcherKQueue::Cancel() the FilePathWatcher is immediately deleted and the kqueue it uses for observing is close()d after that. Because FdWatchController::StopWatchingFileDescriptor() is invoked as part of ~Watcher, which runs via a posted task to the IO task runner, the MessagePump will be told to stop watching a descriptor that has been closed (and maybe reused).

These issues don't result in libevent complaining because it swallows EBADF (https://cs.chromium.org/chromium/src/base/third_party/libevent/kqueue.c?l=242&rcl=029dc5666f102091950f645ef9023be7b32d4ba3). Being permissive here is probably not the best, though, and doesn't address the concern of operating on an FD number that could have been re-used after close().