Title: [220289] trunk
- Revision
- 220289
- Author
- [email protected]
- Date
- 2017-08-04 13:42:43 -0700 (Fri, 04 Aug 2017)
Log Message
RenderImageResourceStyleImage::image() should return the nullImage() if the image is not available
https://bugs.webkit.org/show_bug.cgi?id=174874
<rdar://problem/33530130>
Patch by Said Abou-Hallawa <[email protected]> on 2017-08-04
Reviewed by Simon Fraser.
Source/WebCore:
If an <img> element has a non-CachedImage content data, e.g. -webkit-named-image,
RenderImageResourceStyleImage will be created and attached to the RenderImage.
RenderImageResourceStyleImage::m_cachedImage will be set to null at the
beginning because the m_styleImage->isCachedImage() is false in this case.
When ImageLoader finishes loading the url of the src attribute,
RenderImageResource::setCachedImage() will be called to set m_cachedImage.
A crash will happen when the RenderImage is destroyed. Destroying the
RenderImage calls RenderImageResourceStyleImage::shutdown() which checks
m_cachedImage and finds it not null, so it calls RenderImageResourceStyleImage::image()
which ends up calling CSSNamedImageValue::image() which returns a null pointer
because the size is empty. RenderImageResourceStyleImage::shutdown() calls
image()->stopAnimation() without checking the return value of image().
Another crash will happen later when deleting the CachedImage from the memory
cache if CachedImage::canDestroyDecodedData() is called because the client
it gets from m_clients is a freed pointer. This happens because RenderImageResourceStyleImage
has m_styleImage of type StyleGeneratedImage but its m_cachedImage is set
by RenderImageResource::setCachedImage(). When RenderImageResourceStyleImage::shutdown()
is called, it calls StyleGeneratedImage::removeClient() which does not
know anything about RenderImageResourceStyleImage::m_cachedImage. So we
end up having a freed pointer in the m_clients of the CachedImage.
Test: fast/images/image-element-image-content-data.html
* rendering/RenderImageResourceStyleImage.cpp:
(WebCore::RenderImageResourceStyleImage::shutdown): Revert back the changes
of r208511 in this function. Add a call to image()->stopAnimation() without
checking the return of image() since it will return the nullImage() if
the image not available. There is no need to check m_cachedImage before
calling image() because image() does not check or access m_cachedImage.
If m_styleImage is not a CachedStyleImage but m_cachedImage is not null,
we need to remove m_renderer from the set of the clients of this m_cachedImage.
(WebCore::RenderImageResourceStyleImage::image const): The base class method
RenderImageResource::image() returns the nullImage() if the image not
available. This is because CachedImage::imageForRenderer() returns
the nullImage() if the image is not available; see CachedImage.h. We should
do the same for the derived class for consistency.
LayoutTests:
* fast/images/image-element-image-content-data-expected.txt: Added.
* fast/images/image-element-image-content-data.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (220288 => 220289)
--- trunk/LayoutTests/ChangeLog 2017-08-04 20:07:20 UTC (rev 220288)
+++ trunk/LayoutTests/ChangeLog 2017-08-04 20:42:43 UTC (rev 220289)
@@ -1,3 +1,14 @@
+2017-08-04 Said Abou-Hallawa <[email protected]>
+
+ RenderImageResourceStyleImage::image() should return the nullImage() if the image is not available
+ https://bugs.webkit.org/show_bug.cgi?id=174874
+ <rdar://problem/33530130>
+
+ Reviewed by Simon Fraser.
+
+ * fast/images/image-element-image-content-data-expected.txt: Added.
+ * fast/images/image-element-image-content-data.html: Added.
+
2017-08-04 Matt Lewis <[email protected]>
Rebaslining fast/text/font-selection-font-loading-api-parse.html for iOS 11.
Added: trunk/LayoutTests/fast/images/image-element-image-content-data-expected.txt (0 => 220289)
--- trunk/LayoutTests/fast/images/image-element-image-content-data-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/images/image-element-image-content-data-expected.txt 2017-08-04 20:42:43 UTC (rev 220289)
@@ -0,0 +1,3 @@
+PASS if no crash happens.
+
+
Added: trunk/LayoutTests/fast/images/image-element-image-content-data.html (0 => 220289)
--- trunk/LayoutTests/fast/images/image-element-image-content-data.html (rev 0)
+++ trunk/LayoutTests/fast/images/image-element-image-content-data.html 2017-08-04 20:42:43 UTC (rev 220289)
@@ -0,0 +1,20 @@
+<style>
+ img {
+ width: 100px;
+ height: 100px;
+ border: 2px solid black;
+ content: -webkit-named-image(apple-pay-logo-white);
+ }
+</style>
+<body>
+ <p>PASS if no crash happens.</p>
+ <img src=''>
+ <script>
+ if (window.testRunner)
+ testRunner.dumpAsText(true);
+ setTimeout(function() {
+ var image = document.querySelector('img');
+ image.remove();
+ }, 0);
+ </script>
+</body>
Modified: trunk/Source/WebCore/ChangeLog (220288 => 220289)
--- trunk/Source/WebCore/ChangeLog 2017-08-04 20:07:20 UTC (rev 220288)
+++ trunk/Source/WebCore/ChangeLog 2017-08-04 20:42:43 UTC (rev 220289)
@@ -1,3 +1,52 @@
+2017-08-04 Said Abou-Hallawa <[email protected]>
+
+ RenderImageResourceStyleImage::image() should return the nullImage() if the image is not available
+ https://bugs.webkit.org/show_bug.cgi?id=174874
+ <rdar://problem/33530130>
+
+ Reviewed by Simon Fraser.
+
+ If an <img> element has a non-CachedImage content data, e.g. -webkit-named-image,
+ RenderImageResourceStyleImage will be created and attached to the RenderImage.
+ RenderImageResourceStyleImage::m_cachedImage will be set to null at the
+ beginning because the m_styleImage->isCachedImage() is false in this case.
+ When ImageLoader finishes loading the url of the src attribute,
+ RenderImageResource::setCachedImage() will be called to set m_cachedImage.
+
+ A crash will happen when the RenderImage is destroyed. Destroying the
+ RenderImage calls RenderImageResourceStyleImage::shutdown() which checks
+ m_cachedImage and finds it not null, so it calls RenderImageResourceStyleImage::image()
+ which ends up calling CSSNamedImageValue::image() which returns a null pointer
+ because the size is empty. RenderImageResourceStyleImage::shutdown() calls
+ image()->stopAnimation() without checking the return value of image().
+
+ Another crash will happen later when deleting the CachedImage from the memory
+ cache if CachedImage::canDestroyDecodedData() is called because the client
+ it gets from m_clients is a freed pointer. This happens because RenderImageResourceStyleImage
+ has m_styleImage of type StyleGeneratedImage but its m_cachedImage is set
+ by RenderImageResource::setCachedImage(). When RenderImageResourceStyleImage::shutdown()
+ is called, it calls StyleGeneratedImage::removeClient() which does not
+ know anything about RenderImageResourceStyleImage::m_cachedImage. So we
+ end up having a freed pointer in the m_clients of the CachedImage.
+
+ Test: fast/images/image-element-image-content-data.html
+
+ * rendering/RenderImageResourceStyleImage.cpp:
+ (WebCore::RenderImageResourceStyleImage::shutdown): Revert back the changes
+ of r208511 in this function. Add a call to image()->stopAnimation() without
+ checking the return of image() since it will return the nullImage() if
+ the image not available. There is no need to check m_cachedImage before
+ calling image() because image() does not check or access m_cachedImage.
+
+ If m_styleImage is not a CachedStyleImage but m_cachedImage is not null,
+ we need to remove m_renderer from the set of the clients of this m_cachedImage.
+
+ (WebCore::RenderImageResourceStyleImage::image const): The base class method
+ RenderImageResource::image() returns the nullImage() if the image not
+ available. This is because CachedImage::imageForRenderer() returns
+ the nullImage() if the image is not available; see CachedImage.h. We should
+ do the same for the derived class for consistency.
+
2017-08-04 Jeremy Jones <[email protected]>
Use MPAVRoutingController instead of deprecated versions.
Modified: trunk/Source/WebCore/rendering/RenderImageResourceStyleImage.cpp (220288 => 220289)
--- trunk/Source/WebCore/rendering/RenderImageResourceStyleImage.cpp 2017-08-04 20:07:20 UTC (rev 220288)
+++ trunk/Source/WebCore/rendering/RenderImageResourceStyleImage.cpp 2017-08-04 20:42:43 UTC (rev 220289)
@@ -56,17 +56,21 @@
void RenderImageResourceStyleImage::shutdown()
{
ASSERT(m_renderer);
+ image()->stopAnimation();
m_styleImage->removeClient(m_renderer);
- if (m_cachedImage) {
- image()->stopAnimation();
- m_cachedImage = nullptr;
- }
+ if (!m_styleImage->isCachedImage() && m_cachedImage)
+ m_cachedImage->removeClient(*m_renderer);
+ m_cachedImage = nullptr;
}
RefPtr<Image> RenderImageResourceStyleImage::image(const IntSize& size) const
{
// Generated content may trigger calls to image() while we're still pending, don't assert but gracefully exit.
- return !m_styleImage->isPending() ? m_styleImage->image(m_renderer, size) : &Image::nullImage();
+ if (m_styleImage->isPending())
+ return &Image::nullImage();
+ if (auto image = m_styleImage->image(m_renderer, size))
+ return image;
+ return &Image::nullImage();
}
void RenderImageResourceStyleImage::setContainerSizeForRenderer(const IntSize& size)
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
