- Revision
- 215700
- Author
- cdu...@apple.com
- Date
- 2017-04-24 15:46:11 -0700 (Mon, 24 Apr 2017)
Log Message
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.
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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (215699 => 215700)
--- trunk/LayoutTests/ChangeLog 2017-04-24 22:43:22 UTC (rev 215699)
+++ trunk/LayoutTests/ChangeLog 2017-04-24 22:46:11 UTC (rev 215700)
@@ -1,3 +1,16 @@
+2017-04-24 Chris Dumez <cdu...@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 <sbar...@apple.com>
[mac debug] LayoutTest workers/wasm-long-compile-many.html is a flaky timeout
Added: trunk/LayoutTests/fast/images/animated-gif-scrolling-crash-expected.txt (0 => 215700)
--- trunk/LayoutTests/fast/images/animated-gif-scrolling-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/images/animated-gif-scrolling-crash-expected.txt 2017-04-24 22:46:11 UTC (rev 215700)
@@ -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
+
Added: trunk/LayoutTests/fast/images/animated-gif-scrolling-crash.html (0 => 215700)
--- trunk/LayoutTests/fast/images/animated-gif-scrolling-crash.html (rev 0)
+++ trunk/LayoutTests/fast/images/animated-gif-scrolling-crash.html 2017-04-24 22:46:11 UTC (rev 215700)
@@ -0,0 +1,40 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src=""
+<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 = ""
+ 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="" style="position:absolute; left: 600px; top: 800px;">
+</div>
+<script src=""
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (215699 => 215700)
--- trunk/Source/WebCore/ChangeLog 2017-04-24 22:43:22 UTC (rev 215699)
+++ trunk/Source/WebCore/ChangeLog 2017-04-24 22:46:11 UTC (rev 215700)
@@ -1,3 +1,35 @@
+2017-04-24 Chris Dumez <cdu...@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 <aes...@apple.com>
[macOS] Fix two minor issues with MediaSelectionOption::Type
Modified: trunk/Source/WebCore/loader/cache/CachedImage.cpp (215699 => 215700)
--- trunk/Source/WebCore/loader/cache/CachedImage.cpp 2017-04-24 22:43:22 UTC (rev 215699)
+++ trunk/Source/WebCore/loader/cache/CachedImage.cpp 2017-04-24 22:46:11 UTC (rev 215700)
@@ -133,6 +133,8 @@
m_svgImageCache->removeClientFromCache(&static_cast<CachedImageClient&>(client));
CachedResource::didRemoveClient(client);
+
+ static_cast<CachedImageClient&>(client).didRemoveCachedImageClient(*this);
}
void CachedImage::switchClientsToRevalidatedResource()
Modified: trunk/Source/WebCore/loader/cache/CachedImageClient.h (215699 => 215700)
--- trunk/Source/WebCore/loader/cache/CachedImageClient.h 2017-04-24 22:43:22 UTC (rev 215699)
+++ trunk/Source/WebCore/loader/cache/CachedImageClient.h 2017-04-24 22:46:11 UTC (rev 215700)
@@ -41,6 +41,8 @@
// Called when GIF animation progresses.
virtual void newImageAnimationFrameAvailable(CachedImage& image, bool& canPause) { imageChanged(&image); canPause = true; }
+
+ virtual void didRemoveCachedImageClient(CachedImage&) { }
};
} // namespace WebCore
Modified: trunk/Source/WebCore/rendering/RenderElement.cpp (215699 => 215700)
--- trunk/Source/WebCore/rendering/RenderElement.cpp 2017-04-24 22:43:22 UTC (rev 215699)
+++ trunk/Source/WebCore/rendering/RenderElement.cpp 2017-04-24 22:46:11 UTC (rev 215700)
@@ -1506,6 +1506,12 @@
imageChanged(&image);
}
+void RenderElement::didRemoveCachedImageClient(CachedImage& cachedImage)
+{
+ if (hasPausedImageAnimations())
+ view().removeRendererWithPausedImageAnimations(*this, cachedImage);
+}
+
bool RenderElement::repaintForPausedImageAnimationsIfNeeded(const IntRect& visibleRect, CachedImage& cachedImage)
{
ASSERT(m_hasPausedImageAnimations);
Modified: trunk/Source/WebCore/rendering/RenderElement.h (215699 => 215700)
--- trunk/Source/WebCore/rendering/RenderElement.h 2017-04-24 22:43:22 UTC (rev 215699)
+++ trunk/Source/WebCore/rendering/RenderElement.h 2017-04-24 22:46:11 UTC (rev 215700)
@@ -319,6 +319,7 @@
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;
Modified: trunk/Source/WebCore/rendering/style/StyleCachedImage.cpp (215699 => 215700)
--- trunk/Source/WebCore/rendering/style/StyleCachedImage.cpp 2017-04-24 22:43:22 UTC (rev 215699)
+++ trunk/Source/WebCore/rendering/style/StyleCachedImage.cpp 2017-04-24 22:46:11 UTC (rev 215700)
@@ -188,9 +188,6 @@
return;
ASSERT(renderer);
- if (renderer->hasPausedImageAnimations())
- renderer->view().removeRendererWithPausedImageAnimations(*renderer, *m_cachedImage);
-
m_cachedImage->removeClient(*renderer);
}
