Attachment 145334 Details for Bug 87792 – patch (with test)

[patch] patch (with test)

87792c (text/plain), 20.67 KB, created by Xianzhu Wang on 2012-06-01 10:04:28 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Xianzhu Wang

Created: 2012-06-01 10:04:28 PDT

Size: 20.67 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 119243)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,57 @@
>+2012-06-01  Xianzhu Wang  <wangxianzhu@chromium.org>
>+
>+        SVGImageCache leaks image data
>+        https://bugs.webkit.org/show_bug.cgi?id=87792
>+
>+        There are two functions to remove a client from a CachedImage:
>+        - CachedResource::removeClient()
>+        - CachedImage::removeClientForRenderer().
>+        It's easy to make error to call the former which will leak the cached
>+        image buffers in SVGImageCache.
>+
>+        This change combined the two by adding the virtual 
>+        CachedResource::didRemoveClient(). CachedImage will do SVGImageCache
>+        cleanup in the function.
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Test: svg/as-image/svg-image-leak-cached-data.html
>+
>+        * loader/cache/CachedFont.h:
>+        (WebCore::CachedFontClient::resourceClientType): Added 'const'.
>+        * loader/cache/CachedImage.cpp:
>+        (WebCore):
>+        (WebCore::CachedImage::didRemoveClient): Removes the client from SVGImageCache.
>+        (WebCore::CachedImage::lookupOrCreateImageForRenderer):
>+        * loader/cache/CachedImage.h:
>+        (CachedImage):
>+        (WebCore::CachedImageClient::resourceClientType): Added 'const'.
>+        * loader/cache/CachedRawResource.h:
>+        (WebCore::CachedRawResourceClient::resourceClientType): Added 'const'.
>+        * loader/cache/CachedResource.cpp:
>+        (WebCore::CachedResource::removeClient): Added invocation of didRemoveClient().
>+        * loader/cache/CachedResource.h:
>+        (WebCore::CachedResource::didRemoveClient): Added for subclasses to do additional works.
>+        * loader/cache/CachedResourceClient.h:
>+        (WebCore::CachedResourceClient::resourceClientType): Added 'const'.
>+        * loader/cache/CachedSVGDocument.h:
>+        (WebCore::CachedSVGDocumentClient::resourceClientType): Added 'const'.
>+        * loader/cache/CachedStyleSheetClient.h:
>+        (WebCore::CachedStyleSheetClient::resourceClientType): Added 'const'.
>+        * rendering/style/StyleCachedImage.cpp:
>+        (WebCore::StyleCachedImage::removeClient):
>+        * rendering/style/StyleCachedImageSet.cpp:
>+        (WebCore::StyleCachedImageSet::removeClient):
>+        * svg/graphics/SVGImageCache.cpp:
>+        (WebCore::SVGImageCache::~SVGImageCache): Added checking for leaks.
>+        (WebCore::SVGImageCache::removeClientFromCache):
>+        (WebCore::SVGImageCache::setRequestedSizeAndScales): 
>+        (WebCore::SVGImageCache::requestedSizeAndScales):
>+        (WebCore::SVGImageCache::lookupOrCreateBitmapImageForClient):
>+        * svg/graphics/SVGImageCache.h:
>+        (WebCore):
>+        (SVGImageCache):
>+
> 2012-06-01  Christophe Dumez  <christophe.dumez@intel.com>
> 
>         [EFL] EFL port does not enable WEB_INTENTS_TAG flag
>Index: Source/WebCore/loader/cache/CachedFont.h
>===================================================================
>--- Source/WebCore/loader/cache/CachedFont.h	(revision 119241)
>+++ Source/WebCore/loader/cache/CachedFont.h	(working copy)
>@@ -85,7 +85,7 @@ class CachedFontClient : public CachedRe
> public:
>     virtual ~CachedFontClient() { }
>     static CachedResourceClientType expectedType() { return FontType; }
>-    virtual CachedResourceClientType resourceClientType() { return expectedType(); }
>+    virtual CachedResourceClientType resourceClientType() const { return expectedType(); }
>     virtual void fontLoaded(CachedFont*) { }
> };
> 
>Index: Source/WebCore/loader/cache/CachedImage.cpp
>===================================================================
>--- Source/WebCore/loader/cache/CachedImage.cpp	(revision 119241)
>+++ Source/WebCore/loader/cache/CachedImage.cpp	(working copy)
>@@ -92,15 +92,6 @@ void CachedImage::load(CachedResourceLoa
>         setLoading(false);
> }
> 
>-void CachedImage::removeClientForRenderer(RenderObject* renderer)
>-{
>-#if ENABLE(SVG)
>-    if (m_svgImageCache)
>-        m_svgImageCache->removeRendererFromCache(renderer);
>-#endif
>-    removeClient(renderer);
>-}
>-
> void CachedImage::didAddClient(CachedResourceClient* c)
> {
>     if (m_decodedDataDeletionTimer.isActive())
>@@ -118,6 +109,17 @@ void CachedImage::didAddClient(CachedRes
>     CachedResource::didAddClient(c);
> }
> 
>+void CachedImage::didRemoveClient(CachedResourceClient* c)
>+{
>+    ASSERT(c->resourceClientType() == CachedImageClient::expectedType());
>+#if ENABLE(SVG)
>+    if (m_svgImageCache)
>+        m_svgImageCache->removeClientFromCache(static_cast<CachedImageClient*>(c));
>+#endif
>+
>+    CachedResource::didRemoveClient(c);
>+}
>+
> void CachedImage::allClientsRemoved()
> {
>     if (m_image && !errorOccurred())
>@@ -150,7 +152,7 @@ inline Image* CachedImage::lookupOrCreat
>         return 0;
>     if (!m_image->isSVGImage())
>         return m_image.get();
>-    Image* useImage = m_svgImageCache->lookupOrCreateBitmapImageForRenderer(renderer);
>+    Image* useImage = m_svgImageCache->lookupOrCreateBitmapImageForClient(renderer);
>     if (useImage == Image::nullImage())
>         return m_image.get();
>     return useImage;
>Index: Source/WebCore/loader/cache/CachedImage.h
>===================================================================
>--- Source/WebCore/loader/cache/CachedImage.h	(revision 119241)
>+++ Source/WebCore/loader/cache/CachedImage.h	(working copy)
>@@ -67,9 +67,9 @@ public:
>     IntSize imageSizeForRenderer(const RenderObject*, float multiplier); // returns the size of the complete image.
>     void computeIntrinsicDimensions(Length& intrinsicWidth, Length& intrinsicHeight, FloatSize& intrinsicRatio);
> 
>-    void removeClientForRenderer(RenderObject*);
>     virtual void didAddClient(CachedResourceClient*);
>-    
>+    virtual void didRemoveClient(CachedResourceClient*);
>+
>     virtual void allClientsRemoved();
>     virtual void destroyDecodedData();
> 
>@@ -118,7 +118,7 @@ class CachedImageClient : public CachedR
> public:
>     virtual ~CachedImageClient() { }
>     static CachedResourceClientType expectedType() { return ImageType; }
>-    virtual CachedResourceClientType resourceClientType() { return expectedType(); }
>+    virtual CachedResourceClientType resourceClientType() const { return expectedType(); }
> 
>     // Called whenever a frame of an image changes, either because we got more data from the network or
>     // because we are animating. If not null, the IntRect is the changed rect of the image.
>Index: Source/WebCore/loader/cache/CachedRawResource.h
>===================================================================
>--- Source/WebCore/loader/cache/CachedRawResource.h	(revision 119241)
>+++ Source/WebCore/loader/cache/CachedRawResource.h	(working copy)
>@@ -65,7 +65,7 @@ class CachedRawResourceClient : public C
> public:
>     virtual ~CachedRawResourceClient() { }
>     static CachedResourceClientType expectedType() { return RawResourceType; }
>-    virtual CachedResourceClientType resourceClientType() { return expectedType(); }
>+    virtual CachedResourceClientType resourceClientType() const { return expectedType(); }
> 
>     virtual void dataSent(CachedResource*, unsigned long long /* bytesSent */, unsigned long long /* totalBytesToBeSent */) { }
>     virtual void responseReceived(CachedResource*, const ResourceResponse&) { }
>Index: Source/WebCore/loader/cache/CachedResource.cpp
>===================================================================
>--- Source/WebCore/loader/cache/CachedResource.cpp	(revision 119241)
>+++ Source/WebCore/loader/cache/CachedResource.cpp	(working copy)
>@@ -432,6 +432,7 @@ void CachedResource::removeClient(Cached
>     } else {
>         ASSERT(m_clients.contains(client));
>         m_clients.remove(client);
>+        didRemoveClient(client);
>     }
> 
>     if (canDelete() && !inCache())
>Index: Source/WebCore/loader/cache/CachedResource.h
>===================================================================
>--- Source/WebCore/loader/cache/CachedResource.h	(revision 119241)
>+++ Source/WebCore/loader/cache/CachedResource.h	(working copy)
>@@ -126,6 +126,7 @@ public:
>     PreloadResult preloadResult() const { return static_cast<PreloadResult>(m_preloadResult); }
> 
>     virtual void didAddClient(CachedResourceClient*);
>+    virtual void didRemoveClient(CachedResourceClient*) { }
>     virtual void allClientsRemoved();
> 
>     unsigned count() const { return m_clients.size(); }
>Index: Source/WebCore/loader/cache/CachedResourceClient.h
>===================================================================
>--- Source/WebCore/loader/cache/CachedResourceClient.h	(revision 119241)
>+++ Source/WebCore/loader/cache/CachedResourceClient.h	(working copy)
>@@ -50,7 +50,7 @@ public:
>     virtual void didReceiveData(CachedResource*) { };
>     
>     static CachedResourceClientType expectedType() { return BaseResourceType; }
>-    virtual CachedResourceClientType resourceClientType() { return expectedType(); }
>+    virtual CachedResourceClientType resourceClientType() const { return expectedType(); }
> 
> protected:
>     CachedResourceClient() { }
>Index: Source/WebCore/loader/cache/CachedSVGDocument.h
>===================================================================
>--- Source/WebCore/loader/cache/CachedSVGDocument.h	(revision 119241)
>+++ Source/WebCore/loader/cache/CachedSVGDocument.h	(working copy)
>@@ -52,7 +52,7 @@ class CachedSVGDocumentClient : public C
> public:
>     virtual ~CachedSVGDocumentClient() { }
>     static CachedResourceClientType expectedType() { return SVGDocumentType; }
>-    virtual CachedResourceClientType resourceClientType() { return expectedType(); }
>+    virtual CachedResourceClientType resourceClientType() const { return expectedType(); }
> };
> 
> }
>Index: Source/WebCore/loader/cache/CachedStyleSheetClient.h
>===================================================================
>--- Source/WebCore/loader/cache/CachedStyleSheetClient.h	(revision 119241)
>+++ Source/WebCore/loader/cache/CachedStyleSheetClient.h	(working copy)
>@@ -36,7 +36,7 @@ class CachedStyleSheetClient : public Ca
> public:
>     virtual ~CachedStyleSheetClient() { }
>     static CachedResourceClientType expectedType() { return StyleSheetType; }
>-    virtual CachedResourceClientType resourceClientType() { return expectedType(); }
>+    virtual CachedResourceClientType resourceClientType() const { return expectedType(); }
>     virtual void setCSSStyleSheet(const String& /* href */, const KURL& /* baseURL */, const String& /* charset */, const CachedCSSStyleSheet*) { }
>     virtual void setXSLStyleSheet(const String& /* href */, const KURL& /* baseURL */, const String& /* sheet */) { }
> };
>Index: Source/WebCore/rendering/style/StyleCachedImage.cpp
>===================================================================
>--- Source/WebCore/rendering/style/StyleCachedImage.cpp	(revision 119241)
>+++ Source/WebCore/rendering/style/StyleCachedImage.cpp	(working copy)
>@@ -97,7 +97,7 @@ void StyleCachedImage::addClient(RenderO
> 
> void StyleCachedImage::removeClient(RenderObject* renderer)
> {
>-    m_image->removeClientForRenderer(renderer);
>+    m_image->removeClient(renderer);
> }
> 
> PassRefPtr<Image> StyleCachedImage::image(RenderObject* renderer, const IntSize&) const
>Index: Source/WebCore/rendering/style/StyleCachedImageSet.cpp
>===================================================================
>--- Source/WebCore/rendering/style/StyleCachedImageSet.cpp	(revision 119241)
>+++ Source/WebCore/rendering/style/StyleCachedImageSet.cpp	(working copy)
>@@ -108,7 +108,7 @@ void StyleCachedImageSet::addClient(Rend
> 
> void StyleCachedImageSet::removeClient(RenderObject* renderer)
> {
>-    m_bestFitImage->removeClientForRenderer(renderer);
>+    m_bestFitImage->removeClient(renderer);
> }
> 
> PassRefPtr<Image> StyleCachedImageSet::image(RenderObject* renderer, const IntSize&) const
>Index: Source/WebCore/svg/graphics/SVGImageCache.cpp
>===================================================================
>--- Source/WebCore/svg/graphics/SVGImageCache.cpp	(revision 119241)
>+++ Source/WebCore/svg/graphics/SVGImageCache.cpp	(working copy)
>@@ -21,6 +21,7 @@
> #include "SVGImageCache.h"
> 
> #if ENABLE(SVG)
>+#include "CachedImage.h"
> #include "FrameView.h"
> #include "GraphicsContext.h"
> #include "ImageBuffer.h"
>@@ -41,18 +42,23 @@ SVGImageCache::~SVGImageCache()
>     m_sizeAndScalesMap.clear();
> 
>     ImageDataMap::iterator end = m_imageDataMap.end();
>-    for (ImageDataMap::iterator it = m_imageDataMap.begin(); it != end; ++it)
>+    for (ImageDataMap::iterator it = m_imageDataMap.begin(); it != end; ++it) {
>+        // Checks if the client (it->first) is still valid. The client should remove itself from this
>+        // cache before its end of life, otherwise the following ASSERT will crash on pure virtual
>+        // function call or a general crash.
>+        ASSERT(it->first->resourceClientType() == CachedImageClient::expectedType());
>         delete it->second.buffer;
>+    }
> 
>     m_imageDataMap.clear();
> }
> 
>-void SVGImageCache::removeRendererFromCache(const RenderObject* renderer)
>+void SVGImageCache::removeClientFromCache(const CachedImageClient* client)
> {
>-    ASSERT(renderer);
>-    m_sizeAndScalesMap.remove(renderer);
>+    ASSERT(client);
>+    m_sizeAndScalesMap.remove(client);
> 
>-    ImageDataMap::iterator it = m_imageDataMap.find(renderer);
>+    ImageDataMap::iterator it = m_imageDataMap.find(client);
>     if (it == m_imageDataMap.end())
>         return;
> 
>@@ -60,17 +66,17 @@ void SVGImageCache::removeRendererFromCa
>     m_imageDataMap.remove(it);
> }
> 
>-void SVGImageCache::setRequestedSizeAndScales(const RenderObject* renderer, const SizeAndScales& sizeAndScales)
>+void SVGImageCache::setRequestedSizeAndScales(const CachedImageClient* client, const SizeAndScales& sizeAndScales)
> {
>-    ASSERT(renderer);
>+    ASSERT(client);
>     ASSERT(!sizeAndScales.size.isEmpty());
>-    m_sizeAndScalesMap.set(renderer, sizeAndScales);
>+    m_sizeAndScalesMap.set(client, sizeAndScales);
> }
> 
>-SVGImageCache::SizeAndScales SVGImageCache::requestedSizeAndScales(const RenderObject* renderer) const
>+SVGImageCache::SizeAndScales SVGImageCache::requestedSizeAndScales(const CachedImageClient* client) const
> {
>-    ASSERT(renderer);
>-    SizeAndScalesMap::const_iterator it = m_sizeAndScalesMap.find(renderer);
>+    ASSERT(client);
>+    SizeAndScalesMap::const_iterator it = m_sizeAndScalesMap.find(client);
>     if (it == m_sizeAndScalesMap.end())
>         return SizeAndScales();
>     return it->second;
>@@ -122,12 +128,12 @@ void SVGImageCache::redrawTimerFired(Tim
>        redraw();
> }
> 
>-Image* SVGImageCache::lookupOrCreateBitmapImageForRenderer(const RenderObject* renderer)
>+Image* SVGImageCache::lookupOrCreateBitmapImageForClient(const CachedImageClient* client)
> {
>-    ASSERT(renderer);
>+    ASSERT(client);
> 
>-    // The cache needs to know the size of the renderer before querying an image for it.
>-    SizeAndScalesMap::iterator sizeIt = m_sizeAndScalesMap.find(renderer);
>+    // The cache needs to know the size of the client before querying an image for it.
>+    SizeAndScalesMap::iterator sizeIt = m_sizeAndScalesMap.find(client);
>     if (sizeIt == m_sizeAndScalesMap.end())
>         return Image::nullImage();
> 
>@@ -136,8 +142,8 @@ Image* SVGImageCache::lookupOrCreateBitm
>     float scale = sizeIt->second.scale;
>     ASSERT(!size.isEmpty());
> 
>-    // Lookup image for renderer in cache and eventually update it.
>-    ImageDataMap::iterator it = m_imageDataMap.find(renderer);
>+    // Lookup image for client in cache and eventually update it.
>+    ImageDataMap::iterator it = m_imageDataMap.find(client);
>     if (it != m_imageDataMap.end()) {
>         ImageData& data = it->second;
> 
>@@ -145,7 +151,7 @@ Image* SVGImageCache::lookupOrCreateBitm
>         if (data.sizeAndScales.size == size && data.sizeAndScales.zoom == zoom && data.sizeAndScales.scale == scale)
>             return data.image.get();
> 
>-        // If the image size for the renderer changed, we have to delete the buffer, remove the item from the cache and recreate it.
>+        // If the image size for the client changed, we have to delete the buffer, remove the item from the cache and recreate it.
>         delete data.buffer;
>         m_imageDataMap.remove(it);
>     }
>@@ -164,7 +170,7 @@ Image* SVGImageCache::lookupOrCreateBitm
>     Image* newImagePtr = newImage.get();
>     ASSERT(newImagePtr);
> 
>-    m_imageDataMap.add(renderer, ImageData(newBuffer.leakPtr(), newImage.release(), sizeIt->second));
>+    m_imageDataMap.add(client, ImageData(newBuffer.leakPtr(), newImage.release(), sizeIt->second));
>     return newImagePtr;
> }
> 
>Index: Source/WebCore/svg/graphics/SVGImageCache.h
>===================================================================
>--- Source/WebCore/svg/graphics/SVGImageCache.h	(revision 119241)
>+++ Source/WebCore/svg/graphics/SVGImageCache.h	(working copy)
>@@ -31,8 +31,8 @@
> namespace WebCore {
> 
> class CachedImage;
>+class CachedImageClient;
> class ImageBuffer;
>-class RenderObject;
> class SVGImage;
> 
> class SVGImageCache {
>@@ -63,12 +63,12 @@ public:
>         float scale;
>     };
> 
>-    void removeRendererFromCache(const RenderObject*);
>+    void removeClientFromCache(const CachedImageClient*);
> 
>-    void setRequestedSizeAndScales(const RenderObject*, const SizeAndScales&);
>-    SizeAndScales requestedSizeAndScales(const RenderObject*) const;
>+    void setRequestedSizeAndScales(const CachedImageClient*, const SizeAndScales&);
>+    SizeAndScales requestedSizeAndScales(const CachedImageClient*) const;
> 
>-    Image* lookupOrCreateBitmapImageForRenderer(const RenderObject*);
>+    Image* lookupOrCreateBitmapImageForClient(const CachedImageClient*);
>     void imageContentChanged();
> 
> private:
>@@ -98,8 +98,8 @@ private:
>         RefPtr<Image> image;
>     };
> 
>-    typedef HashMap<const RenderObject*, SizeAndScales> SizeAndScalesMap;
>-    typedef HashMap<const RenderObject*, ImageData> ImageDataMap;
>+    typedef HashMap<const CachedImageClient*, SizeAndScales> SizeAndScalesMap;
>+    typedef HashMap<const CachedImageClient*, ImageData> ImageDataMap;
> 
>     SVGImage* m_svgImage;
>     SizeAndScalesMap m_sizeAndScalesMap;
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 119243)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,15 @@
>+2012-06-01  Xianzhu Wang  <wangxianzhu@chromium.org>
>+
>+        SVGImageCache leaks image data
>+        https://bugs.webkit.org/show_bug.cgi?id=87792
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        New test case.
>+
>+        * svg/as-image/svg-image-leak-cached-data-expected.txt: Added.
>+        * svg/as-image/svg-image-leak-cached-data.html: Added.
>+
> 2012-06-01  Christophe Dumez  <christophe.dumez@intel.com>
> 
>         [EFL] EFL port does not enable WEB_INTENTS_TAG flag
>Index: LayoutTests/svg/as-image/svg-image-leak-cached-data-expected.txt
>===================================================================
>--- LayoutTests/svg/as-image/svg-image-leak-cached-data-expected.txt	(revision 0)
>+++ LayoutTests/svg/as-image/svg-image-leak-cached-data-expected.txt	(revision 0)
>@@ -0,0 +1,5 @@
>+This test checks if SVGImageCache leaks SVG image data as reported in https://bugs.webkit.org/show_bug.cgi?id=87792. Its layout has no particular meaning. The test will cause crash of debug version when leaks of SVG image data is detected.
>+
>+Note: the code detects leaks of SVG image data on destruction of SVGImageCache, which doesn't work on platforms that DumpRenderTree leaks the cache itself.
>+
>+
>
>Property changes on: LayoutTests/svg/as-image/svg-image-leak-cached-data-expected.txt
>___________________________________________________________________
>Added: svn:eol-style
>   + LF
>
>Index: LayoutTests/svg/as-image/svg-image-leak-cached-data.html
>===================================================================
>--- LayoutTests/svg/as-image/svg-image-leak-cached-data.html	(revision 0)
>+++ LayoutTests/svg/as-image/svg-image-leak-cached-data.html	(revision 0)
>@@ -0,0 +1,29 @@
>+<html>
>+<head>
>+<script>
>+if (window.layoutTestController) {
>+    layoutTestController.dumpAsText();
>+    layoutTestController.waitUntilDone();
>+}
>+
>+var count = 0;
>+function test() {
>+    var img = document.getElementById('img');
>+    document.body.replaceChild(img.cloneNode(), img);
>+    if (++count < 10)
>+        setTimeout(test, 0);
>+    else if (window.layoutTestController)
>+        layoutTestController.notifyDone();
>+}
>+</script>
>+</head>
>+
>+<body onload='test()'>
>+    <p>This test checks if SVGImageCache leaks SVG image data as reported in
>+    https://bugs.webkit.org/show_bug.cgi?id=87792. Its layout has no particular meaning.
>+    The test will cause crash of debug version when leaks of SVG image data is detected.</p>
>+    <p>Note: the code detects leaks of SVG image data on destruction of SVGImageCache,
>+    which doesn't work on platforms that DumpRenderTree leaks the cache itself.</p>
>+    <img id='img' src='resources/circle.svg'>
>+</body>
>+</html>
>
>Property changes on: LayoutTests/svg/as-image/svg-image-leak-cached-data.html
>___________________________________________________________________
>Added: svn:eol-style
>   + LF
>

Actions: View | Formatted Diff | Diff

Attachments on bug 87792: 144642 | 144662 | 144923 | 145334 | 146880