RefCounted types should not have public destructors, delegate cleanup

RefCounted types should not have public destructors, delegate cleanup

For Delegate/Observer-type classes that specify an
interface but do not have any particular lifetime
requirements, make their destructors protected. This is to
allow their interfaces to be implemented safely by
RefCounted types. With public destructors, it's possible to
do "scoped_ptr<Delegate> foo", and then assign a
RefCountedDelegateImpl, which would lead to a double free.

As none of these Delegates actually need public destructors
(ownership of the Delegate* is not transferred during a
function call / class constructor), mark the destructors
protected so that it becomes a compile warning to try to
delete them via the Delegate*.

BUG=123295
TEST=it compiles


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144086

[Ryan Sleevi, 2012-05-21 08:12:03]

Updating a bunch of delegates to have protected destructors, since their
interfaces may be implemented by RefCounted types.

Since none of these delegates have their lifetimes managed by their parent
class, this is all safe and good, and prevents any accidental deleting of
RefCounted types by way of the delegate's virtual destructor.


brettw: cleanup stamp for content/, chrome/, and ppapi/ (and ipc/, which is not
set noparent)
fischman: media/
wez: remoting/

[Ami GONE FROM CHROMIUM, 2012-05-21 15:50:16]

On 2012/05/21 08:12:03, Ryan Sleevi wrote:
> Updating a bunch of delegates to have protected destructors, since their
> interfaces may be implemented by RefCounted types.

The inference in this statement seems incomplete to me.

> [This] prevents any accidental deleting of
> RefCounted types by way of the delegate's virtual destructor.

IIUC you're trading off the ability to use a scoped_ptr<BaseDelegate> (which is
nice for decoupling classes that don't need to know about implementation
details) against the compiler protecting you from code like
delete static_cast<BaseDelegate*>(impl_instance_which_is_ref_counted_);
Is that summary correct or am I missing something?
(if it's correct, I'm not sure this is a tradeoff worth making)

But if this is indeed what you're trying to do, the changes in media/ look
correct to me.

[brettw, 2012-05-21 17:48:16]

I'm sort of in the same place as Ami. I see how you could mess this up, but I'm
not sure it's worth doing so much work to prevent people from making this
mistake. Have we had problems with subtle bugs along these lines?

[jam, 2012-05-21 17:53:46]

that's my feeling as well on  http://codereview.chromium.org/10416003/ .
the tradeoff isn't worth it cause the readability becomes worse.

On Mon, May 21, 2012 at 10:48 AM, <brettw@chromium.org> wrote:

> I'm sort of in the same place as Ami. I see how you could mess this up,
> but I'm
> not sure it's worth doing so much work to prevent people from making this
> mistake. Have we had problems with subtle bugs along these lines?
>
>
http://codereview.chromium.**org/10383262/<http://codereview.chromium.org/103...
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[jam, 2012-05-21 17:53:47]

that's my feeling as well on  http://codereview.chromium.org/10416003/ .
the tradeoff isn't worth it cause the readability becomes worse.

On Mon, May 21, 2012 at 10:48 AM, <brettw@chromium.org> wrote:

> I'm sort of in the same place as Ami. I see how you could mess this up,
> but I'm
> not sure it's worth doing so much work to prevent people from making this
> mistake. Have we had problems with subtle bugs along these lines?
>
>
http://codereview.chromium.**org/10383262/<http://codereview.chromium.org/103...
>

[Ryan Sleevi, 2012-05-21 18:48:11]

On 2012/05/21 15:50:16, Ami Fischman wrote:
> On 2012/05/21 08:12:03, Ryan Sleevi wrote:
> > Updating a bunch of delegates to have protected destructors, since their
> > interfaces may be implemented by RefCounted types.
> 
> The inference in this statement seems incomplete to me.

Sorry, all of these interfaces /do/ have RefCounted classes implementing their
interfaces.

There were detected with a Clang plugin that looked for public destructors on
RefCounted classes ( http://codereview.chromium.org/10407057/ with "-Xclang
plugin-arg-find-bad-refcounts -Xclang -check-base -classes"

> 
> > [This] prevents any accidental deleting of
> > RefCounted types by way of the delegate's virtual destructor.
> 
> IIUC you're trading off the ability to use a scoped_ptr<BaseDelegate> (which
is
> nice for decoupling classes that don't need to know about implementation
> details) against the compiler protecting you from code like
> delete static_cast<BaseDelegate*>(impl_instance_which_is_ref_counted_);
> Is that summary correct or am I missing something?
> (if it's correct, I'm not sure this is a tradeoff worth making)

No. It's to prevent something like

class Delegate {}

class Bar {
 public:
  // Takes ownership of |delegate|
  explicit Bar(Delegate* delegate) : delegate_(delegate) {}

 private:
  scoped_ptr<Delegate> delegate_;
};

