Make TestMockTimeTaskRunner a RunLoop::Delegate.

Introducing TestMockTimeTaskRunner::Type::kBound which will make that
TestMockTimeTaskRunner takeover the thread it's created on (a la
MessageLoop), enabling RunLoop and Thread/SequencedTaskRunnerHandle.

Also introduces RunLoop::ScopedDisallowRunningForTesting to enforce
mutual exclusion TestMockTimeTaskRunner::ScopedContext (used to toggle
context to another task runner on the main thread) and RunLoop::Run()
(meant to run the current thread's associated task runner). Mixing the
two would result in running the incorrect task runner. While I don't
think this is a use case worth supporting, experience with //base
APIs has taught me that if there's a way to use it wrong, someone
will, and it's much easier to prevent than to heal; hence this check.
(there should already be no RunLoop usage during TaskRunnerHandle 
overrides per RunLoop not being previously supported by 
TestMockTimeTaskRunner and this check ensures it stays that way :))

EDIT: Well except that HeartbeatSenderTest had found a way to use it
the deprecated way, but it's a nice fit for using a kBound 
TestMockTimeTaskRunner so all good :).

Had to drop support for virtual 
TestMockTimeTaskRunner::IsElapsingStopped() which in turn forced
removal of custom task runner in remote_commands_service_unittest.cc
whose use case is now supported by the new API :).

This enables follow-ups to:
 1) Add mock time to base::test::ScopedTaskEnvironment :)
    https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3Pkk/edit
 2) Fixing a race in base::Timer which requires RunLoop from
    TestMockTimeTaskRunner to get rid of the two remaining problematic
    use cases (see bug blocked by 703346).

Bug: 703346
Change-Id: I062b77b669853a36c30813e44dd984d01fcefbe2

TBR=pastarmovj@chromium.org (for components/policy test side-effects)

Change-Id: I062b77b669853a36c30813e44dd984d01fcefbe2
Reviewed-on: https://chromium-review.googlesource.com/614788
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Scott Nichols <nicholss@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496111}

PRESUBMIT.py

@@ -426,6 +426,14 @@
       ),
       True,
       (),
+    ),
+    (
+      'base::ScopedMockTimeMessageLoopTaskRunner',
+      (
+        'ScopedMockTimeMessageLoopTaskRunner is deprecated.',
+      ),
+      True,
+      (),
     )
 )
 

base/BUILD.gn

