Make base::Timer sequence-friendly.

base/timer/timer.cc

Index: base/timer/timer.cc
diff --git a/base/timer/timer.cc b/base/timer/timer.cc
index 31eec1bb574e83b32308dfbb3a94e335d550499c..4b200be50b87764dac8f283b3c716fbb52341775 100644
--- a/base/timer/timer.cc
+++ b/base/timer/timer.cc
@@ -11,16 +11,14 @@
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
-#include "base/single_thread_task_runner.h"
 #include "base/threading/platform_thread.h"
-#include "base/threading/thread_task_runner_handle.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/time/tick_clock.h"
 
 namespace base {
 
-// BaseTimerTaskInternal is a simple delegate for scheduling a callback to
-// Timer in the thread's default task runner. It also handles the following
-// edge cases:
+// BaseTimerTaskInternal is a simple delegate for scheduling a callback to Timer
+// on the current sequence. It also handles the following edge cases:
 // - deleted by the task runner.
 // - abandoned (orphaned) by Timer.
 class BaseTimerTaskInternal {
@@ -34,33 +32,31 @@ class BaseTimerTaskInternal {
     // destructed.  If so, don't leave Timer with a dangling pointer
     // to this.
     if (timer_)
-      timer_->StopAndAbandon();
+      timer_->Stop();
   }
 
   void Run() {
-    // timer_ is NULL if we were abandoned.
+    // |timer_| is nullptr if we were abandoned.
     if (!timer_)
       return;
 
-    // *this will be deleted by the task runner, so Timer needs to
-    // forget us:
-    timer_->scheduled_task_ = NULL;
+    // |this| will be deleted by the task runner, so Timer needs to forget us:
+    timer_->scheduled_task_ = nullptr;
 
-    // Although Timer should not call back into *this, let's clear
-    // the timer_ member first to be pedantic.
+    // Although Timer should not call back into |this|, let's clear |timer_|
+    // first to be pedantic.
     Timer* timer = timer_;
-    timer_ = NULL;
+    timer_ = nullptr;
     timer->RunScheduledTask();
   }
 
-  // The task remains in the MessageLoop queue, but nothing will happen when it
-  // runs.
-  void Abandon() {
-    timer_ = NULL;
-  }
+  // The task remains in the queue, but nothing will happen when it runs.
+  void Abandon() { timer_ = nullptr; }
 
  private:
   Timer* timer_;
+
+  DISALLOW_COPY_AND_ASSIGN(BaseTimerTaskInternal);
 };
 
 Timer::Timer(bool retain_user_task, bool is_repeating)
@@ -68,11 +64,16 @@ Timer::Timer(bool retain_user_task, bool is_repeating)
 
 Timer::Timer(bool retain_user_task, bool is_repeating, TickClock* tick_clock)
     : scheduled_task_(nullptr),
-      thread_id_(0),
       is_repeating_(is_repeating),
       retain_user_task_(retain_user_task),
       tick_clock_(tick_clock),
-      is_running_(false) {}
+      is_running_(false) {
+  // It is safe for the timer to be created on a different thread/sequence than
+  // the one from which the timer APIs are called. The first call to the
+  // checker's CalledOnValidSequence() method will re-bind the checker, and
+  // later calls will verify that the same task runner is used.
+  origin_sequence_checker_.DetachFromSequence();
+}
 
 Timer::Timer(const tracked_objects::Location& posted_from,
              TimeDelta delay,
@@ -89,44 +90,85 @@ Timer::Timer(const tracked_objects::Location& posted_from,
       posted_from_(posted_from),
       delay_(delay),
       user_task_(user_task),
-      thread_id_(0),
       is_repeating_(is_repeating),
       retain_user_task_(true),
       tick_clock_(tick_clock),
-      is_running_(false) {}
+      is_running_(false) {
+  // See comment in other constructor.
+  origin_sequence_checker_.DetachFromSequence();
+}
 
 Timer::~Timer() {
-  StopAndAbandon();
+  // As highlighted in the constructor. It's okay to start the Timer on a
+  // different sequence but it must then be sequentially stopped on that
+  // sequence as well before it can be deleted on its owning sequence.
+  DCHECK(origin_sequence_checker_.CalledOnValidSequence() || !is_running_);
+
+  // Don't call Stop() if |!origin_sequence_checker_.CalledOnValidSequence()|
+  // (which, per the above, implies |!is_running|) -- there shouldn't be a
+  // pending |scheduled_task_| pointing at |this| if |!is_running|.
+  DCHECK(is_running_ || !scheduled_task_);
+  if (is_running_)
+    Stop();
 }
 
 bool Timer::IsRunning() const {
+  DCHECK(origin_sequence_checker_.CalledOnValidSequence());
   return is_running_;
 }
 
 TimeDelta Timer::GetCurrentDelay() const {
+  DCHECK(origin_sequence_checker_.CalledOnValidSequence());
   return delay_;
 }
 
-void Timer::SetTaskRunner(scoped_refptr<SingleThreadTaskRunner> task_runner) {
+void Timer::SetTaskRunner(scoped_refptr<SequencedTaskRunner> task_runner) {
   // Do not allow changing the task runner once something has been scheduled.

[fdoray, 2017/05/10 14:22:58]

... when the Timer is running.

Because it is possible to change the TaskRunner after Start()+Stop().

[gab, 2017/05/26 04:26:57]

Done.

-  DCHECK_EQ(thread_id_, 0);
+  // 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_);
   task_runner_.swap(task_runner);
 }
 
 void Timer::Start(const tracked_objects::Location& posted_from,
                   TimeDelta delay,
                   const base::Closure& user_task) {
-  SetTaskInfo(posted_from, delay, user_task);
+  DCHECK(origin_sequence_checker_.CalledOnValidSequence());
+
+  posted_from_ = posted_from;
+  delay_ = delay;
+  user_task_ = user_task;
+
   Reset();
 }
 
 void Timer::Stop() {
+  DCHECK(origin_sequence_checker_.CalledOnValidSequence());
+
+  // While a pending scheduled task could in theory be left in the queue in the
+  // hope of recycling it should the Timer be restarted before it fires, it's
+  // simpler to unconditionally abandon it as it helps avoid races between it
+  // and ~Timer() should its owning sequence (where it's constructed/destroyed)
+  // differ from its running sequence (where it was started -- bound to
+  // |origin_sequence_checker_|). Hopefully repeated fast start/stop of the
+  // same Timer is rare...
+  // Note: It's also important to abandon before |user_task_.Reset()| below as
+  // |user_task_| may indirectly own this Timer.
+  AbandonScheduledTask();
+
   is_running_ = false;
   if (!retain_user_task_)
     user_task_.Reset();
+  // No more member accesses here: |this| could be deleted after freeing
+  // |user_task_|.
 }
 
 void Timer::Reset() {
+  DCHECK(origin_sequence_checker_.CalledOnValidSequence());
   DCHECK(!user_task_.is_null());
 
   // If there's no pending task, start one up and return.
@@ -135,73 +177,60 @@ void Timer::Reset() {
     return;
   }
 
-  // Set the new desired_run_time_.
+  // Set the new |desired_run_time_|.
   if (delay_ > TimeDelta::FromMicroseconds(0))
     desired_run_time_ = Now() + delay_;
   else
     desired_run_time_ = TimeTicks();
 
   // We can use the existing scheduled task if it arrives before the new

[fdoray, 2017/05/10 14:22:58]

In a world where TaskScheduler is able to ignore canceled tasks, reusing a task
that will run before the delay and posting a second task to wait the remaining
delay from there = one unnecessary wake up. It would have been better to cancel
the existing task and post the second task right away.

Timer would also be simpler without this "optimization".

[gab, 2017/05/26 04:26:57]

I agree, but that's a behaviour change that belongs in another CL once we do
support that.

-  // desired_run_time_.
+  // |desired_run_time_|.
   if (desired_run_time_ >= scheduled_run_time_) {
     is_running_ = true;
     return;
   }
 
-  // We can't reuse the scheduled_task_, so abandon it and post a new one.
+  // We can't reuse the |scheduled_task_|, so abandon it and post a new one.
   AbandonScheduledTask();
   PostNewScheduledTask(delay_);
 }
 
 TimeTicks Timer::Now() const {
+  DCHECK(origin_sequence_checker_.CalledOnValidSequence());
   return tick_clock_ ? tick_clock_->NowTicks() : TimeTicks::Now();
 }
 
-void Timer::SetTaskInfo(const tracked_objects::Location& posted_from,
-                        TimeDelta delay,
-                        const base::Closure& user_task) {
-  posted_from_ = posted_from;
-  delay_ = delay;
-  user_task_ = user_task;
-}
-
 void Timer::PostNewScheduledTask(TimeDelta delay) {
-  DCHECK(scheduled_task_ == NULL);
+  DCHECK(origin_sequence_checker_.CalledOnValidSequence());
+  DCHECK(!scheduled_task_);
   is_running_ = true;
   scheduled_task_ = new BaseTimerTaskInternal(this);

[fdoray, 2017/05/10 14:22:58]

Instead of having a BaseTimerTaskInternal, simply post a
Timer::RunScheduledTask() task bound to a weak ptr to this? That would simplify
the code and would allow TaskScheduler/MessageLoop to know when tasks are
canceled.

[gab, 2017/05/26 04:26:57]

Again, this can definitely be improved with WeakPtrs and all, but that isn't the
purpose of this CL.

   if (delay > TimeDelta::FromMicroseconds(0)) {
-    GetTaskRunner()->PostDelayedTask(
+    SequencedTaskRunnerHandle::Get()->PostDelayedTask(
         posted_from_,
         base::BindOnce(&BaseTimerTaskInternal::Run,
                        base::Owned(scheduled_task_)),
         delay);
     scheduled_run_time_ = desired_run_time_ = Now() + delay;
   } else {
-    GetTaskRunner()->PostTask(posted_from_,
-                              base::BindOnce(&BaseTimerTaskInternal::Run,
-                                             base::Owned(scheduled_task_)));
+    SequencedTaskRunnerHandle::Get()->PostTask(
+        posted_from_, base::BindOnce(&BaseTimerTaskInternal::Run,
+                                     base::Owned(scheduled_task_)));
     scheduled_run_time_ = desired_run_time_ = TimeTicks();
   }
-  // Remember the thread ID that posts the first task -- this will be verified
-  // later when the task is abandoned to detect misuse from multiple threads.
-  if (!thread_id_)
-    thread_id_ = static_cast<int>(PlatformThread::CurrentId());
-}
-
-scoped_refptr<SingleThreadTaskRunner> Timer::GetTaskRunner() {
-  return task_runner_.get() ? task_runner_ : ThreadTaskRunnerHandle::Get();
 }
 
 void Timer::AbandonScheduledTask() {
-  DCHECK(thread_id_ == 0 ||
-         thread_id_ == static_cast<int>(PlatformThread::CurrentId()));
+  DCHECK(origin_sequence_checker_.CalledOnValidSequence());
   if (scheduled_task_) {
     scheduled_task_->Abandon();
-    scheduled_task_ = NULL;
+    scheduled_task_ = nullptr;
   }
 }
 
 void Timer::RunScheduledTask() {
+  DCHECK(origin_sequence_checker_.CalledOnValidSequence());
+
   // Task may have been disabled.
   if (!is_running_)
     return;
@@ -209,10 +238,10 @@ void Timer::RunScheduledTask() {
   // First check if we need to delay the task because of a new target time.
   if (desired_run_time_ > scheduled_run_time_) {
     // Now() can be expensive, so only call it if we know the user has changed
-    // the desired_run_time_.
+    // the |desired_run_time_|.
     TimeTicks now = Now();
     // Task runner may have called us late anyway, so only post a continuation
-    // task if the desired_run_time_ is in the future.
+    // task if the |desired_run_time_| is in the future.
     if (desired_run_time_ > now) {
       // Post a new task to span the remaining time.
       PostNewScheduledTask(desired_run_time_ - now);
@@ -221,7 +250,7 @@ void Timer::RunScheduledTask() {
   }
 
   // Make a local copy of the task to run. The Stop method will reset the
-  // user_task_ member if retain_user_task_ is false.
+  // |user_task_| member if |retain_user_task_| is false.
   base::Closure task = user_task_;
 
   if (is_repeating_)
@@ -229,9 +258,12 @@ void Timer::RunScheduledTask() {
   else
     Stop();
 
-  task.Run();
+  if (task_runner_ && !task_runner_->RunsTasksOnCurrentThread())
+    task_runner_->PostTask(posted_from_, std::move(task));
+  else
+    task.Run();
 
-  // No more member accesses here: *this could be deleted at this point.
+  // No more member accesses here: |this| could be deleted at this point.
 }
 
 }  // namespace base