[base] Make Timer::SetTaskRunner sequence-affine.

This is a prereq to make all of base::Timer sequence-affine.

Ideally we would get rid of Timer::SetTaskRunner but it's used in many
tests to mock the clock (by redirecting the delayed task to a
TestSimpleTaskRunner or TestMockTimeTaskRunner explicitly managed by
the test). Hence this CL bans multi-threaded use cases while still
allowing same-thread use cases for old tests.

Bug: 587199, 896990

Change-Id: I1785ff404f115309ea6eec743004709048d52f07
Reviewed-on: https://chromium-review.googlesource.com/c/1394311
Commit-Queue: Gabriel Charette <gab@chromium.org>
Auto-Submit: Gabriel Charette <gab@chromium.org>
Reviewed-by: Fran├žois Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624470}
diff --git a/base/timer/timer.cc b/base/timer/timer.cc
index 973aae8..815bc11 100644
--- a/base/timer/timer.cc
+++ b/base/timer/timer.cc
@@ -100,14 +100,9 @@
 }
 
 void TimerBase::SetTaskRunner(scoped_refptr<SequencedTaskRunner> task_runner) {
-  // Do not allow changing the task runner when the Timer is running.
-  // Don't check for |origin_sequence_checker_.CalledOnValidSequence()| here to
-  // allow the use case of constructing the Timer and immediatetly invoking
-  // SetTaskRunner() before starting it (CalledOnValidSequence() would undo the
-  // DetachFromSequence() from the constructor). The |!is_running| check kind of
-  // verifies the same thing (and TSAN should catch callers that do it wrong but
-  // somehow evade all debug checks).
-  DCHECK(!is_running_);
+  DCHECK(origin_sequence_checker_.CalledOnValidSequence());
+  DCHECK(task_runner->RunsTasksInCurrentSequence());
+  DCHECK(!IsRunning());
   task_runner_.swap(task_runner);
 }
 
diff --git a/base/timer/timer.h b/base/timer/timer.h
index 6f7b450..d66e17c 100644
--- a/base/timer/timer.h
+++ b/base/timer/timer.h
@@ -42,19 +42,15 @@
 // would postpone DoStuff by another 1 second.  In other words, Reset is
 // shorthand for calling Stop and then Start again with the same arguments.
 //
-// These APIs are not thread safe. All methods must be called from the same
-// sequence (not necessarily the construction sequence), except for the
-// destructor and SetTaskRunner().
-// - The destructor may be called from any sequence when the timer is not
-// running and there is no scheduled task active, i.e. when Start() has never
-// been called or after AbandonAndStop() has been called.
-// - SetTaskRunner() may be called from any sequence when the timer is not
-// running, i.e. when Start() has never been called or Stop() has been called
-// since the last Start().
+// These APIs are not thread safe. When a method is called (except the
+// constructor), all further method calls must be on the same sequence until
+// Stop().
 //
 // By default, the scheduled tasks will be run on the same sequence that the
-// Timer was *started on*, but this can be changed *prior* to Start() via
-// SetTaskRunner().
+// Timer was *started on*. To mock time in unit tests, some old tests used
+// SetTaskRunner() to schedule the delay on a test-controlled TaskRunner. The
+// modern and preferred approach to mock time is to use ScopedTaskEnvironment's
+// MOCK_TIME mode.
 
 #ifndef BASE_TIMER_TIMER_H_
 #define BASE_TIMER_TIMER_H_
@@ -114,11 +110,17 @@
   // Returns the current delay for this timer.
   TimeDelta GetCurrentDelay() const;
 
-  // Set the task runner on which the task should be scheduled. This method can
-  // only be called before any tasks have been scheduled. If |task_runner| runs
-  // tasks on a different sequence than the sequence owning this Timer, the user
-  // task will be posted to it when the Timer fires (note that this means the
-  // user task can run after ~Timer() and should support that).
+  // Sets the task runner on which the delayed task should be scheduled when
+  // this Timer is running. This method can only be called while this Timer
+  // isn't running. This is an alternative (old) approach to mock time in tests.
+  // The modern and preferred approach is to use
+  // ScopedTaskEnvironment::MainThreadType::MOCK_TIME
+  // (ScopedTaskEnvironment::NowSource::MAIN_THREAD_MOCK_TIME may also be useful
+  // if the Timer is ever restarted). To avoid racy usage of Timer,
+  // |task_runner| must run tasks on the same sequence which this Timer is bound
+  // to (started from).
+  // TODO(gab): Migrate all callers to
+  // ScopedTaskEnvironment::MainThreadType::MOCK_TIME.
   virtual void SetTaskRunner(scoped_refptr<SequencedTaskRunner> task_runner);
 
   // Call this method to stop and cancel the timer.  It is a no-op if the timer
diff --git a/base/timer/timer_unittest.cc b/base/timer/timer_unittest.cc
index b137a6657..bcfde1c 100644
--- a/base/timer/timer_unittest.cc
+++ b/base/timer/timer_unittest.cc
@@ -20,8 +20,10 @@
 #include "base/stl_util.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/task/post_task.h"
