From cf1488f378638819eeb5fc276213437f6cbd0783 Mon Sep 17 00:00:00 2001 From: Chris Craik Date: Fri, 31 Aug 2012 14:52:50 -0700 Subject: Avoid unsafe use of SkRefCnt::getRefCnt() in ImagesManager The function isn't threadsafe, so races would occur and certain textures wouldn't be removed from ImagesManager's master list. The list would then be iterated over, all items dereferenced, and bad times would ensue. The SkRefCnt class uses atomic inc/dec to decide when to destroy an object, but reading such an integer non-atomically isn't safe. Instead use the real signal we're looking for - when the ImageTexture is deleted - to know when to remove the ImageTexture from ImagesManager's list. Mutual exclusion from editing the list is now maintained by only unref-ing the ImageTexture within releaseImage, which holds the ImagesManager's m_imagesLock (which we already do anyway). bug:6859278 Change-Id: I75ebf79f2617484e7df355d6539226ce64882369 --- .../platform/graphics/android/rendering/ImageTexture.cpp | 1 + .../platform/graphics/android/rendering/ImagesManager.cpp | 12 ++++++++++-- .../platform/graphics/android/rendering/ImagesManager.h | 3 +++ 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/Source/WebCore/platform/graphics/android/rendering/ImageTexture.cpp b/Source/WebCore/platform/graphics/android/rendering/ImageTexture.cpp index aa84427..db03753 100644 --- a/Source/WebCore/platform/graphics/android/rendering/ImageTexture.cpp +++ b/Source/WebCore/platform/graphics/android/rendering/ImageTexture.cpp @@ -101,6 +101,7 @@ ImageTexture::~ImageTexture() delete m_image; delete m_tileGrid; SkSafeUnref(m_picture); + ImagesManager::instance()->onImageTextureDestroy(m_crc); } SkBitmap* ImageTexture::convertBitmap(SkBitmap* bitmap) diff --git a/Source/WebCore/platform/graphics/android/rendering/ImagesManager.cpp b/Source/WebCore/platform/graphics/android/rendering/ImagesManager.cpp index 82ea3fa..d2bd8a0 100644 --- a/Source/WebCore/platform/graphics/android/rendering/ImagesManager.cpp +++ b/Source/WebCore/platform/graphics/android/rendering/ImagesManager.cpp @@ -102,12 +102,20 @@ void ImagesManager::releaseImage(unsigned imgCRC) android::Mutex::Autolock lock(m_imagesLock); if (m_images.contains(imgCRC)) { ImageTexture* image = m_images.get(imgCRC); - if (image->getRefCnt() == 1) - m_images.remove(imgCRC); + // don't need to remove image from the HashMap, it will unregister + // itself by calling onImageTextureDestroy(). + SkSafeUnref(image); } } +void ImagesManager::onImageTextureDestroy(unsigned imgCRC) +{ + // NOTE: all unrefs must go through releaseImage, to ensure that + // onImageTextureDestroy is called under the m_imagesLock + m_images.remove(imgCRC); +} + int ImagesManager::nbTextures() { android::Mutex::Autolock lock(m_imagesLock); diff --git a/Source/WebCore/platform/graphics/android/rendering/ImagesManager.h b/Source/WebCore/platform/graphics/android/rendering/ImagesManager.h index b915a46..718cfdd 100644 --- a/Source/WebCore/platform/graphics/android/rendering/ImagesManager.h +++ b/Source/WebCore/platform/graphics/android/rendering/ImagesManager.h @@ -47,6 +47,9 @@ public: ImageTexture* retainImage(unsigned imgCRC); void releaseImage(unsigned imgCRC); + // should be called only by ~ImageTexture() + void onImageTextureDestroy(unsigned imgCRC); + bool prepareTextures(GLWebViewState*); int nbTextures(); -- cgit v1.1