summaryrefslogtreecommitdiffstats
path: root/base/timer.h
diff options
context:
space:
mode:
authormbelshe@google.com <mbelshe@google.com@0039d316-1c4b-4281-b951-d872f2087c98>2008-12-02 23:16:55 +0000
committermbelshe@google.com <mbelshe@google.com@0039d316-1c4b-4281-b951-d872f2087c98>2008-12-02 23:16:55 +0000
commita0287cbdc1a56a27bf0eb29671942e44c8da599e (patch)
treea7b23f88c2c7245e98f1e668c6d1b3eb37aed0d7 /base/timer.h
parentc1f67c9851a0462012c0c9687fa0f151d4aed109 (diff)
downloadchromium_src-a0287cbdc1a56a27bf0eb29671942e44c8da599e.zip
chromium_src-a0287cbdc1a56a27bf0eb29671942e44c8da599e.tar.gz
chromium_src-a0287cbdc1a56a27bf0eb29671942e44c8da599e.tar.bz2
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
Diffstat (limited to 'base/timer.h')
-rw-r--r--base/timer.h41
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_;
};