REGRESSION (214503): Webkit crash under RenderElement::repaintForPausedImageAnimation...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 24 Apr 2017 22:46:11 +0000 (22:46 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 24 Apr 2017 22:46:11 +0000 (22:46 +0000)
https://bugs.webkit.org/show_bug.cgi?id=171243
<rdar://problem/31715572>

Reviewed by Antti Koivisto.

Source/WebCore:

After r214503, we would frequently crash when scrolling giphy pages because we were failing to call
RenderView::removeRendererWithPausedImageAnimations(renderer, cachedImage) in some cases. This would
cause a RenderElement to still be associated to a CachedImage in RenderView but not in practice.
If the CachedImage then gets destroyed and the user scrolls, we end up calling
RenderElement::repaintForPausedImageAnimationsIfNeeded() with a bad CachedImage.

StyleCachedImage was properly calling RenderView::removeRendererWithPausedImageAnimations() before
unregistering the renderer as a client to the CachedImage. However, RenderImageResource was failing
to do the same. To make this less error-prone, I added a didRemoveCachedImageClient(CachedImage&)
function to the CachedImageClient interface. It is overriden in RenderElement only to call
RenderView::removeRendererWithPausedImageAnimations().

Test: fast/images/animated-gif-scrolling-crash.html

* loader/cache/CachedImage.cpp:
(WebCore::CachedImage::didRemoveClient):
* loader/cache/CachedImageClient.h:
(WebCore::CachedImageClient::didRemoveCachedImageClient):
* rendering/RenderElement.cpp:
(WebCore::RenderElement::didRemoveCachedImageClient):
* rendering/RenderElement.h:
* rendering/style/StyleCachedImage.cpp:
(WebCore::StyleCachedImage::removeClient):

LayoutTests:

Add layout test coverage.

* fast/images/animated-gif-scrolling-crash-expected.txt: Added.
* fast/images/animated-gif-scrolling-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/images/animated-gif-scrolling-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/images/animated-gif-scrolling-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/loader/cache/CachedImage.cpp
Source/WebCore/loader/cache/CachedImageClient.h
Source/WebCore/rendering/RenderElement.cpp
Source/WebCore/rendering/RenderElement.h
Source/WebCore/rendering/style/StyleCachedImage.cpp

index 8828de5..6515a39 100644 (file)
@@ -1,3 +1,16 @@
+2017-04-24  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION (214503): Webkit crash under RenderElement::repaintForPausedImageAnimationsIfNeeded() when scrolling giphy pages
+        https://bugs.webkit.org/show_bug.cgi?id=171243
+        <rdar://problem/31715572>
+
+        Reviewed by Antti Koivisto.
+
+        Add layout test coverage.
+
+        * fast/images/animated-gif-scrolling-crash-expected.txt: Added.
+        * fast/images/animated-gif-scrolling-crash.html: Added.
+
 2017-04-24  Saam Barati  <sbarati@apple.com>
 
         [mac debug] LayoutTest workers/wasm-long-compile-many.html is a flaky timeout
diff --git a/LayoutTests/fast/images/animated-gif-scrolling-crash-expected.txt b/LayoutTests/fast/images/animated-gif-scrolling-crash-expected.txt
new file mode 100644 (file)
index 0000000..6ab4504
--- /dev/null
@@ -0,0 +1,16 @@
+Tests that we do not crash when scrolling after a paused animated GIF's CachedImage gets destroyed.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+Initially outside the viewport
+PASS internals.isImageAnimating(image) became false
+Scrolling animation into view
+PASS internals.isImageAnimating(image) became true
+Scrolling animation out of view
+PASS internals.isImageAnimating(image) became false
+Scrolling back down after image removal
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/images/animated-gif-scrolling-crash.html b/LayoutTests/fast/images/animated-gif-scrolling-crash.html
new file mode 100644 (file)
index 0000000..dea98b0
--- /dev/null
@@ -0,0 +1,40 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../resources/js-test-pre.js"></script>
+<script>
+description("Tests that we do not crash when scrolling after a paused animated GIF's CachedImage gets destroyed.");
+jsTestIsAsync = true;
+
+onload = function() {
+    image = document.querySelector("img");
+
+    debug("Initially outside the viewport");
+    shouldBecomeEqual("internals.isImageAnimating(image)", "false", function() {
+        debug("Scrolling animation into view");
+        internals.scrollElementToRect(image, 0, 0, 300, 300);
+        shouldBecomeEqual("internals.isImageAnimating(image)", "true", function() {
+            debug("Scrolling animation out of view");
+            scroll(0, 0);
+            shouldBecomeEqual("internals.isImageAnimating(image)", "false", function() {
+                image.src = "resources/animated2.gif";
+                gc();
+                internals.clearMemoryCache();
+
+                setTimeout(function() {
+                    gc();
+                    debug("Scrolling back down after image removal");
+                    scroll(500, 500);
+                    setTimeout(finishJSTest, 30);
+            }, 0);
+            });
+        });
+    });
+}
+</script>
+<div style="position: relative; width: 1600px; height: 2400px;">
+    <img src="resources/animated.gif" style="position:absolute; left: 600px; top: 800px;">
+</div>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
index 99288b2..7421f13 100644 (file)
@@ -1,3 +1,35 @@
+2017-04-24  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION (214503): Webkit crash under RenderElement::repaintForPausedImageAnimationsIfNeeded() when scrolling giphy pages
+        https://bugs.webkit.org/show_bug.cgi?id=171243
+        <rdar://problem/31715572>
+
+        Reviewed by Antti Koivisto.
+
+        After r214503, we would frequently crash when scrolling giphy pages because we were failing to call
+        RenderView::removeRendererWithPausedImageAnimations(renderer, cachedImage) in some cases. This would
+        cause a RenderElement to still be associated to a CachedImage in RenderView but not in practice.
+        If the CachedImage then gets destroyed and the user scrolls, we end up calling
+        RenderElement::repaintForPausedImageAnimationsIfNeeded() with a bad CachedImage.
+
+        StyleCachedImage was properly calling RenderView::removeRendererWithPausedImageAnimations() before
+        unregistering the renderer as a client to the CachedImage. However, RenderImageResource was failing
+        to do the same. To make this less error-prone, I added a didRemoveCachedImageClient(CachedImage&)
+        function to the CachedImageClient interface. It is overriden in RenderElement only to call
+        RenderView::removeRendererWithPausedImageAnimations().
+
+        Test: fast/images/animated-gif-scrolling-crash.html
+
+        * loader/cache/CachedImage.cpp:
+        (WebCore::CachedImage::didRemoveClient):
+        * loader/cache/CachedImageClient.h:
+        (WebCore::CachedImageClient::didRemoveCachedImageClient):
+        * rendering/RenderElement.cpp:
+        (WebCore::RenderElement::didRemoveCachedImageClient):
+        * rendering/RenderElement.h:
+        * rendering/style/StyleCachedImage.cpp:
+        (WebCore::StyleCachedImage::removeClient):
+
 2017-04-24  Andy Estes  <aestes@apple.com>
 
         [macOS] Fix two minor issues with MediaSelectionOption::Type
index 9d11f14..c5bea5e 100644 (file)
@@ -133,6 +133,8 @@ void CachedImage::didRemoveClient(CachedResourceClient& client)
         m_svgImageCache->removeClientFromCache(&static_cast<CachedImageClient&>(client));
 
     CachedResource::didRemoveClient(client);
+
+    static_cast<CachedImageClient&>(client).didRemoveCachedImageClient(*this);
 }
 
 void CachedImage::switchClientsToRevalidatedResource()
