detect if the scaledimagecache returns a purged bitmap

src/core/SkBitmapProcState.cpp

Index: src/core/SkBitmapProcState.cpp
diff --git a/src/core/SkBitmapProcState.cpp b/src/core/SkBitmapProcState.cpp
index 39f6a7803920fcbc2d0f9d3c1741aa762086dc09..31e7edc116128e2327e40285bd661da563c37548 100644
--- a/src/core/SkBitmapProcState.cpp
+++ b/src/core/SkBitmapProcState.cpp
@@ -106,11 +106,31 @@ static SkScalar effective_matrix_scale_sqrd(const SkMatrix& mat) {
     return SkMaxScalar(v1.lengthSqd(), v2.lengthSqd());
 }
 
+class AutoScaledCacheUnlocker {

[scroggo, 2013/12/12 20:34:55]

Maybe not necessary, since this is hidden in a source file, but should this use
SK_REQUIRE_LOCAL_VAR?

[reed1, 2013/12/12 21:13:49]

Done.

+public:
+    AutoScaledCacheUnlocker(SkScaledImageCache::ID** idPtr) : fIDPtr(idPtr) {}
+    ~AutoScaledCacheUnlocker() {
+        if (fIDPtr && *fIDPtr) {
+            SkScaledImageCache::Unlock(*fIDPtr);
+            *fIDPtr = NULL;
+        }
+    }
+
+    void keepLock() {

[scroggo, 2013/12/12 20:34:55]

How about reset? That keeps it consistent with our other Auto classes. I also
think it's a clearer name (since "reset" applies to the class who owns it, where
as "keepLock" refers to its private member).

[hal.canary, 2013/12/12 20:48:43]

it's more of a detach() than a reset()

[reed1, 2013/12/12 21:13:49]

How about release() -- release, since detach usually returns something, which
isn't the case here.

+        fIDPtr = NULL;
+    }
+
+private:
+    SkScaledImageCache::ID** fIDPtr;
+};
+
 // TODO -- we may want to pass the clip into this function so we only scale
 // the portion of the image that we're going to need.  This will complicate
 // the interface to the cache, but might be well worth it.
 
 bool SkBitmapProcState::possiblyScaleImage() {
+    AutoScaledCacheUnlocker unlocker(&fScaledCacheID);
+
     SkASSERT(NULL == fBitmap);
     SkASSERT(NULL == fScaledCacheID);
 
@@ -132,6 +152,17 @@ bool SkBitmapProcState::possiblyScaleImage() {
         fScaledCacheID = SkScaledImageCache::FindAndLock(fOrigBitmap,
                                                          invScaleX, invScaleY,
                                                          &fScaledBitmap);
+        if (fScaledCacheID) {
+            fScaledBitmap.lockPixels();
+            if (!fScaledBitmap.getPixels()) {
+                fScaledBitmap.unlockPixels();
+                // found a purged entry (discardablememory?), release it
+                SkScaledImageCache::Unlock(fScaledCacheID);
+                fScaledCacheID = NULL;
+                // fall through to rebuild
+            }
+        }
+
         if (NULL == fScaledCacheID) {
             int dest_width  = SkScalarCeilToInt(fOrigBitmap.width() / invScaleX);
             int dest_height = SkScalarCeilToInt(fOrigBitmap.height() / invScaleY);
@@ -154,18 +185,19 @@ bool SkBitmapProcState::possiblyScaleImage() {
                 return false;
 
             }
+            SkASSERT(NULL != fScaledBitmap.getPixels());
             fScaledCacheID = SkScaledImageCache::AddAndLock(fOrigBitmap,
                                                             invScaleX,
                                                             invScaleY,
                                                             fScaledBitmap);
-        }
-        fScaledBitmap.lockPixels(); // wonder if Resize() should have locked this
-        if (!fScaledBitmap.getPixels()) {
-            // TODO: find out how this can happen, and add a unittest to exercise
-            // inspired by BUG=chromium:295895
-            return false;
+            if (!fScaledCacheID) {
+                fScaledBitmap.reset();
+                return false;
+            }
+            SkASSERT(NULL != fScaledBitmap.getPixels());
         }
 
+        SkASSERT(NULL != fScaledBitmap.getPixels());
         fBitmap = &fScaledBitmap;
 
         // set the inv matrix type to translate-only;
@@ -174,6 +206,7 @@ bool SkBitmapProcState::possiblyScaleImage() {
 
         // no need for any further filtering; we just did it!
         fFilterLevel = SkPaint::kNone_FilterLevel;
+        unlocker.keepLock();
         return true;
     }
 
@@ -248,6 +281,7 @@ bool SkBitmapProcState::possiblyScaleImage() {
                 fScaledBitmap.setPixels(level.fPixels);
                 fBitmap = &fScaledBitmap;
                 fFilterLevel = SkPaint::kLow_FilterLevel;
+                unlocker.keepLock();
                 return true;
             }
         }
@@ -273,15 +307,32 @@ static bool get_locked_pixels(const SkBitmap& src, int pow2, SkBitmap* dst) {
 }
 
 bool SkBitmapProcState::lockBaseBitmap() {
+    AutoScaledCacheUnlocker unlocker(&fScaledCacheID);
+
     SkPixelRef* pr = fOrigBitmap.pixelRef();
 
+    SkASSERT(NULL == fScaledCacheID);
+
     if (pr->isLocked() || !pr->implementsDecodeInto()) {
         // fast-case, no need to look in our cache
         fScaledBitmap = fOrigBitmap;
+        fScaledBitmap.lockPixels();
+        SkASSERT(NULL != fScaledBitmap.getPixels());
     } else {
         fScaledCacheID = SkScaledImageCache::FindAndLock(fOrigBitmap,
                                                          SK_Scalar1, SK_Scalar1,
                                                          &fScaledBitmap);
+        if (fScaledCacheID) {
+            fScaledBitmap.lockPixels();
+            if (!fScaledBitmap.getPixels()) {
+                fScaledBitmap.unlockPixels();
+                // found a purged entry (discardablememory?), release it
+                SkScaledImageCache::Unlock(fScaledCacheID);
+                fScaledCacheID = NULL;
+                // fall through to rebuild
+            }
+        }
+
         if (NULL == fScaledCacheID) {
             if (!get_locked_pixels(fOrigBitmap, 0, &fScaledBitmap)) {
                 return false;
@@ -299,13 +350,8 @@ bool SkBitmapProcState::lockBaseBitmap() {
             }
         }
     }
-    fScaledBitmap.lockPixels(); // just 'cause the cache made a copy :(
-    if (!fScaledBitmap.getPixels()) {
-        // TODO: find out how this can happen, and add a unittest to exercise
-        // inspired by BUG=chromium:295895
-        return false;
-    }
     fBitmap = &fScaledBitmap;
+    unlocker.keepLock();
     return true;
 }
 
@@ -334,6 +380,8 @@ bool SkBitmapProcState::chooseProcs(const SkMatrix& inv, const SkPaint& paint) {
     fInvMatrix = inv;
     fFilterLevel = paint.getFilterLevel();
 
+    SkASSERT(NULL == fScaledCacheID);
+
     // possiblyScaleImage will look to see if it can rescale the image as a
     // preprocess; either by scaling up to the target size, or by selecting
     // a nearby mipmap level.  If it does, it will adjust the working