[base] Use TaskRunner instead of MessageLoop in FileDescriptorWatcher

Do not use MessageLoopForIO as it is used to post tasks anyway.

R=gab@chromium.org
TBR=gab@chromium.org
BUG=891670

Change-Id: Ib255c6a5a5dd55c2654eaa284d43d7f78f06b244
Reviewed-on: https://chromium-review.googlesource.com/c/1299239
Commit-Queue: Alexander Timin <altimin@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603076}

base/files/file_descriptor_watcher_posix.cc

@@ -22,21 +22,21 @@ namespace {
 
 // MessageLoopForIO used to watch file descriptors for which callbacks are
 // registered from a given thread.
-LazyInstance<ThreadLocalPointer<MessageLoopForIO>>::Leaky
-    tls_message_loop_for_io = LAZY_INSTANCE_INITIALIZER;
+LazyInstance<ThreadLocalPointer<FileDescriptorWatcher>>::Leaky tls_fd_watcher =
+    LAZY_INSTANCE_INITIALIZER;
 
 }  // namespace
 
 FileDescriptorWatcher::Controller::~Controller() {
   DCHECK(sequence_checker_.CalledOnValidSequence());
 
-  // Delete |watcher_| on the MessageLoopForIO.
+  // Delete |watcher_| on the IO thread task runner.
   //
   // 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());
+  io_thread_task_runner_->DeleteSoon(FROM_HERE, watcher_.release());
 
   // Since WeakPtrs are invalidated by the destructor, RunCallback() won't be
   // invoked after this returns.
@@ -161,22 +161,22 @@ FileDescriptorWatcher::Controller::Controller(MessagePumpForIO::Mode mode,
                                               int fd,
                                               const Closure& callback)
     : callback_(callback),
-      message_loop_for_io_task_runner_(
-          tls_message_loop_for_io.Get().Get()->task_runner()),
+      io_thread_task_runner_(
+          tls_fd_watcher.Get().Get()->io_thread_task_runner()),
       weak_factory_(this) {
   DCHECK(!callback_.is_null());
-  DCHECK(message_loop_for_io_task_runner_);
+  DCHECK(io_thread_task_runner_);
   watcher_ = std::make_unique<Watcher>(weak_factory_.GetWeakPtr(), mode, fd);
   StartWatching();
 }
 
 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
+  // by a delete task posted to |io_thread_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(
+  io_thread_task_runner_->PostTask(
       FROM_HERE, BindOnce(&Watcher::StartWatching, Unretained(watcher_.get())));
 }
 
@@ -193,14 +193,18 @@ void FileDescriptorWatcher::Controller::RunCallback() {
 }
 
 FileDescriptorWatcher::FileDescriptorWatcher(
-    MessageLoopForIO* message_loop_for_io) {
-  DCHECK(message_loop_for_io);
-  DCHECK(!tls_message_loop_for_io.Get().Get());
-  tls_message_loop_for_io.Get().Set(message_loop_for_io);
+    scoped_refptr<SingleThreadTaskRunner> io_thread_task_runner)
+    : io_thread_task_runner_(std::move(io_thread_task_runner)) {
+  DCHECK(!tls_fd_watcher.Get().Get());
+  tls_fd_watcher.Get().Set(this);
+#if DCHECK_IS_ON()
+  io_thread_task_runner_->PostTask(
+      FROM_HERE, BindOnce([]() { DCHECK(MessageLoopCurrentForIO::IsSet()); }));
+#endif  // DCHECK_IS_ON()
 }
 
 FileDescriptorWatcher::~FileDescriptorWatcher() {
-  tls_message_loop_for_io.Get().Set(nullptr);
+  tls_fd_watcher.Get().Set(nullptr);
 }
 
 std::unique_ptr<FileDescriptorWatcher::Controller>

base/files/file_descriptor_watcher_posix.h

@@ -12,9 +12,9 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
-#include "base/message_loop/message_loop.h"
 #include "base/message_loop/message_pump_for_io.h"
 #include "base/sequence_checker.h"
