Fixing refcount leak in SkBitmapHeap

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
Committed: https://code.google.com/p/skia/source/detail?r=5982

[reed1, 2012-10-16 21:02:17]

Do we already have, or do you have in mind, a test that triggers this bug
(leak), so we can see the before and after?

[Leon, 2012-10-16 21:24:07]

On 2012/10/16 20:27:48, junov1 wrote:

At first glance this looks appropriate, but I don't understand how this changes
anything.

Here is how I expect fDeferAddingOwners to work.

fDeferAddingOwners is set to true by SkGPipeCanvas::flattenToIndex. Then the
SkFlattenable (i.e. BitmapShader) is flattened and compared to the existing
flattened SkFlattenables. If the newly flattened data is new, SkBitmapHeap
increases the owner count afterwards, which will be decreased later when the
flat data is unflattened.

However, if the flat data is a duplicate, SkBitmapHeap does not end up
increasing the owner count, because the flat data will not be unflattened
(again), so its owner count would not be decreased as a result.

So, if the bitmap is new to the SkBitmapHeap, I would expect that the flattened
data will always be new, therefore we will always end up adding the references
anyway (the first case).

On the other hand, I just applied it locally and it fixes the problem on this
website, so I am in favor of the change. Will it be possible to come up with a
test?

[junov1, 2012-10-17 13:23:59]

@reed: I will definitely add a test once I have a better understanding of what
is going on.

@scroggo: This patch is not quite right.  It eliminates the memory leak, but
causes other problems. I am seeing glitches when several bitmaps are in play.  I
think it releases refcounts in some cases where it shouldn't (opposite problem
from before). As a result referenced bitmaps sometimes get replaced. I will
continue digging, but it seems we are looking at the right part of the code.

[junov1, 2012-10-17 14:56:12]

Ok, I figured it out now.  The problem was cause by collision in
SkFlatDictionary.  It turns out that two SkBitmapProcShaders that are different
can be identical once flattened if they reuse the same slot in the bitmap heap. 
This happens when the first instance was unflattened (so bitmap heap slot is
unrefed), then we flatten a second that uses a different bitmap.  The second
instance recycles the bitmap slot of the first instance, resulting in an
identical flattened representation.

Preliminary CL take 2 on the way...

[Leon, 2012-10-17 15:23:16]

On 2012/10/17 14:56:12, junov1 wrote:
> Ok, I figured it out now.  The problem was cause by collision in
> SkFlatDictionary.  It turns out that two SkBitmapProcShaders that are
different
> can be identical once flattened if they reuse the same slot in the bitmap
heap. 
> This happens when the first instance was unflattened (so bitmap heap slot is
> unrefed), then we flatten a second that uses a different bitmap.  The second
> instance recycles the bitmap slot of the first instance, resulting in an
> identical flattened representation.
> 
> Preliminary CL take 2 on the way...

Exactly what I was thinking.

[junov1, 2012-10-17 15:29:04]

New Patch.

No test yet... it's on the way.

[Leon, 2012-10-17 16:00:36]

lgtm

https://codereview.appspot.com/6713048/diff/4002/src/core/SkOrderedReadBuffer...
File src/core/SkOrderedReadBuffer.cpp (right):

https://codereview.appspot.com/6713048/diff/4002/src/core/SkOrderedReadBuffer...
src/core/SkOrderedReadBuffer.cpp:190: fReader.readU32();
Could you add a comment here explaining why this is necessary? (It's obvious
from the other file, but not from this code.)

https://codereview.appspot.com/6713048/diff/4002/src/core/SkOrderedWriteBuffe...
File src/core/SkOrderedWriteBuffer.cpp (right):

https://codereview.appspot.com/6713048/diff/4002/src/core/SkOrderedWriteBuffe...
src/core/SkOrderedWriteBuffer.cpp:175:
fWriter.write32(fBitmapHeap->getBitmap(slot)->pixelRef()->getGenerationID());
fWriter.write32(bitmap.getGenerationID());

Simpler, plus you don't need to include SkPixelRef.h (although now that I look
into it, we currently include SkBitmap.h in the header file unnecessarily, so if
we changed that to a forward declaration, you'd need to include SkBitmap.h
here).

[junov1, 2012-10-17 17:53:48]

New patch with corrections + new test

https://codereview.appspot.com/6713048/diff/10001/src/core/SkBitmapHeap.cpp
File src/core/SkBitmapHeap.cpp (right):

https://codereview.appspot.com/6713048/diff/10001/src/core/SkBitmapHeap.cpp#n...
src/core/SkBitmapHeap.cpp:370: entry->fBytesAllocated =
originalBitmap.getSize();
Correcting an accounting error that was revealed by the new test.  When
replacing an existing entry we were adding the new size to the old size, which
was incorrect.

[Leon, 2012-10-17 19:19:26]

LGTM

[reed1, 2012-10-17 19:36:52]

Can a test be written strictly in terms of SkBitmapHeap's api? Just wondering,
as that seems more/also useful/direct.

[junov1, 2012-10-17 19:48:02]

On 2012/10/17 19:36:52, reed1 wrote:
> Can a test be written strictly in terms of SkBitmapHeap's api? Just wondering,
> as that seems more/also useful/direct.

To repro the bug you need Something that uses an SkFlatDictionary, SkBitmapHeap,
and a flattenable object type that refs a bitmap, and you have to replicate some
of the flushing behaviour of the DeferredCanvasPipeController.
It would be tricky and would involve an interesting ecosystem of fakes and mocks
to repro without using SkDeferredCanvas or SkGPipe.