From ce65f385a0d31a93a31ffd57478de4b8c4e833b3 Mon Sep 17 00:00:00 2001 From: "junov@chromium.org" Date: Wed, 17 Oct 2012 19:36:09 +0000 Subject: [PATCH] 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 --- src/core/SkBitmapHeap.cpp | 2 +- src/core/SkOrderedReadBuffer.cpp | 1 + src/core/SkOrderedWriteBuffer.cpp | 11 +++++++- src/core/SkOrderedWriteBuffer.h | 2 +- tests/DeferredCanvasTest.cpp | 44 +++++++++++++++++++++++++++++++ 5 files changed, 57 insertions(+), 3 deletions(-) diff --git a/src/core/SkBitmapHeap.cpp b/src/core/SkBitmapHeap.cpp index 936951cc8e..026381450c 100644 --- 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 0a7bd904a3..29c036e49d 100644 --- 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 bdd9516b25..e43a111ddc 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 @@ 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 681efedbdd..dd477f71d9 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 2c27971f09..bb5410871c 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 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"