Skip to content
Permalink
Browse files
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 <sabouhallawa@apple.com> 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.

Canonical link: https://commits.webkit.org/191947@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@220289 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
Said Abou-Hallawa authored and webkit-commit-queue committed Aug 4, 2017
1 parent ab5cd65 commit 5ab07d05574a0f64db6253727a944a8121e089b7
Showing 5 changed files with 92 additions and 5 deletions.
@@ -1,3 +1,14 @@
2017-08-04 Said Abou-Hallawa <sabouhallawa@apple.com>

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 <jlewis3@apple.com>

Rebaslining fast/text/font-selection-font-loading-api-parse.html for iOS 11.
@@ -0,0 +1,3 @@
PASS if no crash happens.


@@ -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='resources/green-400x400.png'>
<script>
if (window.testRunner)
testRunner.dumpAsText(true);
setTimeout(function() {
var image = document.querySelector('img');
image.remove();
}, 0);
</script>
</body>
@@ -1,3 +1,52 @@
2017-08-04 Said Abou-Hallawa <sabouhallawa@apple.com>

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 <jeremyj@apple.com>

Use MPAVRoutingController instead of deprecated versions.
@@ -56,17 +56,21 @@ void RenderImageResourceStyleImage::initialize(RenderElement* renderer)
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)

0 comments on commit 5ab07d0

Please sign in to comment.