cc: Don't leak a ref to SkColorFilter.

cc: Don't leak a ref to SkColorFilter.

The SkColorFilter* we get in GLRenderer is a refcounted
object with a reference on it. Since we just store it in
a raw pointer, we leak that reference. Instead, store
it in a RefPtr.

R=senorblanco
BUG=259815
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213283

[earthdok, 2013-07-16 18:57:37]

This does not fix the issue for me.

[danakj, 2013-07-16 19:48:15]

On 2013/07/16 18:57:37, earthdok wrote:
> This does not fix the issue for me.

Hm ok. Is there something I can do locally to test this?

[danakj, 2013-07-16 19:53:13]

On 2013/07/16 19:48:15, danakj wrote:
> On 2013/07/16 18:57:37, earthdok wrote:
> > This does not fix the issue for me.
> 
> Hm ok. Is there something I can do locally to test this?

I think I see where the problem is for this one..

[danakj, 2013-07-17 23:08:28]

PTAL. This should solve the refleak of the SkImageFilter.

[awong, 2013-07-17 23:12:59]

https://codereview.chromium.org/19267024/diff/13001/skia/ext/refptr.h
File skia/ext/refptr.h (right):

https://codereview.chromium.org/19267024/diff/13001/skia/ext/refptr.h#newcode104
skia/ext/refptr.h:104: Receiver(RefPtr* refptr) : refptr_(refptr), ptr_(NULL) {
explicit?

https://codereview.chromium.org/19267024/diff/13001/skia/ext/refptr.h#newcode114
skia/ext/refptr.h:114: Receiver ReceiveAndAdoptRef() { return Receiver(this); }
Add comment saying to not use this as part of a subexpression?

[danakj, 2013-07-17 23:18:46]

https://codereview.chromium.org/19267024/diff/13001/skia/ext/refptr.h
File skia/ext/refptr.h (right):

https://codereview.chromium.org/19267024/diff/13001/skia/ext/refptr.h#newcode104
skia/ext/refptr.h:104: Receiver(RefPtr* refptr) : refptr_(refptr), ptr_(NULL) {
On 2013/07/17 23:12:59, awong wrote:
> explicit?

Done.

https://codereview.chromium.org/19267024/diff/13001/skia/ext/refptr.h#newcode114
skia/ext/refptr.h:114: Receiver ReceiveAndAdoptRef() { return Receiver(this); }
On 2013/07/17 23:12:59, awong wrote:
> Add comment saying to not use this as part of a subexpression?

Good point! Done, with examples.

[awong, 2013-07-17 23:51:02]

https://codereview.chromium.org/19267024/diff/22001/skia/ext/refptr.h
File skia/ext/refptr.h (right):

https://codereview.chromium.org/19267024/diff/22001/skia/ext/refptr.h#newcode115
skia/ext/refptr.h:115: // sets it to an object with an unowned reference.
suggestion:
Call this and pass the returned temporary to a function taking a T** argument.
The returned temporary will set the original RefPtr to an object with an unowned
reference.

https://codereview.chromium.org/19267024/diff/22001/skia/ext/refptr.h#newcode116
skia/ext/refptr.h:116: // NOTE: Do not use this function as part of a
subexpression, and try to use
suggestion:
Do not use this function as part of a subexpression where the RefPtr is also
used somewhere else. Destructors of temporaries are only run at the end of a
full expression so the initialization will not be complete and the RefPtr will
remain empty.

[danakj, 2013-07-17 23:58:29]

https://codereview.chromium.org/19267024/diff/22001/skia/ext/refptr.h
File skia/ext/refptr.h (right):

https://codereview.chromium.org/19267024/diff/22001/skia/ext/refptr.h#newcode116
skia/ext/refptr.h:116: // NOTE: Do not use this function as part of a
subexpression, and try to use
On 2013/07/17 23:51:02, awong wrote:
> suggestion:
> Do not use this function as part of a subexpression where the RefPtr is also
> used somewhere else. Destructors of temporaries are only run at the end of a
> full expression so the initialization will not be complete and the RefPtr will
> remain empty.

Thanks, will do.

I think that I can also DCHECK this to guarantee safety (tho it requires adding
a bool RefPtr and adding DCHECK to a lot of places, so I think only in debug
builds would be appropriate).

[danakj, 2013-07-17 23:59:18]

Oh, this file is included transitively from webkit, so it can't include
base/logging :|

[earthdok, 2013-07-18 09:21:32]

On 2013/07/16 19:48:15, danakj wrote:
> On 2013/07/16 18:57:37, earthdok wrote:
> > This does not fix the issue for me.
> 
> Hm ok. Is there something I can do locally to test this?

Sorry, didn't see your question - somehow I'm not on the CC list anymore. The
general instructions are at
http://www.chromium.org/developers/testing/leaksanitizer (you can probably
ignore the part about checking out fresh clang - the one that comes with chrome
should be enough).

[danakj, 2013-07-18 15:31:51]

Oh, not sure how you were removed. The latest CL should fix the leak.

[danakj, 2013-07-18 15:34:32]

PTAL, added some debug-crash assertions without using base/logging which is a
compiler error at this time to do so. (see patch set 5 errors)

Also added unit tests.

[Stephen White, 2013-07-18 15:50:30]

https://codereview.chromium.org/19267024/diff/30001/skia/ext/refptr.h
File skia/ext/refptr.h (right):

https://codereview.chromium.org/19267024/diff/30001/skia/ext/refptr.h#newcode144
skia/ext/refptr.h:144: // the RefPtr will remain empty.
Maybe I'm old-fashioned, but I'm wondering if this is just too clever. Would it
be so bad to simply continue to pass a raw pointer to the function that expects
one, and then AdoptRef() it into a skia::RefPtr? There wouldn't be any explicit
ref/unref, so I don't think it would violate any of the principles we've set for
ourselves. The only ugliness would be the existing raw pointer, but it's in the
same scope as the RefPtr, so at least its lifetime is clear. And we wouldn't
need all this code and checking. JMHO.

[danakj, 2013-07-18 15:59:36]

https://codereview.chromium.org/19267024/diff/30001/skia/ext/refptr.h
File skia/ext/refptr.h (right):

https://codereview.chromium.org/19267024/diff/30001/skia/ext/refptr.h#newcode144
skia/ext/refptr.h:144: // the RefPtr will remain empty.
On 2013/07/18 15:50:30, Stephen White wrote:
> Maybe I'm old-fashioned, but I'm wondering if this is just too clever. Would
it
> be so bad to simply continue to pass a raw pointer to the function that
expects
> one, and then AdoptRef() it into a skia::RefPtr? There wouldn't be any
explicit
> ref/unref, so I don't think it would violate any of the principles we've set
for
> ourselves. The only ugliness would be the existing raw pointer, but it's in
the
> same scope as the RefPtr, so at least its lifetime is clear. And we wouldn't
> need all this code and checking. JMHO.

My own preference would be to never receive something skia refcounted into a raw
pointer ever, and to raise reviewer alarm bells when a raw pointer appears.

RawPtr* foo = NULL;
asRawPtr(&foo);
skia::RefPtr<RawPtr> foo_ref = skia::AdoptRef(foo);

Compared to:

skia::RefPtr<RawPtr> foo;
asRawPtr(foo.ReceiveAndAdoptRef());

In the end they are the same, but I would prefer to provide primitives that
don't require raw refcounted pointers.

[awong, 2013-07-18 16:00:42]

https://codereview.chromium.org/19267024/diff/30001/skia/ext/refptr.h
File skia/ext/refptr.h (right):

https://codereview.chromium.org/19267024/diff/30001/skia/ext/refptr.h#newcode144
skia/ext/refptr.h:144: // the RefPtr will remain empty.
On 2013/07/18 15:50:30, Stephen White wrote:
> Maybe I'm old-fashioned, but I'm wondering if this is just too clever. Would
it
> be so bad to simply continue to pass a raw pointer to the function that
expects
> one, and then AdoptRef() it into a skia::RefPtr? There wouldn't be any
explicit
> ref/unref, so I don't think it would violate any of the principles we've set
for
> ourselves. The only ugliness would be the existing raw pointer, but it's in
the
> same scope as the RefPtr, so at least its lifetime is clear. And we wouldn't
> need all this code and checking. JMHO.

Yes...I agree but there's precedent (bleck).  See Tommi and Darin's posts on
this thread about scoped_ptr:

https://groups.google.com/a/chromium.org/d/msg/chromium-dev/RTd7rNxHjqk/1WHHv...

Eventually we left it out of scoped_ptr.  At the time, no one thought about the
lifetime wierdnesses either.

Not sure if this context is helpful or not in coming to a decision.

[Stephen White, 2013-07-18 19:05:32]

On 2013/07/18 16:00:42, awong wrote:
> https://codereview.chromium.org/19267024/diff/30001/skia/ext/refptr.h
> File skia/ext/refptr.h (right):
> 
>
https://codereview.chromium.org/19267024/diff/30001/skia/ext/refptr.h#newcode144
> skia/ext/refptr.h:144: // the RefPtr will remain empty.
> On 2013/07/18 15:50:30, Stephen White wrote:
> > Maybe I'm old-fashioned, but I'm wondering if this is just too clever. Would
> it
> > be so bad to simply continue to pass a raw pointer to the function that
> expects
> > one, and then AdoptRef() it into a skia::RefPtr? There wouldn't be any
> explicit
> > ref/unref, so I don't think it would violate any of the principles we've set
> for
> > ourselves. The only ugliness would be the existing raw pointer, but it's in
> the
> > same scope as the RefPtr, so at least its lifetime is clear. And we wouldn't
> > need all this code and checking. JMHO.
> 
> Yes...I agree but there's precedent (bleck).  See Tommi and Darin's posts on
> this thread about scoped_ptr:
> 
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/RTd7rNxHjqk/1WHHv...
> 
> Eventually we left it out of scoped_ptr.  At the time, no one thought about
the
> lifetime wierdnesses either.
> 
> Not sure if this context is helpful or not in coming to a decision.

Thanks, Albert. Just to be clear: the decision was to leave this type of
Receiver out of scoped_ptr? (I don't do a lot of Chrome code, so pardon my
ignorance.)

I guess I feel as though if (WebKit/Blink) RefPtr doesn't have it, and
scoped_ptr doesn't have it, we shouldn't add it here either.

[awong, 2013-07-18 19:54:10]

On 2013/07/18 19:05:32, Stephen White wrote:
> On 2013/07/18 16:00:42, awong wrote:
> > https://codereview.chromium.org/19267024/diff/30001/skia/ext/refptr.h
> > File skia/ext/refptr.h (right):
> > 
> >
>
https://codereview.chromium.org/19267024/diff/30001/skia/ext/refptr.h#newcode144
> > skia/ext/refptr.h:144: // the RefPtr will remain empty.
> > On 2013/07/18 15:50:30, Stephen White wrote:
> > > Maybe I'm old-fashioned, but I'm wondering if this is just too clever.
Would
> > it
> > > be so bad to simply continue to pass a raw pointer to the function that
> > expects
> > > one, and then AdoptRef() it into a skia::RefPtr? There wouldn't be any
> > explicit
> > > ref/unref, so I don't think it would violate any of the principles we've
set
> > for
> > > ourselves. The only ugliness would be the existing raw pointer, but it's
in
> > the
> > > same scope as the RefPtr, so at least its lifetime is clear. And we
wouldn't
> > > need all this code and checking. JMHO.
> > 
> > Yes...I agree but there's precedent (bleck).  See Tommi and Darin's posts on
> > this thread about scoped_ptr:
> > 
> >
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/RTd7rNxHjqk/1WHHv...
> > 
> > Eventually we left it out of scoped_ptr.  At the time, no one thought about
> the
> > lifetime wierdnesses either.
> > 
> > Not sure if this context is helpful or not in coming to a decision.
> 
> Thanks, Albert. Just to be clear: the decision was to leave this type of
> Receiver out of scoped_ptr? (I don't do a lot of Chrome code, so pardon my
> ignorance.)

The proposal was to add some sort of global function that could operate on
scoped_ptr<> (touching scoped_ptr<>'s API in a way that diverges much from
std::unique_ptr<> is a no-no).  To my knowledge, this global function was never
added and the only use of this pattern is in ScopedHandle for accepting Win
HANDLEs.

> I guess I feel as though if (WebKit/Blink) RefPtr doesn't have it, and
> scoped_ptr doesn't have it, we shouldn't add it here either.

I defer on this to you both.

[danakj, 2013-07-18 19:58:22]

On Thu, Jul 18, 2013 at 12:05 PM, <senorblanco@chromium.org> wrote:

> On 2013/07/18 16:00:42, awong wrote:
>
>>
https://codereview.chromium.**org/19267024/diff/30001/skia/**ext/refptr.h<htt...
>> File skia/ext/refptr.h (right):
>>
>
>
> https://codereview.chromium.**org/19267024/diff/30001/skia/**
>
ext/refptr.h#newcode144<https://codereview.chromium.org/19267024/diff/30001/skia/ext/refptr.h#newcode144>
>
>> skia/ext/refptr.h:144: // the RefPtr will remain empty.
>> On 2013/07/18 15:50:30, Stephen White wrote:
>> > Maybe I'm old-fashioned, but I'm wondering if this is just too clever.
>> Would
>> it
>> > be so bad to simply continue to pass a raw pointer to the function that
>> expects
>> > one, and then AdoptRef() it into a skia::RefPtr? There wouldn't be any
>> explicit
>> > ref/unref, so I don't think it would violate any of the principles
>> we've set
>> for
>> > ourselves. The only ugliness would be the existing raw pointer, but
>> it's in
>> the
>> > same scope as the RefPtr, so at least its lifetime is clear. And we
>> wouldn't
>> > need all this code and checking. JMHO.
>>
>
>  Yes...I agree but there's precedent (bleck).  See Tommi and Darin's posts
>> on
>> this thread about scoped_ptr:
>>
>
>
> https://groups.google.com/a/**chromium.org/d/msg/chromium-**
>
dev/RTd7rNxHjqk/1WHHvCdRP1UJ<https://groups.google.com/a/chromium.org/d/msg/chromium-dev/RTd7rNxHjqk/1WHHvCdRP1UJ>
>
>  Eventually we left it out of scoped_ptr.  At the time, no one thought
>> about
>>
> the
>
>> lifetime wierdnesses either.
>>
>
>  Not sure if this context is helpful or not in coming to a decision.
>>
>
> Thanks, Albert. Just to be clear: the decision was to leave this type of
> Receiver out of scoped_ptr? (I don't do a lot of Chrome code, so pardon my
> ignorance.)
>
> I guess I feel as though if (WebKit/Blink) RefPtr doesn't have it, and
> scoped_ptr doesn't have it, we shouldn't add it here either.
>

We don't have it on scoped_ptr, but we do have it on another
platform-specific smart pointer:

https://code.google.com/p/chromium/codesearch#chromium/src/base/win/scoped_ha...

I think the problem here is that with scoped_refptr, raw pointers are
pretty normal to see, so it seems harmless to allow a raw pointer here as
well. But the big difference is you never get a scoped_refptr that has a
reference added to it; it is always reference-neutral. Whereas Skia
pointers do come with a reference, so not storing it into a RefPtr
immediately suddenly opens you up to leaks. So you stick it in a raw
pointer, and then into a refptr. What happens when later on some developer
sticks an early out in between the two by mistake - you have another silent
leak.

I really want an API that allows for us to never get it wrong any more.



>
>
https://codereview.chromium.**org/19267024/<https://codereview.chromium.org/1...
>

[Stephen White, 2013-07-18 20:15:17]

On 2013/07/18 19:58:22, danakj wrote:
> On Thu, Jul 18, 2013 at 12:05 PM, <mailto:senorblanco@chromium.org> wrote:
> 
> > On 2013/07/18 16:00:42, awong wrote:
> >
> >>
>
https://codereview.chromium.**org/19267024/diff/30001/skia/**ext/refptr.h%3Ch...>
> >> File skia/ext/refptr.h (right):
> >>
> >
> >
> > https://codereview.chromium.**org/19267024/diff/30001/skia/**
> >
>
ext/refptr.h#newcode144<https://codereview.chromium.org/19267024/diff/30001/skia/ext/refptr.h#newcode144>
> >
> >> skia/ext/refptr.h:144: // the RefPtr will remain empty.
> >> On 2013/07/18 15:50:30, Stephen White wrote:
> >> > Maybe I'm old-fashioned, but I'm wondering if this is just too clever.
> >> Would
> >> it
> >> > be so bad to simply continue to pass a raw pointer to the function that
> >> expects
> >> > one, and then AdoptRef() it into a skia::RefPtr? There wouldn't be any
> >> explicit
> >> > ref/unref, so I don't think it would violate any of the principles
> >> we've set
> >> for
> >> > ourselves. The only ugliness would be the existing raw pointer, but
> >> it's in
> >> the
> >> > same scope as the RefPtr, so at least its lifetime is clear. And we
> >> wouldn't
> >> > need all this code and checking. JMHO.
> >>
> >
> >  Yes...I agree but there's precedent (bleck).  See Tommi and Darin's posts
> >> on
> >> this thread about scoped_ptr:
> >>
> >
> >
> > https://groups.google.com/a/**chromium.org/d/msg/chromium-**
> >
>
dev/RTd7rNxHjqk/1WHHvCdRP1UJ<https://groups.google.com/a/chromium.org/d/msg/chromium-dev/RTd7rNxHjqk/1WHHvCdRP1UJ>
> >
> >  Eventually we left it out of scoped_ptr.  At the time, no one thought
> >> about
> >>
> > the
> >
> >> lifetime wierdnesses either.
> >>
> >
> >  Not sure if this context is helpful or not in coming to a decision.
> >>
> >
> > Thanks, Albert. Just to be clear: the decision was to leave this type of
> > Receiver out of scoped_ptr? (I don't do a lot of Chrome code, so pardon my
> > ignorance.)
> >
> > I guess I feel as though if (WebKit/Blink) RefPtr doesn't have it, and
> > scoped_ptr doesn't have it, we shouldn't add it here either.
> >
> 
> We don't have it on scoped_ptr, but we do have it on another
> platform-specific smart pointer:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/win/scoped_ha...
> 
> I think the problem here is that with scoped_refptr, raw pointers are
> pretty normal to see, so it seems harmless to allow a raw pointer here as
> well. But the big difference is you never get a scoped_refptr that has a
> reference added to it; it is always reference-neutral. Whereas Skia
> pointers do come with a reference, so not storing it into a RefPtr
> immediately suddenly opens you up to leaks. So you stick it in a raw
> pointer, and then into a refptr. What happens when later on some developer
> sticks an early out in between the two by mistake - you have another silent
> leak.
> 
> I really want an API that allows for us to never get it wrong any more.

I understand your motivation. (And I apologize, since it was probably me who
introduced this leak in the first place.) However, my feeling is that the real
problem is the lack of smart pointers in Skia proper, so ownership transfers are
undocumented. Without that, it's always going to be possible to get it wrong in
ways that are not obvious at the call site (e.g., passing a raw pointer from a
creation function to another function which doesn't know it's supposed to take
ownership).

[danakj, 2013-07-18 20:17:28]

On Thu, Jul 18, 2013 at 1:15 PM, <senorblanco@chromium.org> wrote:

> We don't have it on scoped_ptr, but we do have it on another
>> platform-specific smart pointer:
>>
>
>
> https://code.google.com/p/**chromium/codesearch#chromium/**
> src/base/win/scoped_handle.h&**q=Receiver%2520file:base/*&sq=**
>
package:chromium&type=cs&l=47<https://code.google.com/p/chromium/codesearch#chromium/src/base/win/scoped_handle.h&q=Receiver%2520file:base/*&sq=package:chromium&type=cs&l=47>
>
>
>  I think the problem here is that with scoped_refptr, raw pointers are
>> pretty normal to see, so it seems harmless to allow a raw pointer here as
>> well. But the big difference is you never get a scoped_refptr that has a
>> reference added to it; it is always reference-neutral. Whereas Skia
>> pointers do come with a reference, so not storing it into a RefPtr
>> immediately suddenly opens you up to leaks. So you stick it in a raw
>> pointer, and then into a refptr. What happens when later on some developer
>> sticks an early out in between the two by mistake - you have another
>> silent
>> leak.
>>
>
>  I really want an API that allows for us to never get it wrong any more.
>>
>
> I understand your motivation. (And I apologize, since it was probably me
> who
> introduced this leak in the first place.) However, my feeling is that the
> real
> problem is the lack of smart pointers in Skia proper, so ownership
> transfers are
> undocumented. Without that, it's always going to be possible to get it
> wrong in
> ways that are not obvious at the call site (e.g., passing a raw pointer
> from a
> creation function to another function which doesn't know it's supposed to
> take
> ownership).
>


Ohhh yes, I could not agree with you more on that :) I'm just trying to
mitigate the damage as aggressively as I can from the chromium side. If we
have primitives that allow authors to completely avoid a raw pointer, then
it becomes highly suspicious when a raw pointer appears, and everything
should work correctly when they do not.


>
>
https://codereview.chromium.**org/19267024/<https://codereview.chromium.org/1...
>

[earthdok, 2013-07-22 11:59:10]

On 2013/07/18 15:31:51, danakj wrote:
> Oh, not sure how you were removed. The latest CL should fix the leak.

In case you're wondering, I confirm that this fixes it.

[danakj, 2013-07-23 17:15:47]

ping: I really want to see us able to not use raw SkRefCounted pointers at all
in chromium to prevent this kind of thing in the future. I don't see "get a raw
pointer then stick it into a RefPtr" as much different from "get a raw pointer
then deref it when you're done". But I do see "adopt all ref counted things into
RefPtr immediately" as being something very different and much more clear.

I'm willing to do it the other way if that's the only way we can agree to move
forward, but do you feel strongly about this? Do you feel like
foo.asColorMatrix(refptr.ReceiveAndAdoptRef()) is less clear or more difficult
to use? Do you feel like that will be causing more mistakes than using a
temporary raw pointer in between?

[Stephen White, 2013-07-23 17:22:30]

On 2013/07/23 17:15:47, danakj wrote:
> ping: I really want to see us able to not use raw SkRefCounted pointers at all
> in chromium to prevent this kind of thing in the future. I don't see "get a
raw
> pointer then stick it into a RefPtr" as much different from "get a raw pointer
> then deref it when you're done". But I do see "adopt all ref counted things
into
> RefPtr immediately" as being something very different and much more clear.
> 
> I'm willing to do it the other way if that's the only way we can agree to move
> forward, but do you feel strongly about this? Do you feel like
> foo.asColorMatrix(refptr.ReceiveAndAdoptRef()) is less clear or more difficult
> to use? Do you feel like that will be causing more mistakes than using a
> temporary raw pointer in between?

It's not the call site that I object to so much (although that is a bit
strange), it's the complexity of the implementation. It seems like there are
lots of places to go wrong.

I'd prefer we just use a temporary raw pointer for now and use this as more
evidence that "the skia API needs to use smart pointers".

[danakj, 2013-07-23 17:30:17]

PTAL

[Stephen White, 2013-07-23 17:33:34]

LGTM. Thanks!

[commit-bot: I haz the power, 2013-07-23 17:41:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/19267024/56001

[commit-bot: I haz the power, 2013-07-23 21:22:17]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-07-23 21:24:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/19267024/56001

[commit-bot: I haz the power, 2013-07-24 00:03:43]

Change committed as 213283