Found a bug in the BaseTimer (used by OneShotTimer).

If you have a OneShotTimer pending, and you destroy your
message loop, the cleanup of the timer will use memory
which was already freed by the MessageLoop.  When the
MessageLoop shuts down, it deletes pending tasks.
BaseTimer did not provide a virtual destructor to cleanup
its "base".  Thus, when the actual OneShotTimer cleaned
up, it would use deleted memory.

This manifested for me when I had accidentally had cleanup
of a oneshottimer occurring through the Singleton, which 
occurs AtExit, after the messageloop is already gone.

Created a unit test for this, which does trip the assert due
to heap corruption. 

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

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

2 files changed, 48 insertions, 6 deletions

diff --git a/base/timer.h b/base/timer.h
index 5361130..f449d2a 100644
--- a/base/timer.h
+++ b/base/timer.h
@@ -135,21 +135,42 @@ class BaseTimer : public BaseTimer_Helper {
           receiver_(receiver),
           method_(method) {
     }
+
+    virtual ~TimerTask() {
+      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_);
+        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_;
   };
diff --git a/base/timer_unittest.cc b/base/timer_unittest.cc
index face9c8..1fb44a3 100644
--- a/base/timer_unittest.cc
+++ b/base/timer_unittest.cc
@@ -145,3 +145,24 @@ TEST(TimerTest, RepeatingTimer_Cancel) {
   RunTest_RepeatingTimer_Cancel(MessageLoop::TYPE_UI);
   RunTest_RepeatingTimer_Cancel(MessageLoop::TYPE_IO);
 }
+
+TEST(TimerTest, MessageLoopShutdown) {
+  // This test is designed to verify that shutdown of the
+  // message loop does not cause crashes if there were pending
+  // timers not yet fired.  It may only trigger exceptions
+  // if debug heap checking (or purify) is enabled.
+  bool did_run = false;
+  {
+    OneShotTimerTester a(&did_run);
+    OneShotTimerTester b(&did_run);
+    OneShotTimerTester c(&did_run);
+    OneShotTimerTester d(&did_run);
+    {
+      MessageLoop loop(MessageLoop::TYPE_DEFAULT);
+      a.Start();
+      b.Start();
+    }  // MessageLoop destructs by falling out of scope.
+  }  // OneShotTimers destruct.  SHOULD NOT CRASH, of course.
+
+  EXPECT_EQ(false, did_run);
+}