- Revision
- 153406
- Author
- [email protected]
- Date
- 2013-07-27 17:04:14 -0700 (Sat, 27 Jul 2013)
Log Message
Make SuspendableTimer safer
https://bugs.webkit.org/show_bug.cgi?id=119127
Reviewed by Sam Weinig.
SuspendableTimer now enforces that it stays suspended until resumed (or until stopped
and started again). To ensure this, TimerBase is now a private base class, and parts of
its interface that clients use are reimplemented with suspend/resume in mind.
Derived classes are still allowed to override TimerBase virtual functions (notably
fired() and alignedFireTime()).
* dom/DocumentEventQueue.cpp:
(WebCore::DocumentEventQueueTimer): Removed an extraneous WTF_MAKE_NONCOPYABLE,
TimerBase has it already.
(WebCore::DocumentEventQueueTimer::create): Use our normal create() pattern.
(WebCore::DocumentEventQueue::DocumentEventQueue): Made the constructor private, accordingly.
(WebCore::DocumentEventQueue::cancelEvent): Use SuspendableTimer::cancel(), which
is a new name to disambiguate TimerBase::stop() and ActiveDOMObject::stop().
(WebCore::DocumentEventQueue::close): Ditto.
* page/DOMTimer.cpp:
(WebCore::DOMTimer::fired): Now that SuspendableTimer knows whether it's currently
suspended, assert that it's not.
(WebCore::DOMTimer::didStop): Separated ActiveDOMObject::stop() implementation from
additional cleanup, allowing for better SuspendableTimer encapsulation.
* page/DOMTimer.h: Added FINAL and OVVERIDE specifiers as appropriate.
* page/SuspendableTimer.h: Added FINAL (and OVERRIDE) qualifiers to ActiveDOMObject
methods. A derived class that wants to override current behavior is most likely not
a timer, and thus shouldn't be a derived class.
(WebCore::SuspendableTimer::isActive): SuspendableTimer with a next fire time is
active even if suspended, we shouldn't overwrite its saved data thinking that it's
inactive.
(WebCore::SuspendableTimer::isSuspended): Exposed to clients (m_suspended is no
longer debug only).
* page/SuspendableTimer.cpp:
(WebCore::SuspendableTimer::SuspendableTimer): Updated for new variable names.
(WebCore::SuspendableTimer::stop): This is ActiveDOMObject::stop(), which is called
before final destruction. We don't track this state directly, but can approximate
with setting m_suspended, so even if someone tries to start the timer afterwards,
it won't fire.
(WebCore::SuspendableTimer::suspend): Updated for new names.
(WebCore::SuspendableTimer::resume): Ditto.
(WebCore::SuspendableTimer::didStop): No-op default implementation for client hook.
(WebCore::SuspendableTimer::cancel): Equivalent of TimerBase::stop(), which also
works when suspended.
(WebCore::SuspendableTimer::startRepeating): Replacement for TimerBase function with
the same name, which works correctly when suspended. We don't want to actually start
the timer in this case.
(WebCore::SuspendableTimer::startOneShot): Ditto.
(WebCore::SuspendableTimer::repeatInterval): Ditto.
(WebCore::SuspendableTimer::augmentFireInterval): Ditto.
(WebCore::SuspendableTimer::augmentRepeatInterval): Ditto.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (153405 => 153406)
--- trunk/Source/WebCore/ChangeLog 2013-07-27 23:53:36 UTC (rev 153405)
+++ trunk/Source/WebCore/ChangeLog 2013-07-28 00:04:14 UTC (rev 153406)
@@ -1,3 +1,62 @@
+2013-07-27 Alexey Proskuryakov <[email protected]>
+
+ Make SuspendableTimer safer
+ https://bugs.webkit.org/show_bug.cgi?id=119127
+
+ Reviewed by Sam Weinig.
+
+ SuspendableTimer now enforces that it stays suspended until resumed (or until stopped
+ and started again). To ensure this, TimerBase is now a private base class, and parts of
+ its interface that clients use are reimplemented with suspend/resume in mind.
+
+ Derived classes are still allowed to override TimerBase virtual functions (notably
+ fired() and alignedFireTime()).
+
+ * dom/DocumentEventQueue.cpp:
+ (WebCore::DocumentEventQueueTimer): Removed an extraneous WTF_MAKE_NONCOPYABLE,
+ TimerBase has it already.
+ (WebCore::DocumentEventQueueTimer::create): Use our normal create() pattern.
+ (WebCore::DocumentEventQueue::DocumentEventQueue): Made the constructor private, accordingly.
+ (WebCore::DocumentEventQueue::cancelEvent): Use SuspendableTimer::cancel(), which
+ is a new name to disambiguate TimerBase::stop() and ActiveDOMObject::stop().
+ (WebCore::DocumentEventQueue::close): Ditto.
+
+ * page/DOMTimer.cpp:
+ (WebCore::DOMTimer::fired): Now that SuspendableTimer knows whether it's currently
+ suspended, assert that it's not.
+ (WebCore::DOMTimer::didStop): Separated ActiveDOMObject::stop() implementation from
+ additional cleanup, allowing for better SuspendableTimer encapsulation.
+
+ * page/DOMTimer.h: Added FINAL and OVVERIDE specifiers as appropriate.
+
+ * page/SuspendableTimer.h: Added FINAL (and OVERRIDE) qualifiers to ActiveDOMObject
+ methods. A derived class that wants to override current behavior is most likely not
+ a timer, and thus shouldn't be a derived class.
+ (WebCore::SuspendableTimer::isActive): SuspendableTimer with a next fire time is
+ active even if suspended, we shouldn't overwrite its saved data thinking that it's
+ inactive.
+ (WebCore::SuspendableTimer::isSuspended): Exposed to clients (m_suspended is no
+ longer debug only).
+
+ * page/SuspendableTimer.cpp:
+ (WebCore::SuspendableTimer::SuspendableTimer): Updated for new variable names.
+ (WebCore::SuspendableTimer::stop): This is ActiveDOMObject::stop(), which is called
+ before final destruction. We don't track this state directly, but can approximate
+ with setting m_suspended, so even if someone tries to start the timer afterwards,
+ it won't fire.
+ (WebCore::SuspendableTimer::suspend): Updated for new names.
+ (WebCore::SuspendableTimer::resume): Ditto.
+ (WebCore::SuspendableTimer::didStop): No-op default implementation for client hook.
+ (WebCore::SuspendableTimer::cancel): Equivalent of TimerBase::stop(), which also
+ works when suspended.
+ (WebCore::SuspendableTimer::startRepeating): Replacement for TimerBase function with
+ the same name, which works correctly when suspended. We don't want to actually start
+ the timer in this case.
+ (WebCore::SuspendableTimer::startOneShot): Ditto.
+ (WebCore::SuspendableTimer::repeatInterval): Ditto.
+ (WebCore::SuspendableTimer::augmentFireInterval): Ditto.
+ (WebCore::SuspendableTimer::augmentRepeatInterval): Ditto.
+
2013-07-27 Sam Weinig <[email protected]>
Add assertions for CSSPrimitiveValue's m_value.valueID accessor
Modified: trunk/Source/WebCore/dom/DocumentEventQueue.cpp (153405 => 153406)
--- trunk/Source/WebCore/dom/DocumentEventQueue.cpp 2013-07-27 23:53:36 UTC (rev 153405)
+++ trunk/Source/WebCore/dom/DocumentEventQueue.cpp 2013-07-28 00:04:14 UTC (rev 153406)
@@ -37,15 +37,24 @@
namespace WebCore {
-class DocumentEventQueueTimer : public SuspendableTimer {
- WTF_MAKE_NONCOPYABLE(DocumentEventQueueTimer);
+class DocumentEventQueueTimer FINAL : public SuspendableTimer {
public:
+ static PassOwnPtr<DocumentEventQueueTimer> create(DocumentEventQueue* eventQueue, ScriptExecutionContext* context)
+ {
+ return adoptPtr(new DocumentEventQueueTimer(eventQueue, context));
+ }
+
+private:
DocumentEventQueueTimer(DocumentEventQueue* eventQueue, ScriptExecutionContext* context)
: SuspendableTimer(context)
, m_eventQueue(eventQueue) { }
-private:
- virtual void fired() { m_eventQueue->pendingEventTimerFired(); }
+ virtual void fired() OVERRIDE
+ {
+ ASSERT(!isSuspended());
+ m_eventQueue->pendingEventTimerFired();
+ }
+
DocumentEventQueue* m_eventQueue;
};
@@ -55,7 +64,7 @@
}
DocumentEventQueue::DocumentEventQueue(ScriptExecutionContext* context)
- : m_pendingEventTimer(adoptPtr(new DocumentEventQueueTimer(this, context)))
+ : m_pendingEventTimer(DocumentEventQueueTimer::create(this, context))
, m_isClosed(false)
{
m_pendingEventTimer->suspendIfNeeded();
@@ -103,14 +112,14 @@
if (found)
m_queuedEvents.remove(it);
if (m_queuedEvents.isEmpty())
- m_pendingEventTimer->stop();
+ m_pendingEventTimer->cancel();
return found;
}
void DocumentEventQueue::close()
{
m_isClosed = true;
- m_pendingEventTimer->stop();
+ m_pendingEventTimer->cancel();
m_queuedEvents.clear();
}
Modified: trunk/Source/WebCore/page/DOMTimer.cpp (153405 => 153406)
--- trunk/Source/WebCore/page/DOMTimer.cpp 2013-07-27 23:53:36 UTC (rev 153405)
+++ trunk/Source/WebCore/page/DOMTimer.cpp 2013-07-28 00:04:14 UTC (rev 153406)
@@ -107,6 +107,7 @@
{
ScriptExecutionContext* context = scriptExecutionContext();
timerNestingLevel = m_nestingLevel;
+ ASSERT(!isSuspended());
ASSERT(!context->activeDOMObjectsAreSuspended());
UserGestureIndicator gestureIndicator(m_shouldForwardUserGesture ? DefinitelyProcessingUserGesture : PossiblyProcessingUserGesture);
// Only the first execution of a multi-shot timer should get an affirmative user gesture indicator.
@@ -150,9 +151,8 @@
delete this;
}
-void DOMTimer::stop()
+void DOMTimer::didStop()
{
- SuspendableTimer::stop();
// Need to release JS objects potentially protected by ScheduledAction
// because they can form circular references back to the ScriptExecutionContext
// which will cause a memory leak.
Modified: trunk/Source/WebCore/page/DOMTimer.h (153405 => 153406)
--- trunk/Source/WebCore/page/DOMTimer.h 2013-07-27 23:53:36 UTC (rev 153405)
+++ trunk/Source/WebCore/page/DOMTimer.h 2013-07-28 00:04:14 UTC (rev 153406)
@@ -35,7 +35,7 @@
class ScheduledAction;
- class DOMTimer : public SuspendableTimer {
+ class DOMTimer FINAL : public SuspendableTimer {
public:
virtual ~DOMTimer();
// Creates a new timer owned by specified ScriptExecutionContext, starts it
@@ -44,8 +44,7 @@
static void removeById(ScriptExecutionContext*, int timeoutId);
// ActiveDOMObject
- virtual void contextDestroyed();
- virtual void stop();
+ virtual void contextDestroyed() OVERRIDE;
// Adjust to a change in the ScriptExecutionContext's minimum timer interval.
// This allows the minimum allowable interval time to be changed in response
@@ -54,12 +53,15 @@
private:
DOMTimer(ScriptExecutionContext*, PassOwnPtr<ScheduledAction>, int interval, bool singleShot);
- virtual void fired();
+ virtual void fired() OVERRIDE;
+ // SuspendableTimer
+ virtual void didStop() OVERRIDE;
+
double intervalClampedToMinimum(int timeout, double minimumTimerInterval) const;
// Retuns timer fire time rounded to the next multiple of timer alignment interval.
- virtual double alignedFireTime(double) const;
+ virtual double alignedFireTime(double) const OVERRIDE;
int m_timeoutId;
int m_nestingLevel;
Modified: trunk/Source/WebCore/page/SuspendableTimer.cpp (153405 => 153406)
--- trunk/Source/WebCore/page/SuspendableTimer.cpp 2013-07-27 23:53:36 UTC (rev 153405)
+++ trunk/Source/WebCore/page/SuspendableTimer.cpp 2013-07-28 00:04:14 UTC (rev 153406)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2008 Apple Inc. All Rights Reserved.
+ * Copyright (C) 2008, 2013 Apple Inc. All Rights Reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -33,12 +33,10 @@
SuspendableTimer::SuspendableTimer(ScriptExecutionContext* context)
: ActiveDOMObject(context)
- , m_nextFireInterval(0)
- , m_repeatInterval(0)
- , m_active(false)
-#if !ASSERT_DISABLED
, m_suspended(false)
-#endif
+ , m_savedNextFireInterval(0)
+ , m_savedRepeatInterval(0)
+ , m_savedIsActive(false)
{
}
@@ -53,31 +51,37 @@
void SuspendableTimer::stop()
{
- TimerBase::stop();
+ if (!m_suspended)
+ TimerBase::stop();
+
+ // Make it less likely that SuspendableTimer is accidentally revived and fires after being stopped.
+ // It is not allowed for an ActiveDOMObject to become active again after stop().
+ m_suspended = true;
+ m_savedIsActive = false;
+
+ didStop();
}
void SuspendableTimer::suspend(ReasonForSuspension)
{
-#if !ASSERT_DISABLED
ASSERT(!m_suspended);
m_suspended = true;
-#endif
- m_active = isActive();
- if (m_active) {
- m_nextFireInterval = nextUnalignedFireInterval();
- m_repeatInterval = repeatInterval();
+
+ m_savedIsActive = TimerBase::isActive();
+ if (m_savedIsActive) {
+ m_savedNextFireInterval = nextUnalignedFireInterval();
+ m_savedRepeatInterval = repeatInterval();
TimerBase::stop();
}
}
void SuspendableTimer::resume()
{
-#if !ASSERT_DISABLED
ASSERT(m_suspended);
m_suspended = false;
-#endif
- if (m_active)
- start(m_nextFireInterval, m_repeatInterval);
+
+ if (m_savedIsActive)
+ start(m_savedNextFireInterval, m_savedRepeatInterval);
}
bool SuspendableTimer::canSuspend() const
@@ -85,4 +89,74 @@
return true;
}
+void SuspendableTimer::didStop()
+{
+}
+
+void SuspendableTimer::cancel()
+{
+ if (!m_suspended)
+ TimerBase::stop();
+ else
+ m_suspended = false;
+}
+
+void SuspendableTimer::startRepeating(double repeatInterval)
+{
+ if (!m_suspended)
+ TimerBase::startRepeating(repeatInterval);
+ else {
+ m_savedIsActive = true;
+ m_savedNextFireInterval = repeatInterval;
+ m_savedRepeatInterval = repeatInterval;
+ }
+}
+
+void SuspendableTimer::startOneShot(double interval)
+{
+ if (!m_suspended)
+ TimerBase::startOneShot(interval);
+ else {
+ m_savedIsActive = true;
+ m_savedNextFireInterval = interval;
+ m_savedRepeatInterval = 0;
+ }
+}
+
+double SuspendableTimer::repeatInterval() const
+{
+ if (!m_suspended)
+ return TimerBase::repeatInterval();
+ if (m_savedIsActive)
+ return m_savedRepeatInterval;
+ return 0;
+}
+
+void SuspendableTimer::augmentFireInterval(double delta)
+{
+ if (!m_suspended)
+ TimerBase::augmentFireInterval(delta);
+ else if (m_savedIsActive) {
+ m_savedNextFireInterval += delta;
+ } else {
+ m_savedIsActive = true;
+ m_savedNextFireInterval = delta;
+ m_savedRepeatInterval = 0;
+ }
+}
+
+void SuspendableTimer::augmentRepeatInterval(double delta)
+{
+ if (!m_suspended)
+ TimerBase::augmentRepeatInterval(delta);
+ else if (m_savedIsActive) {
+ m_savedNextFireInterval += delta;
+ m_savedRepeatInterval += delta;
+ } else {
+ m_savedIsActive = true;
+ m_savedNextFireInterval = delta;
+ m_savedRepeatInterval = delta;
+ }
+}
+
} // namespace WebCore
Modified: trunk/Source/WebCore/page/SuspendableTimer.h (153405 => 153406)
--- trunk/Source/WebCore/page/SuspendableTimer.h 2013-07-27 23:53:36 UTC (rev 153405)
+++ trunk/Source/WebCore/page/SuspendableTimer.h 2013-07-28 00:04:14 UTC (rev 153406)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2008 Apple Inc. All Rights Reserved.
+ * Copyright (C) 2008, 2013 Apple Inc. All Rights Reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -32,27 +32,43 @@
namespace WebCore {
-class SuspendableTimer : public TimerBase, public ActiveDOMObject {
+class SuspendableTimer : private TimerBase, public ActiveDOMObject {
public:
explicit SuspendableTimer(ScriptExecutionContext*);
virtual ~SuspendableTimer();
// ActiveDOMObject
- virtual bool hasPendingActivity() const;
- virtual void stop();
- virtual bool canSuspend() const;
- virtual void suspend(ReasonForSuspension);
- virtual void resume();
+ virtual bool hasPendingActivity() const FINAL OVERRIDE;
+ virtual void stop() FINAL OVERRIDE;
+ virtual bool canSuspend() const FINAL OVERRIDE;
+ virtual void suspend(ReasonForSuspension) FINAL OVERRIDE;
+ virtual void resume() FINAL OVERRIDE;
+ // A hook for derived classes to perform cleanup.
+ virtual void didStop();
+
+ // Part of TimerBase interface used by SuspendableTimer clients, modified to work when suspended.
+ bool isActive() const { return TimerBase::isActive() || (m_suspended && m_savedIsActive); }
+ bool isSuspended() const { return m_suspended; }
+ void startRepeating(double repeatInterval);
+ void startOneShot(double interval);
+ double repeatInterval() const;
+ void augmentFireInterval(double delta);
+ void augmentRepeatInterval(double delta);
+ using TimerBase::didChangeAlignmentInterval;
+ using TimerBase::operator new;
+ using TimerBase::operator delete;
+
+ void cancel(); // Equivalent to TimerBase::stop(), whose name conflicts with ActiveDOMObject::stop().
+
private:
virtual void fired() = 0;
- double m_nextFireInterval;
- double m_repeatInterval;
- bool m_active;
-#if !ASSERT_DISABLED
bool m_suspended;
-#endif
+
+ double m_savedNextFireInterval;
+ double m_savedRepeatInterval;
+ bool m_savedIsActive;
};
} // namespace WebCore
