RefCounted types should not have public destructors

RefCounted types should not have public destructors

Change content::ResourceResponse to be base::RefCountedData<> rather than
directly inheriting from both ResourceResponseHead and base::RefCounted.

BUG=123295
TEST=existing


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

[Ryan Sleevi, 2012-05-21 07:54:22]

jar: base/ (and chrome/browser/net since its noparent)
jam: everything else

[jam, 2012-05-21 15:18:42]

I find that the additional "->data."  makes things less readable, so I prefer
the original approach.

(comment below was written before I looked at the other changes)

http://codereview.chromium.org/10416003/diff/1/content/public/common/resource...
File content/public/common/resource_response.h (right):

http://codereview.chromium.org/10416003/diff/1/content/public/common/resource...
content/public/common/resource_response.h:43: class CONTENT_EXPORT
ResourceResponse
please keep this as a struct. for the content api, we only want either structs
or classes that are intefaces in the public dir

[Ryan Sleevi, 2012-05-21 15:32:19]

On May 21, 2012 8:18 AM, <jam@chromium.org> wrote:
>
> I find that the additional "->data."  makes things less readable, so I
prefer
> the original approach.

The original approach is not safe, and directly prevents enabling a set of
compiler warnings from the Chromium Clang plugin.

The goal is to ensure RefCounted classes do not have any public destructors
- including via any base classes when multiply-inheriting. The first pass
picked up the cases of single inheritence and shook out some bugs, and this
pass for muktiple-inheritence has also revealed issues with ambiguous or
conflicting lifetimes.

As it was, it was possible to "delete (ResourceResponseHead*)foo" - an
anti-pattern I would like to prevent.

>
> (comment below was written before I looked at the other changes)
>
>
>
http://codereview.chromium.org/10416003/diff/1/content/public/common/resource...
> File content/public/common/resource_response.h (right):
>
>
http://codereview.chromium.org/10416003/diff/1/content/public/common/resource...
> content/public/common/resource_response.h:43: class CONTENT_EXPORT
> ResourceResponse
> please keep this as a struct. for the content api, we only want either
> structs or classes that are intefaces in the public dir

base::RefCounted/ThreadSafe is a complex class, so this seemed counter to
export it as a struct - according to our style guide at least. Is content
exempt from these guidelines?

>
> http://codereview.chromium.org/10416003/

[jar (doing other things), 2012-05-21 16:17:09]

