Deprecate content's test_utils.h APIs for task running in favor of modern task running APIs.

And introduce TestBrowserThreadBundle::RunIOThreadUntilIdle() to replace the only method
that didn't have an immediate replacement.

Bug: 824431
Change-Id: I2137660a745491d6096ef7f2e3d76818d2a9fa8e
Reviewed-on: https://chromium-review.googlesource.com/974166Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545099}

PRESUBMIT.py

@@ -454,7 +454,7 @@ _BANNED_CPP_FUNCTIONS = (
     (
       r'/\bbase::Bind\(',
       (
-          'Please consider using base::Bind{Once,Repeating} instead '
+          'Please consider using base::Bind{Once,Repeating} instead',
           'of base::Bind. (crbug.com/714018)',
       ),
       False,
@@ -463,7 +463,7 @@ _BANNED_CPP_FUNCTIONS = (
     (
       r'/\bbase::Callback<',
       (
-          'Please consider using base::{Once,Repeating}Callback instead '
+          'Please consider using base::{Once,Repeating}Callback instead',
           'of base::Callback. (crbug.com/714018)',
       ),
       False,
@@ -472,12 +472,65 @@ _BANNED_CPP_FUNCTIONS = (
     (
       r'/\bbase::Closure\b',
       (
-          'Please consider using base::{Once,Repeating}Closure instead '
+          'Please consider using base::{Once,Repeating}Closure instead',
           'of base::Closure. (crbug.com/714018)',
       ),
       False,
       (),
     ),
+    (
+      r'RunMessageLoop',
+      (
+          'RunMessageLoop is deprecated, use RunLoop instead.',
+      ),
+      False,
+      (),
+    ),
+    (
+      r'RunThisRunLoop',
+      (
+          'RunThisRunLoop is deprecated, use RunLoop directly instead.',
+      ),
+      False,
+      (),
+    ),
+    (
+      r'RunAllPendingInMessageLoop()',
+      (
+          "Prefer RunLoop over RunAllPendingInMessageLoop, please contact gab@",
+          "if you're convinced you need this.",
+      ),
+      False,
+      (),
+    ),
+    (
+      r'RunAllPendingInMessageLoop(BrowserThread',
+      (
+          'RunAllPendingInMessageLoop is deprecated. Use RunLoop for',
+          'BrowserThread::UI, TestBrowserThreadBundle::RunIOThreadUntilIdle',
+          'for BrowserThread::IO, and prefer RunLoop::QuitClosure to observe',
+          'async events instead of flushing threads.',
+      ),
+      False,
+      (),
+    ),
+    (
+      r'MessageLoopRunner',
+      (
+          'MessageLoopRunner is deprecated, use RunLoop instead.',
+      ),
+      False,
+      (),
+    ),
+    (
+      r'GetDeferredQuitTaskForRunLoop',
+      (
+          "GetDeferredQuitTaskForRunLoop shouldn't be needed, please contact",
+          "gab@ if you found a use case where this is the only solution.",
+      ),
+      False,
+      (),
+    ),
     (
       'sqlite3_initialize',
       (

content/public/test/test_browser_thread_bundle.cc

@@ -132,4 +132,24 @@ void TestBrowserThreadBundle::RunUntilIdle() {
   scoped_task_environment_->RunUntilIdle();
 }
 
+void TestBrowserThreadBundle::RunIOThreadUntilIdle() {
+  // Use a RunLoop to run until idle if already on BrowserThread::IO (which is
+  // the main thread unless using Options::REAL_IO_THREAD).
+  DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  base::WaitableEvent io_thread_idle(
+      base::WaitableEvent::ResetPolicy::MANUAL,
+      base::WaitableEvent::InitialState::NOT_SIGNALED);
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::BindOnce(
+          [](base::WaitableEvent* io_thread_idle) {
+            base::RunLoop(base::RunLoop::Type::kNestableTasksAllowed)
+                .RunUntilIdle();
+            io_thread_idle->Signal();
+          },
+          Unretained(&io_thread_idle)));
+  io_thread_idle.Wait();
+}
+
 }  // namespace content

content/public/test/test_browser_thread_bundle.h

@@ -123,13 +123,28 @@ class TestBrowserThreadBundle {
   // DONT_CREATE_BROWSER_THREADS option was used when the bundle was created.
   void CreateBrowserThreads();
 
-  // Runs all tasks posted to TaskScheduler and main thread until idle. Note: at
-  // the momment, this will not process BrowserThread::IO if this
-  // TestBrowserThreadBundle is using a REAL_IO_THREAD. TODO(robliao): fix this
-  // by making TaskScheduler aware of all MessageLoops.
-  // Prefer this to the equivalent content::RunAllTasksUntilIdle().
+  // Runs all tasks posted to TaskScheduler and main thread until idle.
+  // Note: At the moment, this will not process BrowserThread::IO if this
+  // TestBrowserThreadBundle is using a REAL_IO_THREAD.
+  // TODO(robliao): fix this by making TaskScheduler aware of all MessageLoops.
+  //
+  // Note that this is not the cleanest way to run until idle as it will return
+  // early if it depends on an async condition that isn't guaranteed to have
+  // occurred yet. The best way to run until a condition is met is with RunLoop:
+  //
+  //   void KickoffAsyncFoo(base::OnceClosure on_done);
+  //
+  //   base::RunLoop run_loop;
+  //   KickoffAsyncFoo(run_loop.QuitClosure());
+  //   run_loop.Run();
+  //
   void RunUntilIdle();
 
+  // Flush the IO thread. Replacement for RunLoop::RunUntilIdle() for tests that
+  // have a REAL_IO_THREAD. As with RunUntilIdle() above, prefer using
+  // RunLoop+QuitClosure() to await an async condition.
+  void RunIOThreadUntilIdle();
+
   ~TestBrowserThreadBundle();
 
  private:

content/public/test/test_browser_thread_bundle_unittest.cc

@@ -100,6 +100,38 @@ TEST(TestBrowserThreadBundleTest, RunUntilIdle) {
   EXPECT_EQ(kNumTasks * kNumHops, base::subtle::NoBarrier_Load(&tasks_run));
 }
 
+namespace {
+
+void PostRecurringTaskToIOThread(int iteration, int* tasks_run) {
+  // All iterations but the first come from a task that was posted.
+  if (iteration > 0)
+    (*tasks_run)++;
+
+  if (iteration == kNumHops)
+    return;
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::BindOnce(&PostRecurringTaskToIOThread, iteration + 1, tasks_run));
+}
+
+}  // namespace
+
+TEST(TestBrowserThreadBundleTest, RunIOThreadUntilIdle) {
+  TestBrowserThreadBundle test_browser_thread_bundle(
+      TestBrowserThreadBundle::Options::REAL_IO_THREAD);
+
+  int tasks_run = 0;
+
+  for (int i = 0; i < kNumTasks; ++i) {
+    PostRecurringTaskToIOThread(0, &tasks_run);
+  }
+
+  test_browser_thread_bundle.RunIOThreadUntilIdle();
+
+  EXPECT_EQ(kNumTasks * kNumHops, tasks_run);
+}
+
 TEST(TestBrowserThreadBundleTest, MessageLoopTypeMismatch) {
   base::MessageLoopForUI message_loop;
 

content/public/test/test_utils.h

@@ -39,34 +39,36 @@ namespace content {
 class RenderFrameHost;
 class TestServiceManagerContext;
 
-// Turns on nestable tasks, runs the message loop, then resets nestable tasks
-// to what they were originally. Prefer this over MessageLoop::Run for in
-// process browser tests that need to block until a condition is met.
+// Deprecated: Use RunLoop::Run(). Use RunLoop::Type::kNestableTasksAllowed to
+// force nesting in browser tests.
 void RunMessageLoop();
 
-// Variant of RunMessageLoop that takes RunLoop.
+// Deprecated: Invoke |run_loop->Run()| directly.
 void RunThisRunLoop(base::RunLoop* run_loop);
 
-// Turns on nestable tasks, runs all pending tasks in the message loop,
-// then resets nestable tasks to what they were originally. Prefer this
-// over MessageLoop::RunAllPending for in process browser tests to run
-// all pending tasks. Can only be called from the UI thread.
+// Turns on nestable tasks, runs all pending tasks in the message loop, then
+// resets nestable tasks to what they were originally. Can only be called from
+// the UI thread. Only use this instead of RunLoop::RunUntilIdle() to work
+// around cases where a task keeps reposting itself and prevents the loop from
+// going idle.
+// TODO(gab): Assess whether this API is really needed. If you find yourself
+// needing this, post a comment on https://crbug.com/824431.
 void RunAllPendingInMessageLoop();
 
-// Blocks the current thread until all the pending messages in the loop of the
-// thread |thread_id| have been processed. Can only be called from the UI
-// thread.
+// Deprecated: For BrowserThread::IO use
+// TestBrowserThreadBundle::RunIOThreadUntilIdle. For the main thread use
+// RunLoop. In non-unit-tests use RunLoop::QuitClosure to observe async events
+// rather than flushing entire threads.
 void RunAllPendingInMessageLoop(BrowserThread::ID thread_id);
 
-// Runs until the task scheduler and the current message loop are all empty
-// (have no more immediate tasks, delayed tasks may still exist). Tasks may
-// still be running from sources outside of the task scheduler and the current
-// message loop.
-// Prefer TestBrowserThreadBundle::RunUntilIdle() over this static method.
+// Deprecated: Use TestBrowserThreadBundle::RunUntilIdle().
 void RunAllTasksUntilIdle();
 
 // Get task to quit the given RunLoop. It allows a few generations of pending
 // tasks to run as opposed to run_loop->QuitClosure().
+// Prefer RunLoop::RunUntilIdle() to this.
+// TODO(gab): Assess the need for this API (see comment on
+// RunAllPendingInMessageLoop() above).
 base::Closure GetDeferredQuitTaskForRunLoop(base::RunLoop* run_loop);
 
 // Executes the specified JavaScript in the specified frame, and runs a nested
@@ -119,9 +121,8 @@ WebContents* CreateAndAttachInnerContents(RenderFrameHost* rfh);
 //
 // DEPRECATED. Consider using base::RunLoop, in most cases MessageLoopRunner is
 // not needed.  If you need to defer quitting the loop, use
-// GetDeferredQuitTaskForRunLoop directly.
-// If you found a case where base::RunLoop is inconvenient or can not be used at
-// all, please post details in a comment on https://crbug.com/668707.
+// RunLoop::RunUntilIdle() and if you really think you need deferred quit (can't
+// reach idle, please post details in a comment on https://crbug.com/668707).
 class MessageLoopRunner : public base::RefCountedThreadSafe<MessageLoopRunner> {
  public:
   enum class QuitMode {