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..abcd70aefa1c31212945b01867e6c039c27a3b9e 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_->AbandonAndStop();
   }
 
   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,68 @@ 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();
+  // TODO(gab): Enable this once Stop() properly detaches from sequence.
+  // DCHECK(origin_sequence_checker_.CalledOnValidSequence());
+  AbandonAndStop();
 }
 
 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) {
-  // Do not allow changing the task runner once something has been scheduled.
-  DCHECK_EQ(thread_id_, 0);
+void Timer::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_);
   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() {
+  // TODO(gab): Enable this when it's no longer called racily from
+  // RunScheduledTask(): https://crbug.com/587199.
+  // DCHECK(origin_sequence_checker_.CalledOnValidSequence());
+
   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,41 +160,41 @@ 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
-  // 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 {
+  // TODO(gab): Enable this when it's no longer called racily from
+  // RunScheduledTask(): https://crbug.com/587199.
+  // 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);
+  // TODO(gab): Enable this when it's no longer called racily from
+  // RunScheduledTask(): https://crbug.com/587199.
+  // DCHECK(origin_sequence_checker_.CalledOnValidSequence());
+  DCHECK(!scheduled_task_);
   is_running_ = true;
   scheduled_task_ = new BaseTimerTaskInternal(this);
   if (delay > TimeDelta::FromMicroseconds(0)) {
+    // TODO(gab): Posting BaseTimerTaskInternal::Run to another sequence makes
+    // this code racy. https://crbug.com/587199
     GetTaskRunner()->PostDelayedTask(
         posted_from_,
         base::BindOnce(&BaseTimerTaskInternal::Run,
@@ -182,26 +207,27 @@ void Timer::PostNewScheduledTask(TimeDelta delay) {
                                              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();
+scoped_refptr<SequencedTaskRunner> Timer::GetTaskRunner() {
+  return task_runner_.get() ? task_runner_ : SequencedTaskRunnerHandle::Get();
 }
 
 void Timer::AbandonScheduledTask() {
-  DCHECK(thread_id_ == 0 ||
-         thread_id_ == static_cast<int>(PlatformThread::CurrentId()));
+  // TODO(gab): Enable this when it's no longer called racily from
+  // RunScheduledTask() -> Stop(): https://crbug.com/587199.
+  // DCHECK(origin_sequence_checker_.CalledOnValidSequence());
   if (scheduled_task_) {
     scheduled_task_->Abandon();
-    scheduled_task_ = NULL;
+    scheduled_task_ = nullptr;
   }
 }
 
 void Timer::RunScheduledTask() {
+  // TODO(gab): Enable this when it's no longer called racily:
+  // https://crbug.com/587199.
+  // DCHECK(origin_sequence_checker_.CalledOnValidSequence());
+
   // Task may have been disabled.
   if (!is_running_)
     return;
@@ -209,10 +235,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 +247,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_)
@@ -231,7 +257,7 @@ void Timer::RunScheduledTask() {
 
   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