base/memory/ref_counted.h LGTM (I didn't review the rest)

[jam, 2012-05-22 00:22:29]

On 2012/05/21 15:32:19, Ryan Sleevi wrote:
> On May 21, 2012 8:18 AM, <mailto:jam@chromium.org> wrote:
> >
> > I find that the additional "->data."  makes things less readable, so I
> prefer
> > the original approach.
> 
> The original approach is not safe, and directly prevents enabling a set of
> compiler warnings from the Chromium Clang plugin.

We have dchecks in place that catch someone trying to delete a ref pointer
directly. So in this specific case, I don't think it matters.

> 
> The goal is to ensure RefCounted classes do not have any public destructors
> - including via any base classes when multiply-inheriting. The first pass
> picked up the cases of single inheritence and shook out some bugs, and this
> pass for muktiple-inheritence has also revealed issues with ambiguous or
> conflicting lifetimes.
> 
> As it was, it was possible to "delete (ResourceResponseHead*)foo" - an
> anti-pattern I would like to prevent.

This will dcheck.

> 
> >
> > (comment below was written before I looked at the other changes)
> >
> >
> >
>
http://codereview.chromium.org/10416003/diff/1/content/public/common/resource...
> > File content/public/common/resource_response.h (right):
> >
> >
>
http://codereview.chromium.org/10416003/diff/1/content/public/common/resource...
> > content/public/common/resource_response.h:43: class CONTENT_EXPORT
> > ResourceResponse
> > please keep this as a struct. for the content api, we only want either
> > structs or classes that are intefaces in the public dir
> 
> base::RefCounted/ThreadSafe is a complex class, so this seemed counter to
> export it as a struct - according to our style guide at least. Is content
> exempt from these guidelines?

content follows the style guide. which part of it are you referring to? note
that we don't count deriving from RefCounted as a concrete object
> 
> >
> > http://codereview.chromium.org/10416003/

[Ryan Sleevi, 2012-05-22 00:37:39]

On 2012/05/22 00:22:29, John Abd-El-Malek wrote:
> On 2012/05/21 15:32:19, Ryan Sleevi wrote:
> > On May 21, 2012 8:18 AM, <mailto:jam@chromium.org> wrote:
> > >
> > > I find that the additional "->data."  makes things less readable, so I
> > prefer
> > > the original approach.
> > 
> > The original approach is not safe, and directly prevents enabling a set of
> > compiler warnings from the Chromium Clang plugin.
> 
> We have dchecks in place that catch someone trying to delete a ref pointer
> directly. So in this specific case, I don't think it matters.

None of the real and actual security bugs related to this have been caught
through these DCHECKs.

Further, I think there's quite a bit to be gained by having a compile-time check
that warns for dangerous patterns than assuming a run-time check will catch it.
Especially when you factor in the degree of multiple inheritance and abstraction
at the chrome/ layers, it's quite possible/probable that the affected code paths
may not be hit until run time.

> 
> > 
> > The goal is to ensure RefCounted classes do not have any public destructors
> > - including via any base classes when multiply-inheriting. The first pass
> > picked up the cases of single inheritence and shook out some bugs, and this
> > pass for muktiple-inheritence has also revealed issues with ambiguous or
> > conflicting lifetimes.
> > 
> > As it was, it was possible to "delete (ResourceResponseHead*)foo" - an
> > anti-pattern I would like to prevent.
> 
> This will dcheck.
> 
> > 
> > >
> > > (comment below was written before I looked at the other changes)
> > >
> > >
> > >
> >
>
http://codereview.chromium.org/10416003/diff/1/content/public/common/resource...
> > > File content/public/common/resource_response.h (right):
> > >
> > >
> >
>
http://codereview.chromium.org/10416003/diff/1/content/public/common/resource...
> > > content/public/common/resource_response.h:43: class CONTENT_EXPORT
> > > ResourceResponse
> > > please keep this as a struct. for the content api, we only want either
> > > structs or classes that are intefaces in the public dir
> > 
> > base::RefCounted/ThreadSafe is a complex class, so this seemed counter to
> > export it as a struct - according to our style guide at least. Is content
> > exempt from these guidelines?
> 
> content follows the style guide. which part of it are you referring to? note
> that we don't count deriving from RefCounted as a concrete object


http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Struct...

AddRef() and Release() are definitely class methods. This is the first I've
heard of any style-exemptions for derived classes - if your base is a class,
shouldn't the derived also be a class? Certainly in keeping with the spirit "If
in doubt, make it a class".

I think the comments on http://codereview.chromium.org/10383262/ should be
considered, as should http://crbug.com/128940 / http://crbug.com/128936 /
http://crbug.com/128942 (and past instances in http://crrev.com/134186 ,
http://crrev.com/130511 , and http://crrev.com/130835 )

[jam, 2012-05-22 15:40:28]

I'm not a fan of the extra "data->". I'll defer to Darin, since he wrote this
struct hierarchy. If he's ok with it, I'll relent.

On 2012/05/22 00:37:39, Ryan Sleevi wrote:
> On 2012/05/22 00:22:29, John Abd-El-Malek wrote:
> > On 2012/05/21 15:32:19, Ryan Sleevi wrote:
> > > On May 21, 2012 8:18 AM, <mailto:jam@chromium.org> wrote:
> > > >
> > > > I find that the additional "->data."  makes things less readable, so I
> > > prefer
> > > > the original approach.
> > > 
> > > The original approach is not safe, and directly prevents enabling a set of
> > > compiler warnings from the Chromium Clang plugin.
> > 
> > We have dchecks in place that catch someone trying to delete a ref pointer
> > directly. So in this specific case, I don't think it matters.
> 
> None of the real and actual security bugs related to this have been caught
> through these DCHECKs.
> 
> Further, I think there's quite a bit to be gained by having a compile-time
check
> that warns for dangerous patterns than assuming a run-time check will catch
it.
> Especially when you factor in the degree of multiple inheritance and
abstraction
> at the chrome/ layers, it's quite possible/probable that the affected code
paths
> may not be hit until run time.
> 
> > 
> > > 
> > > The goal is to ensure RefCounted classes do not have any public
destructors
> > > - including via any base classes when multiply-inheriting. The first pass
> > > picked up the cases of single inheritence and shook out some bugs, and
this
> > > pass for muktiple-inheritence has also revealed issues with ambiguous or
> > > conflicting lifetimes.
> > > 
> > > As it was, it was possible to "delete (ResourceResponseHead*)foo" - an
> > > anti-pattern I would like to prevent.
> > 
> > This will dcheck.
> > 
> > > 
> > > >
> > > > (comment below was written before I looked at the other changes)
> > > >
> > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/10416003/diff/1/content/public/common/resource...
> > > > File content/public/common/resource_response.h (right):
> > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/10416003/diff/1/content/public/common/resource...
> > > > content/public/common/resource_response.h:43: class CONTENT_EXPORT
> > > > ResourceResponse
> > > > please keep this as a struct. for the content api, we only want either
> > > > structs or classes that are intefaces in the public dir
> > > 
> > > base::RefCounted/ThreadSafe is a complex class, so this seemed counter to
> > > export it as a struct - according to our style guide at least. Is content
> > > exempt from these guidelines?
> > 
> > content follows the style guide. which part of it are you referring to? note
> > that we don't count deriving from RefCounted as a concrete object
> 
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Struct...
> 
> AddRef() and Release() are definitely class methods. This is the first I've
> heard of any style-exemptions for derived classes - if your base is a class,
> shouldn't the derived also be a class? 

It seems convenient that someone has a struct that they want to make it
refcounted. Making it a class seems overkill and is explicitly against the style
guide, since now you'd need to hide the data members and add getters/setters.

I have never seen base::RefCountedData before, is that new? I only see two uses
of it in the code base. Before that, we used to refcount structs. I see a number
of examples searching cs for this.

What I meant about deriving from RefCounted is that we ignore it for multiple
inheritance. So it's fine that a class derives from a concrete class and
RefCounted.

> Certainly in keeping with the spirit "If
> in doubt, make it a class".
> 
> I think the comments on http://codereview.chromium.org/10383262/ should be
> considered, as should http://crbug.com/128940 / http://crbug.com/128936 /
> http://crbug.com/128942 (and past instances in http://crrev.com/134186 ,
> http://crrev.com/130511 , and http://crrev.com/130835 )

[darin (slow to review), 2012-05-22 18:01:31]

http://codereview.chromium.org/10416003/diff/1/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

http://codereview.chromium.org/10416003/diff/1/base/memory/ref_counted.h#newc...
base/memory/ref_counted.h:169: ~RefCountedData() {}
probably you want to make this destructor virtual or private.  otherwise,
someone will be sad if they try to inherit from RefCountedData.

http://codereview.chromium.org/10416003/diff/1/content/browser/download/downl...
File content/browser/download/download_resource_handler.cc (right):

http://codereview.chromium.org/10416003/diff/1/content/browser/download/downl...
content/browser/download/download_resource_handler.cc:108:
set_content_length(response->data.content_length);
yuck, the extra "data." is quite unfortunate.

http://codereview.chromium.org/10416003/diff/1/content/public/common/resource...
File content/public/common/resource_response.h (right):

http://codereview.chromium.org/10416003/diff/1/content/public/common/resource...
content/public/common/resource_response.h:43: class CONTENT_EXPORT
ResourceResponse
On 2012/05/21 15:18:43, John Abd-El-Malek wrote:
> please keep this as a struct. for the content api, we only want either structs
> or classes that are intefaces in the public dir

+1

This should be a struct.  What problem was the old code causing?

[darin (slow to review), 2012-05-22 18:03:08]

I'm confused about the changes to ResourceResponse.  It seemed to already have a
non-public destructor.

[Ryan Sleevi, 2012-05-22 18:34:28]

On 2012/05/22 18:03:08, darin wrote:
> I'm confused about the changes to ResourceResponse.  It seemed to already have
a
> non-public destructor.

ResourceResponse did, but there was a public destructor by way of
ResourceResponseHead (a struct). Even though this destructor was not virtual,
meaning one should not be polymorphicly deleting a "ResourceResponseHead*", the
fact that ResourceResponse inherited from this meant that it was possible it
could get burned if a caller did

scoped_ptr<ResourceResponseHead> foo(some_resource_response);

In practice, and in the code today, this doesn't happen. However, the overall
goal is to make it a compile error if a RefCounted type has a public destructor
(on its class or on any of the base classes being implemented by the RefCounted
type). This is to prevent the class of errors that lead to double frees and
ambiguous lifetimes - a class of errors that caused some issues recently and
still have some more yet to be addressed.

To get to that point, I think we should resolve the existing instances, rather
than trying to whitelist and hope no one ever abuses. It's very easy to catch
these at compile time, and that's a much better signal for anti-patterns than
relying on DCHECKs - especially if the DCHECKs are timing and memory dependent
(as they have been in the past), and because they're not in release mode.

[Ryan Sleevi, 2012-05-22 18:36:20]

http://codereview.chromium.org/10416003/diff/1/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

http://codereview.chromium.org/10416003/diff/1/base/memory/ref_counted.h#newc...
base/memory/ref_counted.h:169: ~RefCountedData() {}
On 2012/05/22 18:01:31, darin wrote:
> probably you want to make this destructor virtual or private.  otherwise,
> someone will be sad if they try to inherit from RefCountedData.

See the thread I started on chromium-dev regarding destructors.

Making it private means that this cannot be inherited (as ResourceResponse wants
to do). Making it virtual seems to support polymorphic deletion, but that's
explicitly what we don't want to allow (see the chromium-dev thread).

This is following http://www.gotw.ca/publications/mill18.htm

" A base class destructor should be either public and virtual, or protected and
nonvirtual."

[jam, 2012-05-22 19:10:07]

On Tue, May 22, 2012 at 11:36 AM, <rsleevi@chromium.org> wrote:

>
> http://codereview.chromium.**org/10416003/diff/1/base/**
>
memory/ref_counted.h<http://codereview.chromium.org/10416003/diff/1/base/memory/ref_counted.h>
> File base/memory/ref_counted.h (right):
>
> http://codereview.chromium.**org/10416003/diff/1/base/**
>
memory/ref_counted.h#**newcode169<http://codereview.chromium.org/10416003/diff/1/base/memory/ref_counted.h#newcode169>
> base/memory/ref_counted.h:169: ~RefCountedData() {}
> On 2012/05/22 18:01:31, darin wrote:
>
>> probably you want to make this destructor virtual or private.
>>
> otherwise,
>
>> someone will be sad if they try to inherit from RefCountedData.
>>
>
> See the thread I started on chromium-dev regarding destructors.
>
> Making it private means that this cannot be inherited (as
> ResourceResponse wants to do). Making it virtual seems to support
> polymorphic deletion, but that's explicitly what we don't want to allow
> (see the chromium-dev thread).
>
> This is following
http://www.gotw.ca/**publications/mill18.htm<http://www.gotw.ca/publications/...


We follow the Chrome/Google style guide. There are many other guides on the
internet, starting to quote them is not useful since there are links to
support either side of any guideline..


>
> " A base class destructor should be either public and virtual, or
> protected and nonvirtual."
>
>
http://codereview.chromium.**org/10416003/<http://codereview.chromium.org/104...
>

[darin (slow to review), 2012-05-22 21:27:58]

On 2012/05/22 18:36:20, Ryan Sleevi wrote:
> http://codereview.chromium.org/10416003/diff/1/base/memory/ref_counted.h
> File base/memory/ref_counted.h (right):
> 
>
http://codereview.chromium.org/10416003/diff/1/base/memory/ref_counted.h#newc...
> base/memory/ref_counted.h:169: ~RefCountedData() {}
> On 2012/05/22 18:01:31, darin wrote:
> > probably you want to make this destructor virtual or private.  otherwise,
> > someone will be sad if they try to inherit from RefCountedData.
> 
> See the thread I started on chromium-dev regarding destructors.
> 
> Making it private means that this cannot be inherited (as ResourceResponse
wants
> to do). Making it virtual seems to support polymorphic deletion, but that's
> explicitly what we don't want to allow (see the chromium-dev thread).

I think you are overlooking a very important detail.  If A defines a non-virtual
protected destructor, then when B inherits from A, bad things happen if we
delete B through an A pointer.  In the case we are talking about we have:

class A : RefCounted<A>;
class B : A;

Here, objects of type B are typically deleted through an A pointer.  When the
ref count drops to zero, 'delete (A*) <some address>' is called.  If ~A is
non-virtual, then B's destructor will fail to run!!!  Bad times :-(

So, if you want to allow inheritance of something extending RefCounted<A>, then
A had better have a virtual destructor.

If on the other hand, consider this scenario:

class C { protected: ~C(); };
class D : C {};

Here, it is only possible to delete objects of type D through ~D, so there is no
need for C to have a virtual destructor.  I suspect this is the use case you are
thinking of.

[Ryan Sleevi, 2012-05-22 21:41:19]

On 2012/05/22 21:27:58, darin wrote:
> On 2012/05/22 18:36:20, Ryan Sleevi wrote:
> > http://codereview.chromium.org/10416003/diff/1/base/memory/ref_counted.h
> > File base/memory/ref_counted.h (right):
> > 
> >
>
http://codereview.chromium.org/10416003/diff/1/base/memory/ref_counted.h#newc...
> > base/memory/ref_counted.h:169: ~RefCountedData() {}
> > On 2012/05/22 18:01:31, darin wrote:
> > > probably you want to make this destructor virtual or private.  otherwise,
> > > someone will be sad if they try to inherit from RefCountedData.
> > 
> > See the thread I started on chromium-dev regarding destructors.
> > 
> > Making it private means that this cannot be inherited (as ResourceResponse
> wants
> > to do). Making it virtual seems to support polymorphic deletion, but that's
> > explicitly what we don't want to allow (see the chromium-dev thread).
> 
> I think you are overlooking a very important detail.  If A defines a
non-virtual
> protected destructor, then when B inherits from A, bad things happen if we
> delete B through an A pointer.  In the case we are talking about we have:
> 
> class A : RefCounted<A>;
> class B : A;
> 
> Here, objects of type B are typically deleted through an A pointer.  When the
> ref count drops to zero, 'delete (A*) <some address>' is called.  If ~A is
> non-virtual, then B's destructor will fail to run!!!  Bad times :-(

Right, I was overlooking that RefCounted<typename Foo> calls delete (Foo*)this,
and that in this case, Foo is base::RefCountedData<typename Bar>. Rather than
Bar needing the virtual dtor, base::RefCountedData<...> needs the virtual dtor,
/if/ its to be inherited from.

> 
> So, if you want to allow inheritance of something extending RefCounted<A>,
then
> A had better have a virtual destructor.
> 
> If on the other hand, consider this scenario:
> 
> class C { protected: ~C(); };
> class D : C {};
> 
> Here, it is only possible to delete objects of type D through ~D, so there is
no
> need for C to have a virtual destructor.  I suspect this is the use case you
are
> thinking of.

Right, I follow your point now. Any of the typenames passed to
base::RefCounted[ThreadSafe] need to have polymorphic destructors if they're to
be inherited from, which is well-understood (maybe :-p... More clang plugin
time!)

Since the typename here is base::RefCountedData<>, which is inherited from, the
destructor needs to be virtual.

Seems like private dtor on base::RefCountedData<> + a "typedef
base::RefCountedData<ResourceResponseHead> ResourceResponse" would be better
here, since there isn't a good reason to inherit from RCD. Agreed?

[darin (slow to review), 2012-05-22 21:47:18]

On Tue, May 22, 2012 at 2:41 PM, <rsleevi@chromium.org> wrote:

> On 2012/05/22 21:27:58, darin wrote:
>
>> On 2012/05/22 18:36:20, Ryan Sleevi wrote:
>> > http://codereview.chromium.**org/10416003/diff/1/base/**
>>
memory/ref_counted.h<http://codereview.chromium.org/10416003/diff/1/base/memory/ref_counted.h>
>> > File base/memory/ref_counted.h (right):
>> >
>> >
>>
>
> http://codereview.chromium.**org/10416003/diff/1/base/**
>
memory/ref_counted.h#**newcode169<http://codereview.chromium.org/10416003/diff/1/base/memory/ref_counted.h#newcode169>
>
>> > base/memory/ref_counted.h:169: ~RefCountedData() {}
>> > On 2012/05/22 18:01:31, darin wrote:
>> > > probably you want to make this destructor virtual or private.
>>  otherwise,
>> > > someone will be sad if they try to inherit from RefCountedData.
>> >
>> > See the thread I started on chromium-dev regarding destructors.
>> >
>> > Making it private means that this cannot be inherited (as
>> ResourceResponse
>> wants
>> > to do). Making it virtual seems to support polymorphic deletion, but
>> that's
>> > explicitly what we don't want to allow (see the chromium-dev thread).
>>
>
>  I think you are overlooking a very important detail.  If A defines a
>>
> non-virtual
>
>> protected destructor, then when B inherits from A, bad things happen if we
>> delete B through an A pointer.  In the case we are talking about we have:
>>
>
>  class A : RefCounted<A>;
>> class B : A;
>>
>
>  Here, objects of type B are typically deleted through an A pointer.  When
>> the
>> ref count drops to zero, 'delete (A*) <some address>' is called.  If ~A is
>> non-virtual, then B's destructor will fail to run!!!  Bad times :-(
>>
>
> Right, I was overlooking that RefCounted<typename Foo> calls delete
> (Foo*)this,
> and that in this case, Foo is base::RefCountedData<typename Bar>. Rather
> than
> Bar needing the virtual dtor, base::RefCountedData<...> needs the virtual
> dtor,
> /if/ its to be inherited from.
>
>
>
>  So, if you want to allow inheritance of something extending RefCounted<A>,
>>
> then
>
>> A had better have a virtual destructor.
>>
>
>  If on the other hand, consider this scenario:
>>
>
>  class C { protected: ~C(); };
>> class D : C {};
>>
>
>  Here, it is only possible to delete objects of type D through ~D, so
>> there is
>>
> no
>
>> need for C to have a virtual destructor.  I suspect this is the use case
>> you
>>
> are
>
>> thinking of.
>>
>
> Right, I follow your point now. Any of the typenames passed to
> base::RefCounted[ThreadSafe] need to have polymorphic destructors if
> they're to
> be inherited from, which is well-understood (maybe :-p... More clang plugin
> time!)
>
> Since the typename here is base::RefCountedData<>, which is inherited
> from, the
> destructor needs to be virtual.
>
> Seems like private dtor on base::RefCountedData<> + a "typedef
> base::RefCountedData<**ResourceResponseHead> ResourceResponse" would be
> better
> here, since there isn't a good reason to inherit from RCD. Agreed?
>
>
I think it makes sense to disable inheritance of RCD, but I'm still not so
thrilled about changing ResponseResponse to no longer be a
ResourceResponseHead.  That change introduces the "data" accessor for
users, which mucks up the interface.  I'd prefer to optimize for consumers.

One idea might be to make ResourceResponse still extend from RefCounted,
but it could have a ResourceResponseHead instead.  Then, consumers would do
response->head.foo instead of response->data.foo. That's at least a little
bit better, and it avoids the problem you were concerned about.

-Darin


>
http://codereview.chromium.**org/10416003/<http://codereview.chromium.org/104...
>

[Ryan Sleevi, 2012-05-22 21:59:22]

On 2012/05/22 21:47:18, darin wrote:
> On Tue, May 22, 2012 at 2:41 PM, <mailto:rsleevi@chromium.org> wrote:
> 
> > On 2012/05/22 21:27:58, darin wrote:
> >
> >> On 2012/05/22 18:36:20, Ryan Sleevi wrote:
> >> > http://codereview.chromium.**org/10416003/diff/1/base/**
> >>
>
memory/ref_counted.h<http://codereview.chromium.org/10416003/diff/1/base/memory/ref_counted.h>
> >> > File base/memory/ref_counted.h (right):
> >> >
> >> >
> >>
> >
> > http://codereview.chromium.**org/10416003/diff/1/base/**
> >
>
memory/ref_counted.h#**newcode169<http://codereview.chromium.org/10416003/diff/1/base/memory/ref_counted.h#newcode169>
> >
> >> > base/memory/ref_counted.h:169: ~RefCountedData() {}
> >> > On 2012/05/22 18:01:31, darin wrote:
> >> > > probably you want to make this destructor virtual or private.
> >>  otherwise,
> >> > > someone will be sad if they try to inherit from RefCountedData.
> >> >
> >> > See the thread I started on chromium-dev regarding destructors.
> >> >
> >> > Making it private means that this cannot be inherited (as
> >> ResourceResponse
> >> wants
> >> > to do). Making it virtual seems to support polymorphic deletion, but
> >> that's
> >> > explicitly what we don't want to allow (see the chromium-dev thread).
> >>
> >
> >  I think you are overlooking a very important detail.  If A defines a
> >>
> > non-virtual
> >
> >> protected destructor, then when B inherits from A, bad things happen if we
> >> delete B through an A pointer.  In the case we are talking about we have:
> >>
> >
> >  class A : RefCounted<A>;
> >> class B : A;
> >>
> >
> >  Here, objects of type B are typically deleted through an A pointer.  When
> >> the
> >> ref count drops to zero, 'delete (A*) <some address>' is called.  If ~A is
> >> non-virtual, then B's destructor will fail to run!!!  Bad times :-(
> >>
> >
> > Right, I was overlooking that RefCounted<typename Foo> calls delete
> > (Foo*)this,
> > and that in this case, Foo is base::RefCountedData<typename Bar>. Rather
> > than
> > Bar needing the virtual dtor, base::RefCountedData<...> needs the virtual
> > dtor,
> > /if/ its to be inherited from.
> >
> >
> >
> >  So, if you want to allow inheritance of something extending RefCounted<A>,
> >>
> > then
> >
> >> A had better have a virtual destructor.
> >>
> >
> >  If on the other hand, consider this scenario:
> >>
> >
> >  class C { protected: ~C(); };
> >> class D : C {};
> >>
> >
> >  Here, it is only possible to delete objects of type D through ~D, so
> >> there is
> >>
> > no
> >
> >> need for C to have a virtual destructor.  I suspect this is the use case
> >> you
> >>
> > are
> >
> >> thinking of.
> >>
> >
> > Right, I follow your point now. Any of the typenames passed to
> > base::RefCounted[ThreadSafe] need to have polymorphic destructors if
> > they're to
> > be inherited from, which is well-understood (maybe :-p... More clang plugin
> > time!)
> >
> > Since the typename here is base::RefCountedData<>, which is inherited
> > from, the
> > destructor needs to be virtual.
> >
> > Seems like private dtor on base::RefCountedData<> + a "typedef
> > base::RefCountedData<**ResourceResponseHead> ResourceResponse" would be
> > better
> > here, since there isn't a good reason to inherit from RCD. Agreed?
> >
> >
> I think it makes sense to disable inheritance of RCD, but I'm still not so
> thrilled about changing ResponseResponse to no longer be a
> ResourceResponseHead.  That change introduces the "data" accessor for
> users, which mucks up the interface.  I'd prefer to optimize for consumers.
> 
> One idea might be to make ResourceResponse still extend from RefCounted,
> but it could have a ResourceResponseHead instead.  Then, consumers would do
> response->head.foo instead of response->data.foo. That's at least a little
> bit better, and it avoids the problem you were concerned about.
> 
> -Darin

Absolutely addresses my concerns, by avoiding the M-I. Just to confirm at the
content/ and stylistic layer, we're talking

struct CONTENT_EXPORT ResourceResponse : public
base::RefCounted<ResourceResponse> {
  ResourceResponseHead head;

 private:
  friend class base::RefCounted<ResourceResponse>;
  ~ResourceResponse() {}
};

> 
> 
> >
>
http://codereview.chromium.**org/10416003/%3Chttp://codereview.chromium.org/1...>
> >

[darin (slow to review), 2012-05-22 22:43:47]

On Tue, May 22, 2012 at 2:59 PM, <rsleevi@chromium.org> wrote:

> On 2012/05/22 21:47:18, darin wrote:
>
>  On Tue, May 22, 2012 at 2:41 PM, <mailto:rsleevi@chromium.org> wrote:
>>
>
>  > On 2012/05/22 21:27:58, darin wrote:
>> >
>> >> On 2012/05/22 18:36:20, Ryan Sleevi wrote:
>> >> > http://codereview.chromium.****org/10416003/diff/1/base/**
>> >>
>>
>
> memory/ref_counted.h<http://**codereview.chromium.org/**
>
10416003/diff/1/base/memory/**ref_counted.h<http://codereview.chromium.org/10416003/diff/1/base/memory/ref_counted.h>
> >
>
>> >> > File base/memory/ref_counted.h (right):
>> >> >
>> >> >
>> >>
>> >
>> > http://codereview.chromium.****org/10416003/diff/1/base/**
>> >
>>
>
> memory/ref_counted.h#****newcode169<http://codereview.**
>
chromium.org/10416003/diff/1/**base/memory/ref_counted.h#**newcode169<http://codereview.chromium.org/10416003/diff/1/base/memory/ref_counted.h#newcode169>
> >
>
>> >
>> >> > base/memory/ref_counted.h:169: ~RefCountedData() {}
>> >> > On 2012/05/22 18:01:31, darin wrote:
>> >> > > probably you want to make this destructor virtual or private.
>> >>  otherwise,
>> >> > > someone will be sad if they try to inherit from RefCountedData.
>> >> >
>> >> > See the thread I started on chromium-dev regarding destructors.
>> >> >
>> >> > Making it private means that this cannot be inherited (as
>> >> ResourceResponse
>> >> wants
>> >> > to do). Making it virtual seems to support polymorphic deletion, but
>> >> that's
>> >> > explicitly what we don't want to allow (see the chromium-dev thread).
>> >>
>> >
>> >  I think you are overlooking a very important detail.  If A defines a
>> >>
>> > non-virtual
>> >
>> >> protected destructor, then when B inherits from A, bad things happen
>> if we
>> >> delete B through an A pointer.  In the case we are talking about we
>> have:
>> >>
>> >
>> >  class A : RefCounted<A>;
>> >> class B : A;
>> >>
>> >
>> >  Here, objects of type B are typically deleted through an A pointer.
>>  When
>> >> the
>> >> ref count drops to zero, 'delete (A*) <some address>' is called.  If
>> ~A is
>> >> non-virtual, then B's destructor will fail to run!!!  Bad times :-(
>> >>
>> >
>> > Right, I was overlooking that RefCounted<typename Foo> calls delete
>> > (Foo*)this,
>> > and that in this case, Foo is base::RefCountedData<typename Bar>. Rather
>> > than
>> > Bar needing the virtual dtor, base::RefCountedData<...> needs the
>> virtual
>> > dtor,
>> > /if/ its to be inherited from.
>> >
>> >
>> >
>> >  So, if you want to allow inheritance of something extending
>> RefCounted<A>,
>> >>
>> > then
>> >
>> >> A had better have a virtual destructor.
>> >>
>> >
>> >  If on the other hand, consider this scenario:
>> >>
>> >
>> >  class C { protected: ~C(); };
>> >> class D : C {};
>> >>
>> >
>> >  Here, it is only possible to delete objects of type D through ~D, so
>> >> there is
>> >>
>> > no
>> >
>> >> need for C to have a virtual destructor.  I suspect this is the use
>> case
>> >> you
>> >>
>> > are
>> >
>> >> thinking of.
>> >>
>> >
>> > Right, I follow your point now. Any of the typenames passed to
>> > base::RefCounted[ThreadSafe] need to have polymorphic destructors if
>> > they're to
>> > be inherited from, which is well-understood (maybe :-p... More clang
>> plugin
>> > time!)
>> >
>> > Since the typename here is base::RefCountedData<>, which is inherited
>> > from, the
>> > destructor needs to be virtual.
>> >
>> > Seems like private dtor on base::RefCountedData<> + a "typedef
>> > base::RefCountedData<****ResourceResponseHead> ResourceResponse" would
>> be
>>
>> > better
>> > here, since there isn't a good reason to inherit from RCD. Agreed?
>> >
>> >
>> I think it makes sense to disable inheritance of RCD, but I'm still not so
>> thrilled about changing ResponseResponse to no longer be a
>> ResourceResponseHead.  That change introduces the "data" accessor for
>> users, which mucks up the interface.  I'd prefer to optimize for
>> consumers.
>>
>
>  One idea might be to make ResourceResponse still extend from RefCounted,
>> but it could have a ResourceResponseHead instead.  Then, consumers would
>> do
>> response->head.foo instead of response->data.foo. That's at least a little
>> bit better, and it avoids the problem you were concerned about.
>>
>
>  -Darin
>>
>
> Absolutely addresses my concerns, by avoiding the M-I. Just to confirm at
> the
> content/ and stylistic layer, we're talking
>
> struct CONTENT_EXPORT ResourceResponse : public
> base::RefCounted<**ResourceResponse> {
>  ResourceResponseHead head;
>
>  private:
>  friend class base::RefCounted<**ResourceResponse>;
>  ~ResourceResponse() {}
> };
>

Yes.



>
>
>
>  >
>>
>
> http://codereview.chromium.****org/10416003/%3Chttp://coderev**
> iew.chromium.org/10416003/ <http://codereview.chromium.org/10416003/>>
>
>> >
>>
>
>
>
>
http://codereview.chromium.**org/10416003/<http://codereview.chromium.org/104...
>

[commit-bot: I haz the power, 2012-06-04 07:59:46]

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

[commit-bot: I haz the power, 2012-06-04 07:59:56]

Presubmit check for 10416003-26001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser/renderer_host
    content/browser/debugger
    content/public
    content/browser/download

Presubmit checks took 1.5s to calculate.

[Ryan Sleevi, 2012-06-11 21:19:22]

On 2012/06/04 07:59:56, I haz the power (commit-bot) wrote:
> Presubmit check for 10416003-26001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     content/browser/renderer_host
>     content/browser/debugger
>     content/public
>     content/browser/download
> 
> Presubmit checks took 1.5s to calculate.

Sorry about that. Changes were made in Patchset 2, rebased to ToT in 3/4 just to
update callers.

Darin or John, does this work for you?

[jam, 2012-06-14 19:05:36]

lgtm

[commit-bot: I haz the power, 2012-06-14 19:26:43]

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

[commit-bot: I haz the power, 2012-06-14 19:26:55]

Failed to apply patch for
content/browser/renderer_host/resource_dispatcher_host_impl.cc:
While running patch -p1 --forward --force;
patching file content/browser/renderer_host/resource_dispatcher_host_impl.cc
Hunk #1 FAILED at 195.
Hunk #2 FAILED at 1566.
2 out of 2 hunks FAILED -- saving rejects to file
content/browser/renderer_host/resource_dispatcher_host_impl.cc.rej

[commit-bot: I haz the power, 2012-06-25 06:57:35]

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

[commit-bot: I haz the power, 2012-06-25 08:15:44]

Change committed as 143880