Changeset 220289 in webkit

Timestamp:

Aug 4, 2017 1:42:43 PM (7 years ago)

Author:

commit-queue@webkit.org

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 <​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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/images/image-element-image-content-data-expected.txt (added)
• LayoutTests/fast/images/image-element-image-content-data.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/RenderImageResourceStyleImage.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/rendering/RenderImageResourceStyleImage.cpp

@@ -57 +57 @@
 {
     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;
 }
 
@@ -67 +67 @@
 {
     // 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();
 }