RenderImageResourceStyleImage::image() should return the nullImage() if the image...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 30 Jul 2017 07:38:31 +0000 (07:38 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 30 Jul 2017 07:38:31 +0000 (07:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=174874
<rdar://problem/33530130>

Patch by Said Abou-Hallawa <sabouhallawa@apple.com> on 2017-07-30
Reviewed by Darin Adler.

Source/WebCore:

If an <img> element has image content data for a none cached image, e.g.
-webkit-named-image, RenderImageResourceStyleImage will be created and
attached to the RenderImage. RenderImageResourceStyleImage::m_cachedImage
will be set to null 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().

Like the base class virtual method RenderImageResource::image(),
RenderImageResourceStyleImage::image() should return the nullImage() if
the image is not available.

Test: fast/images/image-element-image-content-data.html

* css/CSSCrossfadeValue.cpp:
* css/CSSFilterImageValue.cpp:
* page/EventHandler.cpp:
* page/PageSerializer.cpp:
* rendering/RenderElement.cpp:
* rendering/RenderImageResource.cpp:
* rendering/RenderImageResourceStyleImage.cpp:
(WebCore::RenderImageResourceStyleImage::initialize):

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

(WebCore::RenderImageResourceStyleImage::image): 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.

* rendering/style/ContentData.cpp:
* rendering/style/StyleCachedImage.cpp:
* style/StylePendingResources.cpp:

LayoutTests:

* fast/images/image-element-image-content-data-expected.txt: Added.
* fast/images/image-element-image-content-data.html: Added.

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

14 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/images/image-element-image-content-data-expected.txt [new file with mode: 0644]
LayoutTests/fast/images/image-element-image-content-data.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/css/CSSCrossfadeValue.cpp
Source/WebCore/css/CSSFilterImageValue.cpp
Source/WebCore/page/EventHandler.cpp
Source/WebCore/page/PageSerializer.cpp
Source/WebCore/rendering/RenderElement.cpp
Source/WebCore/rendering/RenderImageResource.cpp
Source/WebCore/rendering/RenderImageResourceStyleImage.cpp
Source/WebCore/rendering/style/ContentData.cpp
Source/WebCore/rendering/style/StyleCachedImage.cpp
Source/WebCore/style/StylePendingResources.cpp

index 6a25a39..18c87a9 100644 (file)
@@ -1,3 +1,14 @@
+2017-07-30  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 Darin Adler.
+
+        * fast/images/image-element-image-content-data-expected.txt: Added.
+        * fast/images/image-element-image-content-data.html: Added.
+
 2017-07-29  Nan Wang  <n_wang@apple.com>
 
         AX: findMatchingObjects doesn't work when the startObject is ignored
diff --git a/LayoutTests/fast/images/image-element-image-content-data-expected.txt b/LayoutTests/fast/images/image-element-image-content-data-expected.txt
new file mode 100644 (file)
index 0000000..6640c22
--- /dev/null
@@ -0,0 +1,3 @@
+PASS if no crash happens.
+
+
diff --git a/LayoutTests/fast/images/image-element-image-content-data.html b/LayoutTests/fast/images/image-element-image-content-data.html
new file mode 100644 (file)
index 0000000..9e27a91
--- /dev/null
@@ -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>
index 248c5ed..02fcee6 100644 (file)
@@ -1,3 +1,56 @@
+2017-07-30  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 Darin Adler.
+
+        If an <img> element has image content data for a none cached image, e.g.
+        -webkit-named-image, RenderImageResourceStyleImage will be created and 
+        attached to the RenderImage. RenderImageResourceStyleImage::m_cachedImage
+        will be set to null 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().
+
+        Like the base class virtual method RenderImageResource::image(), 
+        RenderImageResourceStyleImage::image() should return the nullImage() if
+        the image is not available.
+
+        Test: fast/images/image-element-image-content-data.html
+
+        * css/CSSCrossfadeValue.cpp:
+        * css/CSSFilterImageValue.cpp:
+        * page/EventHandler.cpp:
+        * page/PageSerializer.cpp:
+        * rendering/RenderElement.cpp:
+        * rendering/RenderImageResource.cpp:
+        * rendering/RenderImageResourceStyleImage.cpp:
+        (WebCore::RenderImageResourceStyleImage::initialize):
+
+        (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.
+
+        (WebCore::RenderImageResourceStyleImage::image): 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.
+
+        * rendering/style/ContentData.cpp:
+        * rendering/style/StyleCachedImage.cpp:
+        * style/StylePendingResources.cpp:
+
 2017-07-29  Filip Pizlo  <fpizlo@apple.com>
 
         Unreviewed, rollout r220044 because it set the bots on fire.
index 9eb6f0f..706af08 100644 (file)
@@ -33,7 +33,6 @@
 #include "CachedResourceLoader.h"
 #include "CrossfadeGeneratedImage.h"
 #include "RenderElement.h"
-#include "StyleCachedImage.h"
 #include <wtf/text/StringBuilder.h>
 
 namespace WebCore {
index a8b0b14..ac23aaf 100644 (file)
@@ -33,7 +33,6 @@
 #include "GraphicsContext.h"
 #include "ImageBuffer.h"
 #include "RenderElement.h"
-#include "StyleCachedImage.h"
 #include "StyleResolver.h"
 #include <wtf/text/StringBuilder.h>
 
index 816ebb2..721c6b1 100644 (file)
@@ -86,7 +86,6 @@
 #include "Settings.h"
 #include "ShadowRoot.h"
 #include "SpatialNavigation.h"
-#include "StyleCachedImage.h"
 #include "TextEvent.h"
 #include "TextIterator.h"
 #include "UserGestureIndicator.h"
index ec10cd5..bc3abf1 100644 (file)
@@ -52,7 +52,6 @@
 #include "MarkupAccumulator.h"
 #include "Page.h"
 #include "RenderElement.h"
-#include "StyleCachedImage.h"
 #include "StyleImage.h"
 #include "StyleProperties.h"
 #include "StyleRule.h"
index c9dc5d6..86a18be 100644 (file)
@@ -50,7 +50,6 @@
 #include "RenderDescendantIterator.h"
 #include "RenderFlexibleBox.h"
 #include "RenderImage.h"
-#include "RenderImageResourceStyleImage.h"
 #include "RenderInline.h"
 #include "RenderIterator.h"
 #include "RenderLayer.h"
index d8de5f9..f269da6 100644 (file)
@@ -32,7 +32,6 @@
 #include "Image.h"
 #include "RenderElement.h"
 #include "RenderImage.h"
-#include "RenderImageResourceStyleImage.h"
 
 namespace WebCore {
 
index 5c37cab..61f85cb 100644 (file)
@@ -30,7 +30,6 @@
 
 #include "CachedImage.h"
 #include "RenderElement.h"
-#include "StyleCachedImage.h"
 
 namespace WebCore {
 
@@ -48,7 +47,7 @@ void RenderImageResourceStyleImage::initialize(RenderElement* renderer)
     RenderImageResource::initialize(renderer);
 
     if (m_styleImage->isCachedImage())
-        m_cachedImage = m_styleImage.get().cachedImage();
+        m_cachedImage = m_styleImage->cachedImage();
 
     m_styleImage->addClient(m_renderer);
 }
@@ -56,17 +55,19 @@ 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;
-    }
+    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)
index c6c0224..d802818 100644 (file)
@@ -25,7 +25,6 @@
 #include "RenderCounter.h"
 #include "RenderImage.h"
 #include "RenderImageResource.h"
-#include "RenderImageResourceStyleImage.h"
 #include "RenderQuote.h"
 #include "RenderStyle.h"
 #include "RenderTextFragment.h"
index 9428c4e..eb27664 100644 (file)
@@ -29,7 +29,6 @@
 #include "CSSImageValue.h"
 #include "CachedImage.h"
 #include "RenderElement.h"
-#include "RenderView.h"
 
 namespace WebCore {
 
index 10319f8..df1a2af 100644 (file)
@@ -34,7 +34,6 @@
 #include "Document.h"
 #include "RenderStyle.h"
 #include "SVGURIReference.h"
-#include "StyleCachedImage.h"
 #include "StyleGeneratedImage.h"
 #include "TransformFunctions.h"