REGRESSION (222040): Google Maps Street View CrashTracer: [USER] com.apple.WebKit...
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 26 Sep 2017 15:51:55 +0000 (15:51 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 26 Sep 2017 15:51:55 +0000 (15:51 +0000)
https://bugs.webkit.org/show_bug.cgi?id=177485

Reviewed by Zalan Bujtas.

Source/WebCore:

We crash when animating between two different types of transforms because renderer is null for the first frame.

Test: fast/animation/animation-mixed-transform-crash.html

* page/animation/AnimationBase.cpp:
(WebCore::AnimationBase::currentStyle const):

    Add a way to get the current style from animations.
    This is either the render style or the inital style.

* page/animation/AnimationBase.h:
* page/animation/CSSPropertyAnimation.cpp:
(WebCore::blendFunc):

    Renderer may be null when computing the first frame of the animation. Null check.

(WebCore::blendFilter):

    Here too.

* page/animation/CompositeAnimation.cpp:
(WebCore::CompositeAnimation::updateTransitions):
* page/animation/ImplicitAnimation.h:
* page/animation/KeyframeAnimation.h:

LayoutTests:

* fast/animation/animation-mixed-transform-crash-expected.html: Added.
* fast/animation/animation-mixed-transform-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/animation/animation-mixed-transform-crash-expected.html [new file with mode: 0644]
LayoutTests/fast/animation/animation-mixed-transform-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/animation/AnimationBase.cpp
Source/WebCore/page/animation/AnimationBase.h
Source/WebCore/page/animation/CSSPropertyAnimation.cpp
Source/WebCore/page/animation/CompositeAnimation.cpp
Source/WebCore/page/animation/ImplicitAnimation.h
Source/WebCore/page/animation/KeyframeAnimation.h

index b90f0d5..c988977 100644 (file)
@@ -1,3 +1,13 @@
+2017-09-26  Antti Koivisto  <antti@apple.com>
+
+        REGRESSION (222040): Google Maps Street View CrashTracer: [USER] com.apple.WebKit.WebContent.Development at com.apple.WebCore: WebCore::PropertyWrapperAcceleratedTransform::blend const + 92
+        https://bugs.webkit.org/show_bug.cgi?id=177485
+
+        Reviewed by Zalan Bujtas.
+
+        * fast/animation/animation-mixed-transform-crash-expected.html: Added.
+        * fast/animation/animation-mixed-transform-crash.html: Added.
+
 2017-09-26  Ryan Haddad  <ryanhaddad@apple.com>
 
         Mark compositing/masks/compositing-clip-path-change-no-repaint.html as flaky.
diff --git a/LayoutTests/fast/animation/animation-mixed-transform-crash-expected.html b/LayoutTests/fast/animation/animation-mixed-transform-crash-expected.html
new file mode 100644 (file)
index 0000000..206df8b
--- /dev/null
@@ -0,0 +1,4 @@
+<style>
+div { transform: rotate(-90deg); will-change: transform }
+</style>
+<div>This should animate sideways</div>
diff --git a/LayoutTests/fast/animation/animation-mixed-transform-crash.html b/LayoutTests/fast/animation/animation-mixed-transform-crash.html
new file mode 100644 (file)
index 0000000..b2626e7
--- /dev/null
@@ -0,0 +1,12 @@
+<script>
+if (window.testRunner)
+    testRunner.waitUntilDone();
+</script>
+<style>
+@keyframes frames {
+    from { transform: translate(-10px, -10px)  }
+    to { transform: rotate(-90deg)  }
+}
+div { animation: frames 0.1s forwards }
+</style>
+<div onanimationend="if (window.testRunner) testRunner.notifyDone()">This should animate sideways</div>
index 2d058af..0626606 100644 (file)
@@ -1,3 +1,35 @@
+2017-09-26  Antti Koivisto  <antti@apple.com>
+
+        REGRESSION (222040): Google Maps Street View CrashTracer: [USER] com.apple.WebKit.WebContent.Development at com.apple.WebCore: WebCore::PropertyWrapperAcceleratedTransform::blend const + 92
+        https://bugs.webkit.org/show_bug.cgi?id=177485
+
+        Reviewed by Zalan Bujtas.
+
+        We crash when animating between two different types of transforms because renderer is null for the first frame.
+
+        Test: fast/animation/animation-mixed-transform-crash.html
+
+        * page/animation/AnimationBase.cpp:
+        (WebCore::AnimationBase::currentStyle const):
+
+            Add a way to get the current style from animations.
+            This is either the render style or the inital style.
+
+        * page/animation/AnimationBase.h:
+        * page/animation/CSSPropertyAnimation.cpp:
+        (WebCore::blendFunc):
+
+            Renderer may be null when computing the first frame of the animation. Null check.
+
+        (WebCore::blendFilter):
+
+            Here too.
+
+        * page/animation/CompositeAnimation.cpp:
+        (WebCore::CompositeAnimation::updateTransitions):
+        * page/animation/ImplicitAnimation.h:
+        * page/animation/KeyframeAnimation.h:
+
 2017-09-26  Zan Dobersek  <zdobersek@igalia.com>
 
         [EME] Add ClearKey support for persistent session data load and removal
index 0eae5a2..242228b 100644 (file)
@@ -90,6 +90,13 @@ AnimationBase::~AnimationBase()
 {
 }
 
+const RenderStyle& AnimationBase::currentStyle() const
+{
+    if (auto* renderer = this->renderer())
+        return renderer->style();
+    return unanimatedStyle();
+}
+
 RenderElement* AnimationBase::renderer() const
 {
     return m_element ? m_element->renderer() : nullptr;
index cf49531..1442ddf 100644 (file)
@@ -52,6 +52,7 @@ public:
     virtual ~AnimationBase();
 
     Element* element() const { return m_element.get(); }
+    const RenderStyle& currentStyle() const;
     RenderElement* renderer() const;
     RenderBoxModelObject* compositedRenderer() const;
     void clear();
@@ -221,6 +222,8 @@ protected:
     virtual void pauseAnimation(double /*timeOffset*/) { }
     virtual void endAnimation() { }
 
+    virtual const RenderStyle& unanimatedStyle() const = 0;
+
     void goIntoEndingOrLoopingState();
 
     AnimationState state() const { return m_animationState; }
index 7d09735..67d4b87 100644 (file)
@@ -124,7 +124,7 @@ static inline TransformOperations blendFunc(const AnimationBase* animation, cons
 {
     if (animation->transformFunctionListsMatch())
         return to.blendByMatchingOperations(from, progress);
-    return to.blendByUsingMatrixInterpolation(from, progress, is<RenderBox>(*animation->renderer()) ? downcast<RenderBox>(*animation->renderer()).borderBoxRect().size() : LayoutSize());
+    return to.blendByUsingMatrixInterpolation(from, progress, is<RenderBox>(animation->renderer()) ? downcast<RenderBox>(*animation->renderer()).borderBoxRect().size() : LayoutSize());
 }
 
 static inline RefPtr<ClipPathOperation> blendFunc(const AnimationBase*, ClipPathOperation* from, ClipPathOperation* to, double progress)
@@ -222,7 +222,7 @@ static inline RefPtr<StyleImage> blendFilter(const AnimationBase* anim, CachedIm
     FilterOperations filterResult = blendFilterOperations(anim, from, to, progress);
 
     auto imageValue = CSSImageValue::create(*image);
-    auto filterValue = ComputedStyleExtractor::valueForFilter(anim->renderer()->style(), filterResult, DoNotAdjustPixelValues);
+    auto filterValue = ComputedStyleExtractor::valueForFilter(anim->currentStyle(), filterResult, DoNotAdjustPixelValues);
 
     auto result = CSSFilterImageValue::create(WTFMove(imageValue), WTFMove(filterValue));
     result.get().setFilterOperations(filterResult);
@@ -361,8 +361,10 @@ static inline NinePieceImage blendFunc(const AnimationBase* anim, const NinePiec
     if (from.imageSlices() != to.imageSlices() || from.borderSlices() != to.borderSlices() || from.outset() != to.outset() || from.fill() != to.fill() || from.horizontalRule() != to.horizontalRule() || from.verticalRule() != to.verticalRule())
         return to;
 
-    if (from.image()->imageSize(anim->renderer(), 1.0) != to.image()->imageSize(anim->renderer(), 1.0))
-        return to;
+    if (auto* renderer = anim->renderer()) {
+        if (from.image()->imageSize(renderer, 1.0) != to.image()->imageSize(renderer, 1.0))
+            return to;
+    }
 
     return NinePieceImage(blendFunc(anim, from.image(), to.image(), progress),
         from.imageSlices(), from.fill(), from.borderSlices(), from.outset(), from.horizontalRule(), from.verticalRule());
index e5e2c11..676fc68 100644 (file)
@@ -122,7 +122,7 @@ void CompositeAnimation::updateTransitions(Element& element, const RenderStyle*
                 // and we have to use the unanimatedStyle from the animation. We do the test
                 // against the unanimated style here, but we "override" the transition later.
                 auto* keyframeAnimation = animationForProperty(prop);
-                auto* fromStyle = keyframeAnimation ? keyframeAnimation->unanimatedStyle() : currentStyle;
+                auto* fromStyle = keyframeAnimation ? &keyframeAnimation->unanimatedStyle() : currentStyle;
 
                 // See if there is a current transition for this prop
                 ImplicitAnimation* implAnim = m_transitions.get(prop);
index a419d49..b28115c 100644 (file)
@@ -75,6 +75,8 @@ public:
     bool active() const { return m_active; }
     void setActive(bool b) { m_active = b; }
 
+    const RenderStyle& unanimatedStyle() const override { return *m_fromStyle; }
+
 protected:
     bool shouldSendEventForListener(Document::ListenerType) const;    
     bool sendTransitionEvent(const AtomicString&, double elapsedTime);
index e3e4ee1..cb54f7f 100644 (file)
@@ -59,7 +59,7 @@ public:
     bool dependsOnLayout() const { return m_dependsOnLayout; }
 
     void setUnanimatedStyle(std::unique_ptr<RenderStyle> style) { m_unanimatedStyle = WTFMove(style); }
-    RenderStyle* unanimatedStyle() const { return m_unanimatedStyle.get(); }
+    const RenderStyle& unanimatedStyle() const override { return *m_unanimatedStyle; }
 
     std::optional<Seconds> timeToNextService() override;