Diff
Modified: trunk/LayoutTests/ChangeLog (234016 => 234017)
--- trunk/LayoutTests/ChangeLog 2018-07-19 23:37:49 UTC (rev 234016)
+++ trunk/LayoutTests/ChangeLog 2018-07-20 00:06:10 UTC (rev 234017)
@@ -1,3 +1,17 @@
+2018-07-19 Antoine Quint <[email protected]>
+
+ Flaky crash in AnimationTimeline::cancelOrRemoveDeclarativeAnimation
+ https://bugs.webkit.org/show_bug.cgi?id=187530
+ <rdar://problem/42095186>
+
+ Reviewed by Dean Jackson.
+
+ Adjust an existing test which assumes an animation might be running when it's not really, so we test the animation is
+ not running using an alternate method.
+
+ * animations/keyframes-dynamic-expected.txt:
+ * animations/keyframes-dynamic.html:
+
2018-07-19 Ryan Haddad <[email protected]>
Mark storage/indexeddb/modern/opendatabase-after-storage-crash.html as flaky.
Modified: trunk/LayoutTests/animations/keyframes-dynamic-expected.txt (234016 => 234017)
--- trunk/LayoutTests/animations/keyframes-dynamic-expected.txt 2018-07-19 23:37:49 UTC (rev 234016)
+++ trunk/LayoutTests/animations/keyframes-dynamic-expected.txt 2018-07-20 00:06:10 UTC (rev 234017)
@@ -3,6 +3,4 @@
PASS - "left" property for "box1" element at 0.7s saw something close to: 200
PASS - "left" property for "box2" element at 0.3s saw something close to: 100
PASS - "left" property for "box2" element at 0.7s saw something close to: 200
-PASS - "left" property for "box3" element at 0.3s saw something close to: 0
-PASS - "left" property for "box3" element at 0.7s saw something close to: 0
Modified: trunk/LayoutTests/animations/keyframes-dynamic.html (234016 => 234017)
--- trunk/LayoutTests/animations/keyframes-dynamic.html 2018-07-19 23:37:49 UTC (rev 234016)
+++ trunk/LayoutTests/animations/keyframes-dynamic.html 2018-07-20 00:06:10 UTC (rev 234017)
@@ -27,8 +27,6 @@
["anim1", 0.7, "box1", "left", 200, 1],
["anim2", 0.3, "box2", "left", 100, 1],
["anim2", 0.7, "box2", "left", 200, 1],
- ["anim3", 0.3, "box3", "left", 0, 0],
- ["anim3", 0.7, "box3", "left", 0, 0],
];
function addKeyframes() {
@@ -64,7 +62,11 @@
style.sheet.removeRule(box3Index);
- runAnimationTest(expectedValues);
+ runAnimationTest(expectedValues, () => {
+ setTimeout(() => {
+ document.getElementById("result").appendChild(document.createTextNode(`${!box3.getAnimations().length ? "PASS" : "FAIL"} - Animation is not running on box3.`));
+ })
+ });
}
window.addEventListener('DOMContentLoaded', addKeyframes, false);
Modified: trunk/LayoutTests/imported/mozilla/ChangeLog (234016 => 234017)
--- trunk/LayoutTests/imported/mozilla/ChangeLog 2018-07-19 23:37:49 UTC (rev 234016)
+++ trunk/LayoutTests/imported/mozilla/ChangeLog 2018-07-20 00:06:10 UTC (rev 234017)
@@ -1,3 +1,15 @@
+2018-07-19 Antoine Quint <[email protected]>
+
+ Flaky crash in AnimationTimeline::cancelOrRemoveDeclarativeAnimation
+ https://bugs.webkit.org/show_bug.cgi?id=187530
+ <rdar://problem/42095186>
+
+ Reviewed by Dean Jackson.
+
+ Mark a WPT progression now that we correctly ignore animation names that have no matching @keyframes rule.
+
+ * css-animations/test_element-get-animations-expected.txt:
+
2018-07-03 Antoine Quint <[email protected]>
Unreviewed, rebaselining somes Web Animations test expectations.
Modified: trunk/LayoutTests/imported/mozilla/css-animations/test_element-get-animations-expected.txt (234016 => 234017)
--- trunk/LayoutTests/imported/mozilla/css-animations/test_element-get-animations-expected.txt 2018-07-19 23:37:49 UTC (rev 234016)
+++ trunk/LayoutTests/imported/mozilla/css-animations/test_element-get-animations-expected.txt 2018-07-20 00:06:10 UTC (rev 234017)
@@ -7,8 +7,8 @@
PASS getAnimations for CSS Animations that have finished
PASS getAnimations for CSS Animations that have finished but are forwards filling
PASS getAnimations for CSS Animations with animation-name: none
-FAIL getAnimations for CSS Animations with animation-name: missing assert_equals: getAnimations returns an empty sequence for an element with animation-name: missing expected 0 but got 1
-FAIL getAnimations for CSS Animations where the @keyframes rule is added later assert_equals: getAnimations initally only returns Animations for CSS Animations whose animation-name is found expected 1 but got 2
+PASS getAnimations for CSS Animations with animation-name: missing
+FAIL getAnimations for CSS Animations where the @keyframes rule is added later assert_equals: getAnimations includes Animation when @keyframes rule is added later expected 2 but got 1
PASS getAnimations for CSS Animations with duplicated animation-name
PASS getAnimations for CSS Animations with empty keyframes rule
PASS getAnimations for CSS animations in delay phase
Modified: trunk/Source/WebCore/ChangeLog (234016 => 234017)
--- trunk/Source/WebCore/ChangeLog 2018-07-19 23:37:49 UTC (rev 234016)
+++ trunk/Source/WebCore/ChangeLog 2018-07-20 00:06:10 UTC (rev 234017)
@@ -1,3 +1,29 @@
+2018-07-19 Antoine Quint <[email protected]>
+
+ Flaky crash in AnimationTimeline::cancelOrRemoveDeclarativeAnimation
+ https://bugs.webkit.org/show_bug.cgi?id=187530
+ <rdar://problem/42095186>
+
+ Reviewed by Dean Jackson.
+
+ We would crash in cancelOrRemoveDeclarativeAnimation() because updateCSSAnimationsForElement() would pass
+ nullptr values due to the return value of cssAnimationsByName.take(nameOfAnimationToRemove). This is because
+ we would create animations for animation names that may be empty or not match an existing @keyframes rule.
+ Not only was that wasteful, but it was also non-compliant, and as a result of fixing this we're actually
+ seeing a progression in the CSS Animations WPT tests.
+
+ * animation/AnimationTimeline.cpp:
+ (WebCore::shouldConsiderAnimation): New function that performs all required steps to see if a provided animation
+ is valid and has a name that is not "none", not the empty string and matches the name of a @keyframes rule.
+ (WebCore::AnimationTimeline::updateCSSAnimationsForElement):
+ * animation/KeyframeEffectReadOnly.cpp:
+ (WebCore::KeyframeEffectReadOnly::computeCSSAnimationBlendingKeyframes): We no longer need to check whether we have
+ an empty animation name since we're no longer creating CSSAnimation objects in that circumstance.
+ * css/StyleResolver.cpp:
+ (WebCore::StyleResolver::isAnimationNameValid): Add a new method that checks whether the provided animation name
+ a known @keyframes rule.
+ * css/StyleResolver.h:
+
2018-07-19 Chris Dumez <[email protected]>
Crash under WebCore::DocumentWriter::addData()
Modified: trunk/Source/WebCore/animation/AnimationTimeline.cpp (234016 => 234017)
--- trunk/Source/WebCore/animation/AnimationTimeline.cpp 2018-07-19 23:37:49 UTC (rev 234016)
+++ trunk/Source/WebCore/animation/AnimationTimeline.cpp 2018-07-20 00:06:10 UTC (rev 234017)
@@ -39,6 +39,7 @@
#include "RenderStyle.h"
#include "RenderView.h"
#include "StylePropertyShorthand.h"
+#include "StyleResolver.h"
#include "WebAnimationUtilities.h"
#include <wtf/text/TextStream.h>
#include <wtf/text/WTFString.h>
@@ -195,6 +196,23 @@
cssAnimation->cancel();
}
+static bool shouldConsiderAnimation(Element& element, const Animation& animation)
+{
+ if (!animation.isValidAnimation())
+ return false;
+
+ static NeverDestroyed<const String> animationNameNone(MAKE_STATIC_STRING_IMPL("none"));
+
+ auto& name = animation.name();
+ if (name == animationNameNone || name.isEmpty())
+ return false;
+
+ if (auto* styleScope = Style::Scope::forOrdinal(element, animation.nameStyleScopeOrdinal()))
+ return styleScope->resolver().isAnimationNameValid(name);
+
+ return false;
+}
+
void AnimationTimeline::updateCSSAnimationsForElement(Element& element, const RenderStyle* currentStyle, const RenderStyle& afterChangeStyle)
{
// In case this element is newly getting a "display: none" we need to cancel all of its animations and disregard new ones.
@@ -209,8 +227,6 @@
if (currentStyle && currentStyle->hasAnimations() && afterChangeStyle.hasAnimations() && *(currentStyle->animations()) == *(afterChangeStyle.animations()))
return;
- static NeverDestroyed<const String> animationNameNone(MAKE_STATIC_STRING_IMPL("none"));
-
// First, compile the list of animation names that were applied to this element up to this point.
HashSet<String> namesOfPreviousAnimations;
if (currentStyle && currentStyle->hasAnimations()) {
@@ -217,7 +233,7 @@
auto* previousAnimations = currentStyle->animations();
for (size_t i = 0; i < previousAnimations->size(); ++i) {
auto& previousAnimation = previousAnimations->animation(i);
- if (previousAnimation.isValidAnimation() && previousAnimation.name() != animationNameNone)
+ if (shouldConsiderAnimation(element, previousAnimation))
namesOfPreviousAnimations.add(previousAnimation.name());
}
}
@@ -236,7 +252,7 @@
// created a CSSAnimation object for it and need to ensure that this CSSAnimation is backed by the current
// animation object for this animation name.
cssAnimationsByName.get(name)->setBackingAnimation(currentAnimation);
- } else if (currentAnimation.isValidAnimation() && name != animationNameNone) {
+ } else if (shouldConsiderAnimation(element, currentAnimation)) {
// Otherwise we are dealing with a new animation name and must create a CSSAnimation for it.
cssAnimationsByName.set(name, CSSAnimation::create(element, currentAnimation, currentStyle, afterChangeStyle));
}
Modified: trunk/Source/WebCore/animation/KeyframeEffectReadOnly.cpp (234016 => 234017)
--- trunk/Source/WebCore/animation/KeyframeEffectReadOnly.cpp 2018-07-19 23:37:49 UTC (rev 234016)
+++ trunk/Source/WebCore/animation/KeyframeEffectReadOnly.cpp 2018-07-20 00:06:10 UTC (rev 234017)
@@ -871,8 +871,6 @@
auto cssAnimation = downcast<CSSAnimation>(animation());
auto& backingAnimation = cssAnimation->backingAnimation();
- if (backingAnimation.name().isEmpty())
- return;
KeyframeList keyframeList(backingAnimation.name());
if (auto* styleScope = Style::Scope::forOrdinal(*m_target, backingAnimation.nameStyleScopeOrdinal()))
Modified: trunk/Source/WebCore/css/StyleResolver.cpp (234016 => 234017)
--- trunk/Source/WebCore/css/StyleResolver.cpp 2018-07-19 23:37:49 UTC (rev 234016)
+++ trunk/Source/WebCore/css/StyleResolver.cpp 2018-07-20 00:06:10 UTC (rev 234017)
@@ -460,6 +460,11 @@
return state.takeStyle();
}
+bool StyleResolver::isAnimationNameValid(const String& name)
+{
+ return m_keyframesRuleMap.find(AtomicString(name).impl()) != m_keyframesRuleMap.end();
+}
+
void StyleResolver::keyframeStylesForAnimation(const Element& element, const RenderStyle* elementStyle, KeyframeList& list)
{
list.clear();
Modified: trunk/Source/WebCore/css/StyleResolver.h (234016 => 234017)
--- trunk/Source/WebCore/css/StyleResolver.h 2018-07-19 23:37:49 UTC (rev 234016)
+++ trunk/Source/WebCore/css/StyleResolver.h 2018-07-20 00:06:10 UTC (rev 234017)
@@ -161,6 +161,7 @@
void setNewStateWithElement(const Element&);
std::unique_ptr<RenderStyle> styleForKeyframe(const RenderStyle*, const StyleRuleKeyframe*, KeyframeValue&);
+ bool isAnimationNameValid(const String&);
public:
// These methods will give back the set of rules that matched for a given element (or a pseudo-element).
