Title: [140454] trunk/Source/WebCore
- Revision
- 140454
- Author
- commit-qu...@webkit.org
- Date
- 2013-01-22 13:09:07 -0800 (Tue, 22 Jan 2013)
Log Message
Fix a race condition on SkBitmap::lockPixels()/unlockPixels() for lazy image decoding
https://bugs.webkit.org/show_bug.cgi?id=107404
Patch by Min Qin <qin...@chromium.org> on 2013-01-22
Reviewed by Stephen White.
Skbitmap::lockPixels()/unlockPixels() are not threadsafe.
unlike SkPixelRef, these 2 calls are not protected by an internal mutex.
Bugfix, no behaviral change and hard to test as tests will be flaky.
* platform/graphics/chromium/ImageDecodingStore.cpp:
(WebCore::ImageDecodingStore::lockCache):
(WebCore::ImageDecodingStore::unlockCache):
(WebCore::ImageDecodingStore::insertAndLockCache):
(WebCore::ImageDecodingStore::overwriteAndLockCache):
* platform/graphics/chromium/ImageDecodingStore.h:
(ImageDecodingStore):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (140453 => 140454)
--- trunk/Source/WebCore/ChangeLog 2013-01-22 21:01:42 UTC (rev 140453)
+++ trunk/Source/WebCore/ChangeLog 2013-01-22 21:09:07 UTC (rev 140454)
@@ -1,3 +1,22 @@
+2013-01-22 Min Qin <qin...@chromium.org>
+
+ Fix a race condition on SkBitmap::lockPixels()/unlockPixels() for lazy image decoding
+ https://bugs.webkit.org/show_bug.cgi?id=107404
+
+ Reviewed by Stephen White.
+
+ Skbitmap::lockPixels()/unlockPixels() are not threadsafe.
+ unlike SkPixelRef, these 2 calls are not protected by an internal mutex.
+ Bugfix, no behaviral change and hard to test as tests will be flaky.
+
+ * platform/graphics/chromium/ImageDecodingStore.cpp:
+ (WebCore::ImageDecodingStore::lockCache):
+ (WebCore::ImageDecodingStore::unlockCache):
+ (WebCore::ImageDecodingStore::insertAndLockCache):
+ (WebCore::ImageDecodingStore::overwriteAndLockCache):
+ * platform/graphics/chromium/ImageDecodingStore.h:
+ (ImageDecodingStore):
+
2013-01-22 Eric Seidel <e...@webkit.org>
Make CompactHTMLToken a little more compact
Modified: trunk/Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp (140453 => 140454)
--- trunk/Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp 2013-01-22 21:01:42 UTC (rev 140453)
+++ trunk/Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp 2013-01-22 21:09:07 UTC (rev 140454)
@@ -96,23 +96,23 @@
// Increment use count such that it doesn't get evicted.
cacheEntry->incrementUseCount();
- }
- // Complete cache entry doesn't have a decoder.
- ASSERT(!cacheEntry->cachedImage()->isComplete() || !cacheEntry->cachedDecoder());
+ // Complete cache entry doesn't have a decoder.
+ ASSERT(!cacheEntry->cachedImage()->isComplete() || !cacheEntry->cachedDecoder());
- if (decoder)
- *decoder = cacheEntry->cachedDecoder();
- *cachedImage = cacheEntry->cachedImage();
- (*cachedImage)->bitmap().lockPixels();
+ if (decoder)
+ *decoder = cacheEntry->cachedDecoder();
+ *cachedImage = cacheEntry->cachedImage();
+ (*cachedImage)->bitmap().lockPixels();
+ }
+
return true;
}
void ImageDecodingStore::unlockCache(const ImageFrameGenerator* generator, const ScaledImageFragment* cachedImage)
{
- cachedImage->bitmap().unlockPixels();
-
MutexLocker lock(m_mutex);
+ cachedImage->bitmap().unlockPixels();
CacheMap::iterator iter = m_cacheMap.find(std::make_pair(generator, cachedImage->scaledSize()));
ASSERT(iter != m_cacheMap.end());
@@ -129,9 +129,6 @@
// Prune old cache entries to give space for the new one.
prune();
- // Lock the underlying SkBitmap to prevent it from being purged.
- image->bitmap().lockPixels();
-
ScaledImageFragment* cachedImage = image.get();
OwnPtr<CacheEntry> newCacheEntry;
@@ -142,6 +139,8 @@
newCacheEntry = CacheEntry::createAndUse(generator, image, decoder);
MutexLocker lock(m_mutex);
+ // Lock the underlying SkBitmap to prevent it from being purged.
+ cachedImage->bitmap().lockPixels();
ASSERT(!m_cacheMap.contains(newCacheEntry->cacheKey()));
insertCacheInternal(newCacheEntry.release());
return cachedImage;
@@ -149,12 +148,11 @@
const ScaledImageFragment* ImageDecodingStore::overwriteAndLockCache(const ImageFrameGenerator* generator, const ScaledImageFragment* cachedImage, PassOwnPtr<ScaledImageFragment> newImage)
{
- cachedImage->bitmap().unlockPixels();
-
OwnPtr<ImageDecoder> trash;
const ScaledImageFragment* newCachedImage = 0;
{
MutexLocker lock(m_mutex);
+ cachedImage->bitmap().unlockPixels();
CacheMap::iterator iter = m_cacheMap.find(std::make_pair(generator, cachedImage->scaledSize()));
ASSERT(iter != m_cacheMap.end());
@@ -164,10 +162,10 @@
trash = cacheEntry->overwriteCachedImage(newImage);
newCachedImage = cacheEntry->cachedImage();
+ // Lock the underlying SkBitmap to prevent it from being purged.
+ newCachedImage->bitmap().lockPixels();
}
- // Lock the underlying SkBitmap to prevent it from being purged.
- newCachedImage->bitmap().lockPixels();
return newCachedImage;
}
Modified: trunk/Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h (140453 => 140454)
--- trunk/Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h 2013-01-22 21:01:42 UTC (rev 140453)
+++ trunk/Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h 2013-01-22 21:09:07 UTC (rev 140454)
@@ -163,6 +163,8 @@
// m_cachedSizeMap
// m_cacheLimitInBytes
// m_memoryUsageInBytes
+ // This mutex also protects calls to underlying skBitmap's
+ // lockPixels()/unlockPixels() as they are not threadsafe.
Mutex m_mutex;
};
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-changes