@@ -2170,6 +2170,7 @@ test("base_unittests") {
     "test/scoped_feature_list_unittest.cc",
     "test/scoped_mock_time_message_loop_task_runner_unittest.cc",
     "test/scoped_task_environment_unittest.cc",
+    "test/test_mock_time_task_runner_unittest.cc",
     "test/test_pending_task_unittest.cc",
     "test/test_reg_util_win_unittest.cc",
     "test/trace_event_analyzer_unittest.cc",

base/run_loop.cc

@@ -244,10 +244,40 @@ void RunLoop::QuitCurrentWhenIdleDeprecated() {
   tls_delegate.Get().Get()->active_run_loops_.top()->QuitWhenIdle();
 }
 
+#if DCHECK_IS_ON()
+RunLoop::ScopedDisallowRunningForTesting::ScopedDisallowRunningForTesting()
+    : current_delegate_(tls_delegate.Get().Get()),
+      previous_run_allowance_(
+          current_delegate_ ? current_delegate_->allow_running_for_testing_
+                            : false) {
+  if (current_delegate_)
+    current_delegate_->allow_running_for_testing_ = false;
+}
+
+RunLoop::ScopedDisallowRunningForTesting::~ScopedDisallowRunningForTesting() {
+  DCHECK_EQ(current_delegate_, tls_delegate.Get().Get());
+  if (current_delegate_)
+    current_delegate_->allow_running_for_testing_ = previous_run_allowance_;
+}
+#else   // DCHECK_IS_ON()
+// Defined out of line so that the compiler doesn't inline these and realize
+// the scope has no effect and then throws an "unused variable" warning in
+// non-dcheck builds.
+RunLoop::ScopedDisallowRunningForTesting::ScopedDisallowRunningForTesting() =
+    default;
+RunLoop::ScopedDisallowRunningForTesting::~ScopedDisallowRunningForTesting() =
+    default;
+#endif  // DCHECK_IS_ON()
+
 bool RunLoop::BeforeRun() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
 #if DCHECK_IS_ON()
+  DCHECK(delegate_->allow_running_for_testing_)
+      << "RunLoop::Run() isn't allowed in the scope of a "
+         "ScopedDisallowRunningForTesting. Hint: if mixing "
+         "TestMockTimeTaskRunners on same thread, use TestMockTimeTaskRunner's "
+         "API instead of RunLoop to drive individual task runners.";
   DCHECK(!run_called_);
   run_called_ = true;
 #endif  // DCHECK_IS_ON()

base/run_loop.h

@@ -226,6 +226,10 @@ class BASE_EXPORT RunLoop {
     RunLoopStack active_run_loops_;
     ObserverList<RunLoop::NestingObserver> nesting_observers_;
 
+#if DCHECK_IS_ON()
+    bool allow_running_for_testing_ = true;
+#endif
+
     // True once this Delegate is bound to a thread via
     // RegisterDelegateForCurrentThread().
     bool bound_ = false;
@@ -253,6 +257,28 @@ class BASE_EXPORT RunLoop {
   static void QuitCurrentDeprecated();
   static void QuitCurrentWhenIdleDeprecated();
 
+  // Run() will DCHECK if called while there's a ScopedDisallowRunningForTesting
+  // in scope on its thread. This is useful to add safety to some test
+  // constructs which allow multiple task runners to share the main thread in
+  // unit tests. While the main thread can be shared by multiple runners to
+  // deterministically fake multi threading, there can still only be a single
+  // RunLoop::Delegate per thread and RunLoop::Run() should only be invoked from
+  // it (or it would result in incorrectly driving TaskRunner A while in
+  // TaskRunner B's context).
+  class BASE_EXPORT ScopedDisallowRunningForTesting {
+   public:
+    ScopedDisallowRunningForTesting();
+    ~ScopedDisallowRunningForTesting();
+
+   private:
+#if DCHECK_IS_ON()
+    Delegate* current_delegate_;
+    const bool previous_run_allowance_;
+#endif  // DCHECK_IS_ON()
+
+    DISALLOW_COPY_AND_ASSIGN(ScopedDisallowRunningForTesting);
+  };
+
  private:
 #if defined(OS_ANDROID)
   // Android doesn't support the blocking RunLoop::Run, so it calls

base/run_loop_unittest.cc

@@ -524,6 +524,17 @@ TEST_P(RunLoopTest, DisallowNestingDeathTest) {
 }
 #endif  // defined(GTEST_HAS_DEATH_TEST) && !defined(OS_ANDROID)
 
+TEST_P(RunLoopTest, DisallowRunningForTesting) {
+  RunLoop::ScopedDisallowRunningForTesting disallow_running;
+  EXPECT_DCHECK_DEATH({ run_loop_.Run(); });
+}
+
+TEST_P(RunLoopTest, ExpiredDisallowRunningForTesting) {
+  { RunLoop::ScopedDisallowRunningForTesting disallow_running; }
+  // Running should be fine after |disallow_running| goes out of scope.
+  run_loop_.RunUntilIdle();
+}
+
 INSTANTIATE_TEST_CASE_P(Real,
                         RunLoopTest,
                         testing::Values(RunLoopTestType::kRealEnvironment));

base/test/scoped_mock_time_message_loop_task_runner.h

@@ -21,6 +21,10 @@ class SingleThreadTaskRunner;
 // Note: RunLoop() will not work in the scope of a
 // ScopedMockTimeMessageLoopTaskRunner, the underlying TestMockTimeTaskRunner's
 // methods must be used instead to pump tasks.
+//
+// DEPRECATED: Use a TestMockTimeTaskRunner::Type::kBoundToThread instead of a
+// MessageLoop + ScopedMockTimeMessageLoopTaskRunner.
+// TODO(gab): Remove usage of this API and delete it.
 class ScopedMockTimeMessageLoopTaskRunner {
  public:
   ScopedMockTimeMessageLoopTaskRunner();

base/test/test_mock_time_task_runner.cc

@@ -70,6 +70,49 @@ Time MockClock::Now() {
   return task_runner_->Now();
 }
 
+// A SingleThreadTaskRunner which forwards everything to its |target_|. This is
+// useful to break ownership chains when it is known that |target_| will outlive
+// the NonOwningProxyTaskRunner it's injected into. In particular,
+// TestMockTimeTaskRunner is forced to be ref-counted by virtue of being a
+// SingleThreadTaskRunner. As such it is impossible for it to have a
+// ThreadTaskRunnerHandle member that points back to itself as the
+// ThreadTaskRunnerHandle which it owns would hold a ref back to it. To break
+// this dependency cycle, the ThreadTaskRunnerHandle is instead handed a
+// NonOwningProxyTaskRunner which allows the TestMockTimeTaskRunner to not hand
+// a ref to its ThreadTaskRunnerHandle while promising in return that it will
+// outlive that ThreadTaskRunnerHandle instance.
+class NonOwningProxyTaskRunner : public SingleThreadTaskRunner {
+ public:
+  explicit NonOwningProxyTaskRunner(SingleThreadTaskRunner* target)
+      : target_(target) {
+    DCHECK(target_);
+  }
+
+  // SingleThreadTaskRunner:
+  bool RunsTasksInCurrentSequence() const override {
+    return target_->RunsTasksInCurrentSequence();
+  }
+  bool PostDelayedTask(const tracked_objects::Location& from_here,
+                       OnceClosure task,
+                       TimeDelta delay) override {
+    return target_->PostDelayedTask(from_here, std::move(task), delay);
+  }
+  bool PostNonNestableDelayedTask(const tracked_objects::Location& from_here,
+                                  OnceClosure task,
+                                  TimeDelta delay) override {
+    return target_->PostNonNestableDelayedTask(from_here, std::move(task),
+                                               delay);
+  }
+
+ private:
+  friend class RefCountedThreadSafe<NonOwningProxyTaskRunner>;
+  ~NonOwningProxyTaskRunner() override = default;
+
+  SingleThreadTaskRunner* const target_;
+
+  DISALLOW_COPY_AND_ASSIGN(NonOwningProxyTaskRunner);
+};
+
 }  // namespace
 
 // TestMockTimeTaskRunner::TestOrderedPendingTask -----------------------------
@@ -145,13 +188,19 @@ bool TestMockTimeTaskRunner::TemporalOrder::operator()(
   return first_task.GetTimeToRun() > second_task.GetTimeToRun();
 }
 
-TestMockTimeTaskRunner::TestMockTimeTaskRunner()
-    : now_(Time::UnixEpoch()), next_task_ordinal_(0) {
-}
+TestMockTimeTaskRunner::TestMockTimeTaskRunner(Type type)
+    : TestMockTimeTaskRunner(Time::UnixEpoch(), TimeTicks(), type) {}
 
 TestMockTimeTaskRunner::TestMockTimeTaskRunner(Time start_time,
-                                               TimeTicks start_ticks)
-    : now_(start_time), now_ticks_(start_ticks), next_task_ordinal_(0) {}
+                                               TimeTicks start_ticks,
+                                               Type type)
+    : now_(start_time), now_ticks_(start_ticks), tasks_lock_cv_(&tasks_lock_) {
+  if (type == Type::kBoundToThread) {
+    run_loop_client_ = RunLoop::RegisterDelegateForCurrentThread(this);
+    thread_task_runner_handle_ = std::make_unique<ThreadTaskRunnerHandle>(
+        MakeRefCounted<NonOwningProxyTaskRunner>(this));
+  }
+}
 
 TestMockTimeTaskRunner::~TestMockTimeTaskRunner() {
 }
@@ -256,10 +305,6 @@ bool TestMockTimeTaskRunner::PostNonNestableDelayedTask(
   return PostDelayedTask(from_here, std::move(task), delay);
 }
 
-bool TestMockTimeTaskRunner::IsElapsingStopped() {
-  return false;
-}
-
 void TestMockTimeTaskRunner::OnBeforeSelectingTask() {
   // Empty default implementation.
 }
@@ -285,7 +330,7 @@ void TestMockTimeTaskRunner::ProcessAllTasksNoLaterThan(TimeDelta max_delta) {
   }
 
   const TimeTicks original_now_ticks = now_ticks_;
-  while (!IsElapsingStopped()) {
+  while (!quit_run_loop_) {
     OnBeforeSelectingTask();
     TestPendingTask task_info;
     if (!DequeueNextTask(original_now_ticks, max_delta, &task_info))
@@ -312,6 +357,7 @@ void TestMockTimeTaskRunner::ForwardClocksUntilTickTime(TimeTicks later_ticks) {
 bool TestMockTimeTaskRunner::DequeueNextTask(const TimeTicks& reference,
                                              const TimeDelta& max_delta,
                                              TestPendingTask* next_task) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   AutoLock scoped_lock(tasks_lock_);
   if (!tasks_.empty() &&
       (tasks_.top().GetTimeToRun() - reference) <= max_delta) {
@@ -324,4 +370,50 @@ bool TestMockTimeTaskRunner::DequeueNextTask(const TimeTicks& reference,
   return false;
 }
 
+void TestMockTimeTaskRunner::Run() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // Since TestMockTimeTaskRunner doesn't process system messages: there's no
+  // hope for anything but a chrome task to call Quit(). If this RunLoop can't
+  // process chrome tasks (i.e. disallowed by default in nested RunLoops), it's
+  // thereby guaranteed to hang...
+  DCHECK(run_loop_client_->ProcessingTasksAllowed())
+      << "This is a nested RunLoop instance and needs to be of "
+         "Type::NESTABLE_TASKS_ALLOWED.";
+
+  while (!quit_run_loop_) {
+    RunUntilIdle();
+    if (quit_run_loop_ || run_loop_client_->ShouldQuitWhenIdle())
+      break;
+
+    // Peek into |tasks_| to perform one of two things:
+    //   A) If there are no remaining tasks, wait until one is posted and
+    //      restart from the top.
+    //   B) If there is a remaining delayed task. Fast-forward to reach the next
+    //      round of tasks.
+    TimeDelta auto_fast_forward_by;
+    {
+      AutoLock scoped_lock(tasks_lock_);
+      if (tasks_.empty()) {
+        while (tasks_.empty())
+          tasks_lock_cv_.Wait();
+        continue;
+      }
+      auto_fast_forward_by = tasks_.top().GetTimeToRun() - now_ticks_;
+    }
+    FastForwardBy(auto_fast_forward_by);
+  }
+  quit_run_loop_ = false;
+}
+
+void TestMockTimeTaskRunner::Quit() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  quit_run_loop_ = true;
+}
+
+void TestMockTimeTaskRunner::EnsureWorkScheduled() {
+  // Nothing to do: TestMockTimeTaskRunner::Run() will always process tasks and
+  // doesn't need an extra kick on nested runs.
+}
+
 }  // namespace base