Ensure timingFunctionForKeyframeAtIndex() can be used from setAnimatedPropertiesInSty...
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 18 Jul 2018 01:05:29 +0000 (01:05 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 18 Jul 2018 01:05:29 +0000 (01:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=187637
<rdar://problem/42157915>

Reviewed by Dean Jackson.

Source/WebCore:

Test: webanimations/empty-keyframes-crash.html

Unlike what we assumed, it is possible to have a non-declarative animation without any parsed keyframes.
This can happen as a result of calling `Element.animate({}, …)`. In this case, we want to return a null
value in timingFunctionForKeyframeAtIndex() so we update the call site in setAnimatedPropertiesInStyle()
which is the only place where we didn't check for a null value and didn't know for sure that there would
be parsed keyframes to rely on in the case of a WebAnimation instance.

* animation/KeyframeEffectReadOnly.cpp:
(WebCore::KeyframeEffectReadOnly::setAnimatedPropertiesInStyle):
(WebCore::KeyframeEffectReadOnly::timingFunctionForKeyframeAtIndex):

LayoutTests:

Add a new test that would crash prior to this change.

* webanimations/empty-keyframes-crash-expected.txt: Added.
* webanimations/empty-keyframes-crash.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@233903 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/webanimations/empty-keyframes-crash-expected.txt [new file with mode: 0644]
LayoutTests/webanimations/empty-keyframes-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/animation/KeyframeEffectReadOnly.cpp

index 1511b3b..88e3da7 100644 (file)
@@ -1,3 +1,16 @@
+2018-07-17  Antoine Quint  <graouts@apple.com>
+
+        Ensure timingFunctionForKeyframeAtIndex() can be used from setAnimatedPropertiesInStyle().
+        https://bugs.webkit.org/show_bug.cgi?id=187637
+        <rdar://problem/42157915>
+
+        Reviewed by Dean Jackson.
+
+        Add a new test that would crash prior to this change.
+
+        * webanimations/empty-keyframes-crash-expected.txt: Added.
+        * webanimations/empty-keyframes-crash.html: Added.
+
 2018-07-17  Ryan Haddad  <ryanhaddad@apple.com>
 
         Rebaseline imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/pbkdf2.https.worker.html for Sierra after r233898.
diff --git a/LayoutTests/webanimations/empty-keyframes-crash-expected.txt b/LayoutTests/webanimations/empty-keyframes-crash-expected.txt
new file mode 100644 (file)
index 0000000..8b13789
--- /dev/null
@@ -0,0 +1 @@
+
diff --git a/LayoutTests/webanimations/empty-keyframes-crash.html b/LayoutTests/webanimations/empty-keyframes-crash.html
new file mode 100644 (file)
index 0000000..6f627be
--- /dev/null
@@ -0,0 +1,30 @@
+<!DOCTYPE html><!-- webkit-test-runner [ enableWebAnimationsCSSIntegration=true ] -->
+<html>
+<head>
+<style>
+#hr1 {
+    transition: 1s;
+    background: url(data:image/gif;base64,);
+}
+</style>
+<script>
+
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+function crash() {
+    var hr1 = document.getElementById("hr1");
+    var hr2 = document.getElementById("hr2");
+    document.all[2].appendChild(hr2);
+    var animation = document.createElement("hr3").animate({  }, 1);
+    var hr1_animation_effect = hr1.getAnimations()[0].effect;
+    animation.effect = hr1_animation_effect;
+}
+
+</script>
+</head>
+<body onload="crash()">
+<hr id="hr1"></hr>
+<hr id="hr2"></hr>
+</body>
+</html>
index 256abfa..31766a8 100644 (file)
@@ -1,3 +1,23 @@
+2018-07-17  Antoine Quint  <graouts@apple.com>
+
+        Ensure timingFunctionForKeyframeAtIndex() can be used from setAnimatedPropertiesInStyle().
+        https://bugs.webkit.org/show_bug.cgi?id=187637
+        <rdar://problem/42157915>
+
+        Reviewed by Dean Jackson.
+
+        Test: webanimations/empty-keyframes-crash.html
+
+        Unlike what we assumed, it is possible to have a non-declarative animation without any parsed keyframes.
+        This can happen as a result of calling `Element.animate({}, …)`. In this case, we want to return a null
+        value in timingFunctionForKeyframeAtIndex() so we update the call site in setAnimatedPropertiesInStyle()
+        which is the only place where we didn't check for a null value and didn't know for sure that there would
+        be parsed keyframes to rely on in the case of a WebAnimation instance.
+
+        * animation/KeyframeEffectReadOnly.cpp:
+        (WebCore::KeyframeEffectReadOnly::setAnimatedPropertiesInStyle):
+        (WebCore::KeyframeEffectReadOnly::timingFunctionForKeyframeAtIndex):
+
 2018-07-15  Jiewen Tan  <jiewen_tan@apple.com>
 
         [WebCrypto] Crypto operations should copy their parameters before hoping to another thread
index 60819fe..ebff9f2 100644 (file)
@@ -1162,7 +1162,8 @@ void KeyframeEffectReadOnly::setAnimatedPropertiesInStyle(RenderStyle& targetSty
         if (startKeyframeIndex) {
             if (auto iterationDuration = timing()->iterationDuration()) {
                 auto rangeDuration = (endOffset - startOffset) * iterationDuration.seconds();
-                transformedDistance = timingFunctionForKeyframeAtIndex(startKeyframeIndex.value())->transformTime(intervalDistance, rangeDuration);
+                if (auto* timingFunction = timingFunctionForKeyframeAtIndex(startKeyframeIndex.value()))
+                    transformedDistance = timingFunction->transformTime(intervalDistance, rangeDuration);
             }
         }
 
@@ -1181,19 +1182,18 @@ TimingFunction* KeyframeEffectReadOnly::timingFunctionForKeyframeAtIndex(size_t
         return m_parsedKeyframes[index].timingFunction.get();
 
     auto effectAnimation = animation();
+    if (is<DeclarativeAnimation>(effectAnimation)) {
+        // If we're dealing with a CSS Animation, the timing function is specified either on the keyframe itself.
+        if (is<CSSAnimation>(effectAnimation)) {
+            if (auto* timingFunction = m_blendingKeyframes[index].timingFunction())
+                return timingFunction;
+        }
 
-    // If we didn't have parsed keyframes, we must be dealing with a declarative animation.
-    ASSERT(is<DeclarativeAnimation>(effectAnimation));
-
-    // If we're dealing with a CSS Animation, the timing function is specified either on the keyframe itself,
-    // or failing that on the backing Animation object which defines the default for all keyframes.
-    if (is<CSSAnimation>(effectAnimation)) {
-        if (auto* timingFunction = m_blendingKeyframes[index].timingFunction())
-            return timingFunction;
+        // Failing that, or for a CSS Transition, the timing function is inherited from the backing Animation object.
+        return downcast<DeclarativeAnimation>(effectAnimation)->backingAnimation().timingFunction();
     }
 
-    // Failing that, or for a CSS Transition, the timing function is inherited from the backing Animation object. 
-    return downcast<DeclarativeAnimation>(effectAnimation)->backingAnimation().timingFunction();
+    return nullptr;
 }
 
 void KeyframeEffectReadOnly::updateAcceleratedAnimationState()