REGRESSION (r208025) GraphicsContext state stack assertions loading webkit.org
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 3 Nov 2016 00:19:54 +0000 (00:19 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 3 Nov 2016 00:19:54 +0000 (00:19 +0000)
https://bugs.webkit.org/show_bug.cgi?id=164350
rdar://problem/29053414

Reviewed by Dean Jackson.

Source/WebCore:

After r208025 it as possible for KeyframeAnimation::animate() to produce a RenderStyle
with a non-1 opacity, but without the explicit z-index that triggers stacking context.
This confused the RenderLayer paintWithTransparency code, triggering mismsatched GraphicsContext
save/restores.

This occurred when the runningOrFillingForwards state was mis-computed. keyframeAnim->animate()
can spit out a new style when in the StartWaitTimer sometimes, so "!keyframeAnim->waitingToStart() && !keyframeAnim->postActive()"
gave the wrong answser.

Rather than depend on the super-confusing animation state, use a bool out param from animate() to say
when it actually produced a new style, and when true, do the setZIndex(0).

Test: animations/stacking-during-opacity-animation.html

* page/animation/AnimationBase.h:
* page/animation/CSSPropertyAnimation.cpp:
(WebCore::CSSPropertyAnimation::blendProperties): Log after blending so the log shows the blended style.
* page/animation/CompositeAnimation.cpp:
(WebCore::CompositeAnimation::animate):
* page/animation/ImplicitAnimation.cpp:
(WebCore::ImplicitAnimation::animate):
* page/animation/ImplicitAnimation.h:
* page/animation/KeyframeAnimation.cpp:
(WebCore::KeyframeAnimation::animate):
* page/animation/KeyframeAnimation.h:
* platform/graphics/GraphicsContext.cpp:
(WebCore::GraphicsContext::restore):
* platform/graphics/ca/cocoa/PlatformCALayerCocoa.mm:
(PlatformCALayer::drawLayerContents): No functional change, but created scope for the
GraphicsContext so that it didn't outlive the CGContextRestoreGState(context).
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::beginTransparencyLayers): New assertion that catches the problem earlier.

LayoutTests:

Test was reduced from webkit.org.

* animations/stacking-during-opacity-animation-expected.txt: Added.
* animations/stacking-during-opacity-animation.html: Added.

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

14 files changed:
LayoutTests/ChangeLog
LayoutTests/animations/stacking-during-opacity-animation-expected.txt [new file with mode: 0644]
LayoutTests/animations/stacking-during-opacity-animation.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/animation/AnimationBase.h
Source/WebCore/page/animation/CSSPropertyAnimation.cpp
Source/WebCore/page/animation/CompositeAnimation.cpp
Source/WebCore/page/animation/ImplicitAnimation.cpp
Source/WebCore/page/animation/ImplicitAnimation.h
Source/WebCore/page/animation/KeyframeAnimation.cpp
Source/WebCore/page/animation/KeyframeAnimation.h
Source/WebCore/platform/graphics/GraphicsContext.cpp
Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.mm
Source/WebCore/rendering/RenderLayer.cpp

index 88d0646..6da4030 100644 (file)
@@ -1,3 +1,16 @@
+2016-11-02  Simon Fraser  <simon.fraser@apple.com>
+
+        REGRESSION (r208025) GraphicsContext state stack assertions loading webkit.org
+        https://bugs.webkit.org/show_bug.cgi?id=164350
+        rdar://problem/29053414
+
+        Reviewed by Dean Jackson.
+
+        Test was reduced from webkit.org.
+
+        * animations/stacking-during-opacity-animation-expected.txt: Added.
+        * animations/stacking-during-opacity-animation.html: Added.
+
 2016-11-02  Myles C. Maxfield  <mmaxfield@apple.com>
 
         [iOS] [WebGL] Multisample resolve step may operate on stale data