+#include "base/single_thread_task_runner.h"
 
 namespace base {
 
@@ -61,11 +61,10 @@ class BASE_EXPORT FileDescriptorWatcher {
 
     // TaskRunner associated with the MessageLoopForIO that watches the file
     // descriptor.
-    const scoped_refptr<SingleThreadTaskRunner>
-        message_loop_for_io_task_runner_;
+    const scoped_refptr<SingleThreadTaskRunner> io_thread_task_runner_;
 
     // Notified by the MessageLoopForIO associated with
-    // |message_loop_for_io_task_runner_| when the watched file descriptor is
+    // |io_thread_task_runner_| when the watched file descriptor is
     // readable or writable without blocking. Posts a task to run RunCallback()
     // on the sequence on which the Controller was instantiated. When the
     // Controller is deleted, ownership of |watcher_| is transfered to a delete
@@ -82,11 +81,13 @@ class BASE_EXPORT FileDescriptorWatcher {
     DISALLOW_COPY_AND_ASSIGN(Controller);
   };
 
-  // Registers |message_loop_for_io| to watch file descriptors for which
+  // Registers |io_thread_task_runner| to watch file descriptors for which
   // callbacks are registered from the current thread via WatchReadable() or
-  // WatchWritable(). |message_loop_for_io| may run on another thread. The
-  // constructed FileDescriptorWatcher must not outlive |message_loop_for_io|.
-  FileDescriptorWatcher(MessageLoopForIO* message_loop_for_io);
+  // WatchWritable(). |io_thread_task_runner| may run on another thread.
+  // |io_thread_task_runner| must post tasks to a thread which runs
+  // a MessagePumpForIO.
+  explicit FileDescriptorWatcher(
+      scoped_refptr<SingleThreadTaskRunner> io_thread_task_runner);
   ~FileDescriptorWatcher();
 
   // Registers |callback| to be posted on the current sequence when |fd| is
@@ -102,6 +103,12 @@ class BASE_EXPORT FileDescriptorWatcher {
                                                    const Closure& callback);
 
  private:
+  scoped_refptr<SingleThreadTaskRunner> io_thread_task_runner() const {
+    return io_thread_task_runner_;
+  }
+
+  const scoped_refptr<SingleThreadTaskRunner> io_thread_task_runner_;
+
   DISALLOW_COPY_AND_ASSIGN(FileDescriptorWatcher);
 };
 

base/files/file_descriptor_watcher_posix_unittest.cc

@@ -70,7 +70,7 @@ class FileDescriptorWatcherTest
 
     ASSERT_TRUE(message_loop_for_io->IsType(MessageLoop::TYPE_IO));
     file_descriptor_watcher_ = std::make_unique<FileDescriptorWatcher>(
-        static_cast<MessageLoopForIO*>(message_loop_for_io));
+        message_loop_for_io->task_runner());
   }
 
   void TearDown() override {

base/files/file_path_watcher_unittest.cc

@@ -143,7 +143,7 @@ class FilePathWatcherTest : public testing::Test {
  public:
   FilePathWatcherTest()
 #if defined(OS_POSIX)
-      : file_descriptor_watcher_(&loop_)
+      : file_descriptor_watcher_(loop_.task_runner())
 #endif
   {
   }

base/task/task_scheduler/task_tracker_posix.cc

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "base/files/file_descriptor_watcher_posix.h"
+#include "base/message_loop/message_loop.h"
 
 namespace base {
 namespace internal {
@@ -19,7 +20,7 @@ void TaskTrackerPosix::RunOrSkipTask(Task task,
                                      bool can_run_task) {
   DCHECK(watch_file_descriptor_message_loop_);
   FileDescriptorWatcher file_descriptor_watcher(
-      watch_file_descriptor_message_loop_);
+      watch_file_descriptor_message_loop_->task_runner());
   TaskTracker::RunOrSkipTask(std::move(task), sequence, can_run_task);
 }
 

base/test/launcher/test_launcher_nacl_nonsfi.cc

@@ -147,7 +147,7 @@ int TestLauncherNonSfiMain(const std::string& test_binary) {
 
   base::MessageLoopForIO message_loop;
 #if defined(OS_POSIX)
-  FileDescriptorWatcher file_descriptor_watcher(&message_loop);
+  FileDescriptorWatcher file_descriptor_watcher(message_loop.task_runner());
 #endif
 
   NonSfiUnitTestPlatformDelegate platform_delegate;

base/test/launcher/unit_test_launcher.cc

@@ -249,7 +249,7 @@ int LaunchUnitTestsInternal(RunTestSuiteCallback run_test_suite,
 
   MessageLoopForIO message_loop;
 #if defined(OS_POSIX)
-  FileDescriptorWatcher file_descriptor_watcher(&message_loop);
+  FileDescriptorWatcher file_descriptor_watcher(message_loop.task_runner());
 #endif
 
   DefaultUnitTestPlatformDelegate platform_delegate;

base/test/scoped_task_environment.cc

@@ -117,11 +117,10 @@ ScopedTaskEnvironment::ScopedTaskEnvironment(
                     slsm_for_mock_time_.get())
               : nullptr),
 #if defined(OS_POSIX)
-      file_descriptor_watcher_(
-          main_thread_type == MainThreadType::IO
-              ? std::make_unique<FileDescriptorWatcher>(
-                    static_cast<MessageLoopForIO*>(message_loop_.get()))
-              : nullptr),
+      file_descriptor_watcher_(main_thread_type == MainThreadType::IO
+                                   ? std::make_unique<FileDescriptorWatcher>(
+                                         message_loop_->task_runner())
+                                   : nullptr),
 #endif  // defined(OS_POSIX)
       task_tracker_(new TestTaskTracker()) {
   CHECK(!TaskScheduler::GetInstance())

base/test/scoped_task_environment_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/atomicops.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/run_loop.h"
 #include "base/synchronization/atomic_flag.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/task/post_task.h"

base/threading/thread.cc

@@ -323,8 +323,8 @@ void Thread::ThreadMain() {
   // Allow threads running a MessageLoopForIO to use FileDescriptorWatcher API.
   std::unique_ptr<FileDescriptorWatcher> file_descriptor_watcher;
   if (MessageLoopForIO::IsCurrent()) {
-    file_descriptor_watcher.reset(new FileDescriptorWatcher(
-        static_cast<MessageLoopForIO*>(message_loop_)));
+    file_descriptor_watcher.reset(
+        new FileDescriptorWatcher(message_loop_->task_runner()));
   }
 #endif
 

components/timers/BUILD.gn

@@ -23,6 +23,7 @@ source_set("unit_tests") {
   deps = [
     ":timers",
     "//base",
+    "//base/test:test_support",
     "//testing/gtest",
   ]
 }

components/timers/alarm_timer_unittest.cc

@@ -8,12 +8,11 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
-#include "base/files/file_descriptor_watcher_posix.h"
 #include "base/location.h"
 #include "base/macros.h"
-#include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
+#include "base/test/scoped_task_environment.h"
 #include "base/threading/platform_thread.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
@@ -96,8 +95,8 @@ class SelfDeletingAlarmTimerTester {
 // that timers work properly in all configurations.
 
 TEST(AlarmTimerTest, SimpleAlarmTimer) {
-  base::MessageLoopForIO loop;
-  base::FileDescriptorWatcher file_descriptor_watcher(&loop);
+  base::test::ScopedTaskEnvironment task_environment(
+      base::test::ScopedTaskEnvironment::MainThreadType::IO);
 
   base::RunLoop run_loop;
   bool did_run = false;
@@ -111,8 +110,8 @@ TEST(AlarmTimerTest, SimpleAlarmTimer) {
 }
 
 TEST(AlarmTimerTest, SimpleAlarmTimer_Cancel) {
-  base::MessageLoopForIO loop;
-  base::FileDescriptorWatcher file_descriptor_watcher(&loop);
+  base::test::ScopedTaskEnvironment task_environment(
+      base::test::ScopedTaskEnvironment::MainThreadType::IO);
 
   bool did_run_a = false;
   AlarmTimerTester* a =
@@ -139,8 +138,8 @@ TEST(AlarmTimerTest, SimpleAlarmTimer_Cancel) {
 // If underlying timer does not handle this properly, we will crash or fail
 // in full page heap environment.
 TEST(AlarmTimerTest, SelfDeletingAlarmTimer) {
-  base::MessageLoopForIO loop;
-  base::FileDescriptorWatcher file_descriptor_watcher(&loop);
+  base::test::ScopedTaskEnvironment task_environment(
+      base::test::ScopedTaskEnvironment::MainThreadType::IO);
 
   base::RunLoop run_loop;
   bool did_run = false;
@@ -154,8 +153,8 @@ TEST(AlarmTimerTest, SelfDeletingAlarmTimer) {
 }
 
 TEST(AlarmTimerTest, AlarmTimerZeroDelay) {
-  base::MessageLoopForIO loop;
-  base::FileDescriptorWatcher file_descriptor_watcher(&loop);
+  base::test::ScopedTaskEnvironment task_environment(
+      base::test::ScopedTaskEnvironment::MainThreadType::IO);
 
   base::RunLoop run_loop;
   bool did_run = false;
@@ -169,8 +168,8 @@ TEST(AlarmTimerTest, AlarmTimerZeroDelay) {
 }
 
 TEST(AlarmTimerTest, AlarmTimerZeroDelay_Cancel) {
-  base::MessageLoopForIO loop;
-  base::FileDescriptorWatcher file_descriptor_watcher(&loop);
+  base::test::ScopedTaskEnvironment task_environment(
+      base::test::ScopedTaskEnvironment::MainThreadType::IO);
 
   bool did_run_a = false;
   AlarmTimerTester* a =
@@ -201,9 +200,9 @@ TEST(AlarmTimerTest, MessageLoopShutdown) {
   // if debug heap checking is enabled.
   bool did_run = false;
   {
-    auto loop = std::make_unique<base::MessageLoopForIO>();
-    auto file_descriptor_watcher =
-        std::make_unique<base::FileDescriptorWatcher>(loop.get());
+    base::test::ScopedTaskEnvironment task_environment(
+        base::test::ScopedTaskEnvironment::MainThreadType::IO);
+
     AlarmTimerTester a(&did_run, kTenMilliseconds, base::OnceClosure());
     AlarmTimerTester b(&did_run, kTenMilliseconds, base::OnceClosure());
     AlarmTimerTester c(&did_run, kTenMilliseconds, base::OnceClosure());
@@ -216,18 +215,16 @@ TEST(AlarmTimerTest, MessageLoopShutdown) {
     // tasks posted by FileDescriptorWatcher::WatchReadable() are leaked.
     base::RunLoop().RunUntilIdle();
 
-    // MessageLoop and FileDescriptorWatcher destruct.
-    file_descriptor_watcher.reset();
-    loop.reset();
   }  // SimpleAlarmTimers destruct. SHOULD NOT CRASH, of course.
 
   EXPECT_FALSE(did_run);
 }
 
 TEST(AlarmTimerTest, NonRepeatIsRunning) {
   {
-    base::MessageLoopForIO loop;
-    base::FileDescriptorWatcher file_descriptor_watcher(&loop);
+    base::test::ScopedTaskEnvironment task_environment(
+        base::test::ScopedTaskEnvironment::MainThreadType::IO);
+
     timers::SimpleAlarmTimer timer;
     EXPECT_FALSE(timer.IsRunning());
     timer.Start(FROM_HERE, base::TimeDelta::FromDays(1), base::DoNothing());
@@ -247,8 +244,9 @@ TEST(AlarmTimerTest, NonRepeatIsRunning) {
 }
 
 TEST(AlarmTimerTest, RetainNonRepeatIsRunning) {
-  base::MessageLoopForIO loop;
-  base::FileDescriptorWatcher file_descriptor_watcher(&loop);
+  base::test::ScopedTaskEnvironment task_environment(
+      base::test::ScopedTaskEnvironment::MainThreadType::IO);
+
   timers::SimpleAlarmTimer timer;
   EXPECT_FALSE(timer.IsRunning());
   timer.Start(FROM_HERE, base::TimeDelta::FromDays(1), base::DoNothing());
@@ -292,8 +290,9 @@ void SetCallbackHappened2(base::OnceClosure quit_closure) {
 
 TEST(AlarmTimerTest, ContinuationStopStart) {
   ClearAllCallbackHappened();
-  base::MessageLoopForIO loop;
-  base::FileDescriptorWatcher file_descriptor_watcher(&loop);
+  base::test::ScopedTaskEnvironment task_environment(
+      base::test::ScopedTaskEnvironment::MainThreadType::IO);
+
   timers::SimpleAlarmTimer timer;
   timer.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(10),
               base::BindRepeating(&SetCallbackHappened1,
@@ -312,8 +311,8 @@ TEST(AlarmTimerTest, ContinuationStopStart) {
 
 TEST(AlarmTimerTest, ContinuationReset) {
   ClearAllCallbackHappened();
-  base::MessageLoopForIO loop;
-  base::FileDescriptorWatcher file_descriptor_watcher(&loop);
+  base::test::ScopedTaskEnvironment task_environment(
+      base::test::ScopedTaskEnvironment::MainThreadType::IO);
 
   base::RunLoop run_loop;
   timers::SimpleAlarmTimer timer;
@@ -329,8 +328,9 @@ TEST(AlarmTimerTest, ContinuationReset) {
 // Verify that no crash occurs if a timer is deleted while its callback is
 // running.
 TEST(AlarmTimerTest, DeleteTimerWhileCallbackIsRunning) {
-  base::MessageLoopForIO loop;
-  base::FileDescriptorWatcher file_descriptor_watcher(&loop);
+  base::test::ScopedTaskEnvironment task_environment(
+      base::test::ScopedTaskEnvironment::MainThreadType::IO);
+
   base::RunLoop run_loop;
 
   // Will be deleted by the callback.
@@ -350,8 +350,8 @@ TEST(AlarmTimerTest, DeleteTimerWhileCallbackIsRunning) {
 // Verify that no crash occurs if a zero-delay timer is deleted while its
 // callback is running.
 TEST(AlarmTimerTest, DeleteTimerWhileCallbackIsRunningZeroDelay) {
-  base::MessageLoopForIO loop;
-  base::FileDescriptorWatcher file_descriptor_watcher(&loop);
+  base::test::ScopedTaskEnvironment task_environment(
+      base::test::ScopedTaskEnvironment::MainThreadType::IO);
   base::RunLoop run_loop;
 
   // Will be deleted by the callback.