| Summary: | Release assert in compareAnimationsByCompositeOrder in KeyframeEffectStack::ensureEffectsAreSorted via Style::TreeResolver::createAnimatedElementUpdate | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||
| Component: | Animations | Assignee: | Frédéric Wang (:fredw) <fred.wang> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | bfulgham, cgarcia, dino, ews-feeder, fred.wang, gpoo, graouts, graouts, koivisto, product-security, rbuis, svillar, webkit-bug-importer | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=223574 https://bugs.webkit.org/show_bug.cgi?id=223992 |
||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Ryosuke Niwa
2021-03-17 01:32:23 PDT
I was able to reproduce this issue using the test case in <rdar://75152056> with ASAN release build of WebKitTestRunner. I was able to reproduce this issue (with the attached test case) with ASAN release build of WebKitTestRunner and DumpRenderTree. Created attachment 423881 [details]
Reduced testcase
Will take a look.
Inside compareCSSAnimations() we are hitting ASSERT(!cssAnimationList->isEmpty()); in debug mode and RELEASE_ASSERT_NOT_REACHED(); in release mode. This happens when resolving the style of the moved <animate> element, its associated KeyframeEffectStack contains 2 effects (with associated animations) but an empty animation list:
BODY 0x7fc879e30010 (renderer 0x7fc879e61800) (child needs style recalc)
#text 0x7fc879e300a0 "\n "
ANIMATE 0x7fc879e30110 (renderer 0x7fc879e30a30)
#text 0x7fc879e301a0 "\n "
DIV 0x7fc879e30210 (renderer 0x7fc879e30b90)
#text 0x7fc879e302a0 "\n "
#text 0x7fc879e303a0 "\n "
#text 0x7fc879e28068 "\n\n"
* ANIMATE 0x7fc879e30310 (renderer 0x7fc879e30dd0) (needs style recalc)
#0 0x00007fc886c48b9b in WebCore::compareCSSAnimations(WebCore::CSSAnimation const&, WebCore::CSSAnimation const&) (a=..., b=...)
at ../../Source/WebCore/animation/WebAnimationUtilities.cpp:131
#1 0x00007fc886c48e8f in WebCore::compareAnimationsByCompositeOrder(WebCore::WebAnimation const&, WebCore::WebAnimation const&) (a=..., b=...)
at ../../Source/WebCore/animation/WebAnimationUtilities.cpp:171
#2 0x00007fc886c4c595 in operator()<WTF::WeakPtr<WebCore::KeyframeEffect>, WTF::WeakPtr<WebCore::KeyframeEffect> >(WTF::WeakPtr<WebCore::KeyframeEffect, WTF::EmptyCounter>&, WTF::WeakPtr<WebCore::KeyframeEffect, WTF::EmptyCounter>&) const (__closure=0x7ffdfd0fc827, lhs=..., rhs=...)
at ../../Source/WebCore/animation/KeyframeEffectStack.cpp:110
...
#9 0x00007fc886c4184d in WebCore::KeyframeEffectStack::ensureEffectsAreSorted() (this=0x7fc8302dafc0) at ../../Source/WebCore/animation/KeyframeEffectStack.cpp:100
#10 0x00007fc886c417d2 in WebCore::KeyframeEffectStack::sortedEffects() (this=0x7fc8302dafc0) at ../../Source/WebCore/animation/KeyframeEffectStack.cpp:91
#11 0x00007fc886c41a42 in WebCore::KeyframeEffectStack::applyKeyframeEffects(WebCore::RenderStyle&, WebCore::RenderStyle const&, WebCore::RenderStyle const*) (this=0x7fc8302dafc0, targetStyle=..., previousLastStyleChangeEventStyle=..., parentElementStyle=0x7fc879e61858) at ../../Source/WebCore/animation/KeyframeEffectStack.cpp:134
#12 0x00007fc888711504 in WebCore::Styleable::applyKeyframeEffects(WebCore::RenderStyle&, WebCore::RenderStyle const&, WebCore::RenderStyle const*) const (this=0x7ffdfd0fcbf0, targetStyle=..., previousLastStyleChangeEventStyle=..., parentElementStyle=0x7fc879e61858) at ../../Source/WebCore/style/Styleable.h:92
#13 0x00007fc88870ac9a in WebCore::Style::TreeResolver::createAnimatedElementUpdate(std::unique_ptr<WebCore::RenderStyle, std::default_delete<WebCore::RenderStyle> >, WebCore::Styleable const&, WebCore::Style::Change) (this=0x7ffdfd0ff660, newStyle=std::unique_ptr<WebCore::RenderStyle> = {...}, styleable=..., parentChange=WebCore::Style::Change::None) at ../../Source/WebCore/style/StyleTreeResolver.cpp:348
c#14 0x00007fc88870a0ca in WebCore::Style::TreeResolver::resolveElement(WebCore::Element&) (this=0x7ffdfd0ff660, element=...) at ../../Source/WebCore/style/StyleTreeResolver.cpp:225
...
...
#32 0x00007fc887a40de5 in WebCore::DocumentLoader::finishedLoading() (this=0x7fc8324f2400) at ../../Source/WebCore/loader/DocumentLoader.cpp:480
Running back to when the animation list was emptied, we found it happenned in AnimationTimeline::removeCSSAnimationCreatedByMarkup, when the <animate> element is moved from the inner div to the body by the JS code. At that time, the KeyframeEffectStack only contains 1 effect, which is removed when the animation is later canceled in cancelDeclarativeAnimationsForStyleable.
#0 WebCore::AnimationTimeline::removeCSSAnimationCreatedByMarkup(WebCore::Styleable const&, WebCore::CSSAnimation&)
(this=0x7fc8302b1000, styleable=..., cssAnimation=...) at ../../Source/WebCore/animation/AnimationTimeline.cpp:154
#1 0x00007fc88900dab2 in WebCore::AnimationTimeline::cancelDeclarativeAnimationsForStyleable(WebCore::Styleable const&, WebCore::WebAnimation::Silently) (this=0x7fc8302b1000, styleable=..., silently=WebCore::WebAnimation::Silently::Yes)
at ../../Source/WebCore/animation/AnimationTimeline.cpp:179
#2 0x00007fc88900d8e6 in WebCore::AnimationTimeline::elementWasRemoved(WebCore::Styleable const&)
(this=0x7fc8302b1000, styleable=...) at ../../Source/WebCore/animation/AnimationTimeline.cpp:162
#3 0x00007fc88720b08f in WebCore::Element::removedFromAncestor(WebCore::Node::RemovalType, WebCore::ContainerNode&)
(this=0x7fc879e30310, removalType=..., oldParentOfRemovedTree=...) at ../../Source/WebCore/dom/Element.cpp:2321
#4 0x00007fc88712203c in WebCore::notifyNodeRemovedFromDocument(WebCore::ContainerNode&, WebCore::TreeScopeChange, WebCore::Node&) (oldParentOfRemovedTree=..., treeScopeChange=WebCore::TreeScopeChange::Changed, node=...)
at ../../Source/WebCore/dom/ContainerNodeAlgorithms.cpp:126
#5 0x00007fc887122507 in WebCore::notifyChildNodeRemoved(WebCore::ContainerNode&, WebCore::Node&)
(oldParentOfRemovedTree=..., child=...) at ../../Source/WebCore/dom/ContainerNodeAlgorithms.cpp:178
#6 0x00007fc887107828 in WebCore::ContainerNode::removeNodeWithScriptAssertion(WebCore::Node&, WebCore::ContainerNode::ChildChange::Source) (this=0x7fc879e30210, childToRemove=..., source=WebCore::ContainerNode::ChildChange::Source::API)
at ../../Source/WebCore/dom/ContainerNode.cpp:182
#7 0x00007fc887100eaa in WebCore::ContainerNode::removeChild(WebCore::Node&) (this=0x7fc879e30210, oldChild=...)
at ../../Source/WebCore/dom/ContainerNode.cpp:614
#8 0x00007fc8870feb9d in WebCore::ContainerNode::removeSelfOrChildNodesForInsertion(WebCore::Node&, WTF::Vector<WTF::Ref<WebCore::Node, WTF::RawPtrTraits<WebCore::Node> >, 11ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&)
(this=0x7fc879e30010, child=..., nodesForInsertion=...) at ../../Source/WebCore/dom/ContainerNode.cpp:255
#9 0x00007fc8871018c6 in WebCore::ContainerNode::appendChildWithoutPreInsertionValidityCheck(WebCore::Node&)
(this=0x7fc879e30010, newChild=...) at ../../Source/WebCore/dom/ContainerNode.cpp:739
#10 0x00007fc8871017bb in WebCore::ContainerNode::appendChild(WebCore::Node&) (this=0x7fc879e30010, newChild=...)
at ../../Source/WebCore/dom/ContainerNode.cpp:731
#11 0x00007fc88729cb76 in WebCore::Node::appendChild(WebCore::Node&) (this=0x7fc879e30010, newChild=...)
at ../../Source/WebCore/dom/Node.cpp:511
However, one effect is added later when the JS code calls pause():
#3 0x00007fc886c41590 in WebCore::KeyframeEffectStack::addEffect(WebCore::KeyframeEffect&)
(this=0x7fc8302dafc0, effect=...) at ../../Source/WebCore/animation/KeyframeEffectStack.cpp:52
#4 0x00007fc886c3c8ad in WebCore::KeyframeEffect::updateEffectStackMembership() (this=0x7fc8302b7000)
at ../../Source/WebCore/animation/KeyframeEffect.cpp:1130
#5 0x00007fc886c3c7ee in WebCore::KeyframeEffect::animationTimingDidChange() (this=0x7fc8302b7000)
at ../../Source/WebCore/animation/KeyframeEffect.cpp:1119
#6 0x00007fc886c45100 in WebCore::WebAnimation::timingDidChange(WebCore::WebAnimation::DidSeek, WebCore::WebAnimation::SynchronouslyNotify, WebCore::WebAnimation::Silently)
(this=0x7fc879e307d0, didSeek=WebCore::WebAnimation::DidSeek::No, synchronouslyNotify=WebCore::WebAnimation::SynchronouslyNotify::No, silently=WebCore::WebAnimation::Silently::No) at ../../Source/WebCore/animation/WebAnimation.cpp:794
#7 0x00007fc886c46c7d in WebCore::WebAnimation::pause() (this=0x7fc879e307d0)
and a second effect is added when the startTime is changed by the JS code:
#3 0x00007fc886c41590 in WebCore::KeyframeEffectStack::addEffect(WebCore::KeyframeEffect&)
(this=0x7fc8302dafc0, effect=...) at ../../Source/WebCore/animation/KeyframeEffectStack.cpp:52
#4 0x00007fc886c3c8ad in WebCore::KeyframeEffect::updateEffectStackMembership() (this=0x7fc8302b7100)
at ../../Source/WebCore/animation/KeyframeEffect.cpp:1130
#5 0x00007fc886c3c7ee in WebCore::KeyframeEffect::animationTimingDidChange() (this=0x7fc8302b7100)
at ../../Source/WebCore/animation/KeyframeEffect.cpp:1119
#6 0x00007fc886c45100 in WebCore::WebAnimation::timingDidChange(WebCore::WebAnimation::DidSeek, WebCore::WebAnimation::SynchronouslyNotify, WebCore::WebAnimation::Silently)
(this=0x7fc879e30900, didSeek=WebCore::WebAnimation::DidSeek::Yes, synchronouslyNotify=WebCore::WebAnimation::SynchronouslyNotify::No, silently=WebCore::WebAnimation::Silently::No) at ../../Source/WebCore/animation/WebAnimation.cpp:794
#7 0x00007fc886c431c2 in WebCore::WebAnimation::setStartTime(WTF::Optional<WTF::Seconds>)
(this=0x7fc879e30900, newStartTime=...) at ../../Source/WebCore/animation/WebAnimation.cpp:355
So we end up in the bad state reached in comment 5.
Am I so confused here. Is WebAnimation object created from style sheet supposed to survive even when an element is removed? Like... is it supposed to continue to do things? That is very strange. If not, don't we just need to make sure pause(), startTime, etc... are no-op when it's no longer associated with an element? So maybe the issue here is that when an element is removed, we're not clearing the owning element. Namely, AnimationTimeline::elementWasRemoved calls cancelDeclarativeAnimationsForStyleable but that calls WebAnimation::cancel instead of DeclarativeAnimation::cancelFromStyle() which should be clearing the owning element. Created attachment 424107 [details] Reduced testcase This is exactly the same test case as attachment 423881 [details], but replacing the <animate> element with a <p>, so it's less confusing :-) Created attachment 424110 [details] (proof-of-concept) Patch (In reply to Ryosuke Niwa from comment #7) > Am I so confused here. Is WebAnimation object created from style sheet > supposed to survive even when an element is removed? Like... is it supposed > to continue to do things? That is very strange. I'm not really familiar with this WebAnimation thing... @Dean or @Antoine maybe you can provide some advice? BTW in attachment 424107 [details], when we do the appendChild() (and so the removal), this changes the p:only-of-type for the two <p> elements and so the -webkit-animation properties no longer apply to them. > If not, don't we just need to make sure pause(), startTime, etc... are no-op > when it's no longer associated with an element? > > So maybe the issue here is that when an element is removed, we're not > clearing the owning element. Namely, AnimationTimeline::elementWasRemoved > calls cancelDeclarativeAnimationsForStyleable but that calls > WebAnimation::cancel instead of DeclarativeAnimation::cancelFromStyle() > which should be clearing the owning element. Here is a basic patch doing something like that. It indeed prevents the crash, but I'm not sure it is "correct" or has other consequences. Let's check EWS... I'll try to investigate a bit more. Comment on attachment 424110 [details]
(proof-of-concept) Patch
This looks good, thanks Fred! I'll do a little refactoring to remove the Silently parameter afterwards and move that whole method to Styleable since it has nothing to do on the timeline.
Unable to find any modified ChangeLog in Attachment 424110 [details]
Created attachment 424674 [details]
Patch
Committed r275228: <https://commits.webkit.org/r275228> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424674 [details]. Filed https://bugs.webkit.org/show_bug.cgi?id=223992 to refactor the code now that Silently is no longer used by AnimationTimeline::cancelDeclarativeAnimationsForStyleable(). |