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 6ec18f181488c3a5b533abe93cf9b793914e6451..6895f9e2eda9feef7a709b25db1d06830d4493d0 100644
--- a/base/timer/timer.cc
+++ b/base/timer/timer.cc
@@ -11,68 +11,205 @@
 #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/sequenced_task_runner.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:
-// - deleted by the task runner.
-// - abandoned (orphaned) by Timer.
-class BaseTimerTaskInternal {
+// BaseTimerTaskInternal is a simple delegate for scheduling a callback to Timer

[vmpstr, 2016/12/03 01:14:04]

s/simple // :)

Can you also split up the comment into a couple of blocks,

- common functionality.
- non-repeating functionality.
- repeating functionality.

It's a bit unclear which sentences are still referring to non-repeating stuff,
and which ones are a continuation of common description

[gab, 2016/12/23 20:22:11]

:)
Clarified comment.

+// in the destination task runner. It owns itself on the destination task
+// runner. If non-repeating, it will self-destruct when it fires otherwise it
+// will re-enqueue until abandoned. If non-repeating it will also invoke
+// |stop_callback| on |origin_task_runner| *before* running |task|. The WeakPtr
+// ensures that the task to trigger a self-destruction cancels all others. As
+// such, cancellation is racing with the scheduled delayed Run() task but so

[vmpstr, 2016/12/03 01:14:04]

By race here, do you mean that either Abandon will run or Run will run, but not
both at the same time? If they both can run, this is double delete badness

[gab, 2016/12/23 20:22:11]

I mean that Abandon() is not a delayed task while Run() is, so Run() can come
out ahead of it in the sequence if the delay expires just before Abandon() is
posted.

The two tasks are still sequenced so the race is in their scheduling, not in
their execution.

Clarified.

