Revert "Refactor BaseTimer to avoid spamming the MessageLoop with orphaned tasks."

This reverts commit r128412, which is causing DCHECKs in thread
destruction in media unittests.

BUG=119714,119750
TBR=sky@chromium.org,petermayo@chromium.org

Review URL: https://chromiumcodereview.appspot.com/9839059

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@128506 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 185 insertions, 411 deletions

diff --git a/base/timer.cc b/base/timer.cc
index e51660c..5c2aa21 100644
--- a/base/timer.cc
+++ b/base/timer.cc
@@ -4,181 +4,28 @@
 
 #include "base/timer.h"
 
-#include "base/logging.h"
+#include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/message_loop.h"
 
 namespace base {
 
-// BaseTimerTaskInternal is a simple delegate for scheduling a callback to
-// Timer in the MessageLoop. It also handles the following edge
-// cases:
-// - deleted by MessageLoop.
-// - abandoned (orphaned) by Timer.
-class BaseTimerTaskInternal {
- public:
-  BaseTimerTaskInternal(Timer* timer)
-      : timer_(timer) {
+void BaseTimer_Helper::OrphanDelayedTask() {
+  if (delayed_task_) {
+    delayed_task_->timer_ = NULL;
+    delayed_task_ = NULL;
   }
-
-  ~BaseTimerTaskInternal() {
-    // This task may be getting cleared because the MessageLoop has been
-    // destructed.  If so, don't leave Timer with a dangling pointer
-    // to this.
-    if (timer_)
-      timer_->StopAndAbandon();
-  }
-
-  void Run() {
-    // timer_ is NULL if we were abandoned.
-    if (!timer_)
-      return;
-
-    // *this will be deleted by the MessageLoop, so Timer needs to
-    // forget us:
-    timer_->scheduled_task_ = NULL;
-
-    // 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();
-  }
-
-  // The task remains in the MessageLoop queue, but nothing will happen when it
-  // runs.
-  void Abandon() {
-    timer_ = NULL;
-  }
-
- private:
-  Timer* timer_;
-};
-
-Timer::Timer(bool retain_user_task, bool is_repeating)
-    : scheduled_task_(NULL),
-      thread_id_(0),
-      is_repeating_(is_repeating),
-      retain_user_task_(retain_user_task),
-      is_running_(false) {
-}
-
-Timer::Timer(const tracked_objects::Location& posted_from,
-             TimeDelta delay,
-             const base::Closure& user_task,
-             bool is_repeating)
-    : scheduled_task_(NULL),
-      posted_from_(posted_from),
-      delay_(delay),
-      user_task_(user_task),
-      thread_id_(0),
-      is_repeating_(is_repeating),
-      retain_user_task_(true),
-      is_running_(false) {
-}
-
-Timer::~Timer() {
-  StopAndAbandon();
-}
-
-void Timer::Start(const tracked_objects::Location& posted_from,
-                  TimeDelta delay,
-                  const base::Closure& user_task) {
-  SetTaskInfo(posted_from, delay, user_task);
-  Reset();
-}
-
-void Timer::Stop() {
-  is_running_ = false;
-  if (!retain_user_task_)
-    user_task_.Reset();
-}
-
-void Timer::Reset() {
-  DCHECK(!user_task_.is_null());
-
-  // If there's no pending task, start one up and return.
-  if (!scheduled_task_) {
-    PostNewScheduledTask(delay_);
-    return;
-  }
-
-  // Set the new desired_run_time_.
-  desired_run_time_ = TimeTicks::Now() + delay_;
-
-  // 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_);
 }
 
-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);
-  is_running_ = true;
-  scheduled_task_ = new BaseTimerTaskInternal(this);
-  MessageLoop::current()->PostDelayedTask(posted_from_,
-      base::Bind(&BaseTimerTaskInternal::Run, base::Owned(scheduled_task_)),
-      delay);
-  scheduled_run_time_ = desired_run_time_ = TimeTicks::Now() + delay;
-  // 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());
-}
-
-void Timer::AbandonScheduledTask() {
-  DCHECK(thread_id_ == 0 ||
-         thread_id_ == static_cast<int>(PlatformThread::CurrentId()));
-  if (scheduled_task_) {
-    scheduled_task_->Abandon();
-    scheduled_task_ = NULL;
-  }
-}
-
-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_) {
-    // TimeTicks::Now() can be expensive, so only call it if we know the user
-    // has changed the desired_run_time_.
-    TimeTicks now = TimeTicks::Now();
-    // MessageLoop 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;
-    }
-  }
-
-  // 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();
+void BaseTimer_Helper::InitiateDelayedTask(TimerTask* timer_task) {
+  OrphanDelayedTask();
 
-  // No more member accesses here: *this could be deleted at this point.
+  delayed_task_ = timer_task;
+  delayed_task_->timer_ = this;
+  MessageLoop::current()->PostDelayedTask(
+      timer_task->posted_from_,
+      base::Bind(&TimerTask::Run, base::Owned(timer_task)),
+      timer_task->delay_);
 }
 
 }  // namespace base
