Add virtual to some base classes that have virtual methods...

Add virtual to some base classes that have virtual methods

This is defensive coding to avoid memory leaks.

bug=47469
r=wtc

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

[jar (doing other things), 2010-06-25 00:20:18]

Wtc: Can you look at the security related file?
Darin: Can you comment on the message loop related file?

Thanks,

Jim

[wtc, 2010-06-25 00:26:38]

LGTM.

http://codereview.chromium.org/2869024/diff/1/3
File net/base/transport_security_state.h (right):

http://codereview.chromium.org/2869024/diff/1/3#newcode80
net/base/transport_security_state.h:80: virtual ~Delegate() {}
Nit: add a blank line below.

http://codereview.chromium.org/2869024/diff/1/4
File net/url_request/https_prober.h (right):

http://codereview.chromium.org/2869024/diff/1/4#newcode26
net/url_request/https_prober.h:26: virtual ~HTTPSProberDelegate() {}
Nit: list the destructor before other methods.  See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

[jar (doing other things), 2010-06-25 00:54:50]

Changes made per wtc's suggestions.

http://codereview.chromium.org/2869024/diff/1/3
File net/base/transport_security_state.h (right):

http://codereview.chromium.org/2869024/diff/1/3#newcode80
net/base/transport_security_state.h:80: virtual ~Delegate() {}
On 2010/06/25 00:26:38, wtc wrote:
> Nit: add a blank line below.

Done.

http://codereview.chromium.org/2869024/diff/1/4
File net/url_request/https_prober.h (right):

http://codereview.chromium.org/2869024/diff/1/4#newcode26
net/url_request/https_prober.h:26: virtual ~HTTPSProberDelegate() {}
On 2010/06/25 00:26:38, wtc wrote:
> Nit: list the destructor before other methods.  See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

Done.

[darin (slow to review), 2010-06-25 03:40:19]

http://codereview.chromium.org/2869024/diff/7001/8001
File base/message_loop_proxy.h (right):

http://codereview.chromium.org/2869024/diff/7001/8001#newcode22
base/message_loop_proxy.h:22: virtual ~MessageLoopProxy() { }
nit: please move this into the protected section so that consumers cannot
directly delete objects of this type.  Note the "delete this" in the OnDestruct
method.

http://codereview.chromium.org/2869024/diff/7001/8002
File net/base/transport_security_state.h (right):

http://codereview.chromium.org/2869024/diff/7001/8002#newcode80
net/base/transport_security_state.h:80: virtual ~Delegate() {}
I'm guessing that the existing code does not expect someone to delete a Delegate
pointer.  If that's indeed true, then it would be good to say so by making this
destructor protected.

http://codereview.chromium.org/2869024/diff/7001/8003
File net/url_request/https_prober.h (right):

http://codereview.chromium.org/2869024/diff/7001/8003#newcode25
net/url_request/https_prober.h:25: virtual ~HTTPSProberDelegate() {}
same comment.  perhaps this should be protected.

[jar (doing other things), 2010-06-25 16:13:32]

Changes and comments based on Darin's suggestions.

http://codereview.chromium.org/2869024/diff/7001/8001
File base/message_loop_proxy.h (right):

http://codereview.chromium.org/2869024/diff/7001/8001#newcode22
base/message_loop_proxy.h:22: virtual ~MessageLoopProxy() { }
On 2010/06/25 03:40:19, darin wrote:
> nit: please move this into the protected section so that consumers cannot
> directly delete objects of this type.  Note the "delete this" in the
OnDestruct
> method.

Done.

http://codereview.chromium.org/2869024/diff/7001/8002
File net/base/transport_security_state.h (right):

http://codereview.chromium.org/2869024/diff/7001/8002#newcode80
net/base/transport_security_state.h:80: virtual ~Delegate() {}
On 2010/06/25 03:40:19, darin wrote:
> I'm guessing that the existing code does not expect someone to delete a
Delegate
> pointer.  If that's indeed true, then it would be good to say so by making
this
> destructor protected.

Done.

http://codereview.chromium.org/2869024/diff/7001/8003
File net/url_request/https_prober.h (right):

http://codereview.chromium.org/2869024/diff/7001/8003#newcode25
net/url_request/https_prober.h:25: virtual ~HTTPSProberDelegate() {}
On 2010/06/25 03:40:19, darin wrote:
> same comment.  perhaps this should be protected.

In the file url_request_http_job.cc, at line 911 in
URLRequestHttpJob::ProcessStrictTransportSecurityHeader(), the delegate is
deleted, which is not allowed if the destructor is protected.

[darin (slow to review), 2010-06-25 16:37:30]

http://codereview.chromium.org/2869024/diff/7001/8003
File net/url_request/https_prober.h (right):

http://codereview.chromium.org/2869024/diff/7001/8003#newcode25
net/url_request/https_prober.h:25: virtual ~HTTPSProberDelegate() {}
On 2010/06/25 16:13:32, jar wrote:
> On 2010/06/25 03:40:19, darin wrote:
> > same comment.  perhaps this should be protected.
> 
> In the file url_request_http_job.cc, at line 911 in
> URLRequestHttpJob::ProcessStrictTransportSecurityHeader(), the delegate is
> deleted, which is not allowed if the destructor is protected.

You can have your cake and eat it too by deleting the "net::" prefix at line
905.  Then, at line 911 you'll be deleting the concrete type and not the
abstract base type.

btw, the fact that these class names differ only in their namespace is super
confusing!

[jar (doing other things), 2010-06-25 21:55:51]

Change per Darin's suggestion.

http://codereview.chromium.org/2869024/diff/7001/8003
File net/url_request/https_prober.h (right):

http://codereview.chromium.org/2869024/diff/7001/8003#newcode25
net/url_request/https_prober.h:25: virtual ~HTTPSProberDelegate() {}
On 2010/06/25 16:37:30, darin wrote:
> On 2010/06/25 16:13:32, jar wrote:
> > On 2010/06/25 03:40:19, darin wrote:
> > > same comment.  perhaps this should be protected.
> > 
> > In the file url_request_http_job.cc, at line 911 in
> > URLRequestHttpJob::ProcessStrictTransportSecurityHeader(), the delegate is
> > deleted, which is not allowed if the destructor is protected.
> 
> You can have your cake and eat it too by deleting the "net::" prefix at line
> 905.  Then, at line 911 you'll be deleting the concrete type and not the
> abstract base type.
> 
> btw, the fact that these class names differ only in their namespace is super
> confusing!

Done.

[darin (slow to review), 2010-06-25 22:41:57]

LGTM