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