+// would any other cancellation method (e.g. using locks and sharing state with
+// the origin task runner). An invariant assumed below is that there always is
+// exactly one pending Run() task associated with this BaseTimerTaskInternal, as
+// such only Run() needs to repost itself to maintain that invariant until self-
+// destruction.
+//
+// Note: Timer and BaseTimerTaskInternal try to communicate synchronously when
+// possible (i.e. when SetTaskRunner() wasn't used, which they can verify
+// through RunsTasksOnCurrentThread()). Timer must also always use asynchronous
+// calls when invoking Abandon() to avoid calling "delete this" from a reentrant
+// call while handling Run().
+class BaseTimerTaskInternal : public SupportsWeakPtr<BaseTimerTaskInternal> {
  public:
-  explicit BaseTimerTaskInternal(Timer* timer)
-      : timer_(timer) {
+  // A helper which invokes a Closure when destroyed. Required to notify
+  // BaseTimerTaskInternal when its Run() task is suppressed before it's
+  // scheduled (e.g. because the sequence is shutdown). Note: such cleanups
+  // can't be bound directly to ~BaseTimerTaskInternal() as it owns itself (its
+  // tasks use WeakPtrs and deleting them won't trigger
+  // ~BaseTimerTaskInternal()).
+  class CleanupTrigger {

[vmpstr, 2016/12/03 01:14:04]

Consider adding RunOnDestroy type of class to base elsewhere, since I know this
can be a pretty common pattern

[gab, 2016/12/23 20:22:11]

Whereelse? Tried to code search for it and didn't find much.

+   public:
+    ~CleanupTrigger() { std::move(on_destruction_).Run(); }
+
+   private:
+    friend class BaseTimerTaskInternal;
+    CleanupTrigger(OnceClosure on_destruction)
+        : on_destruction_(std::move(on_destruction)) {}
+
+    OnceClosure on_destruction_;
+
+    DISALLOW_COPY_AND_ASSIGN(CleanupTrigger);
+  };
+
+  BaseTimerTaskInternal(const tracked_objects::Location& posted_from,
+                        const Closure& task,
+                        TimeDelta delay,
+                        bool is_repeating,
+                        TickClock* tick_clock,
+                        scoped_refptr<SequencedTaskRunner> origin_task_runner,
+                        const Closure& stop_callback)
+      : posted_from_(posted_from),
+        task_(task),
+        delay_(delay),
+        is_repeating_(is_repeating),
+        tick_clock_(tick_clock),
+        desired_run_time_(Now()),
+        origin_task_runner_(std::move(origin_task_runner)),
+        stop_callback_(stop_callback) {
+    destination_sequence_checker_.DetachFromSequence();

[vmpstr, 2016/12/03 01:14:04]

Can you comment that we need to detatch to ensure that it's bound to the running
sequence? (that's why we do this, right?)

[gab, 2016/12/23 20:22:11]

Right, already have a comment to that effect on |destination_sequence_checker_|
(and this is generally always the reason why some classes detach in
constructor). Let me know if you still think one is needed here as well.

   }
 
-  ~BaseTimerTaskInternal() {
-    // This task may be getting cleared because the task runner has been
-    // destructed.  If so, don't leave Timer with a dangling pointer
-    // to this.
-    if (timer_)
-      timer_->StopAndAbandon();
-  }
+  ~BaseTimerTaskInternal() = default;
 
-  void Run() {
-    // timer_ is NULL if we were abandoned.
-    if (!timer_)
+  void Run(std::unique_ptr<CleanupTrigger> cleanup_trigger) {
+    DCHECK(destination_sequence_checker_.CalledOnValidSequence());
+
+    const TimeDelta delay_remaining = desired_run_time_ - Now();
+    if (delay_remaining > TimeDelta::FromMicroseconds(0)) {
+#if DCHECK_IS_ON()
+      DCHECK(was_reset_) << delay_remaining;
+#endif
+      SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+          posted_from_, base::Bind(&BaseTimerTaskInternal::Run, AsWeakPtr(),
+                                   Passed(std::move(cleanup_trigger))),
+          delay_remaining);
       return;
+    }
 
-    // *this will be deleted by the task runner, so Timer needs to
-    // forget us:
-    timer_->scheduled_task_ = NULL;
+    // Trigger cleanup before running the |task_|. e.g. to Stop() OneShotTimers
+    // so that anything posted from the task runs in the context of a stopped
+    // timer as expected.
+    if (!is_repeating_)
+      cleanup_trigger.reset();
+
+    task_.Run();
+
+    if (is_repeating_) {
+      // TODO(gab): Currently rebasing on Now() to match prior status quo but

[vmpstr, 2016/12/03 01:14:04]

Can you file a bug for this and reference it here?

[gab, 2016/12/23 20:22:10]

Done.

+      // think it would be better to merely |+= delay_| here to avoid the timer
+      // drifting with the delayed task overhead.
+      desired_run_time_ = Now() + delay_;
+      SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+          posted_from_, base::Bind(&BaseTimerTaskInternal::Run, AsWeakPtr(),
+                                   Passed(std::move(cleanup_trigger))),
+          delay_);

[vmpstr, 2016/12/03 01:14:04]

was_reset_ = false?

[gab, 2016/12/23 20:22:10]

Done.

+    } else {
+      delete this;
+    }
+  }
 
-    // Although Timer should not call back into *this, let's clear
-    // the timer_ member first to be pedantic.
-    Timer* timer = timer_;
-    timer_ = NULL;
-    timer->RunScheduledTask();
+  // Postpones the |desired_run_time_| by |delay_|.
+  void Reset() {
+    DCHECK(destination_sequence_checker_.CalledOnValidSequence());
+    DCHECK(!delay_.is_zero());
+
+    // Since this message is sent asynchronously, a few things might occur:
+    //  - This is a one-shot timer and Run() hasn't occurred:
+    //      postpone |desired_run_time_|, Run() will postpone itself when it
+    //      runs.
+    //  - This is a one-shot timer and Run() already occurred:
+    //      Reset() came in too late and will be canceled via the WeakPtr.
+    //  - This is a repeating timer and Run() has occurred zero to many times:
+    //      postpone the next |desired_run_time_|.
+    // So overall, either the Reset() came in too late and will be canceled or

[vmpstr, 2016/12/03 01:14:04]

This comment is a bit confusing to me. So does this mean that there is no actual
reset task, right? Since we just postpone the run task? (You reference "and will
be canceled via the WeakPtr", which is the confusing part... what exactly will
be canceled?)

Or is this function called by the reset task created elsewhere? If that's the
case, aren't we already running the reset task since we're in this function?
Which means it won't be canceled?

[gab, 2016/12/23 20:22:11]

This whole comment is just meant to say that although Reset() appears complex at
first sight it really isn't because either it never gets to run (per the
WeakPtr) or it merely needs to update |desired_run_time_|. 

I first tweaked it to:
    // Since this message is sent asynchronously, a few things might occur:
    //  - This is a one-shot timer and Run() hasn't occurred:
    //      postpone |desired_run_time_|, Run() will postpone itself when it
    //      runs.
    //  - This is a one-shot timer and Run() already occurred:
    //      Reset() came in too late and will be canceled via the WeakPtr.
    //  - This is a repeating timer and Run() has occurred zero to many times:
    //      postpone the next |desired_run_time_|.
    // So overall, either the Reset() task came in too late (and won't even
    // execute per the WeakPtr) or the |desired_run_time_| merely needs to be
    // postponed.

but then I decided that was probably overkill (i.e. adding confusion to an
otherwise simple outcome) and now made it:

    // Since Reset() is sequenced with Run() (and cancelled via the WeakPtr if
    // Run() is scheduled before it -- i.e. delay expires as Reset() is posted):
    // it is sufficient to merely update |desired_run_time_|.

+    // the |desired_run_time_| merely needs to be postponed.
+
+    desired_run_time_ = Now() + delay_;
+
+#if DCHECK_IS_ON()
+    was_reset_ = true;
+#endif
   }
 
-  // The task remains in the MessageLoop queue, but nothing will happen when it
-  // runs.
   void Abandon() {
-    timer_ = NULL;
+    DCHECK(destination_sequence_checker_.CalledOnValidSequence());
+
+    // Delete self, invalidating WeakPtrs and as such, pending Run()/Reset()

[vmpstr, 2016/12/03 01:14:04]

Here as well, what's a reset task here?

[gab, 2016/12/23 20:22:11]

Actually it can only cancel "the pending Run() task" (per invariant there's
always exactly one pending Run()).

It doesn't make sense for there to be a pending Reset() in the queue as Timer
doesn't post anything else to this BaseTimerTaskInternal after sending the
Abandon task.

Done.

+    // tasks.
+    delete this;
+  }
+
+  std::unique_ptr<CleanupTrigger> GetCleanupTrigger() {

[vmpstr, 2016/12/03 01:14:04]

nit: CreateCleanupTrigger

[gab, 2016/12/23 20:22:10]

Done.

+    // Note: it's important that CleanupTrigger's Closure be bound to a WeakPtr
+    // so that it only triggers if the Run() task is suppressed from its
+    // sequence while it was live (i.e. it shouldn't trigger when the task is
+    // destroyed because the WeakPtr was invalid when an abandoned Run() task
+    // was scheduled).
+    return WrapUnique(
+        new CleanupTrigger(Bind(&BaseTimerTaskInternal::Cleanup, AsWeakPtr())));
   }
 
  private:
-  Timer* timer_;
+  TimeTicks Now() const {
+    return tick_clock_ ? tick_clock_->NowTicks() : TimeTicks::Now();
+  }
+
+  // Cleans up state on the Timer side when this BaseTimerTaskInternal is done
+  // with its role.
+  void Cleanup() {
+    DCHECK(destination_sequence_checker_.CalledOnValidSequence());
+
+    if (origin_task_runner_->RunsTasksOnCurrentThread()) {
+      stop_callback_.Run();
+    } else {
+      origin_task_runner_->PostTask(FROM_HERE, stop_callback_);
+    }
+  }
+
+  const tracked_objects::Location posted_from_;
+  const Closure task_;
+  const TimeDelta delay_;
+  const bool is_repeating_;
+
+  TickClock* tick_clock_;
+  TimeTicks desired_run_time_;
+
+  const scoped_refptr<SequencedTaskRunner> origin_task_runner_;
+  const Closure stop_callback_;
+
+  // Verifies that every operation (but construction) happens on the sequence
+  // where the task is intended to run.
+  SequenceChecker destination_sequence_checker_;
+
+#if DCHECK_IS_ON()
+  // Used to verify that reposts only occur when a Reset() was involved.
+  bool was_reset_ = false;
+#endif
+
+  DISALLOW_COPY_AND_ASSIGN(BaseTimerTaskInternal);
 };
 
 Timer::Timer(bool retain_user_task, bool is_repeating)
     : Timer(retain_user_task, is_repeating, nullptr) {}
 
 Timer::Timer(bool retain_user_task, bool is_repeating, TickClock* tick_clock)
