| From 9ae1384af2cdd16539e9caaed69b095737e2c272 Mon Sep 17 00:00:00 2001 |
| From: Qijiang Fan <fqj@chromium.org> |
| 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<ThreadLocalPointer<MessageLoopForIO>>::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> 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 |
| |
