Redo fix from yesterday.

Post Mortem on why yesterday's fix was bogus:

The motivation for this change was because a separate CL had a OneShotTimer<> as
part of a Singleton class.  The Singleton is cleaned up in the AtExit manager,
which currently runs after the MessageLoop is destroyed.  The following
sequence left a dangling pointer:

   1) Start a OneShotTimer
          - Creates a task and registers on the MessageLoop
   2) Destroy MessageLoop   (this deletes all Pending Tasks)
   3) Destroy your OneShotTimer
          - Tries to reference a dangling pointer for the already-deleted task.

The fix was to modify the Task such that at destruction it would zero-out the
wrapper's pointer to the Task.  Now step 3 would not reference a bogus pointer.

Unfortunately, the fix is broken.  The timers work by having a wrapper Timer
class (BaseTimer_Helper) and a Task.  The Task has a pointer back to its wrapper
and the wrapper has a pointer to the task.   For the case where a timer is
re-started from within its Run() loop, things fail:

    1) Start a RepeatingTimer
    2) Timer fires via the MessageLoop
            MessageLoop calls Run()
    3) Run() restarts the timer, which creates a new Task but reuses the
       same BaseTimer_Helper.
             BaseTimer_Helper's timer pointer now points to the NEW timer
    4) The MessageLoop destroys the Task
             The new code zeroes out the task pointer in the
             BaseTimer_Helper, breaking the newly restarted timer.



This fix is the same as yesterday's fix, except:
  1) More comments
  2) Added extra check at line 169.  Don't reset the Task pointer when 
     destroying the Task if the Timer does not point to the Task being
     destroyed.

I'm confident, but not 100% positive that this fixes the problem.  I did
reproduce *something* using PageHeap which now seems to be gone, but I
can't quite confirm.



Review URL: http://codereview.chromium.org/13067

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

1 files changed, 35 insertions, 6 deletions

diff --git a/base/timer.h b/base/timer.h
index 5361130..9aa084b 100644
--- a/base/timer.h
+++ b/base/timer.h
@@ -82,6 +82,7 @@ class BaseTimer_Helper {
     TimerTask(TimeDelta delay) : delay_(delay) {
       // timer_ is set in InitiateDelayedTask.
     }
+    virtual ~TimerTask() {}
     BaseTimer_Helper* timer_;
     TimeDelta delay_;
   };
@@ -135,21 +136,49 @@ class BaseTimer : public BaseTimer_Helper {
           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;
-      SelfType* self = static_cast<SelfType*>(timer_);
-      if (kIsRepeating) {
-        self->Reset();
-      } else {
-        self->delayed_task_ = NULL;
-      }
+      if (kIsRepeating)
+        ResetBaseTimer();
+      else
+        ClearBaseTimer();
       DispatchToMethod(receiver_, method_, Tuple0());
     }
+
     TimerTask* Clone() const {
       return new TimerTask(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;
+      }
+    }
+
+    // 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_;
   };