detect if the scaledimagecache returns a purged bitmap

BUG=
R=scroggo@google.com

Review URL: https://codereview.chromium.org/110383005

git-svn-id: http://skia.googlecode.com/svn/trunk/src@12654 2bbb7eff-a529-9590-31e7-b0007b416f81

4 files changed, 114 insertions, 16 deletions

diff --git a/core/SkBitmapProcState.cpp b/core/SkBitmapProcState.cpp
index 39f6a780..cdc582bf 100644
--- a/core/SkBitmapProcState.cpp
+++ b/core/SkBitmapProcState.cpp
@@ -106,11 +106,33 @@ static SkScalar effective_matrix_scale_sqrd(const SkMatrix& mat) {
     return SkMaxScalar(v1.lengthSqd(), v2.lengthSqd());
 }
 
+class AutoScaledCacheUnlocker {
+public:
+    AutoScaledCacheUnlocker(SkScaledImageCache::ID** idPtr) : fIDPtr(idPtr) {}
+    ~AutoScaledCacheUnlocker() {
+        if (fIDPtr && *fIDPtr) {
+            SkScaledImageCache::Unlock(*fIDPtr);
+            *fIDPtr = NULL;
+        }
+    }
+
+    // forgets the ID, so it won't call Unlock
+    void release() {
+        fIDPtr = NULL;
+    }
+
+private:
+    SkScaledImageCache::ID** fIDPtr;
+};
+#define AutoScaledCacheUnlocker(...) SK_REQUIRE_LOCAL_VAR(AutoScaledCacheUnlocker)
+
 // 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 +154,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 +187,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 +208,7 @@ bool SkBitmapProcState::possiblyScaleImage() {
 
         // no need for any further filtering; we just did it!
         fFilterLevel = SkPaint::kNone_FilterLevel;
+        unlocker.release();
         return true;
     }
 
@@ -248,6 +283,7 @@ bool SkBitmapProcState::possiblyScaleImage() {
                 fScaledBitmap.setPixels(level.fPixels);
                 fBitmap = &fScaledBitmap;
                 fFilterLevel = SkPaint::kLow_FilterLevel;
+                unlocker.release();
                 return true;
             }
         }
@@ -273,15 +309,34 @@ 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();
+        if (NULL == fScaledBitmap.getPixels()) {
+            return false;
+        }
     } 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 +354,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.release();
     return true;
 }
 
@@ -334,6 +384,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
diff --git a/core/SkBitmapScaler.cpp b/core/SkBitmapScaler.cpp
index c28d4779..67a9508e 100644
--- a/core/SkBitmapScaler.cpp
+++ b/core/SkBitmapScaler.cpp
@@ -301,6 +301,8 @@ bool SkBitmapScaler::Resize(SkBitmap* resultPtr,
         convolveProcs, true);
 
     *resultPtr = result;
+    resultPtr->lockPixels();
+    SkASSERT(NULL != resultPtr->getPixels());
     return true;
 }
 
diff --git a/core/SkScaledImageCache.cpp b/core/SkScaledImageCache.cpp
index b3956f4a..7c8b6649 100644
--- a/core/SkScaledImageCache.cpp
+++ b/core/SkScaledImageCache.cpp
@@ -239,9 +239,18 @@ void* SkOneShotDiscardablePixelRef::onLockPixels(SkColorTable** ctable) {
         return fDM->data();
     }
 
-    SkASSERT(!fIsLocked);
-    fIsLocked = fDM->lock();
-    return fIsLocked ? fDM->data() : NULL;
+    // A previous call to onUnlock may have deleted our DM, so check for that
+    if (NULL == fDM) {
+        return NULL;
+    }
+
+    if (!fDM->lock()) {
+        // since it failed, we delete it now, to free-up the resource
+        delete fDM;
+        fDM = NULL;
+        return NULL;
+    }
+    return fDM->data();
 }
 
 void SkOneShotDiscardablePixelRef::onUnlockPixels() {
@@ -613,6 +622,8 @@ void SkScaledImageCache::addToHead(Rec* rec) {
     this->validate();
 }
 
+///////////////////////////////////////////////////////////////////////////////
+
 #ifdef SK_DEBUG
 void SkScaledImageCache::validate() const {
     if (NULL == fHead) {
@@ -658,6 +669,21 @@ void SkScaledImageCache::validate() const {
 }
 #endif
 
+void SkScaledImageCache::dump() const {
+    this->validate();
+
+    const Rec* rec = fHead;
+    int locked = 0;
+    while (rec) {
+        locked += rec->fLockCount > 0;
+        rec = rec->fNext;
+    }
+
+    SkDebugf("SkScaledImageCache: count=%d bytes=%d locked=%d %s\n",
+             fCount, fBytesUsed, locked,
+             fDiscardableFactory ? "discardable" : "malloc");
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 
 #include "SkThread.h"
@@ -730,7 +756,9 @@ SkScaledImageCache::ID* SkScaledImageCache::AddAndLockMip(const SkBitmap& orig,
 
 void SkScaledImageCache::Unlock(SkScaledImageCache::ID* id) {
     SkAutoMutexAcquire am(gMutex);
-    return get_cache()->unlock(id);
+    get_cache()->unlock(id);
+
+//    get_cache()->dump();
 }
 
 size_t SkScaledImageCache::GetBytesUsed() {
@@ -753,6 +781,11 @@ SkBitmap::Allocator* SkScaledImageCache::GetAllocator() {
     return get_cache()->allocator();
 }
 
+void SkScaledImageCache::Dump() {
+    SkAutoMutexAcquire am(gMutex);
+    get_cache()->dump();
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 
 #include "SkGraphics.h"
diff --git a/core/SkScaledImageCache.h b/core/SkScaledImageCache.h
index 311db325..fe072306 100644
--- a/core/SkScaledImageCache.h
+++ b/core/SkScaledImageCache.h
@@ -66,6 +66,11 @@ public:
 
     static SkBitmap::Allocator* GetAllocator();
 
+    /**
+     *  Call SkDebugf() with diagnostic information about the state of the cache
+     */
+    static void Dump();
+
     ///////////////////////////////////////////////////////////////////////////
 
     /**
@@ -151,6 +156,11 @@ public:
 
     SkBitmap::Allocator* allocator() const { return fAllocator; };
 
+    /**
+     *  Call SkDebugf() with diagnostic information about the state of the cache
+     */
+    void dump() const;
+
 public:
     struct Rec;
     struct Key;
@@ -174,6 +184,7 @@ private:
     Rec* findAndLock(const Key& key);
     ID* addAndLock(Rec* rec);
 
+    void purgeRec(Rec*);
     void purgeAsNeeded();
 
     // linklist management