Commit e625a40e authored by cmarrin@apple.com's avatar cmarrin@apple.com

2008-12-05 Chris Marrin <cmarrin@apple.com>

        Reviewed by Dave Hyatt.

        Fix for https://bugs.webkit.org/show_bug.cgi?id=22635
        For iteration and end events, previous fixes to prevent the deletion of
        Animation objects (ref counting and hanging onto a ref during event callbacks)
        was sufficient to prevent dangling pointers. But start events are sent in
        the styleAvailable() call, which iterates over CompositeAnimation objects,
        which are not ref counted. So that object can get destroyed in the event
        handler while still active. So I added refcounting for CompositeAnimations.

        Additionally, when am iterating over the CompositingAnimation list, it can
        be deleted, which mutates the list. So I now make one pass over the list
        building a vector of CompositeAnimation objects that need to be called and
        then iterate over that vector to make the actual calls.

        Finally, to make sure the lifetime of the CompositeAnimation exceeds that of
        the Animation objects it owns, I now keep a ref to the CompositeAnimation
        in the timer callback for the iteration and end events. That means I no
        longer need to keep a ref to the Animation objects themselves in that timer
        callback, since the CompositeAnimation already has one.

        Tests: animations/animation-iteration-event-destroy-renderer.html
               animations/animation-start-event-destroy-renderer.html



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@39059 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent 6955d3c7
2008-12-05 Chris Marrin <cmarrin@apple.com>
Reviewed by Dave Hyatt.
Testcases for https://bugs.webkit.org/show_bug.cgi?id=22635
* animations/animation-iteration-event-destroy-renderer-expected.txt: Added.
* animations/animation-iteration-event-destroy-renderer.html: Added.
* animations/animation-start-event-destroy-renderer-expected.txt: Added.
* animations/animation-start-event-destroy-renderer.html: Added.
2008-12-05 Pamela Greene <pam@chromium.org>
Reviewed by Darin Adler.
......
Tests element removal and hiding within the webkitAnimationIteration event handler. Should not crash.
Did not crash, so PASSED
<html>
<head>
<title>Destroy and Hide Element in Animation End Event</title>
<style type="text/css" media="screen">
.box {
height: 100px;
width: 100px;
margin: 10px;
background-color: blue;
-webkit-animation-duration: 0.2s;
-webkit-animation-iteration-count: 2;
}
@-webkit-keyframes move {
from { -webkit-transform: translate(0px, 0px); }
to { -webkit-transform: translate(100px, 0px); }
}
</style>
<script type="text/javascript" charset="utf-8">
if (window.layoutTestController) {
layoutTestController.dumpAsText();
layoutTestController.waitUntilDone();
}
var numDone = 0;
function animationIterated()
{
++numDone;
if (numDone == 2) {
if (window.GCController)
GCController.collect();
document.getElementById('results').innerHTML = 'Did not crash, so PASSED';
if (window.layoutTestController)
layoutTestController.notifyDone();
}
}
function startTest()
{
var box1 = document.getElementById('box1');
box1.addEventListener('webkitAnimationIteration', function() {
box1.parentNode.removeChild(box1);
animationIterated();
}, false);
box1.style.webkitAnimationName = 'move';
var box2 = document.getElementById('box2');
box2.addEventListener('webkitAnimationIteration', function() {
box2.style.display = 'none';
animationIterated();
}, false);
box2.style.webkitAnimationName = 'move';
}
window.addEventListener('load', startTest, false);
</script>
</head>
<body>
<p>Tests element removal and hiding within the webkitAnimationIteration event handler. Should not crash.</p>
<div id="container">
<div id="box1" class="box"></div>
<div id="box2" class="box"></div>
</div>
<div id="results"></div>
</body>
</html>
Tests element removal and hiding within the webkitAnimationStart event handler. Should not crash.
Did not crash, so PASSED
<html>
<head>
<title>Destroy and Hide Element in Animation End Event</title>
<style type="text/css" media="screen">
.box {
height: 100px;
width: 100px;
margin: 10px;
background-color: blue;
-webkit-animation-duration: 0.2s;
}
@-webkit-keyframes move {
from { -webkit-transform: translate(0px, 0px); }
to { -webkit-transform: translate(100px, 0px); }
}
</style>
<script type="text/javascript" charset="utf-8">
if (window.layoutTestController) {
layoutTestController.dumpAsText();
layoutTestController.waitUntilDone();
}
var numDone = 0;
function animationStarted()
{
++numDone;
if (numDone == 2) {
if (window.GCController)
GCController.collect();
document.getElementById('results').innerHTML = 'Did not crash, so PASSED';
if (window.layoutTestController)
layoutTestController.notifyDone();
}
}
function startTest()
{
var box1 = document.getElementById('box1');
box1.addEventListener('webkitAnimationStart', function() {
box1.parentNode.removeChild(box1);
animationStarted();
}, false);
box1.style.webkitAnimationName = 'move';
var box2 = document.getElementById('box2');
box2.addEventListener('webkitAnimationStart', function() {
box2.style.display = 'none';
animationStarted();
}, false);
box2.style.webkitAnimationName = 'move';
}
window.addEventListener('load', startTest, false);
</script>
</head>
<body>
<p>Tests element removal and hiding within the webkitAnimationStart event handler. Should not crash.</p>
<div id="container">
<div id="box1" class="box"></div>
<div id="box2" class="box"></div>
</div>
<div id="results"></div>
</body>
</html>
2008-12-05 Chris Marrin <cmarrin@apple.com>
Reviewed by Dave Hyatt.
Fix for https://bugs.webkit.org/show_bug.cgi?id=22635
For iteration and end events, previous fixes to prevent the deletion of
Animation objects (ref counting and hanging onto a ref during event callbacks)
was sufficient to prevent dangling pointers. But start events are sent in
the styleAvailable() call, which iterates over CompositeAnimation objects,
which are not ref counted. So that object can get destroyed in the event
handler while still active. So I added refcounting for CompositeAnimations.
Additionally, when am iterating over the CompositingAnimation list, it can
be deleted, which mutates the list. So I now make one pass over the list
building a vector of CompositeAnimation objects that need to be called and
then iterate over that vector to make the actual calls.
Finally, to make sure the lifetime of the CompositeAnimation exceeds that of
the Animation objects it owns, I now keep a ref to the CompositeAnimation
in the timer callback for the iteration and end events. That means I no
longer need to keep a ref to the Animation objects themselves in that timer
callback, since the CompositeAnimation already has one.
Tests: animations/animation-iteration-event-destroy-renderer.html
animations/animation-start-event-destroy-renderer.html
* page/animation/AnimationBase.cpp:
(WebCore::AnimationBase::updateStateMachine):
(WebCore::AnimationBase::animationTimerCallbackFired):
* page/animation/AnimationController.cpp:
(WebCore::AnimationControllerPrivate::~AnimationControllerPrivate):
(WebCore::AnimationControllerPrivate::accessCompositeAnimation):
(WebCore::AnimationControllerPrivate::clear):
(WebCore::AnimationControllerPrivate::styleAvailable):
(WebCore::AnimationControllerPrivate::updateAnimationTimer):
(WebCore::AnimationControllerPrivate::animationTimerFired):
(WebCore::AnimationControllerPrivate::isAnimatingPropertyOnRenderer):
(WebCore::AnimationControllerPrivate::suspendAnimations):
(WebCore::AnimationControllerPrivate::resumeAnimations):
(WebCore::AnimationControllerPrivate::pauseAnimationAtTime):
(WebCore::AnimationControllerPrivate::pauseTransitionAtTime):
(WebCore::AnimationController::updateAnimations):
(WebCore::AnimationController::setAnimationStartTime):
(WebCore::AnimationController::setTransitionStartTime):
* page/animation/CompositeAnimation.cpp:
(WebCore::CompositeAnimationPrivate::animationController):
(WebCore::CompositeAnimationPrivate::isWaitingForStyleAvailable):
(WebCore::CompositeAnimationPrivate::~CompositeAnimationPrivate):
(WebCore::CompositeAnimationPrivate::clearRenderer):
(WebCore::CompositeAnimation::clearRenderer):
(WebCore::CompositeAnimation::animationController):
(WebCore::CompositeAnimation::isWaitingForStyleAvailable):
* page/animation/CompositeAnimation.h:
(WebCore::CompositeAnimation::create):
2008-12-05 David Kilzer <ddkilzer@apple.com>
Bug 22609: Provide a build-time choice when generating hash tables for properties of built-in DOM objects
......@@ -609,8 +609,10 @@ void AnimationBase::updateStateMachine(AnimStateInput input, double param)
primeEventTimers();
// Trigger a render so we can start the animation
setChanged(m_object->element());
m_object->animation()->startUpdateRenderingDispatcher();
if (m_object) {
setChanged(m_object->element());
m_compAnim->animationController()->startUpdateRenderingDispatcher();
}
} else {
// We are pausing while waiting for a start response. Cancel the animation and wait. When
// we unpause, we will act as though the start timer just fired
......@@ -706,8 +708,10 @@ void AnimationBase::updateStateMachine(AnimStateInput input, double param)
void AnimationBase::animationTimerCallbackFired(const AtomicString& eventType, double elapsedTime)
{
// We have to make sure to keep a ref to the this pointer, because it could get destroyed
// during an animation callback that might get called.
RefPtr<AnimationBase> protector(this);
// during an animation callback that might get called. Since the owner is a CompositeAnimation
// and it ref counts this object, we will keep a ref to that instead. That way the AnimationBase
// can still access the resources of its CompositeAnimation as needed.
RefPtr<CompositeAnimation> protector(m_compAnim);
ASSERT(m_object->document() && !m_object->document()->inPageCache());
......
......@@ -42,7 +42,7 @@ public:
AnimationControllerPrivate(Frame*);
~AnimationControllerPrivate();
CompositeAnimation* accessCompositeAnimation(RenderObject*);
PassRefPtr<CompositeAnimation> accessCompositeAnimation(RenderObject*);
bool clear(RenderObject*);
void animationTimerFired(Timer<AnimationControllerPrivate>*);
......@@ -64,7 +64,7 @@ public:
bool pauseTransitionAtTime(RenderObject*, const String& property, double t);
private:
typedef HashMap<RenderObject*, CompositeAnimation*> RenderObjectAnimationMap;
typedef HashMap<RenderObject*, RefPtr<CompositeAnimation> > RenderObjectAnimationMap;
RenderObjectAnimationMap m_compositeAnimations;
Timer<AnimationControllerPrivate> m_animationTimer;
......@@ -81,14 +81,13 @@ AnimationControllerPrivate::AnimationControllerPrivate(Frame* frame)
AnimationControllerPrivate::~AnimationControllerPrivate()
{
deleteAllValues(m_compositeAnimations);
}
CompositeAnimation* AnimationControllerPrivate::accessCompositeAnimation(RenderObject* renderer)
PassRefPtr<CompositeAnimation> AnimationControllerPrivate::accessCompositeAnimation(RenderObject* renderer)
{
CompositeAnimation* animation = m_compositeAnimations.get(renderer);
RefPtr<CompositeAnimation> animation = m_compositeAnimations.get(renderer);
if (!animation) {
animation = new CompositeAnimation(m_frame->animation());
animation = CompositeAnimation::create(m_frame->animation());
m_compositeAnimations.set(renderer, animation);
}
return animation;
......@@ -98,19 +97,28 @@ bool AnimationControllerPrivate::clear(RenderObject* renderer)
{
// Return false if we didn't do anything OR we are suspended (so we don't try to
// do a setChanged() when suspended).
CompositeAnimation* animation = m_compositeAnimations.take(renderer);
PassRefPtr<CompositeAnimation> animation = m_compositeAnimations.take(renderer);
if (!animation)
return false;
bool wasSuspended = animation->isSuspended();
delete animation;
return !wasSuspended;
animation->clearRenderer();
return animation->isSuspended();
}
void AnimationControllerPrivate::styleAvailable()
{
// styleAvailable() can call event handlers which would ultimately delete a CompositeAnimation
// from the m_compositeAnimations table. So we can't iterate it directly. We will instead build
// a list of CompositeAnimations which need the styleAvailable() call iterate over that.
Vector<RefPtr<CompositeAnimation> > list;
RenderObjectAnimationMap::const_iterator animationsEnd = m_compositeAnimations.end();
for (RenderObjectAnimationMap::const_iterator it = m_compositeAnimations.begin(); it != animationsEnd; ++it)
it->second->styleAvailable();
if (it->second->isWaitingForStyleAvailable())
list.append(it->second);
Vector<RefPtr<CompositeAnimation> >::const_iterator listEnd = list.end();
for (Vector<RefPtr<CompositeAnimation> >::const_iterator it = list.begin(); it != listEnd; ++it)
(*it)->styleAvailable();
}
void AnimationControllerPrivate::updateAnimationTimer()
......@@ -119,7 +127,7 @@ void AnimationControllerPrivate::updateAnimationTimer()
RenderObjectAnimationMap::const_iterator animationsEnd = m_compositeAnimations.end();
for (RenderObjectAnimationMap::const_iterator it = m_compositeAnimations.begin(); it != animationsEnd; ++it) {
CompositeAnimation* compAnim = it->second;
RefPtr<CompositeAnimation> compAnim = it->second;
if (!compAnim->isSuspended() && compAnim->isAnimating()) {
isAnimating = true;
break;
......@@ -152,7 +160,7 @@ void AnimationControllerPrivate::animationTimerFired(Timer<AnimationControllerPr
bool isAnimating = false;
RenderObjectAnimationMap::const_iterator animationsEnd = m_compositeAnimations.end();
for (RenderObjectAnimationMap::const_iterator it = m_compositeAnimations.begin(); it != animationsEnd; ++it) {
CompositeAnimation* compAnim = it->second;
RefPtr<CompositeAnimation> compAnim = it->second;
if (!compAnim->isSuspended() && compAnim->isAnimating()) {
isAnimating = true;
compAnim->setAnimating(false);
......@@ -170,7 +178,7 @@ void AnimationControllerPrivate::animationTimerFired(Timer<AnimationControllerPr
bool AnimationControllerPrivate::isAnimatingPropertyOnRenderer(RenderObject* renderer, int property, bool isRunningNow) const
{
CompositeAnimation* animation = m_compositeAnimations.get(renderer);
RefPtr<CompositeAnimation> animation = m_compositeAnimations.get(renderer);
if (!animation)
return false;
......@@ -182,7 +190,7 @@ void AnimationControllerPrivate::suspendAnimations(Document* document)
RenderObjectAnimationMap::const_iterator animationsEnd = m_compositeAnimations.end();
for (RenderObjectAnimationMap::const_iterator it = m_compositeAnimations.begin(); it != animationsEnd; ++it) {
RenderObject* renderer = it->first;
CompositeAnimation* compAnim = it->second;
RefPtr<CompositeAnimation> compAnim = it->second;
if (renderer->document() == document)
compAnim->suspendAnimations();
}
......@@ -195,7 +203,7 @@ void AnimationControllerPrivate::resumeAnimations(Document* document)
RenderObjectAnimationMap::const_iterator animationsEnd = m_compositeAnimations.end();
for (RenderObjectAnimationMap::const_iterator it = m_compositeAnimations.begin(); it != animationsEnd; ++it) {
RenderObject* renderer = it->first;
CompositeAnimation* compAnim = it->second;
RefPtr<CompositeAnimation> compAnim = it->second;
if (renderer->document() == document)
compAnim->resumeAnimations();
}
......@@ -208,7 +216,7 @@ bool AnimationControllerPrivate::pauseAnimationAtTime(RenderObject* renderer, co
if (!renderer)
return false;
CompositeAnimation* compAnim = accessCompositeAnimation(renderer);
RefPtr<CompositeAnimation> compAnim = accessCompositeAnimation(renderer);
if (!compAnim)
return false;
......@@ -225,7 +233,7 @@ bool AnimationControllerPrivate::pauseTransitionAtTime(RenderObject* renderer, c
if (!renderer)
return false;
CompositeAnimation* compAnim = accessCompositeAnimation(renderer);
RefPtr<CompositeAnimation> compAnim = accessCompositeAnimation(renderer);
if (!compAnim)
return false;
......@@ -277,7 +285,7 @@ PassRefPtr<RenderStyle> AnimationController::updateAnimations(RenderObject* rend
// a new style.
ASSERT(renderer->element()); // FIXME: We do not animate generated content yet.
CompositeAnimation* rendererAnimations = m_data->accessCompositeAnimation(renderer);
RefPtr<CompositeAnimation> rendererAnimations = m_data->accessCompositeAnimation(renderer);
RefPtr<RenderStyle> blendedStyle = rendererAnimations->animate(renderer, oldStyle, newStyle);
m_data->updateAnimationTimer();
......@@ -294,13 +302,13 @@ PassRefPtr<RenderStyle> AnimationController::updateAnimations(RenderObject* rend
void AnimationController::setAnimationStartTime(RenderObject* renderer, double t)
{
CompositeAnimation* rendererAnimations = m_data->accessCompositeAnimation(renderer);
RefPtr<CompositeAnimation> rendererAnimations = m_data->accessCompositeAnimation(renderer);
rendererAnimations->setAnimationStartTime(t);
}
void AnimationController::setTransitionStartTime(RenderObject* renderer, int property, double t)
{
CompositeAnimation* rendererAnimations = m_data->accessCompositeAnimation(renderer);
RefPtr<CompositeAnimation> rendererAnimations = m_data->accessCompositeAnimation(renderer);
rendererAnimations->setTransitionStartTime(property, t);
}
......
......@@ -50,8 +50,12 @@ public:
~CompositeAnimationPrivate();
void clearRenderer();
PassRefPtr<RenderStyle> animate(RenderObject*, RenderStyle* currentStyle, RenderStyle* targetStyle);
AnimationController* animationController() { return m_animationController; }
void setAnimating(bool);
bool isAnimating() const;
......@@ -74,6 +78,7 @@ public:
bool isAnimatingProperty(int property, bool isRunningNow) const;
void setWaitingForStyleAvailable(bool);
bool isWaitingForStyleAvailable() const { return m_numStyleAvailableWaiters > 0; }
bool pauseAnimationAtTime(const AtomicString& name, double t);
bool pauseTransitionAtTime(int property, double t);
......@@ -95,6 +100,13 @@ private:
};
CompositeAnimationPrivate::~CompositeAnimationPrivate()
{
// Toss the refs to all animations
m_transitions.clear();
m_keyframeAnimations.clear();
}
void CompositeAnimationPrivate::clearRenderer()
{
// Clear the renderers from all running animations, in case we are in the middle of
// an animation callback (see https://bugs.webkit.org/show_bug.cgi?id=22052)
......@@ -110,9 +122,7 @@ CompositeAnimationPrivate::~CompositeAnimationPrivate()
anim->clearRenderer();
}
// Toss the refs to all animations
m_transitions.clear();
m_keyframeAnimations.clear();
}
void CompositeAnimationPrivate::updateTransitions(RenderObject* renderer, RenderStyle* currentStyle, RenderStyle* targetStyle)
......@@ -561,11 +571,21 @@ CompositeAnimation::~CompositeAnimation()
delete m_data;
}
void CompositeAnimation::clearRenderer()
{
m_data->clearRenderer();
}
PassRefPtr<RenderStyle> CompositeAnimation::animate(RenderObject* renderer, RenderStyle* currentStyle, RenderStyle* targetStyle)
{
return m_data->animate(renderer, currentStyle, targetStyle);
}
AnimationController* CompositeAnimation::animationController()
{
return m_data->animationController();
}
bool CompositeAnimation::isAnimating() const
{
return m_data->isAnimating();
......@@ -576,6 +596,11 @@ void CompositeAnimation::setWaitingForStyleAvailable(bool b)
m_data->setWaitingForStyleAvailable(b);
}
bool CompositeAnimation::isWaitingForStyleAvailable() const
{
return m_data->isWaitingForStyleAvailable();
}
void CompositeAnimation::suspendAnimations()
{
m_data->suspendAnimations();
......
......@@ -43,15 +43,24 @@ class RenderStyle;
// A CompositeAnimation represents a collection of animations that are running
// on a single RenderObject, such as a number of properties transitioning at once.
class CompositeAnimation : public Noncopyable {
class CompositeAnimation : public RefCounted<CompositeAnimation> {
public:
CompositeAnimation(AnimationController* animationController);
static PassRefPtr<CompositeAnimation> create(AnimationController* animationController)
{
return adoptRef(new CompositeAnimation(animationController));
};
~CompositeAnimation();
void clearRenderer();
PassRefPtr<RenderStyle> animate(RenderObject*, RenderStyle* currentStyle, RenderStyle* targetStyle);
bool isAnimating() const;
AnimationController* animationController();
void setWaitingForStyleAvailable(bool);
bool isWaitingForStyleAvailable() const;
void suspendAnimations();
void resumeAnimations();
......@@ -71,6 +80,8 @@ public:
bool pauseTransitionAtTime(int property, double t);
private:
CompositeAnimation(AnimationController* animationController);
CompositeAnimationPrivate* m_data;
};
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment