Add run-time reference adoption checks to SkRefCnt

Add hooks for external run-time reference adoption checks to SkRefCnt.

The reference adoption checks are to help detect memory
leaks and bad usage when using SkRefCnt subclasses with
Blink's RefPtr.

BUG=crbug.com/304265

Committed: http://code.google.com/p/skia/source/detail?r=11811

[Justin Novosad, 2013-10-04 20:15:03]

PTAL

Companion Blink CL: https://codereview.chromium.org/25589003/

[bungeman-skia, 2013-10-07 16:00:48]

This seems like a very intrusive Blink specific change which greatly increases
the amount of code in SkRefCnt. It seems like the issue which prompted this CL
could just as easily be fixed by stating that Blink must never declare a pointer
to an SkRefCnt. Of course, such a thing would be difficult to enforce.

Could this sort of issue be solved by disallowing RefPtr(SkRefCnt*)? (Declare it
but not implement it?) It looks like the RefPtr(T*) constructor is a bit of a
hole, does it only exist for backward compatibility?

[Justin Novosad, 2013-10-07 17:01:45]

On 2013/10/07 16:00:48, bungeman1 wrote:
> This seems like a very intrusive Blink specific change which greatly increases
> the amount of code in SkRefCnt. 

The intent is to only enable that code in Chromium debug builds, much like the
equivalent code that exists is WTF::RefCounted.

> It seems like the issue which prompted this CL
> could just as easily be fixed by stating that Blink must never declare a
pointer
> to an SkRefCnt. Of course, such a thing would be difficult to enforce.

Actually Blink has been going in the opposite direction lately by using RefPtr
on SkReftCnt more and more.
There is nothing inherently wrong with using RefPtr with SkRefCnt, and it is
actually a lot safer that relying on explicit refs/unrefs on dumb pointers, and
it is a lot more flexible than just sticking to OwnPtr and SkAutoTUnref.

The problem I am attempting to solve here that is that Blink has some very
useful run-time checks that are only enabled in debug builds and help catch
errors early. These safety checks are currently limited to subclasses of
WTF::RefCounted. I am attempting to extend these check to skia so that we can
benefit from the same safety net there too.

> 
> Could this sort of issue be solved by disallowing RefPtr(SkRefCnt*)? 

Promoting not using smart ptrs is a step backward IMHO.

> (Declare it but not implement it?) It looks like the RefPtr(T*) constructor is
a bit of a
> hole, does it only exist for backward compatibility?

It is a deliberate hole so that Non-webkit classes can be bootstrapped into
RefPtr. The only requirement is that they implement ref/deref.  In skia, deref
was created as an alias for unref, so that SkRefCnt can fulfill the RefPtr
contract.

[Tom Hudson, 2013-10-07 17:13:53]

On 2013/10/07 17:01:45, junov wrote:
> On 2013/10/07 16:00:48, bungeman1 wrote:
> > Could this sort of issue be solved by disallowing RefPtr(SkRefCnt*)? 
> 
> Promoting not using smart ptrs is a step backward IMHO.

If we're not having a (ref ptr !=? smart ptr) vocabulary issue, the Chrome Style
Guide Smart Pointer Guidelines still say reference counting is a bad idea.
"[J]ust because you see existing code doing it does not mean it's the right
solution."

[Tom Hudson, 2013-10-07 17:15:25]

Sorry, I'd meant to cite:
http://www.chromium.org/developers/smart-pointer-guidelines

[Justin Novosad, 2013-10-07 17:39:48]

Totally agree with those guidelines, I have no desire to challenge any of the
current practices or philosophies.  I am just improving the maintainability and
robustness of the uses cases where reference counting is used (and presumably
desirable).

In Blink code, we use Blink's smart pointer classes in ways that follow the
Blink style guidelines whenever we can, and this is working without issues with
SkRefCnt objects.  It is just that the pointee classes have more debug safety
nets when they are WTF::RefCounted subclasses. I am trying to improve upon that.

[Justin Novosad, 2013-10-07 17:46:41]

Just to emphasize that point: we shipped a Chrome beta release with a severe
memory leak. We had mysterious incoming crash reports that hinted to a leak
probably related to 2D canvas. We finally gained some traction on the issue once
valgrind layout test bot cycled passed a test that triggers the bug (which took
weeks).  With the proposed change, the same leak would have been detected in the
commit queue.

[reed1, 2013-10-07 17:59:21]

I am supported of debug-only code to help correct calling behavior.

In this case, can we simplify it to just be a bool? I don't really see the value
of more than that in the current CL.

Also, the external functions look awfully unscoped for public functions. Can we
give them more restricted/verbose looking names, or scope them inside the
SkRefCnt class?

e.g.

class SkRefCnt {
public:
    ...
    static void RequireRefPtrAdoption(SkRefPtr*) ...
};

[Justin Novosad, 2013-10-07 18:17:04]

> In this case, can we simplify it to just be a bool?

Yeah, in Blink it is just a bool, and that works because WTF::RefCount has a
built-in assumption that its reference must be adopted before we can start
increasing the ref count (i.e. share the pointer).  Enforcing that rule in
SkRefCnt would break skia because there is no explicit notion of "adoption" in
skia. All we do in skia is not call ref() for the first ref, and use the ref
count that the object was created with. To bridge the gap between the Blink and
skia usage patterns for SkRefCnt, I needed to add a signal that tells SkRefCnt
that it is being used in "Blink mode".  I fanyone has a better idea for
resolving this problem, please share.

> Also, the external functions look awfully unscoped for public functions. Can
we
> give them more restricted/verbose looking names, or scope them inside the
> SkRefCnt class?

You're right, also the use of "friend" is icky. I think a cleaner solution would
be to avoid Blink-isms in skia by making these into methods of SkRefCnt, then I
could implement a shim in Chromium's src/skia that bridges the Blink interface
to skia, kinda like what we already do for TRACE_EVENT.

[bungeman-skia, 2013-10-07 19:40:51]

message: On 2013/10/07 17:01:45, junov wrote:
> On 2013/10/07 16:00:48, bungeman1 wrote:
> > This seems like a very intrusive Blink specific change which greatly
increases
> > the amount of code in SkRefCnt. 
> 
> The intent is to only enable that code in Chromium debug builds, much like the
> equivalent code that exists is WTF::RefCounted.
> 
> > It seems like the issue which prompted this CL
> > could just as easily be fixed by stating that Blink must never declare a
> pointer
> > to an SkRefCnt. Of course, such a thing would be difficult to enforce.
> 
> Actually Blink has been going in the opposite direction lately by using RefPtr
> on SkReftCnt more and more.
> There is nothing inherently wrong with using RefPtr with SkRefCnt, and it is
> actually a lot safer that relying on explicit refs/unrefs on dumb pointers,
and
> it is a lot more flexible than just sticking to OwnPtr and SkAutoTUnref.
> 

I think you misunderstand my point; I'm aggressively agreeing here. What I'm
saying is that Blink should never explicitly declare a pointer to an SkRefCnt
object, SkRefCnts should always be in a RefPtr. If declaring a pointer to an
SkRefCnt were impossible in Blink the sort of issues this is intending to solve
wouldn't happen. However, as I was pointing out, while such a rule could be
enforced programatically, it isn't simple.

> The problem I am attempting to solve here that is that Blink has some very
> useful run-time checks that are only enabled in debug builds and help catch
> errors early. These safety checks are currently limited to subclasses of
> WTF::RefCounted. I am attempting to extend these check to skia so that we can
> benefit from the same safety net there too.
> 
> > 
> > Could this sort of issue be solved by disallowing RefPtr(SkRefCnt*)? 
> 
> Promoting not using smart ptrs is a step backward IMHO.
> 

No, I don't mean not using them, I mean use them correctly. As far as I can
tell, it is always an error to call the RefPtr constructor with an SkRefCnt. In
fact, that is one of the things (the only thing?) your patch will catch at
runtime. The 'correct' way of using an SkRefCnt in Blink is to call adoptRef (to
create the PassRefPtr<SkRefCnt>) and then assign to a RefPtr<SkRefCnt>, none of
these things require the RefPtr(T*) constructor.

> > (Declare it but not implement it?) It looks like the RefPtr(T*) constructor
is
> a bit of a
> > hole, does it only exist for backward compatibility?
> 
> It is a deliberate hole so that Non-webkit classes can be bootstrapped into
> RefPtr. The only requirement is that they implement ref/deref.  In skia, deref
> was created as an alias for unref, so that SkRefCnt can fulfill the RefPtr
> contract.

I'm not sure how this helps bootstrap non-WTF classes into RefPtr. Since you
have adoptRef and the class already needs a ref/deref, I don't see what good
having RefPtr(T*) does. Deliberate hole it may be, but it's certainty being
abused (especially since it isn't explicit). Some quick examples that I found
just by making the RefPtr(T*) constructor explicit (or removing it altogether):

StringBuilder::clear() which currently has the line 'm_buffer = 0;' which is far
cleaner with 'm_buffer.clear();'.
The constructor CString(CStringBuffer* buffer) would be better
CString(PassRefPtr<CStringBuffer> buffer).
... and many more (there's a lot of compiler output).

Regardless of what the original justification for RefPtr(T*) was, it is being
abused (used within Blink).

I went and tried out my own suggestion here, to implement 'RefPtr(SkRefCnt*
ptr);' with no definition. However, this fails because of the many places where
Blink code is relying on RefPtr(T*) to implicitly convert '0' to RefPtr. This
declaration makes such statements ambiguous. I find these to be a code smell as
well, in many places this '0' is hiding what is really going on, making the code
more difficult to understand than needed. For example,
HTMLMediaElement::TrackGroup constructor where visibleTrack and defaultTrack
need not even be listed in the initialization list.

[Justin Novosad, 2013-10-15 21:53:24]

New patch.  Now, skia will only contain hooks to allow chromium to inject its
own baseclass.

[reed1, 2013-10-16 12:27:23]

https://codereview.chromium.org/25432003/diff/22001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.chromium.org/25432003/diff/22001/include/core/SkRefCnt.h#n...
include/core/SkRefCnt.h:43: class SK_API SkRefCnt : public SkRefCntBase,
SkNoncopyable {
To avoid multiple inheritance, perhaps we just need to do in SkRefCnt what
SkNoncopyable does for us: hide assignment and copy-constructor.

[Justin Novosad, 2013-10-16 13:19:57]

https://codereview.chromium.org/25432003/diff/22001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.chromium.org/25432003/diff/22001/include/core/SkRefCnt.h#n...
include/core/SkRefCnt.h:43: class SK_API SkRefCnt : public SkRefCntBase,
SkNoncopyable {
On 2013/10/16 12:27:23, reed1 wrote:
> To avoid multiple inheritance, perhaps we just need to do in SkRefCnt what
> SkNoncopyable does for us: hide assignment and copy-constructor.

Done.

[reed1, 2013-10-16 13:37:39]

works for me, deferring to bungeman

[bungeman-skia, 2013-10-16 14:36:34]

lgtm with one comment nit.

https://codereview.chromium.org/25432003/diff/27001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.chromium.org/25432003/diff/27001/include/core/SkRefCnt.h#n...
include/core/SkRefCnt.h:126: *  Make, SkRefCnt non-copyable
Looks like, a magic comma got in here?

[commit-bot: I haz the power, 2013-10-16 14:44:16]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/junov@chromium.org/25432003/33001

[commit-bot: I haz the power, 2013-10-16 14:44:18]

Presubmit check for 25432003-33001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com')

[Justin Novosad, 2013-10-16 14:45:24]

On 2013/10/16 14:44:18, I haz the power (commit-bot) wrote:
> ** Presubmit ERRORS **
> Since the CL is editing public API, you must have an LGTM from one of:
> ('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
> 'bsalomon@google.com')

Oh, look at that.   Mike?

[reed1, 2013-10-16 14:52:46]

lgtm

[commit-bot: I haz the power, 2013-10-16 14:52:56]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/junov@chromium.org/25432003/33001

[commit-bot: I haz the power, 2013-10-16 15:15:59]

Change committed as 11811