Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(89)

Issue 6713048: Fixing refcount leak in SkBitmapHeap (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 1 month ago by junov1
Modified:
12 years, 1 month ago
Reviewers:
Leon, reed1
CC:
skia-review_googlegroups.com
Base URL:
http://skia.googlecode.com/svn/trunk/
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : #

Total comments: 2

Patch Set 3 : #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+57 lines, -3 lines) Patch
M src/core/SkBitmapHeap.cpp View 1 2 1 chunk +1 line, -1 line 1 comment Download
M src/core/SkOrderedReadBuffer.cpp View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M src/core/SkOrderedWriteBuffer.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M src/core/SkOrderedWriteBuffer.cpp View 1 2 2 chunks +10 lines, -1 line 0 comments Download
M tests/DeferredCanvasTest.cpp View 1 2 3 chunks +44 lines, -0 lines 0 comments Download

Messages

Total messages: 12
junov1
12 years, 1 month ago (2012-10-16 20:27:48 UTC) #1
reed1
Do we already have, or do you have in mind, a test that triggers this ...
12 years, 1 month ago (2012-10-16 21:02:17 UTC) #2
Leon
On 2012/10/16 20:27:48, junov1 wrote: At first glance this looks appropriate, but I don't understand ...
12 years, 1 month ago (2012-10-16 21:24:07 UTC) #3
junov1
@reed: I will definitely add a test once I have a better understanding of what ...
12 years, 1 month ago (2012-10-17 13:23:59 UTC) #4
junov1
Ok, I figured it out now. The problem was cause by collision in SkFlatDictionary. It ...
12 years, 1 month ago (2012-10-17 14:56:12 UTC) #5
Leon
On 2012/10/17 14:56:12, junov1 wrote: > Ok, I figured it out now. The problem was ...
12 years, 1 month ago (2012-10-17 15:23:16 UTC) #6
junov1
New Patch. No test yet... it's on the way.
12 years, 1 month ago (2012-10-17 15:29:04 UTC) #7
Leon
lgtm https://codereview.appspot.com/6713048/diff/4002/src/core/SkOrderedReadBuffer.cpp File src/core/SkOrderedReadBuffer.cpp (right): https://codereview.appspot.com/6713048/diff/4002/src/core/SkOrderedReadBuffer.cpp#newcode190 src/core/SkOrderedReadBuffer.cpp:190: fReader.readU32(); Could you add a comment here explaining ...
12 years, 1 month ago (2012-10-17 16:00:36 UTC) #8
junov1
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#newcode370 src/core/SkBitmapHeap.cpp:370: entry->fBytesAllocated = ...
12 years, 1 month ago (2012-10-17 17:53:48 UTC) #9
Leon
LGTM
12 years, 1 month ago (2012-10-17 19:19:26 UTC) #10
reed1
Can a test be written strictly in terms of SkBitmapHeap's api? Just wondering, as that ...
12 years, 1 month ago (2012-10-17 19:36:52 UTC) #11
junov1
12 years, 1 month ago (2012-10-17 19:48:02 UTC) #12
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b