CachedImage does not clear the ImageObserver pointer when dropping its Image ref

https://bugs.webkit.org/show_bug.cgi?id=86689

Reviewed by Eric Seidel.

Image instances keep a weak pointer to their ImageObserver, which may be null. CachedImage is an ImageObserver
and holds a RefPtr<Image> m_image. When CachedImage initializes its m_image to either an SVGImage or BitmapImage,
it sets itself as that Image's ImageObserver. However, CachedImage never clears the ImageObserver pointer, even
when dropping its reference to the Image. This means if other code holds a RefPtr<Image> there is no promise
that calls on that Image will be valid. This patch clears the CachedImage::m_image's ImageObserver pointer
whenever the CachedImage drops its reference. Image already has null checks for its m_imageObserver so this is
always a safe operation.

* loader/cache/CachedImage.cpp:
(WebCore::CachedImage::~CachedImage):
(WebCore::CachedImage::clear):

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

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 0b93f89..7d7e2bb 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,22 @@
+2012-05-16  James Robinson  <jamesr@chromium.org>
+
+        CachedImage does not clear the ImageObserver pointer when dropping its Image ref
+        https://bugs.webkit.org/show_bug.cgi?id=86689
+
+        Reviewed by Eric Seidel.
+
+        Image instances keep a weak pointer to their ImageObserver, which may be null. CachedImage is an ImageObserver
+        and holds a RefPtr<Image> m_image. When CachedImage initializes its m_image to either an SVGImage or BitmapImage,
+        it sets itself as that Image's ImageObserver. However, CachedImage never clears the ImageObserver pointer, even
+        when dropping its reference to the Image. This means if other code holds a RefPtr<Image> there is no promise
+        that calls on that Image will be valid. This patch clears the CachedImage::m_image's ImageObserver pointer
+        whenever the CachedImage drops its reference. Image already has null checks for its m_imageObserver so this is
+        always a safe operation.
+
+        * loader/cache/CachedImage.cpp:
+        (WebCore::CachedImage::~CachedImage):
+        (WebCore::CachedImage::clear):
+
 2012-05-16  Kentaro Hara  <haraken@chromium.org>
 
         [V8] Fix a broken copyright of V8SVGElementCustom.cpp

diff --git a/Source/WebCore/loader/cache/CachedImage.cpp b/Source/WebCore/loader/cache/CachedImage.cpp
index 33a4015..b4f0b27 100644 (file)
--- a/Source/WebCore/loader/cache/CachedImage.cpp
+++ b/Source/WebCore/loader/cache/CachedImage.cpp
@@ -75,6 +75,7 @@ CachedImage::CachedImage(Image* image)
 
 CachedImage::~CachedImage()
 {
+    clearImage();
 }
 
 void CachedImage::decodedDataDeletionTimerFired(Timer<CachedImage>*)
@@ -302,7 +303,7 @@ void CachedImage::clear()
 #if ENABLE(SVG)
     m_svgImageCache.clear();
 #endif
-    m_image = 0;
+    clearImage();
     setEncodedSize(0);
 }
 
@@ -328,6 +329,15 @@ inline void CachedImage::createImage()
     m_image = BitmapImage::create(this);
 }
 
+inline void CachedImage::clearImage()
+{
+    // If our Image has an observer, it's always us so we need to clear the back pointer
+    // before dropping our reference.
+    if (m_image)
+        m_image->setImageObserver(0);
+    m_image.clear();
+}
+
 size_t CachedImage::maximumDecodedImageSize()
 {
     if (!m_loader || m_loader->reachedTerminalState())

diff --git a/Source/WebCore/loader/cache/CachedImage.h b/Source/WebCore/loader/cache/CachedImage.h
index e7184e1..89f019d 100644 (file)
--- a/Source/WebCore/loader/cache/CachedImage.h
+++ b/Source/WebCore/loader/cache/CachedImage.h
@@ -98,6 +98,7 @@ private:
     void clear();
 
     void createImage();
+    void clearImage();
     size_t maximumDecodedImageSize();
     // If not null, changeRect is the changed part of the image.
     void notifyObservers(const IntRect* changeRect = 0);