Title: [286532] trunk
- Revision
- 286532
- Author
- [email protected]
- Date
- 2021-12-04 07:28:47 -0800 (Sat, 04 Dec 2021)
Log Message
CSS animation sorting may crash due to AnimationList copy upon CSS Animation removal
https://bugs.webkit.org/show_bug.cgi?id=233819
rdar://85596065
Reviewed by Dean Jackson and Darin Adler.
Source/WebCore:
Test: webanimations/css-animation-sorting-crash-2.html
When resolving animations during style resolution, we sort them to ensure they're in the right
composite order as defined by the Web Animations and related specifications, in this case the
CSS Animations specification defining how CSS Animations set on a given element are sorted.
Indeed, a given element may have multiple animations specified on it, and in fact it may have
multiple instances of the same animation. For instance, setting `animation-name: anim, anim, anim`
would create three separate CSS Animations all with the same keyframes and timing properties.
The CSS machinery within WebKit creates an AnimationList to reference the animations parsed from
CSS. Each animation is an Animation object. These Animation objects are ref-counted.
When we update animations, using Styleable::updateCSSAnimations(), we compare the current AnimationList
for this style resolution, with the previous AnimationList specified when this method was last called
for this element. The outcome of this comparison will yield new CSSAnimation objects, the removal of
such objects or the update of existing objects by setting the Animation object as its "backing animation".
When we're done we keep a reference to the current AnimationList on the element's KeyframeEffectStack.
Later, when we resolve animations during style resolution and we get to sort the animations, we will
use the AnimationList which contains the Animation objects in the order they were specified in the
`animation-list` property to establish the order in which the CSSAnimation objects should be ordered
relative to one another, based on their "backing animation" which must be an Animation object found
in the AnimationList.
If we fail to find matching Animation objects, we crash due to a call to RELEASE_ASSERT_NOT_REACHED()
in compareCSSAnimations(const CSSAnimation&, const CSSAnimation&).
So, why would we ever get in a situation where we reach this RELEASE_ASSERT_NOT_REACHED? Well, there is
a situation where we manipulate the AnimationList set on the KeyframeEffectStack in Styleable::updateCSSAnimations().
That case is when Styleable::cancelDeclarativeAnimations() is called, and we call the static function
removeCSSAnimationCreatedByMarkup(). In this function, we actually make a copy of the previously recorded
AnimationList because that list is `const` so we can't manipulate it directly. To make this copy we call
AnimationList::copy() which creates a new AnimationList object, which itself is not the issue, but also
makes copies of each Animation object within. Now, that's the problem, because at this point our pointer
comparisons in compareCSSAnimations() will fail since the Animation objects we recorded in
Styleable::updateCSSAnimations() will no longer be the same instances as those in the manipulated AnimationList.
To fix this, we add a new AnimationList::shallowCopy() method to specify whether we want clones or references
of the Animation members, and when calling removeCSSAnimationCreatedByMarkup(), we use this new method such
that we get references and not clones. This ensures that removing an animation from the list will indeed
create a new AnimationList, but the two lists will have references to the same Animation objects.
* platform/animation/AnimationList.cpp:
(WebCore::AnimationList::AnimationList):
* platform/animation/AnimationList.h:
(WebCore::AnimationList::copy const):
(WebCore::AnimationList::shallowCopy const):
* style/Styleable.cpp:
(WebCore::removeCSSAnimationCreatedByMarkup):
LayoutTests:
Add a test that used to crash before this patch.
* webanimations/css-animation-sorting-crash-2-expected.txt: Added.
* webanimations/css-animation-sorting-crash-2.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (286531 => 286532)
--- trunk/LayoutTests/ChangeLog 2021-12-04 15:24:46 UTC (rev 286531)
+++ trunk/LayoutTests/ChangeLog 2021-12-04 15:28:47 UTC (rev 286532)
@@ -1,3 +1,16 @@
+2021-12-04 Antoine Quint <[email protected]>
+
+ CSS animation sorting may crash due to AnimationList copy upon CSS Animation removal
+ https://bugs.webkit.org/show_bug.cgi?id=233819
+ rdar://85596065
+
+ Reviewed by Dean Jackson and Darin Adler.
+
+ Add a test that used to crash before this patch.
+
+ * webanimations/css-animation-sorting-crash-2-expected.txt: Added.
+ * webanimations/css-animation-sorting-crash-2.html: Added.
+
2021-12-04 Rob Buis <[email protected]>
Fix parentNode in CompositeEditCommand::splitTreeToNode
Added: trunk/LayoutTests/webanimations/css-animation-sorting-crash-2-expected.txt (0 => 286532)
--- trunk/LayoutTests/webanimations/css-animation-sorting-crash-2-expected.txt (rev 0)
+++ trunk/LayoutTests/webanimations/css-animation-sorting-crash-2-expected.txt 2021-12-04 15:28:47 UTC (rev 286532)
@@ -0,0 +1,4 @@
+PASS if this test doesn't crash
+
+
+
Added: trunk/LayoutTests/webanimations/css-animation-sorting-crash-2.html (0 => 286532)
--- trunk/LayoutTests/webanimations/css-animation-sorting-crash-2.html (rev 0)
+++ trunk/LayoutTests/webanimations/css-animation-sorting-crash-2.html 2021-12-04 15:28:47 UTC (rev 286532)
@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<style></style>
+<script>
+
+if (window.testRunner)
+ window.testRunner.dumpAsText();
+
+_onload_ = async () => {
+ document.body.append(document.createElement('input'));
+ document.body.append(document.createElement('slot'));
+ let n3 = document.createElement('br');
+ document.body.append(n3);
+ let n7 = document.createElement('div');
+ n3.append(n7);
+ document.body.append(document.createElement('input'));
+ document.body.offsetTop;
+ n7.append(document.createElement('div'));
+ document.execCommand('SelectAll');
+ await caches.has('a');
+ document.designMode = 'on';
+ document.styleSheets[0].insertRule(`@keyframes a1 { }`);
+ document.styleSheets[0].insertRule(`slot { animation-delay: 25ms; }`);
+ _onbeforeunload_ = () => document.execCommand('Bold');
+};
+
+</script>
+<p>PASS if this test doesn't crash</p>
\ No newline at end of file
Modified: trunk/Source/WebCore/ChangeLog (286531 => 286532)
--- trunk/Source/WebCore/ChangeLog 2021-12-04 15:24:46 UTC (rev 286531)
+++ trunk/Source/WebCore/ChangeLog 2021-12-04 15:28:47 UTC (rev 286532)
@@ -1,3 +1,63 @@
+2021-12-04 Antoine Quint <[email protected]>
+
+ CSS animation sorting may crash due to AnimationList copy upon CSS Animation removal
+ https://bugs.webkit.org/show_bug.cgi?id=233819
+ rdar://85596065
+
+ Reviewed by Dean Jackson and Darin Adler.
+
+ Test: webanimations/css-animation-sorting-crash-2.html
+
+ When resolving animations during style resolution, we sort them to ensure they're in the right
+ composite order as defined by the Web Animations and related specifications, in this case the
+ CSS Animations specification defining how CSS Animations set on a given element are sorted.
+
+ Indeed, a given element may have multiple animations specified on it, and in fact it may have
+ multiple instances of the same animation. For instance, setting `animation-name: anim, anim, anim`
+ would create three separate CSS Animations all with the same keyframes and timing properties.
+
+ The CSS machinery within WebKit creates an AnimationList to reference the animations parsed from
+ CSS. Each animation is an Animation object. These Animation objects are ref-counted.
+
+ When we update animations, using Styleable::updateCSSAnimations(), we compare the current AnimationList
+ for this style resolution, with the previous AnimationList specified when this method was last called
+ for this element. The outcome of this comparison will yield new CSSAnimation objects, the removal of
+ such objects or the update of existing objects by setting the Animation object as its "backing animation".
+
+ When we're done we keep a reference to the current AnimationList on the element's KeyframeEffectStack.
+
+ Later, when we resolve animations during style resolution and we get to sort the animations, we will
+ use the AnimationList which contains the Animation objects in the order they were specified in the
+ `animation-list` property to establish the order in which the CSSAnimation objects should be ordered
+ relative to one another, based on their "backing animation" which must be an Animation object found
+ in the AnimationList.
+
+ If we fail to find matching Animation objects, we crash due to a call to RELEASE_ASSERT_NOT_REACHED()
+ in compareCSSAnimations(const CSSAnimation&, const CSSAnimation&).
+
+ So, why would we ever get in a situation where we reach this RELEASE_ASSERT_NOT_REACHED? Well, there is
+ a situation where we manipulate the AnimationList set on the KeyframeEffectStack in Styleable::updateCSSAnimations().
+ That case is when Styleable::cancelDeclarativeAnimations() is called, and we call the static function
+ removeCSSAnimationCreatedByMarkup(). In this function, we actually make a copy of the previously recorded
+ AnimationList because that list is `const` so we can't manipulate it directly. To make this copy we call
+ AnimationList::copy() which creates a new AnimationList object, which itself is not the issue, but also
+ makes copies of each Animation object within. Now, that's the problem, because at this point our pointer
+ comparisons in compareCSSAnimations() will fail since the Animation objects we recorded in
+ Styleable::updateCSSAnimations() will no longer be the same instances as those in the manipulated AnimationList.
+
+ To fix this, we add a new AnimationList::shallowCopy() method to specify whether we want clones or references
+ of the Animation members, and when calling removeCSSAnimationCreatedByMarkup(), we use this new method such
+ that we get references and not clones. This ensures that removing an animation from the list will indeed
+ create a new AnimationList, but the two lists will have references to the same Animation objects.
+
+ * platform/animation/AnimationList.cpp:
+ (WebCore::AnimationList::AnimationList):
+ * platform/animation/AnimationList.h:
+ (WebCore::AnimationList::copy const):
+ (WebCore::AnimationList::shallowCopy const):
+ * style/Styleable.cpp:
+ (WebCore::removeCSSAnimationCreatedByMarkup):
+
2021-12-04 Rob Buis <[email protected]>
Fix parentNode in CompositeEditCommand::splitTreeToNode
Modified: trunk/Source/WebCore/platform/animation/AnimationList.cpp (286531 => 286532)
--- trunk/Source/WebCore/platform/animation/AnimationList.cpp 2021-12-04 15:24:46 UTC (rev 286531)
+++ trunk/Source/WebCore/platform/animation/AnimationList.cpp 2021-12-04 15:28:47 UTC (rev 286532)
@@ -35,12 +35,16 @@
AnimationList::AnimationList() = default;
-AnimationList::AnimationList(const AnimationList& other)
+AnimationList::AnimationList(const AnimationList& other, CopyBehavior copyBehavior)
: RefCounted()
{
m_animations.reserveInitialCapacity(other.size());
- for (auto& animation : other.m_animations)
- m_animations.uncheckedAppend(Animation::create(animation.get()));
+ for (auto& animation : other.m_animations) {
+ if (copyBehavior == CopyBehavior::Reference)
+ m_animations.uncheckedAppend(animation.get());
+ else
+ m_animations.uncheckedAppend(Animation::create(animation.get()));
+ }
}
void AnimationList::fillUnsetProperties()
Modified: trunk/Source/WebCore/platform/animation/AnimationList.h (286531 => 286532)
--- trunk/Source/WebCore/platform/animation/AnimationList.h 2021-12-04 15:24:46 UTC (rev 286531)
+++ trunk/Source/WebCore/platform/animation/AnimationList.h 2021-12-04 15:28:47 UTC (rev 286532)
@@ -35,7 +35,8 @@
public:
static Ref<AnimationList> create() { return adoptRef(*new AnimationList); }
- Ref<AnimationList> copy() const { return adoptRef(*new AnimationList(*this)); }
+ Ref<AnimationList> copy() const { return adoptRef(*new AnimationList(*this, CopyBehavior::Clone)); }
+ Ref<AnimationList> shallowCopy() const { return adoptRef(*new AnimationList(*this, CopyBehavior::Reference)); }
void fillUnsetProperties();
bool operator==(const AnimationList&) const;
@@ -56,8 +57,10 @@
private:
AnimationList();
- AnimationList(const AnimationList&);
+ enum class CopyBehavior : uint8_t { Clone, Reference };
+ AnimationList(const AnimationList&, CopyBehavior);
+
AnimationList& operator=(const AnimationList&);
Vector<Ref<Animation>, 0, CrashOnOverflow, 0> m_animations;
Modified: trunk/Source/WebCore/style/Styleable.cpp (286531 => 286532)
--- trunk/Source/WebCore/style/Styleable.cpp 2021-12-04 15:24:46 UTC (rev 286531)
+++ trunk/Source/WebCore/style/Styleable.cpp 2021-12-04 15:28:47 UTC (rev 286532)
@@ -163,7 +163,11 @@
auto& backingAnimation = cssAnimation.backingAnimation();
for (size_t i = 0; i < cssAnimationList->size(); ++i) {
if (cssAnimationList->animation(i) == backingAnimation) {
- auto newAnimationList = cssAnimationList->copy();
+ // It is important we do not make a clone of the Animation references contained
+ // within cssAnimationList since sorting animations in compareCSSAnimations()
+ // makes pointer comparisons to distinguish between backing animations of various
+ // CSSAnimation objects.
+ auto newAnimationList = cssAnimationList->shallowCopy();
newAnimationList->remove(i);
keyframeEffectStack.setCSSAnimationList(WTFMove(newAnimationList));
return;
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
