Fix a race condition on SkBitmap::lockPixels()/unlockPixels() for lazy image decoding
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 22 Jan 2013 21:09:07 +0000 (21:09 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 22 Jan 2013 21:09:07 +0000 (21:09 +0000)
https://bugs.webkit.org/show_bug.cgi?id=107404

Patch by Min Qin <qinmin@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):

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

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp
Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h

index f512826..bd77e6c 100644 (file)
@@ -1,3 +1,22 @@
+2013-01-22  Min Qin  <qinmin@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  <eric@webkit.org>
 
         Make CompactHTMLToken a little more compact
index 5992c50..501cc55 100644 (file)
@@ -96,23 +96,23 @@ bool ImageDecodingStore::lockCache(const ImageFrameGenerator* generator, const S
 
         // 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 @@ const ScaledImageFragment* ImageDecodingStore::insertAndLockCache(const ImageFra
     // 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 @@ const ScaledImageFragment* ImageDecodingStore::insertAndLockCache(const ImageFra
         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::insertAndLockCache(const ImageFra
 
 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 @@ const ScaledImageFragment* ImageDecodingStore::overwriteAndLockCache(const Image
 
         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;
 }
 
index 5f1fb28..eaac43a 100644 (file)
@@ -163,6 +163,8 @@ private:
     //   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;
 };