-    : scheduled_task_(nullptr),
-      thread_id_(0),
-      is_repeating_(is_repeating),
+    : is_repeating_(is_repeating),
       retain_user_task_(retain_user_task),
       tick_clock_(tick_clock),
-      is_running_(false) {}
+      is_running_(false),
+      weak_ptr_factory_(this) {
+  // 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,
@@ -85,72 +222,86 @@ Timer::Timer(const tracked_objects::Location& posted_from,
              const base::Closure& user_task,
              bool is_repeating,
              TickClock* tick_clock)
-    : scheduled_task_(nullptr),
-      posted_from_(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),
+      weak_ptr_factory_(this) {
+  // See comment in other constructor.
+  origin_sequence_checker_.DetachFromSequence();
+}
 
 Timer::~Timer() {
-  StopAndAbandon();
+  if (origin_sequence_checker_.CalledOnValidSequence()) {

[vmpstr, 2016/12/03 01:14:04]

This is changing code behavior based on whether DCHECKs are on. The
CalledOnValidSequence always returns true if DCHECKs are off... Is this
expected? Can you drop a comment explaining here?

[gab, 2016/12/23 20:22:10]

No it doesn't because the member is a SequenceCheckerImpl (there's already a
comment there but I'll also add one here to make this clear).

+    Stop();
+  } else {
+    // 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 original sequence.
+    DCHECK(!is_running_);
+  }
 }
 
 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);
