From 9ae1384af2cdd16539e9caaed69b095737e2c272 Mon Sep 17 00:00:00 2001 From: Qijiang Fan Date: Tue, 17 Dec 2019 17:32:35 +0900 Subject: [PATCH] backport upstream patch to fix watcher leak. https://chromium-review.googlesource.com/c/chromium/src/+/695914 Change-Id: I91928fc00e9845ff75c49c272ff774ff9810f4eb --- base/files/file_descriptor_watcher_posix.cc | 104 +++++++++++++++----- base/threading/thread_restrictions.h | 4 + 2 files changed, 82 insertions(+), 26 deletions(-) diff --git a/base/files/file_descriptor_watcher_posix.cc b/base/files/file_descriptor_watcher_posix.cc index b26bf6c..98d2cec 100644 --- a/base/files/file_descriptor_watcher_posix.cc +++ b/base/files/file_descriptor_watcher_posix.cc @@ -5,6 +5,7 @@ #include "base/files/file_descriptor_watcher_posix.h" #include "base/bind.h" +#include "base/callback_helpers.h" #include "base/lazy_instance.h" #include "base/logging.h" #include "base/memory/ptr_util.h" @@ -12,6 +13,7 @@ #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" @@ -27,21 +29,7 @@ LazyInstance>::Leaky } // namespace -FileDescriptorWatcher::Controller::~Controller() { - DCHECK(sequence_checker_.CalledOnValidSequence()); - - // Delete |watcher_| on the MessageLoopForIO. - // - // 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(). - message_loop_for_io_task_runner_->DeleteSoon(FROM_HERE, watcher_.release()); - - // Since WeakPtrs are invalidated by the destructor, RunCallback() won't be - // invoked after this returns. -} - +// Move watcher above to prevent delete incomplete type at delete watcher later. class FileDescriptorWatcher::Controller::Watcher : public MessagePumpForIO::FdWatcher, public MessageLoopCurrent::DestructionObserver { @@ -90,6 +78,58 @@ class FileDescriptorWatcher::Controller::Watcher DISALLOW_COPY_AND_ASSIGN(Watcher); }; +FileDescriptorWatcher::Controller::~Controller() { + DCHECK(sequence_checker_.CalledOnValidSequence()); + + if (message_loop_for_io_task_runner_->BelongsToCurrentThread()) { + // If the MessageLoopForIO 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 signaled 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(WaitableEvent::ResetPolicy::MANUAL, + WaitableEvent::InitialState::NOT_SIGNALED); + message_loop_for_io_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()), + // TODO(fqj): change to BindOnce after some uprev. + ScopedClosureRunner(Bind(&WaitableEvent::Signal, + Unretained(&done))))); + // TODO(fqj): change to ScopedAllowBaseSyncPrimitivesOutsideBlockingScope + // after uprev to r586297 + base::ThreadRestrictions::ScopedAllowWait allow_wait __attribute__((unused)); + done.Wait(); + } + + // Since WeakPtrs are invalidated by the destructor, RunCallback() won't be + // invoked after this returns. +} + FileDescriptorWatcher::Controller::Watcher::Watcher( WeakPtr controller, MessagePumpForIO::Mode mode, @@ -150,11 +190,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, @@ -172,12 +220,16 @@ FileDescriptorWatcher::Controller::Controller(MessagePumpForIO::Mode mode, 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 |message_loop_for_io_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. - message_loop_for_io_task_runner_->PostTask( - FROM_HERE, BindOnce(&Watcher::StartWatching, Unretained(watcher_.get()))); + if (message_loop_for_io_task_runner_->BelongsToCurrentThread()) { + watcher_->StartWatching(); + } else { + // It is safe to use Unretained() below because |watcher_| can only be deleted + // by a delete task posted to |message_loop_for_io_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. + message_loop_for_io_task_runner_->PostTask( + FROM_HERE, Bind(&Watcher::StartWatching, Unretained(watcher_.get()))); + } } void FileDescriptorWatcher::Controller::RunCallback() { diff --git a/base/threading/thread_restrictions.h b/base/threading/thread_restrictions.h index 705ba4d..7532f85 100644 --- a/base/threading/thread_restrictions.h +++ b/base/threading/thread_restrictions.h @@ -147,6 +147,7 @@ namespace internal { class TaskTracker; } +class FileDescriptorWatcher; class GetAppOutputScopedAllowBaseSyncPrimitives; class SimpleThread; class StackSamplingProfiler; @@ -479,6 +480,9 @@ class BASE_EXPORT ThreadRestrictions { friend class ui::CommandBufferLocal; friend class ui::GpuState; + // Chrome OS libchrome + friend class base::FileDescriptorWatcher; + // END ALLOWED USAGE. // BEGIN USAGE THAT NEEDS TO BE FIXED. friend class ::chromeos::BlockingMethodCaller; // http://crbug.com/125360 -- 2.24.1.735.g03f4e72817-goog