Fix memory leak in display list canvas overdraw optimization.

Fix memory leak in display list canvas overdraw optimization.

This change simply adds a missing unref() call in the code
that handles overdraw optimization. The memory leak would
manifest itself as a race condition in animation loops that
use setTimeout or setInterval instead of
requestAnimationFrame: If a new frame starts before the
previous frame is consumed by the compositor, the previous
frame is to be droped. But in this case, the frame was
being leaked instead of dropped.

BUG=474510
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193398

[Justin Novosad, 2015-04-08 20:46:59]

junov@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Justin Novosad, 2015-04-08 20:47:00]

PTAL

[danakj, 2015-04-08 20:48:10]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-04-08 20:48:10]

https://codereview.chromium.org/1068413002/diff/1/Source/platform/graphics/Re...
File Source/platform/graphics/RecordingImageBufferSurface.cpp (right):

https://codereview.chromium.org/1068413002/diff/1/Source/platform/graphics/Re...
Source/platform/graphics/RecordingImageBufferSurface.cpp:150:
m_currentFrame->endRecording()->unref();
Maybe not, but is there a way to use RefPtr to not have to deal with this kind
of sadness?

[Justin Novosad, 2015-04-08 20:58:40]

On 2015/04/08 20:48:10, danakj wrote:
> Maybe not, but is there a way to use RefPtr to not have to deal with this kind
> of sadness?

Like this?

[Stephen White, 2015-04-08 21:00:09]

LGTM

As discussed, IWBN to have some kind of don't-ignore-my-return-value warning
goodness in this Skia call.

[jbroman, 2015-04-08 21:00:13]

https://codereview.chromium.org/1068413002/diff/1/Source/platform/graphics/Re...
File Source/platform/graphics/RecordingImageBufferSurface.cpp (right):

https://codereview.chromium.org/1068413002/diff/1/Source/platform/graphics/Re...
Source/platform/graphics/RecordingImageBufferSurface.cpp:150:
m_currentFrame->endRecording()->unref();
On 2015/04/08 20:48:10, danakj wrote:
> Maybe not, but is there a way to use RefPtr to not have to deal with this kind
> of sadness?

drive-by: I'm going to see if we can get endRecording marked with
SK_WARN_UNUSED_RESULT because not using that result is always a memory leak.

[danakj, 2015-04-08 21:00:13]

On 2015/04/08 20:58:40, junov wrote:
> On 2015/04/08 20:48:10, danakj wrote:
> > Maybe not, but is there a way to use RefPtr to not have to deal with this
kind
> > of sadness?
> 
> Like this?

That's how I'd ask for a fix in cc/. We have a strict policy of "any skia
pointer (deriving from SkRefCnt) *must* go into a smart pointer as the first
thing you do with it. always."

Cuz of sad stories like this. :(

Really really WTB smart refpointers from the Skia API. :(

[Stephen White, 2015-04-08 21:01:17]

On 2015/04/08 21:00:13, jbroman wrote:

> drive-by: I'm going to see if we can get endRecording marked with
> SK_WARN_UNUSED_RESULT because not using that result is always a memory leak.

^^^ Yeah, that. :)

[Justin Novosad, 2015-04-08 21:05:13]

On 2015/04/08 21:01:17, Stephen White wrote:
> On 2015/04/08 21:00:13, jbroman wrote:
> 
> > drive-by: I'm going to see if we can get endRecording marked with
> > SK_WARN_UNUSED_RESULT because not using that result is always a memory leak.
> 
> ^^^ Yeah, that. :)

Yes! I had the same thought when I was asking myself how would I write a test
for this: Make a compile-time test!

[Justin Novosad, 2015-04-08 21:05:56]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2015-04-08 21:06:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1068413002/20001

[danakj, 2015-04-08 21:07:30]

On Wed, Apr 8, 2015 at 2:05 PM, <junov@chromium.org> wrote:

> On 2015/04/08 21:01:17, Stephen White wrote:
>
>> On 2015/04/08 21:00:13, jbroman wrote:
>>
>
>  > drive-by: I'm going to see if we can get endRecording marked with
>> > SK_WARN_UNUSED_RESULT because not using that result is always a memory
>> leak.
>>
>
>  ^^^ Yeah, that. :)
>>
>
> Yes! I had the same thought when I was asking myself how would I write a
> test
> for this: Make a compile-time test!
>

Side note: Doing a bunch of work to build a thing that we just delete isn't
super amazing either? :)


>
> https://codereview.chromium.org/1068413002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Justin Novosad, 2015-04-08 21:09:14]

On 2015/04/08 21:07:30, danakj wrote:

> 
> Side note: Doing a bunch of work to build a thing that we just delete isn't
> super amazing either? :)
> 

LOL. This is why rAF exists.

[jbroman, 2015-04-08 21:15:23]

On 2015/04/08 21:01:17, Stephen White wrote:
> On 2015/04/08 21:00:13, jbroman wrote:
> 
> > drive-by: I'm going to see if we can get endRecording marked with
> > SK_WARN_UNUSED_RESULT because not using that result is always a memory leak.
> 
> ^^^ Yeah, that. :)

Out for review in Skia:
https://codereview.chromium.org/1072633002/

[commit-bot: I haz the power, 2015-04-09 00:02:47]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193398