- Revision
- 284312
- Author
- [email protected]
- Date
- 2021-10-16 03:16:32 -0700 (Sat, 16 Oct 2021)
Log Message
CSS Animations creation and sorting is incorrect and may lead to crash
https://bugs.webkit.org/show_bug.cgi?id=231812
<rdar://problem/82980774>
Reviewed by Dean Jackson.
LayoutTests/imported/w3c:
Mark two progressions in a test that now passes entirely.
* web-platform-tests/css/css-animations/Element-getAnimations-dynamic-changes.tentative-expected.txt:
Source/WebCore:
When we parse CSS properties related to CSS Animations, we create as many Animation objects as the maximum number
of values in one of the CSS Animations properties. These Animation objects are stored in an AnimationList which
is owned by RenderStyle and that we use to store all provided values such that they can be read back through the
computed style.
Upon finishing parsing those CSS properties, RenderStyle::adjustAnimations() is called and does two things that
were not quite correct.
First, it would attempt to remove animations that were in fact created for no good reason using the
Animation::isEmpty() method. That method was not quite right as it was only checking if the animation
name wasn't set but not whether the animation was explicitly marked as a "none" animation. This meant
that "none" animations could be considered as bogus animations and any animations after that one would
be mistakenly cleared.
Second, it would finish process the list of remaning animations by making sure that mis-matched CSS properties,
for instance three values for animation-duration and four values for animation-name, would be correctly represented
throughout the Animation objects. However, the function performing this work, AnimationList::fillUnsetProperties(),
used to reset the animation's name in case it wasn't explicitly set, which meant that Animation objects that did not
have a matching animation-name would end up with a valid one. This meant that further down the line, in
Styleable::updateCSSAnimations(), we couldn't distinguish Animation objects that were created without a
matching animation-name and discard those.
We've fixed those incorrect behaviors and we now have the expected number of animations created resulting in an additional
WPT PASS result.
Additionally, this also showed some issues around sorting CSS Animations in compareCSSAnimations() where we weren't
making a pointer comparison. This is now fixed and we now pass another WPT PASS result.
Test: webanimations/css-animation-sorting-crash.html
* animation/WebAnimationUtilities.cpp:
(WebCore::compareCSSAnimations):
* platform/animation/Animation.h:
(WebCore::Animation::isEmpty const):
* platform/animation/AnimationList.cpp:
(WebCore::AnimationList::fillUnsetProperties):
LayoutTests:
Add a new test that used to crash before the source change.
* animations/animation-remove-element-crash.html: Remove a line that was now a failure since only a
single animation should be created.
* webanimations/css-animation-sorting-crash-expected.txt: Added.
* webanimations/css-animation-sorting-crash.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (284311 => 284312)
--- trunk/LayoutTests/ChangeLog 2021-10-16 09:07:26 UTC (rev 284311)
+++ trunk/LayoutTests/ChangeLog 2021-10-16 10:16:32 UTC (rev 284312)
@@ -1,3 +1,18 @@
+2021-10-15 Antoine Quint <[email protected]>
+
+ CSS Animations creation and sorting is incorrect and may lead to crash
+ https://bugs.webkit.org/show_bug.cgi?id=231812
+ <rdar://problem/82980774>
+
+ Reviewed by Dean Jackson.
+
+ Add a new test that used to crash before the source change.
+
+ * animations/animation-remove-element-crash.html: Remove a line that was now a failure since only a
+ single animation should be created.
+ * webanimations/css-animation-sorting-crash-expected.txt: Added.
+ * webanimations/css-animation-sorting-crash.html: Added.
+
2021-10-15 Antti Koivisto <[email protected]>
[LFC][Integration] Enable inline boxes with borders
Modified: trunk/LayoutTests/animations/animation-remove-element-crash.html (284311 => 284312)
--- trunk/LayoutTests/animations/animation-remove-element-crash.html 2021-10-16 09:07:26 UTC (rev 284311)
+++ trunk/LayoutTests/animations/animation-remove-element-crash.html 2021-10-16 10:16:32 UTC (rev 284312)
@@ -14,7 +14,6 @@
console.log('This test passes if it does not crash.');
var animations = A.getAnimations();
document.body.appendChild(A);
- animations[1].pause();
animations[0].startTime = 0;
}
</script>
Modified: trunk/LayoutTests/imported/w3c/ChangeLog (284311 => 284312)
--- trunk/LayoutTests/imported/w3c/ChangeLog 2021-10-16 09:07:26 UTC (rev 284311)
+++ trunk/LayoutTests/imported/w3c/ChangeLog 2021-10-16 10:16:32 UTC (rev 284312)
@@ -1,3 +1,15 @@
+2021-10-15 Antoine Quint <[email protected]>
+
+ CSS Animations creation and sorting is incorrect and may lead to crash
+ https://bugs.webkit.org/show_bug.cgi?id=231812
+ <rdar://problem/82980774>
+
+ Reviewed by Dean Jackson.
+
+ Mark two progressions in a test that now passes entirely.
+
+ * web-platform-tests/css/css-animations/Element-getAnimations-dynamic-changes.tentative-expected.txt:
+
2021-10-15 Kate Cheney <[email protected]>
CSP: Implement src-elem and src-attr directives
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/Element-getAnimations-dynamic-changes.tentative-expected.txt (284311 => 284312)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/Element-getAnimations-dynamic-changes.tentative-expected.txt 2021-10-16 09:07:26 UTC (rev 284311)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/Element-getAnimations-dynamic-changes.tentative-expected.txt 2021-10-16 10:16:32 UTC (rev 284312)
@@ -2,6 +2,6 @@
PASS Animations preserve their startTime when changed
PASS Updated Animations maintain their order in the list
PASS Only the startTimes of existing animations are preserved
-FAIL Animations are removed from the start of the list while preserving the state of existing Animations assert_equals: List of Animations was trimmed expected 1 but got 2
-FAIL Animation state is preserved when interleaving animations in list assert_equals: Second Animation remains in same position after interleaving expected object "[object CSSAnimation]" but got object "[object CSSAnimation]"
+PASS Animations are removed from the start of the list while preserving the state of existing Animations
+PASS Animation state is preserved when interleaving animations in list
Added: trunk/LayoutTests/webanimations/css-animation-sorting-crash-expected.txt (0 => 284312)
--- trunk/LayoutTests/webanimations/css-animation-sorting-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/webanimations/css-animation-sorting-crash-expected.txt 2021-10-16 10:16:32 UTC (rev 284312)
@@ -0,0 +1,3 @@
+PASS if this test doesn't crash
+
+
Added: trunk/LayoutTests/webanimations/css-animation-sorting-crash.html (0 => 284312)
--- trunk/LayoutTests/webanimations/css-animation-sorting-crash.html (rev 0)
+++ trunk/LayoutTests/webanimations/css-animation-sorting-crash.html 2021-10-16 10:16:32 UTC (rev 284312)
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<style></style>
+<script>
+
+if (window.testRunner)
+ window.testRunner.dumpAsText();
+
+_onload_ = async () => {
+ document.styleSheets[0].insertRule(`@keyframes a0 { }`);
+ document.styleSheets[0].insertRule(`a { animation-delay: 0ms, 0ms, 25ms, 25ms; }`);
+ document.styleSheets[0].insertRule(`a { animation-name: a0; }`);
+ document.styleSheets[0].insertRule(`a { min-block-size: 100px; }`);
+ document.styleSheets[0].insertRule(`head { display: block; }`);
+ document.styleSheets[0].insertRule(`style { -webkit-appearance: slider-vertical; }`);
+ document.styleSheets[0].insertRule(`a { -webkit-appearance: slider-vertical; }`);
+ document.styleSheets[0].insertRule(`style{ display: inline; }`);
+ document.designMode = 'on';
+ document.body.appendChild(document.createElement('a'));
+ document.execCommand('SelectAll');
+ await caches.has('a');
+ document.head.appendChild(document.createElement('a'));
+ await caches.has('b');
+ document.execCommand('Italic');
+ document.execCommand('SelectAll');
+ document.execCommand('Bold');
+};
+
+</script>
+<p>PASS if this test doesn't crash</p>
\ No newline at end of file
Modified: trunk/Source/WebCore/ChangeLog (284311 => 284312)
--- trunk/Source/WebCore/ChangeLog 2021-10-16 09:07:26 UTC (rev 284311)
+++ trunk/Source/WebCore/ChangeLog 2021-10-16 10:16:32 UTC (rev 284312)
@@ -1,3 +1,48 @@
+2021-10-15 Antoine Quint <[email protected]>
+
+ CSS Animations creation and sorting is incorrect and may lead to crash
+ https://bugs.webkit.org/show_bug.cgi?id=231812
+ <rdar://problem/82980774>
+
+ Reviewed by Dean Jackson.
+
+ When we parse CSS properties related to CSS Animations, we create as many Animation objects as the maximum number
+ of values in one of the CSS Animations properties. These Animation objects are stored in an AnimationList which
+ is owned by RenderStyle and that we use to store all provided values such that they can be read back through the
+ computed style.
+
+ Upon finishing parsing those CSS properties, RenderStyle::adjustAnimations() is called and does two things that
+ were not quite correct.
+
+ First, it would attempt to remove animations that were in fact created for no good reason using the
+ Animation::isEmpty() method. That method was not quite right as it was only checking if the animation
+ name wasn't set but not whether the animation was explicitly marked as a "none" animation. This meant
+ that "none" animations could be considered as bogus animations and any animations after that one would
+ be mistakenly cleared.
+
+ Second, it would finish process the list of remaning animations by making sure that mis-matched CSS properties,
+ for instance three values for animation-duration and four values for animation-name, would be correctly represented
+ throughout the Animation objects. However, the function performing this work, AnimationList::fillUnsetProperties(),
+ used to reset the animation's name in case it wasn't explicitly set, which meant that Animation objects that did not
+ have a matching animation-name would end up with a valid one. This meant that further down the line, in
+ Styleable::updateCSSAnimations(), we couldn't distinguish Animation objects that were created without a
+ matching animation-name and discard those.
+
+ We've fixed those incorrect behaviors and we now have the expected number of animations created resulting in an additional
+ WPT PASS result.
+
+ Additionally, this also showed some issues around sorting CSS Animations in compareCSSAnimations() where we weren't
+ making a pointer comparison. This is now fixed and we now pass another WPT PASS result.
+
+ Test: webanimations/css-animation-sorting-crash.html
+
+ * animation/WebAnimationUtilities.cpp:
+ (WebCore::compareCSSAnimations):
+ * platform/animation/Animation.h:
+ (WebCore::Animation::isEmpty const):
+ * platform/animation/AnimationList.cpp:
+ (WebCore::AnimationList::fillUnsetProperties):
+
2021-10-15 Antti Koivisto <[email protected]>
[LFC][Integration] Enable inline boxes with borders
Modified: trunk/Source/WebCore/animation/WebAnimationUtilities.cpp (284311 => 284312)
--- trunk/Source/WebCore/animation/WebAnimationUtilities.cpp 2021-10-16 09:07:26 UTC (rev 284311)
+++ trunk/Source/WebCore/animation/WebAnimationUtilities.cpp 2021-10-16 10:16:32 UTC (rev 284312)
@@ -134,9 +134,9 @@
auto& bBackingAnimation = b.backingAnimation();
for (size_t i = 0; i < cssAnimationList->size(); ++i) {
auto& animation = cssAnimationList->animation(i);
- if (animation == aBackingAnimation)
+ if (&animation == &aBackingAnimation)
return true;
- if (animation == bBackingAnimation)
+ if (&animation == &bBackingAnimation)
return false;
}
Modified: trunk/Source/WebCore/platform/animation/Animation.h (284311 => 284312)
--- trunk/Source/WebCore/platform/animation/Animation.h 2021-10-16 09:07:26 UTC (rev 284311)
+++ trunk/Source/WebCore/platform/animation/Animation.h 2021-10-16 10:16:32 UTC (rev 284312)
@@ -59,7 +59,8 @@
{
return !m_directionSet && !m_durationSet && !m_fillModeSet
&& !m_nameSet && !m_playStateSet && !m_iterationCountSet
- && !m_delaySet && !m_timingFunctionSet && !m_propertySet;
+ && !m_delaySet && !m_timingFunctionSet && !m_propertySet
+ && !m_isNone;
}
bool isEmptyOrZeroDuration() const
Modified: trunk/Source/WebCore/platform/animation/AnimationList.cpp (284311 => 284312)
--- trunk/Source/WebCore/platform/animation/AnimationList.cpp 2021-10-16 09:07:26 UTC (rev 284311)
+++ trunk/Source/WebCore/platform/animation/AnimationList.cpp 2021-10-16 10:16:32 UTC (rev 284312)
@@ -52,7 +52,6 @@
FILL_UNSET_PROPERTY(isFillModeSet, fillMode, setFillMode);
FILL_UNSET_PROPERTY(isIterationCountSet, iterationCount, setIterationCount);
FILL_UNSET_PROPERTY(isPlayStateSet, playState, setPlayState);
- FILL_UNSET_PROPERTY(isNameSet, name, setName);
FILL_UNSET_PROPERTY(isTimingFunctionSet, timingFunction, setTimingFunction);
FILL_UNSET_PROPERTY(isPropertySet, property, setProperty);
}