diff --git a/LayoutTests/animations/stacking-during-opacity-animation-expected.txt b/LayoutTests/animations/stacking-during-opacity-animation-expected.txt
new file mode 100644 (file)
index 0000000..b769542
--- /dev/null
@@ -0,0 +1 @@
+This test should not crash or assert.
diff --git a/LayoutTests/animations/stacking-during-opacity-animation.html b/LayoutTests/animations/stacking-during-opacity-animation.html
new file mode 100644 (file)
index 0000000..5e5f3a3
--- /dev/null
@@ -0,0 +1,34 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+    .background-image {
+        position: relative;
+        margin-top: -1px;
+        height: 0;
+        padding-bottom: 80%;
+        opacity: 0.1;
+    }
+
+    .tile {
+        animation: fade-in 0.4s;
+    }
+
+    @keyframes fade-in {
+        from { opacity: 0; }
+        to   { opacity: 1; }
+    }
+
+</style>
+<script>
+    if (window.testRunner)
+        testRunner.dumpAsText();
+</script>
+</head>
+<body>
+    <div class="tile">
+        <div class="background-image"></div>
+        <p>This test should not crash or assert.</p>
+    </div>
+</body>
+</html>
index 34f1ddd..aa74378 100644 (file)
@@ -1,3 +1,44 @@
+2016-11-02  Simon Fraser  <simon.fraser@apple.com>
+
+        REGRESSION (r208025) GraphicsContext state stack assertions loading webkit.org
+        https://bugs.webkit.org/show_bug.cgi?id=164350
+        rdar://problem/29053414
+
+        Reviewed by Dean Jackson.
+
+        After r208025 it as possible for KeyframeAnimation::animate() to produce a RenderStyle
+        with a non-1 opacity, but without the explicit z-index that triggers stacking context.
+        This confused the RenderLayer paintWithTransparency code, triggering mismsatched GraphicsContext
+        save/restores.
+
+        This occurred when the runningOrFillingForwards state was mis-computed. keyframeAnim->animate()
+        can spit out a new style when in the StartWaitTimer sometimes, so "!keyframeAnim->waitingToStart() && !keyframeAnim->postActive()"
+        gave the wrong answser.
+
+        Rather than depend on the super-confusing animation state, use a bool out param from animate() to say
+        when it actually produced a new style, and when true, do the setZIndex(0).
+
+        Test: animations/stacking-during-opacity-animation.html
+
+        * page/animation/AnimationBase.h:
+        * page/animation/CSSPropertyAnimation.cpp:
+        (WebCore::CSSPropertyAnimation::blendProperties): Log after blending so the log shows the blended style.
+        * page/animation/CompositeAnimation.cpp:
+        (WebCore::CompositeAnimation::animate):
+        * page/animation/ImplicitAnimation.cpp:
+        (WebCore::ImplicitAnimation::animate):
+        * page/animation/ImplicitAnimation.h:
+        * page/animation/KeyframeAnimation.cpp:
+        (WebCore::KeyframeAnimation::animate):
+        * page/animation/KeyframeAnimation.h:
+        * platform/graphics/GraphicsContext.cpp:
+        (WebCore::GraphicsContext::restore):
+        * platform/graphics/ca/cocoa/PlatformCALayerCocoa.mm:
+        (PlatformCALayer::drawLayerContents): No functional change, but created scope for the
+        GraphicsContext so that it didn't outlive the CGContextRestoreGState(context).
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::beginTransparencyLayers): New assertion that catches the problem earlier.
+
 2016-11-02  Myles C. Maxfield  <mmaxfield@apple.com>
 
         [iOS] [WebGL] Multisample resolve step may operate on stale data
index a74f01e..d709531 100644 (file)
@@ -136,7 +136,7 @@ public:
     double progress(double scale = 1, double offset = 0, const TimingFunction* = nullptr) const;
 
     // Returns true if the animation state changed.
-    virtual bool animate(CompositeAnimation*, RenderElement*, const RenderStyle* /*currentStyle*/, const RenderStyle* /*targetStyle*/, std::unique_ptr<RenderStyle>& /*animatedStyle*/) = 0;
+    virtual bool animate(CompositeAnimation*, RenderElement*, const RenderStyle* /*currentStyle*/, const RenderStyle* /*targetStyle*/, std::unique_ptr<RenderStyle>& /*animatedStyle*/, bool& didBlendStyle) = 0;
     virtual void getAnimatedStyle(std::unique_ptr<RenderStyle>& /*animatedStyle*/) = 0;
 
     virtual bool computeExtentOfTransformAnimation(LayoutRect&) const = 0;
