RefCounted types should not have public destructors, net/

RefCounted types should not have public destructors, net/

BUG=123295
TEST=none


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

[Ryan Sleevi, 2012-04-13 09:34:21]

cbentzel: PTAL

[eroman, 2012-04-13 18:59:34]

lgtm (i can take this CL from Chris)

http://codereview.chromium.org/10066045/diff/1/net/base/default_server_bound_...
File net/base/default_server_bound_cert_store_unittest.cc (right):

http://codereview.chromium.org/10066045/diff/1/net/base/default_server_bound_...
net/base/default_server_bound_cert_store_unittest.cc:46:
MockPersistentStore::~MockPersistentStore() {}
nit: can you move this down to match declaration order?

http://codereview.chromium.org/10066045/diff/1/net/base/file_stream_net_log_p...
File net/base/file_stream_net_log_parameters.cc (right):

http://codereview.chromium.org/10066045/diff/1/net/base/file_stream_net_log_p...
net/base/file_stream_net_log_parameters.cc:16:
FileStreamErrorParameters::~FileStreamErrorParameters() {}
Please match declaration order -- i.e. put this after ToValue().

This comment applies to other places in this CL. Note that I don't expect you to
do it for all the refactorings that occur in this CL, but it is something to
keep in mind.

http://codereview.chromium.org/10066045/diff/1/net/url_request/url_request_ne...
File net/url_request/url_request_netlog_params.cc (right):

http://codereview.chromium.org/10066045/diff/1/net/url_request/url_request_ne...
net/url_request/url_request_netlog_params.cc:22:
URLRequestStartEventParameters::~URLRequestStartEventParameters() {}
match declaration order.

[Ryan Sleevi, 2012-04-13 19:04:43]

Thanks Eric.

I'm waiting to hear back from erg@, who previously wrote a Clang plugin to warn
about implementation + header mismatches. Given that my observations of the code
are that we have regressed in this style area, I anticipate a "make them all
match" to be another large change. 

Are you receptive to (temporarily) allowing the mismatch so that we can get
Clang to start warning about the RefCounted-public-dtor issue, then take a pass
to clean all of the mismatches up, then enable the Clang plugin to start warning
about those as well? Or is that too un-Chromie, and I should go check the 400+
files again to check for ordering mismatches?

[eroman, 2012-04-13 19:25:02]

Yes absolutely. In fact doing it all as a post-step would be preferable IMO to
keep the changes more readable. Thanks.

[cbentzel, 2012-04-16 17:59:42]

On 2012/04/13 19:25:02, eroman wrote:
> Yes absolutely. In fact doing it all as a post-step would be preferable IMO to
> keep the changes more readable. Thanks.

Sorry, was on vacation when this came in. If eroman says LGTM I agree.

[cbentzel, 2012-04-16 18:01:52]

Also, did this uncover any bugs? At one point I built Chrome using gcc with
-Wno-non-virtual-dtor and it found some actual problems. This is a different
case, but would be neat if this caught a case where an object was being
explicitly deleted while references were still held to it.

[Ryan Sleevi, 2012-04-16 18:18:14]

On 2012/04/16 18:01:52, cbentzel wrote:
> Also, did this uncover any bugs? At one point I built Chrome using gcc with
> -Wno-non-virtual-dtor and it found some actual problems. This is a different
> case, but would be neat if this caught a case where an object was being
> explicitly deleted while references were still held to it.

