1From 9ae1384af2cdd16539e9caaed69b095737e2c272 Mon Sep 17 00:00:00 2001 2From: Qijiang Fan <fqj@chromium.org> 3Date: Tue, 17 Dec 2019 17:32:35 +0900 4Subject: [PATCH] backport upstream patch to fix watcher leak. 5 6https://chromium-review.googlesource.com/c/chromium/src/+/695914 7 8Change-Id: I91928fc00e9845ff75c49c272ff774ff9810f4eb 9--- 10 base/files/file_descriptor_watcher_posix.cc | 104 +++++++++++++++----- 11 base/threading/thread_restrictions.h | 4 + 12 2 files changed, 82 insertions(+), 26 deletions(-) 13 14diff --git a/base/files/file_descriptor_watcher_posix.cc b/base/files/file_descriptor_watcher_posix.cc 15index b26bf6c..98d2cec 100644 16--- a/base/files/file_descriptor_watcher_posix.cc 17+++ b/base/files/file_descriptor_watcher_posix.cc 18@@ -5,6 +5,7 @@ 19 #include "base/files/file_descriptor_watcher_posix.h" 20 21 #include "base/bind.h" 22+#include "base/callback_helpers.h" 23 #include "base/lazy_instance.h" 24 #include "base/logging.h" 25 #include "base/memory/ptr_util.h" 26@@ -12,6 +13,7 @@ 27 #include "base/message_loop/message_pump_for_io.h" 28 #include "base/sequenced_task_runner.h" 29 #include "base/single_thread_task_runner.h" 30+#include "base/synchronization/waitable_event.h" 31 #include "base/threading/sequenced_task_runner_handle.h" 32 #include "base/threading/thread_checker.h" 33 #include "base/threading/thread_local.h" 34@@ -27,21 +29,7 @@ LazyInstance<ThreadLocalPointer<MessageLoopForIO>>::Leaky 35 36 } // namespace 37 38-FileDescriptorWatcher::Controller::~Controller() { 39- DCHECK(sequence_checker_.CalledOnValidSequence()); 40- 41- // Delete |watcher_| on the MessageLoopForIO. 42- // 43- // If the MessageLoopForIO is deleted before Watcher::StartWatching() runs, 44- // |watcher_| is leaked. If the MessageLoopForIO is deleted after 45- // Watcher::StartWatching() runs but before the DeleteSoon task runs, 46- // |watcher_| is deleted from Watcher::WillDestroyCurrentMessageLoop(). 47- message_loop_for_io_task_runner_->DeleteSoon(FROM_HERE, watcher_.release()); 48- 49- // Since WeakPtrs are invalidated by the destructor, RunCallback() won't be 50- // invoked after this returns. 51-} 52- 53+// Move watcher above to prevent delete incomplete type at delete watcher later. 54 class FileDescriptorWatcher::Controller::Watcher 55 : public MessagePumpForIO::FdWatcher, 56 public MessageLoopCurrent::DestructionObserver { 57@@ -90,6 +78,58 @@ class FileDescriptorWatcher::Controller::Watcher 58 DISALLOW_COPY_AND_ASSIGN(Watcher); 59 }; 60 61+FileDescriptorWatcher::Controller::~Controller() { 62+ DCHECK(sequence_checker_.CalledOnValidSequence()); 63+ 64+ if (message_loop_for_io_task_runner_->BelongsToCurrentThread()) { 65+ // If the MessageLoopForIO and the Controller live on the same thread. 66+ watcher_.reset(); 67+ } else { 68+ // Synchronously wait until |watcher_| is deleted on the MessagePumpForIO 69+ // thread. This ensures that the file descriptor is never accessed after 70+ // this destructor returns. 71+ // 72+ // Use a ScopedClosureRunner to ensure that |done| is signaled even if the 73+ // thread doesn't run any more tasks (if PostTask returns true, it means 74+ // that the task was queued, but it doesn't mean that a RunLoop will run the 75+ // task before the queue is deleted). 76+ // 77+ // We considered associating "generations" to file descriptors to avoid the 78+ // synchronous wait. For example, if the IO thread gets a "cancel" for fd=6, 79+ // generation=1 after getting a "start watching" for fd=6, generation=2, it 80+ // can ignore the "Cancel". However, "generations" didn't solve this race: 81+ // 82+ // T1 (client) Start watching fd = 6 with WatchReadable() 83+ // Stop watching fd = 6 84+ // Close fd = 6 85+ // Open a new file, fd = 6 gets reused. 86+ // T2 (io) Watcher::StartWatching() 87+ // Incorrectly starts watching fd = 6 which now refers to a 88+ // different file than when WatchReadable() was called. 89+ WaitableEvent done(WaitableEvent::ResetPolicy::MANUAL, 90+ WaitableEvent::InitialState::NOT_SIGNALED); 91+ message_loop_for_io_task_runner_->PostTask( 92+ FROM_HERE, BindOnce( 93+ [](Watcher *watcher, ScopedClosureRunner closure) { 94+ // Since |watcher| is a raw pointer, it isn't deleted 95+ // if this callback is deleted before it gets to run. 96+ delete watcher; 97+ // |closure| runs at the end of this scope. 98+ }, 99+ Unretained(watcher_.release()), 100+ // TODO(fqj): change to BindOnce after some uprev. 101+ ScopedClosureRunner(Bind(&WaitableEvent::Signal, 102+ Unretained(&done))))); 103+ // TODO(fqj): change to ScopedAllowBaseSyncPrimitivesOutsideBlockingScope 104+ // after uprev to r586297 105+ base::ThreadRestrictions::ScopedAllowWait allow_wait __attribute__((unused)); 106+ done.Wait(); 107+ } 108+ 109+ // Since WeakPtrs are invalidated by the destructor, RunCallback() won't be 110+ // invoked after this returns. 111+} 112+ 113 FileDescriptorWatcher::Controller::Watcher::Watcher( 114 WeakPtr<Controller> controller, 115 MessagePumpForIO::Mode mode, 116@@ -150,11 +190,19 @@ void FileDescriptorWatcher::Controller::Watcher:: 117 WillDestroyCurrentMessageLoop() { 118 DCHECK(thread_checker_.CalledOnValidThread()); 119 120- // A Watcher is owned by a Controller. When the Controller is deleted, it 121- // transfers ownership of the Watcher to a delete task posted to the 122- // MessageLoopForIO. If the MessageLoopForIO is deleted before the delete task 123- // runs, the following line takes care of deleting the Watcher. 124- delete this; 125+ if (callback_task_runner_->RunsTasksInCurrentSequence()) { 126+ // |controller_| can be accessed directly when Watcher runs on the same 127+ // thread. 128+ controller_->watcher_.reset(); 129+ } else { 130+ // If the Watcher and the Controller live on different threads, delete 131+ // |this| synchronously. Pending tasks bound to an unretained Watcher* will 132+ // not run since this loop is dead. The associated Controller still 133+ // technically owns this via unique_ptr but it never uses it directly and 134+ // will ultimately send it to this thread for deletion (and that also won't 135+ // run since the loop being dead). 136+ delete this; 137+ } 138 } 139 140 FileDescriptorWatcher::Controller::Controller(MessagePumpForIO::Mode mode, 141@@ -172,12 +220,16 @@ FileDescriptorWatcher::Controller::Controller(MessagePumpForIO::Mode mode, 142 143 void FileDescriptorWatcher::Controller::StartWatching() { 144 DCHECK(sequence_checker_.CalledOnValidSequence()); 145- // It is safe to use Unretained() below because |watcher_| can only be deleted 146- // by a delete task posted to |message_loop_for_io_task_runner_| by this 147- // Controller's destructor. Since this delete task hasn't been posted yet, it 148- // can't run before the task posted below. 149- message_loop_for_io_task_runner_->PostTask( 150- FROM_HERE, BindOnce(&Watcher::StartWatching, Unretained(watcher_.get()))); 151+ if (message_loop_for_io_task_runner_->BelongsToCurrentThread()) { 152+ watcher_->StartWatching(); 153+ } else { 154+ // It is safe to use Unretained() below because |watcher_| can only be deleted 155+ // by a delete task posted to |message_loop_for_io_task_runner_| by this 156+ // Controller's destructor. Since this delete task hasn't been posted yet, it 157+ // can't run before the task posted below. 158+ message_loop_for_io_task_runner_->PostTask( 159+ FROM_HERE, Bind(&Watcher::StartWatching, Unretained(watcher_.get()))); 160+ } 161 } 162 163 void FileDescriptorWatcher::Controller::RunCallback() { 164diff --git a/base/threading/thread_restrictions.h b/base/threading/thread_restrictions.h 165index 705ba4d..7532f85 100644 166--- a/base/threading/thread_restrictions.h 167+++ b/base/threading/thread_restrictions.h 168@@ -147,6 +147,7 @@ namespace internal { 169 class TaskTracker; 170 } 171 172+class FileDescriptorWatcher; 173 class GetAppOutputScopedAllowBaseSyncPrimitives; 174 class SimpleThread; 175 class StackSamplingProfiler; 176@@ -479,6 +480,9 @@ class BASE_EXPORT ThreadRestrictions { 177 friend class ui::CommandBufferLocal; 178 friend class ui::GpuState; 179 180+ // Chrome OS libchrome 181+ friend class base::FileDescriptorWatcher; 182+ 183 // END ALLOWED USAGE. 184 // BEGIN USAGE THAT NEEDS TO BE FIXED. 185 friend class ::chromeos::BlockingMethodCaller; // http://crbug.com/125360 186-- 1872.24.1.735.g03f4e72817-goog 188 189