index 1fda298..2b9df46 100644 (file)
@@ -1576,10 +1576,10 @@ bool CSSPropertyAnimation::blendProperties(const AnimationBase* anim, CSSPropert
 
     AnimationPropertyWrapperBase* wrapper = CSSPropertyAnimationWrapperMap::singleton().wrapperForProperty(prop);
     if (wrapper) {
+        wrapper->blend(anim, dst, a, b, progress);
 #if !LOG_DISABLED
         wrapper->logBlend(a, b, dst, progress);
 #endif
-        wrapper->blend(anim, dst, a, b, progress);
         return !wrapper->animationIsAccelerated() || !anim->isAccelerated();
     }
     return false;
index b203d3c..7c0fea5 100644 (file)
@@ -299,10 +299,12 @@ bool CompositeAnimation::animate(RenderElement& renderer, const RenderStyle* cur
         // to fill in a RenderStyle*& only if needed.
         bool checkForStackingContext = false;
         for (auto& transition : m_transitions.values()) {
-            if (transition->animate(this, &renderer, currentStyle, &targetStyle, blendedStyle))
+            bool didBlendStyle = false;
+            if (transition->animate(this, &renderer, currentStyle, &targetStyle, blendedStyle, didBlendStyle))
                 animationStateChanged = true;
 
-            checkForStackingContext |= WillChangeData::propertyCreatesStackingContext(transition->animatingProperty());
+            if (didBlendStyle)
+                checkForStackingContext |= WillChangeData::propertyCreatesStackingContext(transition->animatingProperty());
         }
 
         if (blendedStyle && checkForStackingContext) {
@@ -327,11 +329,11 @@ bool CompositeAnimation::animate(RenderElement& renderer, const RenderStyle* cur
     for (auto& name : m_keyframeAnimationOrderMap) {
         RefPtr<KeyframeAnimation> keyframeAnim = m_keyframeAnimations.get(name);
         if (keyframeAnim) {
-            if (keyframeAnim->animate(this, &renderer, currentStyle, &targetStyle, blendedStyle))
+            bool didBlendStyle = false;
+            if (keyframeAnim->animate(this, &renderer, currentStyle, &targetStyle, blendedStyle, didBlendStyle))
                 animationStateChanged = true;
 
-            bool runningOrFillingForwards = !keyframeAnim->waitingToStart() && !keyframeAnim->postActive();
-            forceStackingContext |= runningOrFillingForwards && keyframeAnim->triggersStackingContext();
+            forceStackingContext |= didBlendStyle && keyframeAnim->triggersStackingContext();
         }
     }
 
index 16bd7d4..716bea8 100644 (file)
@@ -61,7 +61,7 @@ bool ImplicitAnimation::shouldSendEventForListener(Document::ListenerType inList
     return m_object->document().hasListenerType(inListenerType);
 }
 
-bool ImplicitAnimation::animate(CompositeAnimation*, RenderElement*, const RenderStyle*, const RenderStyle* targetStyle, std::unique_ptr<RenderStyle>& animatedStyle)
+bool ImplicitAnimation::animate(CompositeAnimation*, RenderElement*, const RenderStyle*, const RenderStyle* targetStyle, std::unique_ptr<RenderStyle>& animatedStyle, bool& didBlendStyle)
 {
     // If we get this far and the animation is done, it means we are cleaning up a just finished animation.
     // So just return. Everything is already all cleaned up.
@@ -85,6 +85,8 @@ bool ImplicitAnimation::animate(CompositeAnimation*, RenderElement*, const Rende
 
     // Fire the start timeout if needed
     fireAnimationEventsIfNeeded();
+    
+    didBlendStyle = true;
     return state() != oldState;
 }
 
index 7f38d8d..68c407a 100644 (file)
@@ -54,7 +54,7 @@ public:
     void pauseAnimation(double timeOffset) override;
     void endAnimation() override;
 
-    bool animate(CompositeAnimation*, RenderElement*, const RenderStyle* currentStyle, const RenderStyle* targetStyle, std::unique_ptr<RenderStyle>& animatedStyle) override;
+    bool animate(CompositeAnimation*, RenderElement*, const RenderStyle* currentStyle, const RenderStyle* targetStyle, std::unique_ptr<RenderStyle>& animatedStyle, bool& didBlendStyle) override;
     void getAnimatedStyle(std::unique_ptr<RenderStyle>& animatedStyle) override;
     void reset(const RenderStyle* to);
 
index ec78be7..d314c3b 100644 (file)
@@ -124,7 +124,7 @@ void KeyframeAnimation::fetchIntervalEndpointsForProperty(CSSPropertyID property
     prog = progress(scale, offset, prevKeyframe.timingFunction(name()));
 }
 
-bool KeyframeAnimation::animate(CompositeAnimation* compositeAnimation, RenderElement*, const RenderStyle*, const RenderStyle* targetStyle, std::unique_ptr<RenderStyle>& animatedStyle)
+bool KeyframeAnimation::animate(CompositeAnimation* compositeAnimation, RenderElement*, const RenderStyle*, const RenderStyle* targetStyle, std::unique_ptr<RenderStyle>& animatedStyle, bool& didBlendStyle)
 {
     // Fire the start timeout if needed
     fireAnimationEventsIfNeeded();
@@ -179,6 +179,7 @@ bool KeyframeAnimation::animate(CompositeAnimation* compositeAnimation, RenderEl
         CSSPropertyAnimation::blendProperties(this, propertyID, animatedStyle.get(), fromStyle, toStyle, progress);
     }
     
+    didBlendStyle = true;
     return state() != oldState;
 }
 
index 499c027..f486311 100644 (file)
@@ -45,7 +45,7 @@ public:
         return adoptRef(*new KeyframeAnimation(animation, renderer, compositeAnimation, unanimatedStyle));
     }
 
-    bool animate(CompositeAnimation*, RenderElement*, const RenderStyle* currentStyle, const RenderStyle* targetStyle, std::unique_ptr<RenderStyle>& animatedStyle) override;
+    bool animate(CompositeAnimation*, RenderElement*, const RenderStyle* currentStyle, const RenderStyle* targetStyle, std::unique_ptr<RenderStyle>& animatedStyle, bool& didBlendStyle) override;
     void getAnimatedStyle(std::unique_ptr<RenderStyle>&) override;
 
     bool computeExtentOfTransformAnimation(LayoutRect&) const override;
index 88b7f5b..2099a1d 100644 (file)
@@ -367,6 +367,7 @@ void GraphicsContext::restore()
         LOG_ERROR("ERROR void GraphicsContext::restore() stack is empty");
         return;
     }
+
     m_state = m_stack.last();
     m_stack.removeLast();
 
index 7c78e44..a31f65b 100644 (file)
@@ -1067,46 +1067,48 @@ void PlatformCALayer::drawLayerContents(CGContextRef context, WebCore::PlatformC
     [NSGraphicsContext setCurrentContext:layerContext];
 #endif
     
-    GraphicsContext graphicsContext(context);
-    graphicsContext.setIsCALayerContext(true);
-    graphicsContext.setIsAcceleratedContext(platformCALayer->acceleratesDrawing());
-    
-    if (!layerContents->platformCALayerContentsOpaque()) {
-        // Turn off font smoothing to improve the appearance of text rendered onto a transparent background.
-        graphicsContext.setShouldSmoothFonts(false);
-    }
-    
+    {
+        GraphicsContext graphicsContext(context);
+        graphicsContext.setIsCALayerContext(true);
+        graphicsContext.setIsAcceleratedContext(platformCALayer->acceleratesDrawing());
+        
+        if (!layerContents->platformCALayerContentsOpaque()) {
+            // Turn off font smoothing to improve the appearance of text rendered onto a transparent background.
+            graphicsContext.setShouldSmoothFonts(false);
+        }
+        
 #if PLATFORM(MAC)
-    // It's important to get the clip from the context, because it may be significantly
-    // smaller than the layer bounds (e.g. tiled layers)
-    ThemeMac::setFocusRingClipRect(CGContextGetClipBoundingBox(context));
+        // It's important to get the clip from the context, because it may be significantly
+        // smaller than the layer bounds (e.g. tiled layers)
+        ThemeMac::setFocusRingClipRect(CGContextGetClipBoundingBox(context));
 #endif
-    
-    for (const auto& rect : dirtyRects) {
-        GraphicsContextStateSaver stateSaver(graphicsContext);
-        graphicsContext.clip(rect);
         
-        layerContents->platformCALayerPaintContents(platformCALayer, graphicsContext, rect);
-    }
-    
+        for (const auto& rect : dirtyRects) {
+            GraphicsContextStateSaver stateSaver(graphicsContext);
+            graphicsContext.clip(rect);
+            
+            layerContents->platformCALayerPaintContents(platformCALayer, graphicsContext, rect);
+        }
+        
 #if PLATFORM(IOS)
-    fontAntialiasingState.restore();
+        fontAntialiasingState.restore();
 #else
-    ThemeMac::setFocusRingClipRect(FloatRect());
-    
-    [NSGraphicsContext restoreGraphicsState];
+        ThemeMac::setFocusRingClipRect(FloatRect());
+        
+        [NSGraphicsContext restoreGraphicsState];
 #endif
-    
+    }
+
+    CGContextRestoreGState(context);
+
     // Re-fetch the layer owner, since <rdar://problem/9125151> indicates that it might have been destroyed during painting.
     layerContents = platformCALayer->owner();
     ASSERT(layerContents);
     
-    CGContextRestoreGState(context);
-    
     // Always update the repaint count so that it's accurate even if the count itself is not shown. This will be useful
     // for the Web Inspector feeding this information through the LayerTreeAgent.
     int repaintCount = layerContents->platformCALayerIncrementRepaintCount(platformCALayer);
-    
+
     if (!platformCALayer->usesTiledBackingLayer() && layerContents && layerContents->platformCALayerShowRepaintCounter(platformCALayer))
         drawRepaintIndicator(context, platformCALayer, repaintCount, nullptr);
 }
index 2595c34..01a2bab 100644 (file)
@@ -1809,6 +1809,7 @@ void RenderLayer::beginTransparencyLayers(GraphicsContext& context, const LayerP
         ancestor->beginTransparencyLayers(context, paintingInfo, dirtyRect);
     
     if (paintsWithTransparency(paintingInfo.paintBehavior)) {
+        ASSERT(isStackingContext());
         m_usedTransparency = true;
         context.save();
         LayoutRect adjustedClipRect = paintingExtent(*this, paintingInfo.rootLayer, dirtyRect, paintingInfo.paintBehavior);