-  task_runner_.swap(task_runner);
+void Timer::SetTaskRunner(scoped_refptr<SequencedTaskRunner> task_runner) {
+  DCHECK(origin_sequence_checker_.CalledOnValidSequence());
+
+  // Do not allow changing the task runner while something is been scheduled.

[vmpstr, 2016/12/03 01:14:04]

s/been //

[gab, 2016/12/23 20:22:11]

Done.

+  DCHECK(!is_running_);
+  destination_task_runner_ = std::move(task_runner);
 }
 
 void Timer::Start(const tracked_objects::Location& posted_from,
                   TimeDelta delay,
                   const base::Closure& user_task) {
+  DCHECK(origin_sequence_checker_.CalledOnValidSequence());
+
+  // Do a best-effort to cancel the previous task, it may still racily fire
+  // though.
+  AbandonScheduledTask();
+
   SetTaskInfo(posted_from, delay, user_task);
   Reset();

[vmpstr, 2016/12/03 01:14:04]

Is this call always just PostNewScheduledTask?

[gab, 2016/12/23 20:22:11]

Actually yes it is, inlined that instead and added DCHECK(!is_running_) to
PostNewScheduledTask().

 }
 
 void Timer::Stop() {
-  is_running_ = false;
+  DCHECK(origin_sequence_checker_.CalledOnValidSequence());
+
+  AbandonScheduledTask();
   if (!retain_user_task_)
     user_task_.Reset();
 }
 
 void Timer::Reset() {
+  DCHECK(origin_sequence_checker_.CalledOnValidSequence());
   DCHECK(!user_task_.is_null());
 
-  // If there's no pending task, start one up and return.
-  if (!scheduled_task_) {
+  if (is_running_) {
+    if (destination_task_runner_) {
+      destination_task_runner_->PostTask(
+          FROM_HERE,
+          base::Bind(&BaseTimerTaskInternal::Reset, scheduled_task_weak_ref_));
+    } else {
+      // |is_running_| guarantees a valid pointer when not using an external
+      // |destination_task_runner_|.
+      DCHECK(scheduled_task_weak_ref_);
+      scheduled_task_weak_ref_->Reset();
+    }
+  } else {
     PostNewScheduledTask(delay_);

[vmpstr, 2016/12/03 01:14:04]

It's unclear to me if there other call sites other than Start that hit this, can
you comment when this is expected to hit?

[gab, 2016/12/23 20:22:11]

The API is a bit of a mess but yes this is required to support an existing use
case, added comment:

    // This is required to handle the following use case (when
    // |retain_user_task_ ==true|): Start() => Stop() => Reset().

-    return;
   }
-
-  // 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_.
-  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.
-  AbandonScheduledTask();
-  PostNewScheduledTask(delay_);
 }
 
 TimeTicks Timer::Now() const {
@@ -160,75 +311,65 @@ TimeTicks Timer::Now() const {
 void Timer::SetTaskInfo(const tracked_objects::Location& posted_from,
                         TimeDelta delay,
                         const base::Closure& user_task) {
+  DCHECK(origin_sequence_checker_.CalledOnValidSequence());
+  DCHECK(!is_running_);
+
   posted_from_ = posted_from;
   delay_ = delay;
   user_task_ = user_task;
 }
 
 void Timer::PostNewScheduledTask(TimeDelta delay) {
-  DCHECK(scheduled_task_ == NULL);
-  is_running_ = true;
-  scheduled_task_ = new BaseTimerTaskInternal(this);
-  if (delay > TimeDelta::FromMicroseconds(0)) {
-    GetTaskRunner()->PostDelayedTask(posted_from_,
-        base::Bind(&BaseTimerTaskInternal::Run, base::Owned(scheduled_task_)),
-        delay);
-    scheduled_run_time_ = desired_run_time_ = Now() + delay;
-  } else {
-    GetTaskRunner()->PostTask(posted_from_,
-        base::Bind(&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());
-}
+  DCHECK(origin_sequence_checker_.CalledOnValidSequence());
+  DCHECK(SequencedTaskRunnerHandle::IsSet());
 
-scoped_refptr<SingleThreadTaskRunner> Timer::GetTaskRunner() {
-  return task_runner_.get() ? task_runner_ : ThreadTaskRunnerHandle::Get();
-}
+  scoped_refptr<SequencedTaskRunner> destination_task_runner =
+      destination_task_runner_ ? destination_task_runner_
+                               : SequencedTaskRunnerHandle::Get();
 
-void Timer::AbandonScheduledTask() {
-  DCHECK(thread_id_ == 0 ||
-         thread_id_ == static_cast<int>(PlatformThread::CurrentId()));
-  if (scheduled_task_) {
-    scheduled_task_->Abandon();
-    scheduled_task_ = NULL;
-  }
+  is_running_ = true;
+  BaseTimerTaskInternal* scheduled_task = new BaseTimerTaskInternal(
+      posted_from_, user_task_, delay, is_repeating_, tick_clock_,
+      SequencedTaskRunnerHandle::Get(),
+      base::Bind(&Timer::Stop, weak_ptr_factory_.GetWeakPtr()));
+  scheduled_task_weak_ref_ = scheduled_task->AsWeakPtr();
+  destination_task_runner->PostDelayedTask(
+      posted_from_,
+      base::Bind(&BaseTimerTaskInternal::Run, scheduled_task_weak_ref_,
+                 Passed(scheduled_task->GetCleanupTrigger())),
+      delay);
 }
 
-void Timer::RunScheduledTask() {
-  // Task may have been disabled.
-  if (!is_running_)
-    return;
-
-  // 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_.
-    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.
-    if (desired_run_time_ > now) {
-      // Post a new task to span the remaining time.
-      PostNewScheduledTask(desired_run_time_ - now);
-      return;
+void Timer::AbandonScheduledTask() {
+  DCHECK(origin_sequence_checker_.CalledOnValidSequence());
+
+  if (is_running_) {
+    // Toggle |is_running| first to be reentrancy safe, just in case.
+    is_running_ = false;
+
+    const Closure abandon =
+        base::Bind(&BaseTimerTaskInternal::Abandon, scheduled_task_weak_ref_);
+    if (destination_task_runner_) {
+      destination_task_runner_->PostTask(FROM_HERE, abandon);
+    } else if (SequencedTaskRunnerHandle::IsSet()) {
+      // Post Abandon() as an asynchronous call to avoid deleting
+      // BaseTimerTaskInternal while it handles Run() (reentrancy is possible,
+      // e.g. when the task itself deletes the Timer or when a OneShotTimer's
+      // BaseTimerTaskInternal::Run() invoked its |stop_callback_|).
+      SequencedTaskRunnerHandle::Get()->PostTask(FROM_HERE, abandon);
+    } else {
+      // |is_running_| guarantees a valid pointer when not using an external
+      // |destination_task_runner_|.
+      DCHECK(scheduled_task_weak_ref_);
+      // The reentrancy situations described above happen while Run() is active
+      // which implies SequencedTaskRunnerHandle::IsSet().
+      // SequencedTaskRunnerHandle not being set typically means this
+      // AbandonScheduledTask() was triggered by ~Timer() on shutdown in which
+      // case it's safe to synchronously abandon on sequence.
+      scheduled_task_weak_ref_->Abandon();
     }
+    scheduled_task_weak_ref_ = nullptr;
   }
-
-  // Make a local copy of the task to run. The Stop method will reset the
-  // user_task_ member if retain_user_task_ is false.
-  base::Closure task = user_task_;
-
-  if (is_repeating_)
-    PostNewScheduledTask(delay_);
-  else
-    Stop();
-
-  task.Run();
-
-  // No more member accesses here: *this could be deleted at this point.
 }
 
 }  // namespace base