+#include "base/test/bind_test_util.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/test/test_mock_time_task_runner.h"
+#include "base/test/test_simple_task_runner.h"
 #include "base/threading/platform_thread.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread.h"
@@ -415,28 +417,23 @@
 }
 
 TEST(TimerTest, OneShotTimer_CustomTaskRunner) {
-  // A task environment is required for the timer events on the other thread to
-  // communicate back to the Timer under test.
-  test::ScopedTaskEnvironment scoped_task_environment;
+  auto task_runner = base::MakeRefCounted<TestSimpleTaskRunner>();
 
-  Thread other_thread("OneShotTimer_CustomTaskRunner");
-  other_thread.Start();
+  OneShotTimer timer;
 
-  WaitableEvent did_run(WaitableEvent::ResetPolicy::MANUAL,
-                        WaitableEvent::InitialState::NOT_SIGNALED);
-  OneShotTimerTester f(&did_run);
-  f.SetTaskRunner(other_thread.task_runner());
-  f.Start();
-  EXPECT_TRUE(f.IsRunning() || did_run.IsSignaled());
+  bool task_ran = false;
 
-  f.WaitAndConfirmTimerFiredAfterDelay();
-  EXPECT_TRUE(did_run.IsSignaled());
+  // The timer will use the TestSimpleTaskRunner to schedule its delays.
+  timer.SetTaskRunner(task_runner);
+  timer.Start(FROM_HERE, TimeDelta::FromDays(1),
+              BindLambdaForTesting([&]() { task_ran = true; }));
 
-  // |f| should already have communicated back to this main thread before
-  // invoking Run() and as such this thread should already be aware that |f| is
-  // no longer running.
-  EXPECT_FALSE(scoped_task_environment.MainThreadHasPendingTask());
-  EXPECT_FALSE(f.IsRunning());
+  EXPECT_FALSE(task_ran);
+  EXPECT_TRUE(task_runner->HasPendingTask());
+
+  task_runner->RunPendingTasks();
+
+  EXPECT_TRUE(task_ran);
 }
 
 TEST(TimerTest, OneShotTimerWithTickClock) {
@@ -707,190 +704,4 @@
                         TimerTestWithThreadType,
                         testing::ValuesIn(testing_main_threads));
 