diff --git a/base/timer.h b/base/timer.h
index 24f2bb1..9c9bec7 100644
--- a/base/timer.h
+++ b/base/timer.h
@@ -38,8 +38,6 @@
 // calling Reset on timer_ would postpone DoStuff by another 1 second.  In
 // other words, Reset is shorthand for calling Stop and then Start again with
 // the same arguments.
-//
-// NOTE: These APIs are not thread safe. Always call from the same thread.
 
 #ifndef BASE_TIMER_H_
 #define BASE_TIMER_H_
@@ -51,162 +49,174 @@
 // should be able to tell the difference.
 
 #include "base/base_export.h"
-#include "base/bind.h"
-#include "base/bind_helpers.h"
-#include "base/callback.h"
 #include "base/location.h"
+#include "base/logging.h"
 #include "base/time.h"
 
 class MessageLoop;
 
 namespace base {
 
-class BaseTimerTaskInternal;
-
 //-----------------------------------------------------------------------------
-// This class wraps MessageLoop::PostDelayedTask to manage delayed and repeating
-// tasks. It must be destructed on the same thread that starts tasks. There are
-// DCHECKs in place to verify this.
+// This class is an implementation detail of OneShotTimer and RepeatingTimer.
+// Please do not use this class directly.
 //
-class BASE_EXPORT Timer {
+// This class exists to share code between BaseTimer<T> template instantiations.
+//
+class BASE_EXPORT BaseTimer_Helper {
  public:
-  // Construct a timer in repeating or one-shot mode. Start or SetTaskInfo must
-  // be called later to set task info. |retain_user_task| determines whether the
-  // user_task is retained or reset when it runs or stops.
-  Timer(bool retain_user_task, bool is_repeating);
-
-  // Construct a timer with retained task info.
-  Timer(const tracked_objects::Location& posted_from,
-        TimeDelta delay,
-        const base::Closure& user_task,
-        bool is_repeating);
-
-  virtual ~Timer();
+  // Stops the timer.
+  ~BaseTimer_Helper() {
+    OrphanDelayedTask();
+  }
 
   // Returns true if the timer is running (i.e., not stopped).
   bool IsRunning() const {
-    return is_running_;
+    return delayed_task_ != NULL;
   }
 
-  // Returns the current delay for this timer.
+  // Returns the current delay for this timer.  May only call this method when
+  // the timer is running!
   TimeDelta GetCurrentDelay() const {
-    return delay_;
+    DCHECK(IsRunning());
+    return delayed_task_->delay_;
   }
 
-  // Start the timer to run at the given |delay| from now. If the timer is
-  // already running, it will be replaced to call the given |user_task|.
-  void Start(const tracked_objects::Location& posted_from,
-             TimeDelta delay,
-             const base::Closure& user_task);
-
-  // Call this method to stop and cancel the timer.  It is a no-op if the timer
-  // is not running.
-  void Stop();
-
-  // Call this method to reset the timer delay. The user_task_ must be set. If
-  // the timer is not running, this will start it by posting a task.
-  void Reset();
-
-  const base::Closure& user_task() const { return user_task_; }
-
  protected:
-  // Used to initiate a new delayed task.  This has the side-effect of disabling
-  // scheduled_task_ if it is non-null.
-  void SetTaskInfo(const tracked_objects::Location& posted_from,
-                   TimeDelta delay,
-                   const base::Closure& user_task);
-
- private:
-  friend class BaseTimerTaskInternal;
-
-  // Allocates a new scheduled_task_ and posts it on the current MessageLoop
-  // with the given |delay|. scheduled_task_ must be NULL. scheduled_run_time_
-  // and desired_run_time_ are reset to Now() + delay.
-  void PostNewScheduledTask(TimeDelta delay);
-
-  // Disable scheduled_task_ and abandon it so that it no longer refers back to
-  // this object.
-  void AbandonScheduledTask();
-
-  // Called by BaseTimerTaskInternal when the MessageLoop runs it.
-  void RunScheduledTask();
-
-  // Stop running task (if any) and abandon scheduled task (if any).
-  void StopAndAbandon() {
-    Stop();
-    AbandonScheduledTask();
-  }
-
-  // When non-NULL, the scheduled_task_ is waiting in the MessageLoop to call
-  // RunScheduledTask() at scheduled_run_time_.
-  BaseTimerTaskInternal* scheduled_task_;
-
-  // Location in user code.
-  tracked_objects::Location posted_from_;
-  // Delay requested by user.
-  TimeDelta delay_;
-  // user_task_ is what the user wants to be run at desired_run_time_.
-  base::Closure user_task_;
-
-  // The estimated time that the MessageLoop will run the scheduled_task_ that
-  // will call RunScheduledTask().
-  TimeTicks scheduled_run_time_;
-
-  // The desired run time of user_task_. The user may update this at any time,
-  // even if their previous request has not run yet. If desired_run_time_ is
-  // greater than scheduled_run_time_, a continuation task will be posted to
-  // wait for the remaining time. This allows us to reuse the pending task so as
-  // not to flood the MessageLoop with orphaned tasks when the user code
-  // excessively Stops and Starts the timer.
-  TimeTicks desired_run_time_;
-
-  // Thread ID of current MessageLoop for verifying single-threaded usage.
-  int thread_id_;
-
-  // Repeating timers automatically post the task again before calling the task
-  // callback.
-  const bool is_repeating_;
-
-  // If true, hold on to the user_task_ closure object for reuse.
-  const bool retain_user_task_;
-
-  // If true, user_task_ is scheduled to run sometime in the future.
-  bool is_running_;
-
-  DISALLOW_COPY_AND_ASSIGN(Timer);
+  BaseTimer_Helper() : delayed_task_(NULL) {}
+
+  // We have access to the timer_ member so we can orphan this task.
+  class TimerTask {
+   public:
+    TimerTask(const tracked_objects::Location& posted_from,
+              TimeDelta delay)
+        : posted_from_(posted_from),
+          timer_(NULL),
+          delay_(delay) {
+    }
+    virtual ~TimerTask() {}
+    virtual void Run() = 0;
+    tracked_objects::Location posted_from_;
+    BaseTimer_Helper* timer_;
+    TimeDelta delay_;
+  };
+
+  // Used to orphan delayed_task_ so that when it runs it does nothing.
+  void OrphanDelayedTask();
+
+  // Used to initiated a new delayed task.  This has the side-effect of
+  // orphaning delayed_task_ if it is non-null.
+  void InitiateDelayedTask(TimerTask* timer_task);
+
+  TimerTask* delayed_task_;
+
+  DISALLOW_COPY_AND_ASSIGN(BaseTimer_Helper);
 };
 
 //-----------------------------------------------------------------------------
 // This class is an implementation detail of OneShotTimer and RepeatingTimer.
 // Please do not use this class directly.
 template <class Receiver, bool kIsRepeating>
-class BaseTimerMethodPointer : public Timer {
+class BaseTimer : public BaseTimer_Helper {
  public:
   typedef void (Receiver::*ReceiverMethod)();
 
-  BaseTimerMethodPointer() : Timer(kIsRepeating, kIsRepeating) {}
-
-  // Start the timer to run at the given |delay| from now. If the timer is
-  // already running, it will be replaced to call a task formed from
-  // |reviewer->*method|.
+  // Call this method to start the timer.  It is an error to call this method
+  // while the timer is already running.
   void Start(const tracked_objects::Location& posted_from,
              TimeDelta delay,
              Receiver* receiver,
              ReceiverMethod method) {
-    Timer::Start(posted_from, delay,
-                 base::Bind(method, base::Unretained(receiver)));
+    DCHECK(!IsRunning());
+    InitiateDelayedTask(new TimerTask(posted_from, delay, receiver, method));
   }
+
+  // Call this method to stop the timer.  It is a no-op if the timer is not
+  // running.
+  void Stop() {
+    OrphanDelayedTask();
+  }
+
+  // Call this method to reset the timer delay of an already running timer.
+  void Reset() {
+    DCHECK(IsRunning());
+    InitiateDelayedTask(static_cast<TimerTask*>(delayed_task_)->Clone());
+  }
+
+ private:
+  typedef BaseTimer<Receiver, kIsRepeating> SelfType;
+
+  class TimerTask : public BaseTimer_Helper::TimerTask {
+   public:
+    TimerTask(const tracked_objects::Location& posted_from,
+              TimeDelta delay,
+              Receiver* receiver,
+              ReceiverMethod method)
+        : BaseTimer_Helper::TimerTask(posted_from, delay),
+          receiver_(receiver),
+          method_(method) {
+    }
+
+    virtual ~TimerTask() {
+      // This task may be getting cleared because the MessageLoop has been
+      // destructed.  If so, don't leave the Timer with a dangling pointer
+      // to this now-defunct task.
+      ClearBaseTimer();
+    }
+
+    virtual void Run() {
+      if (!timer_)  // timer_ is null if we were orphaned.
+        return;
+      if (kIsRepeating)
+        ResetBaseTimer();
+      else
+        ClearBaseTimer();
+      (receiver_->*method_)();
+    }
+
+    TimerTask* Clone() const {
+      return new TimerTask(posted_from_, delay_, receiver_, method_);
+    }
+
+   private:
+    // Inform the Base that the timer is no longer active.
+    void ClearBaseTimer() {
+      if (timer_) {
+        SelfType* self = static_cast<SelfType*>(timer_);
+        // It is possible that the Timer has already been reset, and that this
+        // Task is old.  So, if the Timer points to a different task, assume
+        // that the Timer has already taken care of properly setting the task.
+        if (self->delayed_task_ == this)
+          self->delayed_task_ = NULL;
+        // By now the delayed_task_ in the Timer does not point to us anymore.
+        // We should reset our own timer_ because the Timer can not do this
+        // for us in its destructor.
+        timer_ = NULL;
+      }
+    }
+
+    // Inform the Base that we're resetting the timer.
+    void ResetBaseTimer() {
+      DCHECK(timer_);
+      DCHECK(kIsRepeating);
+      SelfType* self = static_cast<SelfType*>(timer_);
+      self->Reset();
+    }
+
+    Receiver* receiver_;
+    ReceiverMethod method_;
+  };
 };
 
 //-----------------------------------------------------------------------------
 // A simple, one-shot timer.  See usage notes at the top of the file.
 template <class Receiver>
-class OneShotTimer : public BaseTimerMethodPointer<Receiver, false> {};
+class OneShotTimer : public BaseTimer<Receiver, false> {};
 
 //-----------------------------------------------------------------------------
 // A simple, repeating timer.  See usage notes at the top of the file.
 template <class Receiver>
-class RepeatingTimer : public BaseTimerMethodPointer<Receiver, true> {};
+class RepeatingTimer : public BaseTimer<Receiver, true> {};
 
 //-----------------------------------------------------------------------------
 // A Delay timer is like The Button from Lost. Once started, you have to keep
@@ -220,7 +230,7 @@ class RepeatingTimer : public BaseTimerMethodPointer<Receiver, true> {};
 // If destroyed, the timeout is canceled and will not occur even if already
 // inflight.
 template <class Receiver>
-class DelayTimer : protected Timer {
+class DelayTimer {
  public:
   typedef void (Receiver::*ReceiverMethod)();
 
@@ -228,11 +238,51 @@ class DelayTimer : protected Timer {
              TimeDelta delay,
              Receiver* receiver,
              ReceiverMethod method)
-      : Timer(posted_from, delay,
-              base::Bind(method, base::Unretained(receiver)),
-              false) {}
+      : posted_from_(posted_from),
+        receiver_(receiver),
+        method_(method),
+        delay_(delay) {
+  }
+
+  void Reset() {
+    DelayFor(delay_);
+  }
+
+ private:
+  void DelayFor(TimeDelta delay) {
+    trigger_time_ = TimeTicks::Now() + delay;
+
+    // If we already have a timer that will expire at or before the given delay,
+    // then we have nothing more to do now.
+    if (timer_.IsRunning() && timer_.GetCurrentDelay() <= delay)
+      return;
+
+    // The timer isn't running, or will expire too late, so restart it.
+    timer_.Stop();
+    timer_.Start(posted_from_, delay, this, &DelayTimer<Receiver>::Check);
+  }
+
+  void Check() {
+    if (trigger_time_.is_null())
+      return;
+
+    // If we have not waited long enough, then wait some more.
+    const TimeTicks now = TimeTicks::Now();
+    if (now < trigger_time_) {
+      DelayFor(trigger_time_ - now);
+      return;
+    }
+
+    (receiver_->*method_)();
+  }
+
+  tracked_objects::Location posted_from_;
+  Receiver *const receiver_;
+  const ReceiverMethod method_;
+  const TimeDelta delay_;
 
-  void Reset() { Timer::Reset(); }
+  OneShotTimer<DelayTimer<Receiver> > timer_;
+  TimeTicks trigger_time_;
 };
 
 }  // namespace base
diff --git a/base/timer_unittest.cc b/base/timer_unittest.cc
index 71a752e..929bd38 100644
--- a/base/timer_unittest.cc
+++ b/base/timer_unittest.cc
@@ -352,126 +352,3 @@ TEST(TimerTest, MessageLoopShutdown) {
 
   EXPECT_FALSE(did_run);
 }
-
-void TimerTestCallback() {
-}
-
-TEST(TimerTest, NonRepeatIsRunning) {
-  {
-    MessageLoop loop(MessageLoop::TYPE_DEFAULT);
-    base::Timer timer(false, false);
-    EXPECT_FALSE(timer.IsRunning());
-    timer.Start(FROM_HERE, TimeDelta::FromDays(1),
-                base::Bind(&TimerTestCallback));
-    EXPECT_TRUE(timer.IsRunning());
-    timer.Stop();
-    EXPECT_FALSE(timer.IsRunning());
-    EXPECT_TRUE(timer.user_task().is_null());
-  }
-
-  {
-    base::Timer timer(true, false);
-    MessageLoop loop(MessageLoop::TYPE_DEFAULT);
-    EXPECT_FALSE(timer.IsRunning());
-    timer.Start(FROM_HERE, TimeDelta::FromDays(1),
-                base::Bind(&TimerTestCallback));
-    EXPECT_TRUE(timer.IsRunning());
-    timer.Stop();
-    EXPECT_FALSE(timer.IsRunning());
-    ASSERT_FALSE(timer.user_task().is_null());
-    timer.Reset();
-    EXPECT_TRUE(timer.IsRunning());
-  }
-}
-
-TEST(TimerTest, NonRepeatMessageLoopDeath) {
-  base::Timer timer(false, false);
-  {
-    MessageLoop loop(MessageLoop::TYPE_DEFAULT);
-    EXPECT_FALSE(timer.IsRunning());
-    timer.Start(FROM_HERE, TimeDelta::FromDays(1),
-                base::Bind(&TimerTestCallback));
-    EXPECT_TRUE(timer.IsRunning());
-  }
-  EXPECT_FALSE(timer.IsRunning());
-  EXPECT_TRUE(timer.user_task().is_null());
-}
-
-TEST(TimerTest, RetainRepeatIsRunning) {
-  MessageLoop loop(MessageLoop::TYPE_DEFAULT);
-  base::Timer timer(FROM_HERE, TimeDelta::FromDays(1),
-                    base::Bind(&TimerTestCallback), true);
-  EXPECT_FALSE(timer.IsRunning());
-  timer.Reset();
-  EXPECT_TRUE(timer.IsRunning());
-  timer.Stop();
-  EXPECT_FALSE(timer.IsRunning());
-  timer.Reset();
-  EXPECT_TRUE(timer.IsRunning());
-}
-
-TEST(TimerTest, RetainNonRepeatIsRunning) {
-  MessageLoop loop(MessageLoop::TYPE_DEFAULT);
-  base::Timer timer(FROM_HERE, TimeDelta::FromDays(1),
-                    base::Bind(&TimerTestCallback), false);
-  EXPECT_FALSE(timer.IsRunning());
-  timer.Reset();
-  EXPECT_TRUE(timer.IsRunning());
-  timer.Stop();
-  EXPECT_FALSE(timer.IsRunning());
-  timer.Reset();
-  EXPECT_TRUE(timer.IsRunning());
-}
-
-namespace {
-
-bool g_callback_happened1 = false;
-bool g_callback_happened2 = false;
-
-void ClearAllCallbackHappened() {
-  g_callback_happened1 = false;
-  g_callback_happened2 = false;
-}
-
-void SetCallbackHappened1() {
-  g_callback_happened1 = true;
-  MessageLoop::current()->Quit();
-}
-
-void SetCallbackHappened2() {
-  g_callback_happened2 = true;
-  MessageLoop::current()->Quit();
-}
-
-TEST(TimerTest, ContinuationStopStart) {
-  {
-    ClearAllCallbackHappened();
-    MessageLoop loop(MessageLoop::TYPE_DEFAULT);
-    base::Timer timer(false, false);
-    timer.Start(FROM_HERE, TimeDelta::FromMilliseconds(10),
-                base::Bind(&SetCallbackHappened1));
-    timer.Stop();
-    timer.Start(FROM_HERE, TimeDelta::FromMilliseconds(40),
-                base::Bind(&SetCallbackHappened2));
-    MessageLoop::current()->Run();
-    EXPECT_FALSE(g_callback_happened1);
-    EXPECT_TRUE(g_callback_happened2);
-  }
-}
-
-TEST(TimerTest, ContinuationReset) {
-  {
-    ClearAllCallbackHappened();
-    MessageLoop loop(MessageLoop::TYPE_DEFAULT);
-    base::Timer timer(false, false);
-    timer.Start(FROM_HERE, TimeDelta::FromMilliseconds(10),
-                base::Bind(&SetCallbackHappened1));
-    timer.Reset();
-    // Since Reset happened before task ran, the user_task must not be cleared:
-    ASSERT_FALSE(timer.user_task().is_null());
-    MessageLoop::current()->Run();
-    EXPECT_TRUE(g_callback_happened1);
-  }
-}
-
-}  // namespace