From 547e66531d521eb1eadac87edb0f79f8c2f1bbe0 Mon Sep 17 00:00:00 2001 From: Chet Haase Date: Mon, 22 Oct 2012 15:07:26 -0700 Subject: [PATCH] Don't null the reference to Bitmap pixels until we're really ready A change in the VM triggers a native memory error more aggressively than before, showing that there's a bug in the logic of recycling bitmaps. Since the pixel memory is allocated on the Java heap, nulling out the reference to that memory in the Java level Bitmap object can cause that memory to get collected at any time. Meanwhile, we may have a reference to that memory at the native level for rendering purposes, causing an error if/when we access that memory after it has been collected by the VM. The fix is to avoid setting the reference to the pixels to null unless we are not referring to it in native code. This is determined at the time we call recycle() - we return a boolean to indicate whether the native code is still using the memory. if not, the Java code can null out the reference and allow the VM to collect it. Otherwise, it will get collected later when the encompassing Bitmap object is collected. Issue #7339156 HTML5 tests crash the app (Vellamo) Change-Id: I3a0d6b9a6c5dd3b86cc2b0ff7719007e774b5e3c --- core/jni/android/graphics/Bitmap.cpp | 8 ++++---- graphics/java/android/graphics/Bitmap.java | 13 +++++++++---- libs/hwui/ResourceCache.cpp | 21 ++++++++++++++++----- libs/hwui/ResourceCache.h | 4 ++-- 4 files changed, 31 insertions(+), 15 deletions(-) diff --git a/core/jni/android/graphics/Bitmap.cpp b/core/jni/android/graphics/Bitmap.cpp index f485e03576381..63683b4f49542 100644 --- a/core/jni/android/graphics/Bitmap.cpp +++ b/core/jni/android/graphics/Bitmap.cpp @@ -261,14 +261,14 @@ static void Bitmap_destructor(JNIEnv* env, jobject, SkBitmap* bitmap) { delete bitmap; } -static void Bitmap_recycle(JNIEnv* env, jobject, SkBitmap* bitmap) { +static jboolean Bitmap_recycle(JNIEnv* env, jobject, SkBitmap* bitmap) { #ifdef USE_OPENGL_RENDERER if (android::uirenderer::Caches::hasInstance()) { - android::uirenderer::Caches::getInstance().resourceCache.recycle(bitmap); - return; + return android::uirenderer::Caches::getInstance().resourceCache.recycle(bitmap); } #endif // USE_OPENGL_RENDERER bitmap->setPixels(NULL, NULL); + return true; } // These must match the int values in Bitmap.java @@ -665,7 +665,7 @@ static JNINativeMethod gBitmapMethods[] = { { "nativeCopy", "(IIZ)Landroid/graphics/Bitmap;", (void*)Bitmap_copy }, { "nativeDestructor", "(I)V", (void*)Bitmap_destructor }, - { "nativeRecycle", "(I)V", (void*)Bitmap_recycle }, + { "nativeRecycle", "(I)Z", (void*)Bitmap_recycle }, { "nativeCompress", "(IIILjava/io/OutputStream;[B)Z", (void*)Bitmap_compress }, { "nativeErase", "(II)V", (void*)Bitmap_erase }, diff --git a/graphics/java/android/graphics/Bitmap.java b/graphics/java/android/graphics/Bitmap.java index 22ecc6135172c..688fd7aa53550 100644 --- a/graphics/java/android/graphics/Bitmap.java +++ b/graphics/java/android/graphics/Bitmap.java @@ -201,9 +201,14 @@ public final class Bitmap implements Parcelable { */ public void recycle() { if (!mRecycled) { - mBuffer = null; - nativeRecycle(mNativeBitmap); - mNinePatchChunk = null; + if (nativeRecycle(mNativeBitmap)) { + // return value indicates whether native pixel object was actually recycled. + // false indicates that it is still in use at the native level and these + // objects should not be collected now. They will be collected later when the + // Bitmap itself is collected. + mBuffer = null; + mNinePatchChunk = null; + } mRecycled = true; } } @@ -1391,7 +1396,7 @@ public final class Bitmap implements Parcelable { private static native Bitmap nativeCopy(int srcBitmap, int nativeConfig, boolean isMutable); private static native void nativeDestructor(int nativeBitmap); - private static native void nativeRecycle(int nativeBitmap); + private static native boolean nativeRecycle(int nativeBitmap); private static native boolean nativeCompress(int nativeBitmap, int format, int quality, OutputStream stream, diff --git a/libs/hwui/ResourceCache.cpp b/libs/hwui/ResourceCache.cpp index 81f7b94b31eea..347bd78ca93b1 100644 --- a/libs/hwui/ResourceCache.cpp +++ b/libs/hwui/ResourceCache.cpp @@ -265,27 +265,38 @@ void ResourceCache::destructorLocked(SkiaColorFilter* resource) { } } -void ResourceCache::recycle(SkBitmap* resource) { +/** + * Return value indicates whether resource was actually recycled, which happens when RefCnt + * reaches 0. + */ +bool ResourceCache::recycle(SkBitmap* resource) { Mutex::Autolock _l(mLock); - recycleLocked(resource); + return recycleLocked(resource); } -void ResourceCache::recycleLocked(SkBitmap* resource) { +/** + * Return value indicates whether resource was actually recycled, which happens when RefCnt + * reaches 0. + */ +bool ResourceCache::recycleLocked(SkBitmap* resource) { ssize_t index = mCache->indexOfKey(resource); if (index < 0) { // not tracking this resource; just recycle the pixel data resource->setPixels(NULL, NULL); - return; + return true; } ResourceReference* ref = mCache->valueAt(index); if (ref == NULL) { // Should not get here - shouldn't get a call to recycle if we're not yet tracking it - return; + return true; } ref->recycled = true; if (ref->refCount == 0) { deleteResourceReferenceLocked(resource, ref); + return true; } + // Still referring to resource, don't recycle yet + return false; } /** diff --git a/libs/hwui/ResourceCache.h b/libs/hwui/ResourceCache.h index a80670ce36ed1..ab493e54e0b3a 100644 --- a/libs/hwui/ResourceCache.h +++ b/libs/hwui/ResourceCache.h @@ -99,8 +99,8 @@ public: void destructorLocked(SkiaShader* resource); void destructorLocked(SkiaColorFilter* resource); - void recycle(SkBitmap* resource); - void recycleLocked(SkBitmap* resource); + bool recycle(SkBitmap* resource); + bool recycleLocked(SkBitmap* resource); private: void deleteResourceReferenceLocked(void* resource, ResourceReference* ref);