class Foo : public base::RefCounted<Foo>,
            public Delegate {
};

scoped_refptr<Foo> foo(new Foo);
scoped_ptr<Bar> bar(foo.get()); <-- dangerous


This pattern actually occurs in Chromium code. While this reduced case is very
simple and easy to spot, it shows up more when there are multiple layers of
inheritance, where the fact that Foo is implementing Delegate or that Bar takes
ownership of Delegate is hidden.

This makes it a hard error, in that Bar /cannot/ take ownership of Delegate,
because the destructor is private. For the places where it really is appropriate
that Bar takes ownership, then Foo cannot/should not be RefCounted. That's what
this warning adds.

> 
> But if this is indeed what you're trying to do, the changes in media/ look
> correct to me.

[Ryan Sleevi, 2012-05-21 18:52:50]

On 2012/05/21 17:48:16, brettw wrote:
> I'm sort of in the same place as Ami. I see how you could mess this up, but
I'm
> not sure it's worth doing so much work to prevent people from making this
> mistake. Have we had problems with subtle bugs along these lines?

Yes. I try not to call them out because they are essentially use-after-frees,
which is generally SecSeverity-Critical.

The motivation for the clang plugin was due to a set of changes using a
scoped_ptr<> for what was a RefCounted class, leading to double deletion. This
happened in remoting/ and nacl/.

Since this change, there have been several 'questionable' patterns where the
lifetime is ambiguous, and where a U-A-F /may/ be possible. These were just all
the low-hanging fruit where it's clear that the lifetime of the Delegate
interface is ambiguous (eg: "must outlive this class") versus concrete ("This
class takes ownership of Delegate")

[Ryan Sleevi, 2012-05-21 18:53:47]

On 2012/05/21 17:53:47, John Abd-El-Malek wrote:
> that's my feeling as well on  http://codereview.chromium.org/10416003/ .
> the tradeoff isn't worth it cause the readability becomes worse.

Could you explain your concerns about the readability? I can certainly
appreciate your concerns for RefCountedData<>, but for these, making the
destructors explicit and protected doesn't seem to be any loss, and enshrines in
code what the expectations are for class lifetime.

> 
> On Mon, May 21, 2012 at 10:48 AM, <mailto:brettw@chromium.org> wrote:
> 
> > I'm sort of in the same place as Ami. I see how you could mess this up,
> > but I'm
> > not sure it's worth doing so much work to prevent people from making this
> > mistake. Have we had problems with subtle bugs along these lines?
> >
> >
>
http://codereview.chromium.**org/10383262/%3Chttp://codereview.chromium.org/1...>
> >

[alexeypa (please no reviews), 2012-05-21 18:58:00]

On 2012/05/21 18:48:11, Ryan Sleevi wrote:
> This pattern actually occurs in Chromium code. While this reduced case is very
> simple and easy to spot, it shows up more when there are multiple layers of
> inheritance, where the fact that Foo is implementing Delegate or that Bar
takes
> ownership of Delegate is hidden.

+1. This kind issue is annoyingly hard to debug. It causes a double delete which
tends to look more like a race condition so by the time you realize what it is a
few hours of debugging have been wasted already.

[Sergey Ulanov, 2012-05-21 19:13:49]

remoting/ - LGTM

I believe that the work Ryan is doing here is very useful. E.g. the following is
an actual bug that's due to mixing ref-counted and non-ref-counted interfaces:
http://code.google.com/p/chromium/issues/detail?id=128940
It's rather hard to find and debug bugs like that, and they also create security
risks.

http://codereview.chromium.org/10383262/diff/1/remoting/protocol/session_mana...
File remoting/protocol/session_manager.h (right):

http://codereview.chromium.org/10383262/diff/1/remoting/protocol/session_mana...
remoting/protocol/session_manager.h:119: ~Listener() {}
add virtual please

[Ami GONE FROM CHROMIUM, 2012-05-21 20:14:41]

Ah; I misunderstood
http://codereview.chromium.org/10383262/diff/1/media/webm/webm_stream_parser.cc
as
being a forced coupling of implementation details, whereas the previous
code was actually bug-prone b/c *without* the coupling it was
scoped_ptr<>'ing something that might be ref-counted.

I'm sold that this is a worthwhile change.  You might want to document it
in the linked bug to avoid repeating this conversation with others :)

LGTM for media/

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[Ami GONE FROM CHROMIUM, 2012-05-21 20:14:42]

Ah; I misunderstood
http://codereview.chromium.org/10383262/diff/1/media/webm/webm_stream_parser.cc
as
being a forced coupling of implementation details, whereas the previous
code was actually bug-prone b/c *without* the coupling it was
scoped_ptr<>'ing something that might be ref-counted.

