[ScopedTaskEnvironment] Explicitly allow and test nested FastForwardBy()

Discovered this use case @
https://chromium-review.googlesource.com/c/chromium/src/+/1686776/5
and extracted fix from it.

An example of how this can happen is
FtlMessageReceptionChannelTest.TimeoutIncreasesToMaximum
which uses FastForwardBy from a GMOCK WillRepeatedly() hook.

Also fix an issue in calls to AllowRunTasks(). Because of
nesting and the way RunUntilIdle() works (with separate
allow/disallow phases), it was possible to enter a nested call
from either an allow/disallow phase and break the state on exit.
This CL makes RunUntilIdle() always restore the previous state.

This will also be required in
https://chromium-review.googlesource.com/c/chromium/src/+/1686776
which will need to AllowRunTasks() from FastForwardBy().

R=fdoray@chromium.org

Bug: 946657
Change-Id: I634a6ab89add3424383b4f105069b080d6fa25b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1687796
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#675704}

base/test/scoped_task_environment.cc

@@ -4,6 +4,7 @@
 
 #include "base/test/scoped_task_environment.h"
 
+#include <algorithm>
 #include <memory>
 
 #include "base/bind_helpers.h"
@@ -98,8 +99,8 @@ class ScopedTaskEnvironment::TestTaskTracker
  public:
   TestTaskTracker();
 
-  // Allow running tasks.
-  void AllowRunTasks();
+  // Allow running tasks. Returns whether tasks were previously allowed to run.
+  bool AllowRunTasks();
 
   // Disallow running tasks. Returns true on success; success requires there to
   // be no tasks currently running. Returns false if >0 tasks are currently
@@ -263,7 +264,9 @@ class ScopedTaskEnvironment::MockTimeDomain
 
     if (!fast_forward_cap.is_max()) {
       AutoLock lock(now_ticks_lock_);
-      now_ticks_ = fast_forward_cap;
+      // It's possible that Now() is already beyond |fast_forward_cap| when the
+      // caller nests multiple FastForwardBy() calls.
+      now_ticks_ = std::max(now_ticks_, fast_forward_cap);
     }
 
     return false;
@@ -509,6 +512,8 @@ void ScopedTaskEnvironment::RunUntilIdle() {
   // This can also be simplified even further once TaskTracker becomes directly
   // aware of main thread tasks. https://crbug.com/660078.
 
+  const bool could_run_tasks = task_tracker_->AllowRunTasks();
+
   for (;;) {
     task_tracker_->AllowRunTasks();
 
@@ -556,9 +561,9 @@ void ScopedTaskEnvironment::RunUntilIdle() {
   }
 
   // The above loop always ends with running tasks being disallowed. Re-enable
-  // parallel execution before returning unless in
-  // ThreadPoolExecutionMode::QUEUED.
-  if (thread_pool_execution_mode_ != ThreadPoolExecutionMode::QUEUED)
+  // parallel execution before returning if it was allowed at the beginning of
+  // this call.
+  if (could_run_tasks)
     task_tracker_->AllowRunTasks();
 }
 
@@ -625,10 +630,12 @@ ScopedTaskEnvironment::TestTaskTracker::TestTaskTracker()
       can_run_tasks_cv_(&lock_),
       task_completed_(&lock_) {}
 
-void ScopedTaskEnvironment::TestTaskTracker::AllowRunTasks() {
+bool ScopedTaskEnvironment::TestTaskTracker::AllowRunTasks() {
   AutoLock auto_lock(lock_);
+  const bool could_run_tasks = can_run_tasks_;
   can_run_tasks_ = true;
   can_run_tasks_cv_.Broadcast();
+  return could_run_tasks;
 }
 
 bool ScopedTaskEnvironment::TestTaskTracker::DisallowRunTasks() {

base/test/scoped_task_environment.h

@@ -101,13 +101,13 @@ class ScopedTaskEnvironment {
   // Note that this is irrelevant (and ignored) under
   // ThreadingMode::MAIN_THREAD_ONLY
   enum class ThreadPoolExecutionMode {
-    // Thread pool tasks are queued and only executed when RunUntilIdle() is
-    // explicitly
-    // called.
+    // Thread pool tasks are queued and only executed when RunUntilIdle(),
+    // FastForwardBy(), or FastForwardUntilNoTasksRemain() are explicitly
+    // called. Note: RunLoop::Run() does *not* unblock the ThreadPool in this
+    // mode (it strictly runs only the main thread).
     QUEUED,
     // Thread pool tasks run as they are posted. RunUntilIdle() can still be
-    // used to block
-    // until done.
+    // used to block until done.
     ASYNC,
     DEFAULT = ASYNC
   };
@@ -196,8 +196,40 @@ class ScopedTaskEnvironment {
 
   // Only valid for instances with a MOCK_TIME MainThreadType. Fast-forwards
   // virtual time by |delta|, causing all tasks on the main thread with a
-  // remaining delay less than or equal to |delta| to be executed before this
-  // returns. |delta| must be non-negative.
+  // remaining delay less than or equal to |delta| to be executed in their
+  // natural order before this returns. |delta| must be non-negative. Upon
+  // returning from this method, NowTicks() will be >= the initial |NowTicks() +
+  // delta|. It is guaranteed to be == iff tasks executed in this
+  // FastForwardBy() didn't result in nested calls to time-advancing-methods.
+  //
+  // Nesting FastForwardBy() calls is fairly rare but here's what will happen if
+  // you do: Each FastForwardBy() call will set a goal equal to |NowTicks() +
+  // delta|, how far the combined nesting calls reach depends on the current
+  // NowTicks() at the time each nesting fast-forward call is made.
+  //
+  // e.g. nesting a FastForwardBy(2ms) from a 1ms delayed task running from a
+  // FastForwardBy(1ms) will:
+  //   FastForwardBy(1ms)
+  //   ================
+  //                  FastForwardBy(2ms)
+  //                  ================================
+  //   Result: NowTicks() is 3ms further in the future
+  //
+  // but, nesting the same FastForwardBy(2ms) from an immediate task running
+  // from the same FastForwardBy(1ms) would:
+  //   FastForwardBy(1ms)
+  //   ================
+  //    FastForwardBy(2ms)
+  //    ================================
+  //   Result: NowTicks() is 2ms further in the future
+  //
+  // and if the initial call was instead a FastForwardBy(5ms), we'd get:
+  //   FastForwardBy(5ms)                             (cover remaining delta)
+  //   ================                               ==========================
+  //                  FastForwardBy(2ms)
+  //                  ================================
+  //   Result: NowTicks() is 5ms further in the future.
+  //
   // TODO(gab): Make this apply to ThreadPool delayed tasks as well
   // (currently only main thread time is mocked).
   void FastForwardBy(TimeDelta delta);

base/test/scoped_task_environment_unittest.cc

@@ -432,6 +432,30 @@ TEST_F(ScopedTaskEnvironmentTest, FastForwardZero) {
   EXPECT_EQ(2000, run_count.load(std::memory_order_relaxed));
 }
 
+TEST_F(ScopedTaskEnvironmentTest, NestedFastForwardBy) {
+  ScopedTaskEnvironment scoped_task_environment(
+      ScopedTaskEnvironment::MainThreadType::MOCK_TIME);
+
+  constexpr TimeDelta kDelayPerTask = TimeDelta::FromMilliseconds(1);
+  const TimeTicks start_time = scoped_task_environment.NowTicks();
+
+  int max_nesting_level = 0;
+
+  RepeatingClosure post_fast_forwarding_task;
+  post_fast_forwarding_task = BindLambdaForTesting([&]() {
+    if (max_nesting_level < 5) {
+      ++max_nesting_level;
+      ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+          FROM_HERE, post_fast_forwarding_task, kDelayPerTask);
+      scoped_task_environment.FastForwardBy(kDelayPerTask);
+    }
+  });
+  post_fast_forwarding_task.Run();
+
+  EXPECT_EQ(max_nesting_level, 5);
+  EXPECT_EQ(scoped_task_environment.NowTicks(), start_time + kDelayPerTask * 5);
+}
+
 TEST_F(ScopedTaskEnvironmentTest, CrossThreadTaskPostingDoesntAffectMockTime) {
   ScopedTaskEnvironment scoped_task_environment(
       ScopedTaskEnvironment::MainThreadType::MOCK_TIME,