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 936951cc8e532ec7b9c1165d2edeb43158462497..026381450cecab4cfb6f54a6d0528834a0bb8b43 100644 (file)
--- a/src/core/SkBitmapHeap.cpp
+++ b/src/core/SkBitmapHeap.cpp
@@ -367,7 +367,7 @@ int32_t SkBitmapHeap::insert(const SkBitmap& originalBitmap) {
     // 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 0a7bd904a3f9c27b1ea3e76ee91bc1564a79b5a0..29c036e49d07c578f03773fa8f3e972c624573bc 100644 (file)
--- a/src/core/SkOrderedReadBuffer.cpp
+++ b/src/core/SkOrderedReadBuffer.cpp
@@ -187,6 +187,7 @@ void SkOrderedReadBuffer::readBitmap(SkBitmap* bitmap) {
     } 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 bdd9516b25adeb1fd062f2582cc25c43da3e0953..e43a111ddc7bc7a0ec2be490b4f3b6cc0e390b6f 100644 (file)
--- 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 @@ void SkOrderedWriteBuffer::writeBitmap(const SkBitmap& bitmap) {
         // 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 681efedbddecd0516070b7d775b5f4ad5d996358..dd477f71d92a36beaeff59f87ba6d53d1f6ec997 100644 (file)
--- 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 2c27971f09d4a87713103b949c8aa6da9958f58d..bb5410871c273f506c674f2decbcf68b14268a7e 100644 (file)
--- 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 TestDeferredCanvasSkip(skiatest::Reporter* reporter) {
 
 }
 
+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 @@ static void TestDeferredCanvas(skiatest::Reporter* reporter) {
     TestDeferredCanvasMemoryLimit(reporter);
     TestDeferredCanvasBitmapCaching(reporter);
     TestDeferredCanvasSkip(reporter);
+    TestDeferredCanvasBitmapShaderNoLeak(reporter);
 }
 
 #include "TestClassDef.h"