I'm sold that this is a worthwhile change.  You might want to document it
in the linked bug to avoid repeating this conversation with others :)

LGTM for media/

[Wez, 2012-05-21 20:19:38]

On 2012/05/21 20:14:42, Ami Fischman wrote:
> I'm sold that this is a worthwhile change.  You might want to document it
> in the linked bug to avoid repeating this conversation with others :)

I'd prefer to see a summary of the rationale in the CL description.

[Ryan Sleevi, 2012-05-21 22:20:50]

On 2012/05/21 20:19:38, Wez wrote:
> On 2012/05/21 20:14:42, Ami Fischman wrote:
> > I'm sold that this is a worthwhile change.  You might want to document it
> > in the linked bug to avoid repeating this conversation with others :)
> 
> I'd prefer to see a summary of the rationale in the CL description.

Wez: Updated the description with something that hopefully expresses the intent
concisely enough.

brettw, jam: After talking with willchan a bit about the problem (
http://codereview.chromium.org/10417002/#msg10 ), he's on board as well, after
initially expressing similar reservations. Combined with the comments above,
would you be willing to stamp?

[jam, 2012-05-22 00:17:41]

On Mon, May 21, 2012 at 3:20 PM, <rsleevi@chromium.org> wrote:

> On 2012/05/21 20:19:38, Wez wrote:
>
>> On 2012/05/21 20:14:42, Ami Fischman wrote:
>> > I'm sold that this is a worthwhile change.  You might want to document
>> it
>> > in the linked bug to avoid repeating this conversation with others :)
>>
>
>  I'd prefer to see a summary of the rationale in the CL description.
>>
>
> Wez: Updated the description with something that hopefully expresses the
> intent
> concisely enough.
>
> brettw, jam: After talking with willchan a bit about the problem (
>
http://codereview.chromium.**org/10417002/#msg10<http://codereview.chromium.o...),
he's on board as well, after
> initially expressing similar reservations. Combined with the comments
> above,
> would you be willing to stamp?
>

content lgtm

i still have mixed feelings on the other change that you sent me though

>
>
http://codereview.chromium.**org/10383262/<http://codereview.chromium.org/103...
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[jam, 2012-05-22 00:17:42]

On Mon, May 21, 2012 at 3:20 PM, <rsleevi@chromium.org> wrote:

> On 2012/05/21 20:19:38, Wez wrote:
>
>> On 2012/05/21 20:14:42, Ami Fischman wrote:
>> > I'm sold that this is a worthwhile change.  You might want to document
>> it
>> > in the linked bug to avoid repeating this conversation with others :)
>>
>
>  I'd prefer to see a summary of the rationale in the CL description.
>>
>
> Wez: Updated the description with something that hopefully expresses the
> intent
> concisely enough.
>
> brettw, jam: After talking with willchan a bit about the problem (
>
http://codereview.chromium.**org/10417002/#msg10<http://codereview.chromium.o...),
he's on board as well, after
> initially expressing similar reservations. Combined with the comments
> above,
> would you be willing to stamp?
>

content lgtm

i still have mixed feelings on the other change that you sent me though

>
>
http://codereview.chromium.**org/10383262/<http://codereview.chromium.org/103...
>

[Wez, 2012-05-22 00:19:29]

lgtm

[brettw, 2012-05-22 05:07:20]

ppapi lgtm, was there any other place you need me to look at? This is all I
opened

[Ryan Sleevi, 2012-05-28 21:02:43]

On 2012/05/22 05:07:20, brettw wrote:
> ppapi lgtm, was there any other place you need me to look at? This is all I
> opened

chrome/ needs a stamp from an appropriate OWNERS (which is really *). The only
'interesting' change in here would be chrome/browser/nacl_host/nacl_process_host
- because the MessageLoopForIO::Watcher is now protected-virtual, the
scoped_ptr<> now indicates the Derived class rather than the Base class.

[brettw, 2012-05-29 00:15:03]

okay, lgtm on the rest of chrome

[commit-bot: I haz the power, 2012-05-29 00:39:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10383262/13001

[commit-bot: I haz the power, 2012-05-29 00:40:34]

Presubmit check for 10383262-13001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/policy

Presubmit checks took 20.5s to calculate.

[Ryan Sleevi, 2012-05-29 00:47:23]

joaodasilva: Stamp?

[Joao da Silva, 2012-05-29 08:52:31]

lgtm

[commit-bot: I haz the power, 2012-05-29 08:54:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10383262/13001

[commit-bot: I haz the power, 2012-05-29 09:16:37]

Try job failure for 10383262-13001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-06-25 06:58:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10383262/26001

[commit-bot: I haz the power, 2012-06-25 07:29:07]

Try job failure for 10383262-26001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-06-25 23:24:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10383262/29003

[commit-bot: I haz the power, 2012-06-26 01:06:09]

Change committed as 144086