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

This change maintains the same API promises*, but instead of orphaning tasks when they are stopped, the BaseTimer_Helper class holds on to the task until either (1) it expires or (2) the user requests a delay that would arrive earlier than the pending task. If the user requests a longer delay than the pending task, a followup task will be posted when the pending task fires to span the remaining time.

* The one change of usage is related to threading. The threading requirements are now more strict. It is not allowed to destruct a timer on a different thread than the one used to post tasks. A thread ID DCHECK is now in place that will help catch misuse. Some existing instances are changed as part of this CL.

A side effect of this change is that the BaseTimer and DelayTimer are simplified to use features of BaseTimer_Helper (which is now called Timer).

As suggested in timer.h, I ran the disabled TimerTest tests from linux, and they pass consistently. I also added some new tests to verify correct run states.

BUG=117451,103667,119714,119750

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=128412

Reverted: http://src.chromium.org/viewvc/chrome?view=rev&revision=128506

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

TBR=jbates@chromium.org
Review URL: https://chromiumcodereview.appspot.com/9791009

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

1 files changed, 171 insertions, 121 deletions

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