index 5c46ff2..159a74b 100644 (file)
@@ -41,6 +41,8 @@ public:
 
     // Called when GIF animation progresses.
     virtual void newImageAnimationFrameAvailable(CachedImage& image, bool& canPause) { imageChanged(&image); canPause = true; }
+
+    virtual void didRemoveCachedImageClient(CachedImage&) { }
 };
 
 } // namespace WebCore
index 05e442e..4a98e2f 100644 (file)
@@ -1506,6 +1506,12 @@ void RenderElement::newImageAnimationFrameAvailable(CachedImage& image, bool& ca
     imageChanged(&image);
 }
 
+void RenderElement::didRemoveCachedImageClient(CachedImage& cachedImage)
+{
+    if (hasPausedImageAnimations())
+        view().removeRendererWithPausedImageAnimations(*this, cachedImage);
+}
+
 bool RenderElement::repaintForPausedImageAnimationsIfNeeded(const IntRect& visibleRect, CachedImage& cachedImage)
 {
     ASSERT(m_hasPausedImageAnimations);
index d7163c5..bd33dd9 100644 (file)
@@ -319,6 +319,7 @@ private:
     void invalidateCachedFirstLineStyle();
 
     void newImageAnimationFrameAvailable(CachedImage&, bool& canPause) final;
+    void didRemoveCachedImageClient(CachedImage&) final;
 
     bool getLeadingCorner(FloatPoint& output, bool& insideFixed) const;
     bool getTrailingCorner(FloatPoint& output, bool& insideFixed) const;
index 94aa5ec..9428c4e 100644 (file)
@@ -188,9 +188,6 @@ void StyleCachedImage::removeClient(RenderElement* renderer)
         return;
     ASSERT(renderer);
 
-    if (renderer->hasPausedImageAnimations())
-        renderer->view().removeRendererWithPausedImageAnimations(*renderer, *m_cachedImage);
-
     m_cachedImage->removeClient(*renderer);
 }