Support FileDescriptorWatcher in ScopedTaskEnvironment::MainThreadTyp…

…e::IO This is cleaner than having individual tests manage their own FileDescriptorWatcher instances and it removes a few existing usages of MessageLoopForIO::current() as a MessageLoopForIO* (which will no longer be legal after : https://chromium-review.googlesource.com/c/chromium/src/+/957760). R=fdoray@chromium.org TBR=sergeyu@chromium.org (c/b/extensions/api/messaging side-effects) TBR=hidehiko@chromium.org (components/arc cleanup) Bug: 825327, 708584 Change-Id: I2c4507cc7a4af073afaf19cd3d802c3bb5fd7f39 Reviewed-on: https://chromium-review.googlesource.com/1005780 Commit-Queue: Gabriel Charette <gab@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Reviewed-by: François Doray <fdoray@chromium.org> Reviewed-by: Jamie Walch <jamiewalch@chromium.org> Cr-Commit-Position: refs/heads/master@{#549857}
hylschwy · Apr 11, 2018 · 9d8482e · 9d8482e
1 parent 1fcb0b5
commit 9d8482e
Show file tree

Hide file tree

Showing 8 changed files with 69 additions and 43 deletions.
diff --git a/base/files/file_descriptor_watcher_posix.h b/base/files/file_descriptor_watcher_posix.h
@@ -22,6 +22,10 @@ class SingleThreadTaskRunner;
 
 // The FileDescriptorWatcher API allows callbacks to be invoked when file
 // descriptors are readable or writable without blocking.
+//
+// To enable this API in unit tests, use a ScopedTaskEnvironment with
+// MainThreadType::IO.
+//
 // Note: Prefer FileDescriptorWatcher to MessageLoopForIO::WatchFileDescriptor()
 // for non-critical IO. FileDescriptorWatcher works on threads/sequences without
 // MessagePumps but involves going through the task queue after being notified

diff --git a/base/test/scoped_task_environment.cc b/base/test/scoped_task_environment.cc
@@ -19,6 +19,10 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 
+#if defined(OS_POSIX)
+#include "base/files/file_descriptor_watcher_posix.h"
+#endif
+
 namespace base {
 namespace test {
 
@@ -106,6 +110,13 @@ ScopedTaskEnvironment::ScopedTaskEnvironment(
                     internal::ScopedSetSequenceLocalStorageMapForCurrentThread>(
                     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),
+#endif  // defined(OS_POSIX)
       task_tracker_(new TestTaskTracker()) {
   CHECK(!TaskScheduler::GetInstance());
 

diff --git a/base/test/scoped_task_environment.h b/base/test/scoped_task_environment.h
@@ -9,6 +9,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/single_thread_task_runner.h"
 #include "base/task_scheduler/lazy_task_runner.h"
+#include "build/build_config.h"
 
 namespace base {
 
@@ -17,6 +18,7 @@ class ScopedSetSequenceLocalStorageMapForCurrentThread;
 class SequenceLocalStorageMap;
 }  // namespace internal
 
+class FileDescriptorWatcher;
 class MessageLoop;
 class TaskScheduler;
 class TestMockTimeTaskRunner;
@@ -74,7 +76,8 @@ class ScopedTaskEnvironment {
     MOCK_TIME,
     // The main thread pumps UI messages.
     UI,
-    // The main thread pumps asynchronous IO messages.
+    // The main thread pumps asynchronous IO messages and supports the
+    // FileDescriptorWatcher API on POSIX.
     IO,
   };
 
@@ -139,6 +142,11 @@ class ScopedTaskEnvironment {
       internal::ScopedSetSequenceLocalStorageMapForCurrentThread>
       slsm_registration_for_mock_time_;
 
+#if defined(OS_POSIX)
+  // Enables the FileDescriptorWatcher API iff running a MainThreadType::IO.
+  const std::unique_ptr<FileDescriptorWatcher> file_descriptor_watcher_;
+#endif
+
   const TaskScheduler* task_scheduler_ = nullptr;
 
   // Owned by |task_scheduler_|.

diff --git a/base/test/scoped_task_environment_unittest.cc b/base/test/scoped_task_environment_unittest.cc
@@ -17,8 +17,14 @@
 #include "base/threading/sequence_local_storage_slot.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/tick_clock.h"
+#include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+#if defined(OS_POSIX)
+#include <unistd.h>
+#include "base/files/file_descriptor_watcher_posix.h"
+#endif  // defined(OS_POSIX)
+
 namespace base {
 namespace test {
 
@@ -241,6 +247,26 @@ TEST_P(ScopedTaskEnvironmentTest, SupportsSequenceLocalStorageOnMainThread) {
   EXPECT_EQ(5, sls_slot.Get());
 }
 
+#if defined(OS_POSIX)
+TEST_F(ScopedTaskEnvironmentTest, SupportsFileDescriptorWatcherOnIOMainThread) {
+  ScopedTaskEnvironment scoped_task_environment(
+      ScopedTaskEnvironment::MainThreadType::IO,
+      ScopedTaskEnvironment::ExecutionMode::ASYNC);
+
+  int pipe_fds_[2];
+  ASSERT_EQ(0, pipe(pipe_fds_));
+
+  RunLoop run_loop;
+
+  // The write end of a newly created pipe is immediately writable.
+  auto controller = FileDescriptorWatcher::WatchWritable(
+      pipe_fds_[1], run_loop.QuitClosure());
+
+  // This will hang if the notification doesn't occur as expected.
+  run_loop.Run();
+}
+#endif  // defined(OS_POSIX)
+
 // Verify that the TickClock returned by
 // |ScopedTaskEnvironment::GetMockTickClock| gets updated when the
 // FastForward(By|UntilNoTasksRemain) functions are called.

diff --git a/chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc b/chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc
@@ -17,7 +17,6 @@
 #include "base/files/scoped_file.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/json/json_reader.h"
-#include "base/message_loop/message_loop.h"
 #include "base/process/process_metrics.h"
 #include "base/rand_util.h"
 #include "base/run_loop.h"
@@ -106,9 +105,6 @@ class NativeMessagingTest : public ::testing::Test,
   NativeMessagingTest()
       : current_channel_(version_info::Channel::DEV),
         thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
-#if defined(OS_POSIX)
-        file_descriptor_watcher_(base::MessageLoopForIO::current()),
-#endif
         channel_closed_(false) {}
 
   void SetUp() override { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); }
@@ -171,10 +167,6 @@ class NativeMessagingTest : public ::testing::Test,
   std::unique_ptr<NativeMessageHost> native_message_host_;
   std::unique_ptr<base::RunLoop> run_loop_;
   content::TestBrowserThreadBundle thread_bundle_;
-#if defined(OS_POSIX)
-  // Required to watch a file descriptor from NativeMessageProcessHost.
-  base::FileDescriptorWatcher file_descriptor_watcher_;
-#endif
 
   std::string last_message_;
   std::unique_ptr<base::DictionaryValue> last_message_parsed_;

diff --git a/components/arc/timer/arc_timer_bridge_unittest.cc b/components/arc/timer/arc_timer_bridge_unittest.cc
@@ -11,7 +11,6 @@
 #include "base/callback_helpers.h"
 #include "base/files/file_descriptor_watcher_posix.h"
 #include "base/files/scoped_file.h"
-#include "base/message_loop/message_loop.h"
 #include "base/optional.h"
 #include "base/posix/unix_domain_socket.h"
 #include "base/run_loop.h"
@@ -226,9 +225,6 @@ bool WaitForExpiration(clockid_t clock_id, ArcTimerStore* arc_timer_store) {
       arc_timer_store->GetTimerReadFd(clock_id);
   EXPECT_NE(timer_read_fd_opt, base::nullopt);
   int timer_read_fd = timer_read_fd_opt.value();
-  // Required before watching a file descriptor.
-  base::FileDescriptorWatcher file_descriptor_watcher(
-      base::MessageLoopForIO::current());
   base::RunLoop loop;
   std::unique_ptr<base::FileDescriptorWatcher::Controller>
       watch_readable_controller = base::FileDescriptorWatcher::WatchReadable(

diff --git a/content/public/test/test_browser_thread_bundle.h b/content/public/test/test_browser_thread_bundle.h
@@ -111,6 +111,8 @@ class TestBrowserThreadBundle {
   // threads. The UI thread is always the main thread in a unit test.
   enum Options {
     DEFAULT = 0,
+    // The main thread will use a MessageLoopForIO (and support the
+    // base::FileDescriptorWatcher API on POSIX).
     IO_MAINLOOP = 1 << 0,
     REAL_IO_THREAD = 1 << 1,
     DONT_CREATE_BROWSER_THREADS = 1 << 2,

diff --git a/remoting/host/local_input_monitor_unittest.cc b/remoting/host/local_input_monitor_unittest.cc
@@ -7,8 +7,9 @@
 #include <memory>
 
 #include "base/bind.h"
-#include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
+#include "base/test/scoped_task_environment.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "remoting/base/auto_thread_task_runner.h"
 #include "remoting/host/client_session_control.h"
@@ -17,10 +18,6 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-#if defined(OS_POSIX)
-#include "base/files/file_descriptor_watcher_posix.h"
-#endif
-
 namespace remoting {
 
 using testing::_;
@@ -29,27 +26,21 @@ using testing::ReturnRef;
 
 namespace {
 
-#if defined(OS_WIN)
-const base::MessageLoop::Type kDesiredMessageLoopType =
-    base::MessageLoop::TYPE_UI;
-#else  // !defined(OS_WIN)
-const base::MessageLoop::Type kDesiredMessageLoopType =
-    base::MessageLoop::TYPE_IO;
-#endif  // !defined(OS_WIN)
-
-}  // namespace
-
 class LocalInputMonitorTest : public testing::Test {
  public:
   LocalInputMonitorTest();
 
   void SetUp() override;
 
-  base::MessageLoop message_loop_;
-#if defined(OS_POSIX)
-  // Required to watch a file descriptor from NativeMessageProcessHost.
-  base::FileDescriptorWatcher file_descriptor_watcher_;
-#endif
+  base::test::ScopedTaskEnvironment scoped_task_environment_ {
+#if defined(OS_WIN)
+    base::test::ScopedTaskEnvironment::MainThreadType::UI
+#else   // !defined(OS_WIN)
+    // Required to watch a file descriptor from NativeMessageProcessHost.
+    base::test::ScopedTaskEnvironment::MainThreadType::IO
+#endif  // !defined(OS_WIN)
+  };
+
   base::RunLoop run_loop_;
   scoped_refptr<AutoThreadTaskRunner> task_runner_;
 
@@ -59,20 +50,16 @@ class LocalInputMonitorTest : public testing::Test {
 };
 
 LocalInputMonitorTest::LocalInputMonitorTest()
-    : message_loop_(kDesiredMessageLoopType),
-#if defined(OS_POSIX)
-      file_descriptor_watcher_(base::MessageLoopForIO::current()),
-#endif
-      client_jid_("user@domain/rest-of-jid"),
-      client_session_control_factory_(&client_session_control_) {
-}
+    : client_jid_("user@domain/rest-of-jid"),
+      client_session_control_factory_(&client_session_control_) {}
 
 void LocalInputMonitorTest::SetUp() {
-  // Arrange to run |message_loop_| until no components depend on it.
-  task_runner_ = new AutoThreadTaskRunner(
-      message_loop_.task_runner(), run_loop_.QuitClosure());
+  // Run the task environment until no components depend on it.
+  task_runner_ = new AutoThreadTaskRunner(base::ThreadTaskRunnerHandle::Get(),
+                                          run_loop_.QuitClosure());
 }
 
+}  // namespace
 
 // This test is really to exercise only the creation and destruction code in
 // LocalInputMonitor.