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.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@284312 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 759d885..c7e9609 100644
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,18 @@
+2021-10-15 Antoine Quint <graouts@webkit.org>
+
+ 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 <antti@apple.com>
[LFC][Integration] Enable inline boxes with borders
diff --git a/LayoutTests/animations/animation-remove-element-crash.html b/LayoutTests/animations/animation-remove-element-crash.html
index c2cc8bc..f5acb03 100644
--- a/LayoutTests/animations/animation-remove-element-crash.html
+++ b/LayoutTests/animations/animation-remove-element-crash.html
@@ -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>
diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
index 127851b..25a8592 100644
--- a/LayoutTests/imported/w3c/ChangeLog
+++ b/LayoutTests/imported/w3c/ChangeLog
@@ -1,3 +1,15 @@
+2021-10-15 Antoine Quint <graouts@webkit.org>
+
+ 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 <katherine_cheney@apple.com>
CSP: Implement src-elem and src-attr directives
diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/Element-getAnimations-dynamic-changes.tentative-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/Element-getAnimations-dynamic-changes.tentative-expected.txt
index 3b3bcdf..a66670a 100644
--- a/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/Element-getAnimations-dynamic-changes.tentative-expected.txt
+++ b/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/Element-getAnimations-dynamic-changes.tentative-expected.txt
@@ -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
diff --git a/LayoutTests/webanimations/css-animation-sorting-crash-expected.txt b/LayoutTests/webanimations/css-animation-sorting-crash-expected.txt
new file mode 100644
index 0000000..b6867ca
--- /dev/null
+++ b/LayoutTests/webanimations/css-animation-sorting-crash-expected.txt
@@ -0,0 +1,3 @@
+PASS if this test doesn't crash
+
+
diff --git a/LayoutTests/webanimations/css-animation-sorting-crash.html b/LayoutTests/webanimations/css-animation-sorting-crash.html
new file mode 100644
index 0000000..f60aad9
--- /dev/null
+++ b/LayoutTests/webanimations/css-animation-sorting-crash.html
@@ -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
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 6a60111..33889d9 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,48 @@
+2021-10-15 Antoine Quint <graouts@webkit.org>
+
+ 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 <antti@apple.com>
[LFC][Integration] Enable inline boxes with borders
diff --git a/Source/WebCore/animation/WebAnimationUtilities.cpp b/Source/WebCore/animation/WebAnimationUtilities.cpp
index b288e5f..9215916 100644
--- a/Source/WebCore/animation/WebAnimationUtilities.cpp
+++ b/Source/WebCore/animation/WebAnimationUtilities.cpp
@@ -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;
}
diff --git a/Source/WebCore/platform/animation/Animation.h b/Source/WebCore/platform/animation/Animation.h
index 2f37244..499bd91 100644
--- a/Source/WebCore/platform/animation/Animation.h
+++ b/Source/WebCore/platform/animation/Animation.h
@@ -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
diff --git a/Source/WebCore/platform/animation/AnimationList.cpp b/Source/WebCore/platform/animation/AnimationList.cpp
index 2dcb508..735fb21 100644
--- a/Source/WebCore/platform/animation/AnimationList.cpp
+++ b/Source/WebCore/platform/animation/AnimationList.cpp
@@ -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);
}
