You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
189 lines
8.3 KiB
189 lines
8.3 KiB
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
|
|
|