Fixing refcount leak in SkBitmapHeap caused by collisions in SkFlatDictionary
BUG=http://code.google.com/p/chromium/issues/detail?id=155875
TEST=DeferredCanvas unit test, subtest TestDeferredCanvasBitmapShaderNoLeak
Review URL: https://codereview.appspot.com/6713048
git-svn-id: http://skia.googlecode.com/svn/trunk@5982 2bbb7eff-a529-9590-31e7-b0007b416f81
diff --git a/src/core/SkBitmapHeap.cpp b/src/core/SkBitmapHeap.cpp
index 936951c..0263814 100644
--- a/src/core/SkBitmapHeap.cpp
+++ b/src/core/SkBitmapHeap.cpp
@@ -367,7 +367,7 @@
// TODO if there is a shared pixel ref don't count it
// If the SkBitmap does not share an SkPixelRef with an SkBitmap already
// in the SharedHeap, also include the size of its pixels.
- entry->fBytesAllocated += originalBitmap.getSize();
+ entry->fBytesAllocated = originalBitmap.getSize();
// add the bytes from this entry to the total count
fBytesAllocated += entry->fBytesAllocated;
diff --git a/src/core/SkOrderedReadBuffer.cpp b/src/core/SkOrderedReadBuffer.cpp
index 0a7bd90..29c036e 100644
--- a/src/core/SkOrderedReadBuffer.cpp
+++ b/src/core/SkOrderedReadBuffer.cpp
@@ -187,6 +187,7 @@
} else {
if (fBitmapStorage) {
const uint32_t index = fReader.readU32();
+ fReader.readU32(); // bitmap generation ID (see SkOrderedWriteBuffer::writeBitmap)
*bitmap = *fBitmapStorage->getBitmap(index);
fBitmapStorage->releaseRef(index);
} else {
diff --git a/src/core/SkOrderedWriteBuffer.cpp b/src/core/SkOrderedWriteBuffer.cpp
index bdd9516..e43a111 100644
--- a/src/core/SkOrderedWriteBuffer.cpp
+++ b/src/core/SkOrderedWriteBuffer.cpp
@@ -7,6 +7,7 @@
*/
#include "SkOrderedWriteBuffer.h"
+#include "SkBitmap.h"
#include "SkPtrRecorder.h"
#include "SkStream.h"
#include "SkTypeface.h"
@@ -164,7 +165,15 @@
// Bitmap was not encoded. Record a zero, implying that the reader need not decode.
this->writeUInt(0);
if (fBitmapHeap) {
- fWriter.write32(fBitmapHeap->insert(bitmap));
+ int32_t slot = fBitmapHeap->insert(bitmap);
+ fWriter.write32(slot);
+ // crbug.com/155875
+ // The generation ID is not required information. We write it to prevent collisions
+ // in SkFlatDictionary. It is possible to get a collision when a previously
+ // unflattened (i.e. stale) instance of a similar flattenable is in the dictionary
+ // and the instance currently being written is re-using the same slot from the
+ // bitmap heap.
+ fWriter.write32(bitmap.getGenerationID());
} else {
bitmap.flatten(*this);
}
diff --git a/src/core/SkOrderedWriteBuffer.h b/src/core/SkOrderedWriteBuffer.h
index 681efed..dd477f7 100644
--- a/src/core/SkOrderedWriteBuffer.h
+++ b/src/core/SkOrderedWriteBuffer.h
@@ -12,12 +12,12 @@
#include "SkFlattenableBuffers.h"
#include "SkRefCnt.h"
-#include "SkBitmap.h"
#include "SkBitmapHeap.h"
#include "SkPath.h"
#include "SkSerializationHelpers.h"
#include "SkWriter32.h"
+class SkBitmap;
class SkFlattenable;
class SkFactorySet;
class SkNamedFactorySet;
diff --git a/tests/DeferredCanvasTest.cpp b/tests/DeferredCanvasTest.cpp
index 2c27971..bb54108 100644
--- a/tests/DeferredCanvasTest.cpp
+++ b/tests/DeferredCanvasTest.cpp
@@ -7,6 +7,7 @@
*/
#include "Test.h"
#include "SkBitmap.h"
+#include "SkBitmapProcShader.h"
#include "SkDeferredCanvas.h"
#include "SkDevice.h"
#include "SkShader.h"
@@ -348,6 +349,48 @@
}
+static void TestDeferredCanvasBitmapShaderNoLeak(skiatest::Reporter* reporter) {
+ // This is a regression test for crbug.com/155875
+ // This test covers a code path that inserts bitmaps into the bitmap heap through the
+ // flattening of SkBitmapProcShaders. The refcount in the bitmap heap is maintained through
+ // the flattening and unflattening of the shader.
+ SkBitmap store;
+ store.setConfig(SkBitmap::kARGB_8888_Config, 100, 100);
+ store.allocPixels();
+ SkDevice device(store);
+ SkDeferredCanvas canvas(&device);
+ // test will fail if nbIterations is not in sync with
+ // BITMAPS_TO_KEEP in SkGPipeWrite.cpp
+ const int nbIterations = 5;
+ size_t bytesAllocated = 0;
+ for(int pass = 0; pass < 2; ++pass) {
+ for(int i = 0; i < nbIterations; ++i) {
+ SkPaint paint;
+ SkBitmap paintPattern;
+ paintPattern.setConfig(SkBitmap::kARGB_8888_Config, 10, 10);
+ paintPattern.allocPixels();
+ paint.setShader(SkNEW_ARGS(SkBitmapProcShader,
+ (paintPattern, SkShader::kClamp_TileMode, SkShader::kClamp_TileMode)))->unref();
+ canvas.drawPaint(paint);
+ canvas.flush();
+
+ // In the first pass, memory allocation should be monotonically increasing as
+ // the bitmap heap slots fill up. In the second pass memory allocation should be
+ // stable as bitmap heap slots get recycled.
+ size_t newBytesAllocated = canvas.storageAllocatedForRecording();
+ if (pass == 0) {
+ REPORTER_ASSERT(reporter, newBytesAllocated > bytesAllocated);
+ bytesAllocated = newBytesAllocated;
+ } else {
+ REPORTER_ASSERT(reporter, newBytesAllocated == bytesAllocated);
+ }
+ }
+ }
+ // All cached resources should be evictable since last canvas call was flush()
+ canvas.freeMemoryIfPossible(~0);
+ REPORTER_ASSERT(reporter, 0 == canvas.storageAllocatedForRecording());
+}
+
static void TestDeferredCanvas(skiatest::Reporter* reporter) {
TestDeferredCanvasBitmapAccess(reporter);
TestDeferredCanvasFlush(reporter);
@@ -355,6 +398,7 @@
TestDeferredCanvasMemoryLimit(reporter);
TestDeferredCanvasBitmapCaching(reporter);
TestDeferredCanvasSkip(reporter);
+ TestDeferredCanvasBitmapShaderNoLeak(reporter);
}
#include "TestClassDef.h"