Yes. This was originally prompted by a bug that was introduced in the remoting
host (stack allocated instead of heap allocated). I saw the fix go by and looked
for other instances of this pattern by enhancing our clang plugin (
http://codereview.chromium.org/10005022/ ). There was an instance in the NaCL
host where a similar pattern existed - a scoped_ptr<> being used to hold a
ref-counted object ( http://codereview.chromium.org/10069051/ )

This first pass gets most of the low-hanging fruit. To ensure correctness, some
other evening/weekend/period of immense free time, I plan on enhancing the
plugin to walk through the object hierarchy, to handle cases of multiple
inheritance (class Foo : public A, public B, public RefCountedThreadSafe<Foo>),
which is a common pattern in our code. The check will be to see if |A| or |B|
have public destructors, and if so, warn that Foo either shouldn't be
implementing A/B or they shouldn't have public destructors.

This is to cover the case where the interface may be that |A| is some Delegate,
and the contract is "consumer takes ownership of the delegate".

[wtc, 2012-04-17 23:16:17]

Drive-by review comments on Patch set 1:

I only reviewed the first few files.  My first comment below
applies to all derived classes of a ref-counted base class.

http://codereview.chromium.org/10066045/diff/1/net/base/address_list_net_log_...
File net/base/address_list_net_log_param.h (right):

http://codereview.chromium.org/10066045/diff/1/net/base/address_list_net_log_...
net/base/address_list_net_log_param.h:24: virtual ~AddressListNetLogParam() {}

It is not obvious why this class needs to have a private
destructor because the fact that this class is ref-counted
is not shown in this header.

If one tries to delete an instance of this class directly,
won't the compiler bark about that?  If the compiler detects
this, then I prefer that we don't make the destructor of
such a derived class private.

http://codereview.chromium.org/10066045/diff/1/net/base/default_server_bound_...
File net/base/default_server_bound_cert_store.h (right):

http://codereview.chromium.org/10066045/diff/1/net/base/default_server_bound_...
net/base/default_server_bound_cert_store.h:150: friend class
base::RefCountedThreadSafe<PersistentStore>;

Can we use the RefcountedPersistentStore typedef here for
brevity?  I am not sure if that'll be better though.

[Ryan Sleevi, 2012-04-17 23:42:42]

http://codereview.chromium.org/10066045/diff/1/net/base/address_list_net_log_...
File net/base/address_list_net_log_param.h (right):

http://codereview.chromium.org/10066045/diff/1/net/base/address_list_net_log_...
net/base/address_list_net_log_param.h:24: virtual ~AddressListNetLogParam() {}
On 2012/04/17 23:16:18, wtc wrote:
> 
> It is not obvious why this class needs to have a private
> destructor because the fact that this class is ref-counted
> is not shown in this header.
> 
> If one tries to delete an instance of this class directly,
> won't the compiler bark about that?  If the compiler detects
> this, then I prefer that we don't make the destructor of
> such a derived class private.

No. It will not. And several recent bugs show that this does happen.

The destructor must be /protected/ or /private/ for the compiler to know. It's
perfectly valid for a derived class to give greater visibility than its base
class affords.

Here, the default destructor is being implicitly created by the compiler, which
defaults to public. This then permits the following construct:

AddressListNetLogParam* param = new AddressListNetLogParam(...);
delete reinterpret_cast<NetLog::EventParameters*>(param);  // compiler error
delete param;  // valid, as AddressListNetLogParam is public

More commonly (and where the bugs crept in) was people sub-classing some
interface, not declaring a destructor, then sticking the subclassed type in a
scoped_ptr<>. Had they done scoped_ptr<Base> it would have failed to compile,
but because they did scoped_ptr<Derived>, it succeeds.

The compiler warning for Clang that I added to our plugin is able to walk the
object hierarchy and detect these transitive-ref-counted types and warn
appropriately, which is how this CL was generated (
http://codereview.chromium.org/10005022/ ). Once the existing instances are
fixed, this will be enabled, which will cover Mac, Linux, and ChromeOS, and
provide significant coverage for shared code.

http://codereview.chromium.org/10066045/diff/1/net/base/default_server_bound_...
File net/base/default_server_bound_cert_store.h (right):

http://codereview.chromium.org/10066045/diff/1/net/base/default_server_bound_...
net/base/default_server_bound_cert_store.h:150: friend class
base::RefCountedThreadSafe<PersistentStore>;
On 2012/04/17 23:16:18, wtc wrote:
> 
> Can we use the RefcountedPersistentStore typedef here for
> brevity?  I am not sure if that'll be better though.

I'm not sure that's much better. The use of the typedef is itself a bit odd, but
presumably done for line length reasons, as it's never re-used.

[cbentzel, 2012-04-26 20:42:34]

Are you continuing on this CL, or is still being held up due to the style
concerns that you and pkasting (amongst others) raised on chromium-dev?

[Ryan Sleevi, 2012-04-26 20:44:53]

On 2012/04/26 20:42:34, cbentzel wrote:
> Are you continuing on this CL, or is still being held up due to the style
> concerns that you and pkasting (amongst others) raised on chromium-dev?
Slowly going through the 20 or so CLs addressing the style concerns and then
landing.

Currently about halfway through.

[commit-bot: I haz the power, 2012-04-28 06:23:13]

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

[commit-bot: I haz the power, 2012-04-28 06:41:56]

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

[commit-bot: I haz the power, 2012-04-28 07:03:09]

Try job failure for 10066045-27001 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-04-28 07:20:05]

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

[commit-bot: I haz the power, 2012-04-28 07:45:37]

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

[commit-bot: I haz the power, 2012-04-28 08:03:31]

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

[commit-bot: I haz the power, 2012-04-28 09:29:33]

Change committed as 134460