[Web Animations] Querying the current time of a finished CSSAnimation after removing...
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 23 Jul 2018 20:23:18 +0000 (20:23 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 23 Jul 2018 20:23:18 +0000 (20:23 +0000)
commitbe4db3948aaff53486da9f47a3750879d6582a4f
treeabea30742276539680973c745d167fcd3c0cd305
parente61cb8851038de33f0d6e2d8e5bdb9fc2626ee38
[Web Animations] Querying the current time of a finished CSSAnimation after removing its target leads to a crash
https://bugs.webkit.org/show_bug.cgi?id=187906

Reviewed by Dean Jackson.

Source/WebCore:

Test: webanimations/accessing-current-time-after-finished-css-animation-target-removal.html

Because we carelessly look at a CSSAnimation's effect's timing in DeclarativeAnimation::bindingsCurrentTime
without checking that the effect is non-null, we can crash in the case where the animation is finished and
its target element has been removed, which caused the effect to be set to null.

We do not actually fix the lack of a null check, which will be the scope of a different patch, but instead
ensure that we do _not_ set the animation's effect to null when its target is removed, which used to be
performed via a call to WebAnimation::remove(). Instead, we introduce AnimationTimeline::elementWasRemoved()
which notifies the timeline of an element being removed such that we may stop referencing any animation
targeting this element from the various data structures holding strong references to the animation in question,
and we then cancel the animation silently, which is a new option that ensures promises aren't resolved or
rejected as a result.

Finally, the WebAnimation and AnimationEffectReadOnly classes established a ref-cycle as WebAnimation has
`RefPtr<AnimationEffectReadOnly> m_effect` and AnimationEffectReadOnly has `RefPtr<WebAnimation> m_animation`.
While it is correct that WebAnimation owns its effect, which is established by the DOM API, the
reverse is not correct since we only hold the reverse internally for the benefit of our implementation.
As such, we change AnimationEffectReadOnly's m_animation to be a WeakPtr<WebAnimation>. This means not
calling WebAnimation::remove() and simply removing the animation from the animation maps on the timeline
is sufficient to guarantee that the document timeline will not leak (and with it the document).

* animation/AnimationEffectReadOnly.h:
(WebCore::AnimationEffectReadOnly::setAnimation):
* animation/AnimationTimeline.cpp:
(WebCore::AnimationTimeline::elementWasRemoved):
* animation/AnimationTimeline.h:
* animation/WebAnimation.cpp:
(WebCore::WebAnimation::cancel):
(WebCore::WebAnimation::resetPendingTasks):
* animation/WebAnimation.h:
* dom/Element.cpp:
(WebCore::Element::removedFromAncestor):
* rendering/updating/RenderTreeUpdater.cpp:
(WebCore::RenderTreeUpdater::tearDownRenderers):

LayoutTests:

Add a new test that checks the behavior of a CSSAnimation instance after its completion and removal of its target.

* webanimations/accessing-current-time-after-finished-css-animation-target-removal-expected.txt: Added.
* webanimations/accessing-current-time-after-finished-css-animation-target-removal.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@234109 268f45cc-cd09-0410-ab3c-d52691b4dbfc
LayoutTests/ChangeLog
LayoutTests/webanimations/accessing-current-time-after-finished-css-animation-target-removal-expected.txt [new file with mode: 0644]
LayoutTests/webanimations/accessing-current-time-after-finished-css-animation-target-removal.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/animation/AnimationEffectReadOnly.h
Source/WebCore/animation/AnimationTimeline.cpp
Source/WebCore/animation/AnimationTimeline.h
Source/WebCore/animation/WebAnimation.cpp
Source/WebCore/animation/WebAnimation.h
Source/WebCore/dom/Element.cpp
Source/WebCore/rendering/updating/RenderTreeUpdater.cpp