-namespace {
-
-// Fixture for tests requiring ScopedTaskEnvironment. Includes a WaitableEvent
-// so that cases may Wait() on one thread and Signal() (explicitly, or
-// implicitly via helper methods) on another.
-class TimerSequenceTest : public testing::Test {
- public:
-  TimerSequenceTest()
-      : event_(WaitableEvent::ResetPolicy::AUTOMATIC,
-               WaitableEvent::InitialState::NOT_SIGNALED) {}
-
-  // Block until Signal() is called on another thread.
-  void Wait() { event_.Wait(); }
-
-  void Signal() { event_.Signal(); }
-
-  // Helper to augment a task with a subsequent call to Signal().
-  OnceClosure TaskWithSignal(OnceClosure task) {
-    return BindOnce(&TimerSequenceTest::RunTaskAndSignal, Unretained(this),
-                    std::move(task));
-  }
-
-  // Create the timer.
-  void CreateTimer() { timer_.reset(new OneShotTimer); }
-
-  // Schedule an event on the timer.
-  void StartTimer(TimeDelta delay, OnceClosure task) {
-    timer_->Start(FROM_HERE, delay, std::move(task));
-  }
-
-  void SetTaskRunnerForTimer(scoped_refptr<SequencedTaskRunner> task_runner) {
-    timer_->SetTaskRunner(std::move(task_runner));
-  }
-
-  // Tell the timer to abandon the task.
-  void AbandonTask() {
-    EXPECT_TRUE(timer_->IsRunning());
-    // Reset() to call Timer::AbandonScheduledTask()
-    timer_->Reset();
-    EXPECT_TRUE(timer_->IsRunning());
-    timer_->Stop();
-    EXPECT_FALSE(timer_->IsRunning());
-  }
-
-  static void VerifyAffinity(const SequencedTaskRunner* task_runner) {
-    EXPECT_TRUE(task_runner->RunsTasksInCurrentSequence());
-  }
-
-  // Delete the timer.
-  void DeleteTimer() { timer_.reset(); }
-
- private:
-  void RunTaskAndSignal(OnceClosure task) {
-    std::move(task).Run();
-    Signal();
-  }
-
-  test::ScopedTaskEnvironment scoped_task_environment_;
-  WaitableEvent event_;
-  std::unique_ptr<OneShotTimer> timer_;
-
-  DISALLOW_COPY_AND_ASSIGN(TimerSequenceTest);
-};
-
-}  // namespace
-
-TEST_F(TimerSequenceTest, OneShotTimerTaskOnPoolSequence) {
-  scoped_refptr<SequencedTaskRunner> task_runner =
-      base::CreateSequencedTaskRunnerWithTraits({});
-
-  base::RunLoop run_loop_;
-
-  // Timer is created on this thread.
-  CreateTimer();
-
-  // Task will execute on a pool thread.
-  SetTaskRunnerForTimer(task_runner);
-  StartTimer(TimeDelta::FromMilliseconds(1),
-             BindOnce(IgnoreResult(&SequencedTaskRunner::PostTask),
-                      SequencedTaskRunnerHandle::Get(), FROM_HERE,
-                      run_loop_.QuitClosure()));
-
-  // Spin the loop so that the delayed task fires on it, which will forward it
-  // to |task_runner|. And since the Timer's task is one that posts back to this
-  // thread to quit, we finally unblock.
-  run_loop_.Run();
-
-  // Timer will be destroyed on this thread.
-  DeleteTimer();
-}
-
-TEST_F(TimerSequenceTest, OneShotTimerUsedOnPoolSequence) {
-  scoped_refptr<SequencedTaskRunner> task_runner =
-      base::CreateSequencedTaskRunnerWithTraits({});
-
-  // Timer is created on this thread.
-  CreateTimer();
-
-  // Task will be scheduled from a pool thread.
-  task_runner->PostTask(
-      FROM_HERE,
-      BindOnce(&TimerSequenceTest::StartTimer, Unretained(this),
-               TimeDelta::FromMilliseconds(1),
-               BindOnce(&TimerSequenceTest::Signal, Unretained(this))));
-  Wait();
-
-  // Timer must be destroyed on pool thread, too.
-  task_runner->PostTask(FROM_HERE,
-                        TaskWithSignal(BindOnce(&TimerSequenceTest::DeleteTimer,
-                                                Unretained(this))));
-  Wait();
-}
-
-TEST_F(TimerSequenceTest, OneShotTimerTwoSequencesAbandonTask) {
-  scoped_refptr<SequencedTaskRunner> task_runner1 =
-      base::CreateSequencedTaskRunnerWithTraits({});
-  scoped_refptr<SequencedTaskRunner> task_runner2 =
-      base::CreateSequencedTaskRunnerWithTraits({});
-
-  // Create timer on sequence #1.
-  task_runner1->PostTask(
-      FROM_HERE, TaskWithSignal(BindOnce(&TimerSequenceTest::CreateTimer,
-                                         Unretained(this))));
-  Wait();
-
-  // And tell it to execute on a different sequence (#2).
-  task_runner1->PostTask(
-      FROM_HERE,
-      TaskWithSignal(BindOnce(&TimerSequenceTest::SetTaskRunnerForTimer,
-                              Unretained(this), task_runner2)));
-  Wait();
-
-  // Task will be scheduled from sequence #1.
-  task_runner1->PostTask(
-      FROM_HERE, BindOnce(&TimerSequenceTest::StartTimer, Unretained(this),
-                          TimeDelta::FromHours(1), DoNothing().Once()));
-
-  // Abandon task - must be called from scheduling sequence (#1).
-  task_runner1->PostTask(
-      FROM_HERE, TaskWithSignal(BindOnce(&TimerSequenceTest::AbandonTask,
-                                         Unretained(this))));
-  Wait();
-
-  // Timer must be destroyed on the sequence it was scheduled from (#1).
-  task_runner1->PostTask(
-      FROM_HERE, TaskWithSignal(BindOnce(&TimerSequenceTest::DeleteTimer,
-                                         Unretained(this))));
-  Wait();
-}
-
-TEST_F(TimerSequenceTest, OneShotTimerUsedAndTaskedOnDifferentSequences) {
-  scoped_refptr<SequencedTaskRunner> task_runner1 =
-      base::CreateSequencedTaskRunnerWithTraits({});
-  scoped_refptr<SequencedTaskRunner> task_runner2 =
-      base::CreateSequencedTaskRunnerWithTraits({});
-
-  // Create timer on sequence #1.
-  task_runner1->PostTask(
-      FROM_HERE, TaskWithSignal(BindOnce(&TimerSequenceTest::CreateTimer,
-                                         Unretained(this))));
-  Wait();
-
-  // And tell it to execute on a different sequence (#2).
-  task_runner1->PostTask(
-      FROM_HERE,
-      TaskWithSignal(BindOnce(&TimerSequenceTest::SetTaskRunnerForTimer,
-                              Unretained(this), task_runner2)));
-  Wait();
-
-  // Task will be scheduled from sequence #1.
-  task_runner1->PostTask(
-      FROM_HERE,
-      BindOnce(&TimerSequenceTest::StartTimer, Unretained(this),
-               TimeDelta::FromMilliseconds(1),
-               TaskWithSignal(BindOnce(&TimerSequenceTest::VerifyAffinity,
-                                       Unretained(task_runner2.get())))));
-
-  Wait();
-
-  // Timer must be destroyed on the sequence it was scheduled from (#1).
-  task_runner1->PostTask(
-      FROM_HERE, TaskWithSignal(BindOnce(&TimerSequenceTest::DeleteTimer,
-                                         Unretained(this))));
-  Wait();
-}
-
 }  // namespace base