Allow fds passed to a FileDescriptorWatcher::Controller to be closed.

With this CL, it is guaranteed that the file descriptor passed to
FileDescriptorWatcher::Watch(Readable|Writable) isn't used after
the returned Controller has been destroyed.

To enforce this when the MessageLoopForIO used to watch the file
descriptor lives on a separate thread, the destructor of
Controller uses a WaitableEvent to wait until all work on this
separate thread is done.

Bug: 741188
Change-Id: Ie1a5ead5324ae90ce1d73339cfc191c589590222
Reviewed-on: https://chromium-review.googlesource.com/c/695914
Commit-Queue: François Doray <fdoray@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#627021}

base/files/file_descriptor_watcher_posix.cc

@@ -5,42 +5,29 @@
 #include "base/files/file_descriptor_watcher_posix.h"
 
 #include "base/bind.h"
+#include "base/callback_helpers.h"
 #include "base/lazy_instance.h"
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop_current.h"
 #include "base/message_loop/message_pump_for_io.h"
 #include "base/sequenced_task_runner.h"
 #include "base/single_thread_task_runner.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread_checker.h"
 #include "base/threading/thread_local.h"
+#include "base/threading/thread_restrictions.h"
 
 namespace base {
 
 namespace {
 
-// MessageLoopForIO used to watch file descriptors for which callbacks are
-// registered from a given thread.
+// Per-thread FileDescriptorWatcher registration.
 LazyInstance<ThreadLocalPointer<FileDescriptorWatcher>>::Leaky tls_fd_watcher =
     LAZY_INSTANCE_INITIALIZER;
 
 }  // namespace
 
-FileDescriptorWatcher::Controller::~Controller() {
-  DCHECK(sequence_checker_.CalledOnValidSequence());
-
-  // Delete |watcher_| on the IO thread task runner.
-  //
-  // If the MessageLoopForIO is deleted before Watcher::StartWatching() runs,
-  // |watcher_| is leaked. If the MessageLoopForIO is deleted after
-  // Watcher::StartWatching() runs but before the DeleteSoon task runs,
-  // |watcher_| is deleted from Watcher::WillDestroyCurrentMessageLoop().
-  io_thread_task_runner_->DeleteSoon(FROM_HERE, watcher_.release());
-
-  // Since WeakPtrs are invalidated by the destructor, RunCallback() won't be
-  // invoked after this returns.
-}
-
 class FileDescriptorWatcher::Controller::Watcher
     : public MessagePumpForIO::FdWatcher,
       public MessageLoopCurrent::DestructionObserver {
@@ -60,7 +47,7 @@ class FileDescriptorWatcher::Controller::Watcher
   // MessageLoopCurrent::DestructionObserver:
   void WillDestroyCurrentMessageLoop() override;
 
-  // The MessageLoopForIO's watch handle (stops the watch when destroyed).
+  // The MessagePumpForIO's watch handle (stops the watch when destroyed).
   MessagePumpForIO::FdWatchController fd_watch_controller_;
 
   // Runs tasks on the sequence on which this was instantiated (i.e. the
@@ -68,7 +55,9 @@ class FileDescriptorWatcher::Controller::Watcher
   const scoped_refptr<SequencedTaskRunner> callback_task_runner_ =
       SequencedTaskRunnerHandle::Get();
 
-  // The Controller that created this Watcher.
+  // The Controller that created this Watcher. This WeakPtr is bound to the
+  // |controller_| thread and can only be used by this Watcher to post back to
+  // |callback_task_runner_|.
   WeakPtr<Controller> controller_;
 
   // Whether this Watcher is notified when |fd_| becomes readable or writable
@@ -79,11 +68,11 @@ class FileDescriptorWatcher::Controller::Watcher
   const int fd_;
 
   // Except for the constructor, every method of this class must run on the same
-  // MessageLoopForIO thread.
+  // MessagePumpForIO thread.
   ThreadChecker thread_checker_;
 
   // Whether this Watcher was registered as a DestructionObserver on the
-  // MessageLoopForIO thread.
+  // MessagePumpForIO thread.
   bool registered_as_destruction_observer_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(Watcher);
@@ -110,13 +99,10 @@ void FileDescriptorWatcher::Controller::Watcher::StartWatching() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(MessageLoopCurrentForIO::IsSet());
 
-  if (!MessageLoopCurrentForIO::Get()->WatchFileDescriptor(
-          fd_, false, mode_, &fd_watch_controller_, this)) {
-    // TODO(wez): Ideally we would [D]CHECK here, or propagate the failure back
-    // to the caller, but there is no guarantee that they haven't already
-    // closed |fd_| on another thread, so the best we can do is Debug-log.
-    DLOG(ERROR) << "Failed to watch fd=" << fd_;
-  }
+  const bool watch_success =
+      MessageLoopCurrentForIO::Get()->WatchFileDescriptor(
+          fd_, false, mode_, &fd_watch_controller_, this);
+  DCHECK(watch_success) << "Failed to watch fd=" << fd_;
 
   if (!registered_as_destruction_observer_) {
     MessageLoopCurrentForIO::Get()->AddDestructionObserver(this);
@@ -150,11 +136,19 @@ void FileDescriptorWatcher::Controller::Watcher::
     WillDestroyCurrentMessageLoop() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  // A Watcher is owned by a Controller. When the Controller is deleted, it
-  // transfers ownership of the Watcher to a delete task posted to the
-  // MessageLoopForIO. If the MessageLoopForIO is deleted before the delete task
-  // runs, the following line takes care of deleting the Watcher.
-  delete this;
+  if (callback_task_runner_->RunsTasksInCurrentSequence()) {
+    // |controller_| can be accessed directly when Watcher runs on the same
+    // thread.
+    controller_->watcher_.reset();
+  } else {
+    // If the Watcher and the Controller live on different threads, delete
+    // |this| synchronously. Pending tasks bound to an unretained Watcher* will
+    // not run since this loop is dead. The associated Controller still
+    // technically owns this via unique_ptr but it never uses it directly and
+    // will ultimately send it to this thread for deletion (and that also  won't
+    // run since the loop being dead).
+    delete this;
+  }
 }
 
 FileDescriptorWatcher::Controller::Controller(MessagePumpForIO::Mode mode,
@@ -170,14 +164,68 @@ FileDescriptorWatcher::Controller::Controller(MessagePumpForIO::Mode mode,
   StartWatching();
 }
 
+FileDescriptorWatcher::Controller::~Controller() {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+
+  if (io_thread_task_runner_->BelongsToCurrentThread()) {
+    // If the MessagePumpForIO and the Controller live on the same thread.
+    watcher_.reset();
+  } else {
+    // Synchronously wait until |watcher_| is deleted on the MessagePumpForIO
+    // thread. This ensures that the file descriptor is never accessed after
+    // this destructor returns.
+    //
+    // Use a ScopedClosureRunner to ensure that |done| is signalled even if the
+    // thread doesn't run any more tasks (if PostTask returns true, it means
+    // that the task was queued, but it doesn't mean that a RunLoop will run the
+    // task before the queue is deleted).
+    //
+    // We considered associating "generations" to file descriptors to avoid the
+    // synchronous wait. For example, if the IO thread gets a "cancel" for fd=6,
+    // generation=1 after getting a "start watching" for fd=6, generation=2, it
+    // can ignore the "Cancel". However, "generations" didn't solve this race:
+    //
+    // T1 (client) Start watching fd = 6 with WatchReadable()
+    //             Stop watching fd = 6
+    //             Close fd = 6
+    //             Open a new file, fd = 6 gets reused.
+    // T2 (io)     Watcher::StartWatching()
+    //               Incorrectly starts watching fd = 6 which now refers to a
+    //               different file than when WatchReadable() was called.
+    WaitableEvent done;
+    io_thread_task_runner_->PostTask(
+        FROM_HERE, BindOnce(
+                       [](Watcher* watcher, ScopedClosureRunner closure) {
+                         // Since |watcher| is a raw pointer, it isn't deleted
+                         // if this callback is deleted before it gets to run.
+                         delete watcher;
+                         // |closure| runs at the end of this scope.
+                       },
+                       Unretained(watcher_.release()),
+                       ScopedClosureRunner(BindOnce(&WaitableEvent::Signal,
+                                                    Unretained(&done)))));
+    ScopedAllowBaseSyncPrimitivesOutsideBlockingScope allow;
+    done.Wait();
+  }
+
+  // Since WeakPtrs are invalidated by the destructor, any pending RunCallback()
+  // won't be invoked after this returns.
+}
+
 void FileDescriptorWatcher::Controller::StartWatching() {
   DCHECK(sequence_checker_.CalledOnValidSequence());
-  // It is safe to use Unretained() below because |watcher_| can only be deleted
-  // by a delete task posted to |io_thread_task_runner_| by this
-  // Controller's destructor. Since this delete task hasn't been posted yet, it
-  // can't run before the task posted below.
-  io_thread_task_runner_->PostTask(
-      FROM_HERE, BindOnce(&Watcher::StartWatching, Unretained(watcher_.get())));
+  if (io_thread_task_runner_->BelongsToCurrentThread()) {
+    // If the MessagePumpForIO and the Controller live on the same thread.
+    watcher_->StartWatching();
+  } else {
+    // It is safe to use Unretained() below because |watcher_| can only be
+    // deleted by a delete task posted to |io_thread_task_runner_| by this
+    // Controller's destructor. Since this delete task hasn't been posted yet,
+    // it can't run before the task posted below.
+    io_thread_task_runner_->PostTask(
+        FROM_HERE,
+        BindOnce(&Watcher::StartWatching, Unretained(watcher_.get())));
+  }
 }
 
 void FileDescriptorWatcher::Controller::RunCallback() {

base/files/file_descriptor_watcher_posix.h

@@ -84,9 +84,10 @@ class BASE_EXPORT FileDescriptorWatcher {
 
   // Registers |io_thread_task_runner| to watch file descriptors for which
   // callbacks are registered from the current thread via WatchReadable() or
-  // WatchWritable(). |io_thread_task_runner| may run on another thread.
-  // |io_thread_task_runner| must post tasks to a thread which runs
-  // a MessagePumpForIO.
+  // WatchWritable(). |io_thread_task_runner| must post tasks to a thread which
+  // runs a MessagePumpForIO. If it is not the current thread, it must be highly
+  // responsive (i.e. not used to run other expensive tasks such as potentially
+  // blocking I/O) since ~Controller waits for a task posted to it.
   explicit FileDescriptorWatcher(
       scoped_refptr<SingleThreadTaskRunner> io_thread_task_runner);
   ~FileDescriptorWatcher();
@@ -97,7 +98,8 @@ class BASE_EXPORT FileDescriptorWatcher {
   // sequence). To call these methods, a FileDescriptorWatcher must have been
   // instantiated on the current thread and SequencedTaskRunnerHandle::IsSet()
   // must return true (these conditions are met at least on all TaskScheduler
-  // threads as well as on threads backed by a MessageLoopForIO).
+  // threads as well as on threads backed by a MessageLoopForIO). |fd| must
+  // outlive the returned Controller.
   static std::unique_ptr<Controller> WatchReadable(int fd,
                                                    const Closure& callback);
   static std::unique_ptr<Controller> WatchWritable(int fd,

base/threading/thread_restrictions.h

@@ -254,6 +254,7 @@ class TaskTracker;
 }
 
 class AdjustOOMScoreHelper;
+class FileDescriptorWatcher;
 class GetAppOutputScopedAllowBaseSyncPrimitives;
 class MessageLoopImpl;
 class ScopedAllowThreadRecallForStackSamplingProfiler;
@@ -419,6 +420,7 @@ class BASE_EXPORT ScopedAllowBaseSyncPrimitivesOutsideBlockingScope {
       AwFormDatabaseService;  // http://crbug.com/904431
   friend class android_webview::CookieManager;
   friend class audio::OutputDevice;
+  friend class base::FileDescriptorWatcher;
   friend class base::MessageLoopImpl;
   friend class base::ScopedAllowThreadRecallForStackSamplingProfiler;
   friend class base::StackSamplingProfiler;