cc: Increment the ref count when assigning the SkImageFilter

cc: Increment the ref count when assigning the SkImageFilter

This is to fix a crash introduced in r168998. We should not assign
SkImageFilter* without ref-counting it.

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

[enne (OOO), 2012-11-21 16:52:02]

https://codereview.chromium.org/11418117/diff/2001/cc/render_pass.cc
File cc/render_pass.cc (right):

https://codereview.chromium.org/11418117/diff/2001/cc/render_pass.cc#newcode56
cc/render_pass.cc:56: SkRefCnt_SafeAssign(this->filter, filter);
What about just making filter an SkRefPtr<SkImageFilter> so you don't have to
remember to un-ref it and it automatically safe assigns?

[danakj, 2012-11-21 17:08:06]

Excellent plan! Done.

[Stephen White, 2012-11-21 17:23:00]

We don't use SkRefPtr much in Skia, since it's somewhat incompatible with Skia
semantics.  All refcounted objects are created with a refcount of 1.  SkRefPtr's
constructor bumps that to 2 (as far as I can tell).  SkAutoTUnref<> is more
compatible with skia semantics, since it doesn't bump the refcount on
assignment, but doesn't work well to connote ownership transfer via function
parameters.

I actually prefer the original patch, sad to say.

https://codereview.chromium.org/11418117/diff/4007/cc/damage_tracker_unittest.cc
File cc/damage_tracker_unittest.cc (right):

https://codereview.chromium.org/11418117/diff/4007/cc/damage_tracker_unittest...
cc/damage_tracker_unittest.cc:399: SkRefPtr<SkImageFilter> filter(new
SkBlurImageFilter(SkIntToScalar(2), SkIntToScalar(2)));
I don't think this is correct.  I think this'll end up with a refcount of 2
after assignment to the SkRefPtr, and 1 at the end of the function.

https://codereview.chromium.org/11418117/diff/4007/cc/render_pass_unittest.cc
File cc/render_pass_unittest.cc (left):

https://codereview.chromium.org/11418117/diff/4007/cc/render_pass_unittest.cc...
cc/render_pass_unittest.cc:51: SkAutoTUnref<SkBlurImageFilter> filter(new
SkBlurImageFilter(SK_Scalar1, SK_Scalar1));
Same here.  I think this'll leak.

[danakj, 2012-11-21 18:19:07]

PTAL.

I've implemented a cc::SkiaRefPtr class that will steal the first reference to
avoid leaks.

[danakj, 2012-11-21 18:43:17]

OK. First patch again. We need to not crash in the meantime.

[Stephen White, 2012-11-21 18:45:34]

On 2012/11/21 18:43:17, danakj wrote:
> OK. First patch again. We need to not crash in the meantime.

LGTM

[enne (OOO), 2012-11-26 14:59:15]

On 2012/11/21 17:23:00, Stephen White wrote:
> We don't use SkRefPtr much in Skia, since it's somewhat incompatible with Skia
> semantics.

Argh, can I ask why it's like this? Can it get changed?

Maybe it's just me, but I feel like there's some correlation between "nobody
wants to use ref-counting templates" and "crash/leak bugs due to missing manual
ref/unrefs".

[Stephen White, 2012-11-27 00:21:31]

On 2012/11/26 14:59:15, enne wrote:
> On 2012/11/21 17:23:00, Stephen White wrote:
> > We don't use SkRefPtr much in Skia, since it's somewhat incompatible with
Skia
> > semantics.
> 
> Argh, can I ask why it's like this? Can it get changed?

Yeah, I think we should add something like adoptRef() to SkRefPtr.  That might
make it less error-prone, and removing the existing bare ptr constructor would
be a logical second step I think (although more refactoring would be required).

> Maybe it's just me, but I feel like there's some correlation between "nobody
> wants to use ref-counting templates" and "crash/leak bugs due to missing
manual
> ref/unrefs".

I agree.  Skia is kind of old-school, and we should bring it into the 1990s at
least.  :)