Implement ClientSocketFactory::ClearSSLSessionCache

Add a new method of ClientSocketFactory for clearing the cache used for SSL session resumption.

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

[Ryan Hamilton, 2011-02-23 19:50:45]

wtc:  Here is a *very* short CL which implements a non-static
ClientSocketFactory::ClearSSLSessionCache, as per your most recent feedback at:
http://codereview.chromium.org/6487012/diff/30051/net/socket/ssl_client_socke...

This CL requires that http://codereview.chromium.org/6561003 be applied first,
since it makes use of g_use_system_ssl.

[wtc, 2011-02-24 01:00:14]

Thanks for the patch.  Please see my suggested changes below.

http://codereview.chromium.org/6580006/diff/4001/net/socket/client_socket_fac...
File net/socket/client_socket_factory.cc (right):

http://codereview.chromium.org/6580006/diff/4001/net/socket/client_socket_fac...
net/socket/client_socket_factory.cc:96: void
ClientSocketFactory::ClearSSLSessionCache() {
This should be defined by the DefaultClientSocketFactory
class.  In the ClientSocketFactory class this should be
a pure virtual method (= 0).

http://codereview.chromium.org/6580006/diff/4001/net/socket/client_socket_fac...
net/socket/client_socket_factory.cc:100: }
Nit: don't use curly braces {} around simple one-liner "if"
bodies.  Make the same change to lines 106-108 below.

http://codereview.chromium.org/6580006/diff/4001/net/socket/client_socket_fac...
net/socket/client_socket_factory.cc:111: return NULL;
Bug: remove "return NULL;" because this function returns void.

http://codereview.chromium.org/6580006/diff/4001/net/socket/client_socket_fac...
File net/socket/client_socket_factory.h (right):

http://codereview.chromium.org/6580006/diff/4001/net/socket/client_socket_fac...
net/socket/client_socket_factory.h:56: // for the NSS SSL library, which is the
default for Windows, Mac and Linux.
I think we don't need to mention that this ClearSSLSessionCache
is only implemented for the NSS SSL library.  Alternatively,
make this a TODO comment.

IMPORTANT: this should be a pure virtual method (= 0).
I'm sorry that this will make the CL bigger.

[Ryan Hamilton, 2011-02-24 22:38:46]

http://codereview.chromium.org/6580006/diff/4001/net/socket/client_socket_fac...
File net/socket/client_socket_factory.cc (right):

http://codereview.chromium.org/6580006/diff/4001/net/socket/client_socket_fac...
net/socket/client_socket_factory.cc:96: void
ClientSocketFactory::ClearSSLSessionCache() {
On 2011/02/24 01:00:14, wtc wrote:
> This should be defined by the DefaultClientSocketFactory
> class.  In the ClientSocketFactory class this should be
> a pure virtual method (= 0).

Done.

http://codereview.chromium.org/6580006/diff/4001/net/socket/client_socket_fac...
net/socket/client_socket_factory.cc:100: }
On 2011/02/24 01:00:14, wtc wrote:
> Nit: don't use curly braces {} around simple one-liner "if"
> bodies.  Make the same change to lines 106-108 below.

Done!  Sorry about that!  GFE *requires* braces on one line "if"s, Chrome
rejects them.   So many standards to choose from :>

http://codereview.chromium.org/6580006/diff/4001/net/socket/client_socket_fac...
net/socket/client_socket_factory.cc:111: return NULL;
On 2011/02/24 01:00:14, wtc wrote:
> Bug: remove "return NULL;" because this function returns void.

Done.

http://codereview.chromium.org/6580006/diff/4001/net/socket/client_socket_fac...
File net/socket/client_socket_factory.h (right):

http://codereview.chromium.org/6580006/diff/4001/net/socket/client_socket_fac...
net/socket/client_socket_factory.h:56: // for the NSS SSL library, which is the
default for Windows, Mac and Linux.
On 2011/02/24 01:00:14, wtc wrote:
> I think we don't need to mention that this ClearSSLSessionCache
> is only implemented for the NSS SSL library.  Alternatively,
> make this a TODO comment.

Done.

> IMPORTANT: this should be a pure virtual method (= 0).
> I'm sorry that this will make the CL bigger.

This only added a couple of files to the CL.  No biggie...

[wtc, 2011-02-24 22:49:37]

LGTM.  Please check in after making my suggested changes
below, in particular the one marked with "IMPORTANT".  Thanks!

http://codereview.chromium.org/6580006/diff/5004/jingle/notifier/base/xmpp_cl...
File jingle/notifier/base/xmpp_client_socket_factory.h (right):

http://codereview.chromium.org/6580006/diff/5004/jingle/notifier/base/xmpp_cl...
jingle/notifier/base/xmpp_client_socket_factory.h:38: virtual void
ClearSSLSessionCache() {}
IMPORTANT: Please implement this method in xmpp_client_socket_factory.cc.

Comparing with the
XmppClientSocketFactory::CreateSSLClientSocket definition,
you will see that this method probably should be implemented as

  client_socket_factory_->ClearSSLSessionCache();

instead of a no-op.

http://codereview.chromium.org/6580006/diff/5004/net/socket/client_socket_poo...
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/6580006/diff/5004/net/socket/client_socket_poo...
net/socket/client_socket_pool_base_unittest.cc:120: virtual void
ClearSSLSessionCache() {}
Probably should add NOTIMPLEMENTED() because the
CreateSSLClientSocket() method has that.

http://codereview.chromium.org/6580006/diff/5004/net/socket/socket_test_util.h
File net/socket/socket_test_util.h (right):

http://codereview.chromium.org/6580006/diff/5004/net/socket/socket_test_util....
net/socket/socket_test_util.h:549: virtual void ClearSSLSessionCache() {}
Nit: please define these two ClearSSLSessionCache() methods
in socket_test_util.cc, next to the corresponding
CreateSSLClientSocket() method definitions.

http://codereview.chromium.org/6580006/diff/5004/net/socket/tcp_client_socket...
File net/socket/tcp_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/6580006/diff/5004/net/socket/tcp_client_socket...
net/socket/tcp_client_socket_pool_unittest.cc:262: virtual void
ClearSSLSessionCache() {}
Consider adding NOTIMPLEMENTED().  See the
CreateSSLClientSocket() method above.

[wtc, 2011-02-24 22:50:44]

Please fix the Description (commit message to be) of the CL.
It is now copied from the source code verbatim.

[Ryan Hamilton, 2011-02-24 23:35:57]

On 2011/02/24 22:50:44, wtc wrote:
> Please fix the Description (commit message to be) of the CL.
> It is now copied from the source code verbatim.

Done.

[Ryan Hamilton, 2011-02-24 23:38:34]

http://codereview.chromium.org/6580006/diff/5004/jingle/notifier/base/xmpp_cl...
File jingle/notifier/base/xmpp_client_socket_factory.h (right):

http://codereview.chromium.org/6580006/diff/5004/jingle/notifier/base/xmpp_cl...
jingle/notifier/base/xmpp_client_socket_factory.h:38: virtual void
ClearSSLSessionCache() {}
On 2011/02/24 22:49:37, wtc wrote:
> IMPORTANT: Please implement this method in xmpp_client_socket_factory.cc.
> 
> Comparing with the
> XmppClientSocketFactory::CreateSSLClientSocket definition,
> you will see that this method probably should be implemented as
> 
>   client_socket_factory_->ClearSSLSessionCache();
> 
> instead of a no-op.
>   

Done.

http://codereview.chromium.org/6580006/diff/5004/net/socket/client_socket_poo...
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/6580006/diff/5004/net/socket/client_socket_poo...
net/socket/client_socket_pool_base_unittest.cc:120: virtual void
ClearSSLSessionCache() {}
On 2011/02/24 22:49:37, wtc wrote:
> Probably should add NOTIMPLEMENTED() because the
> CreateSSLClientSocket() method has that.

Done.

http://codereview.chromium.org/6580006/diff/5004/net/socket/socket_test_util.h
File net/socket/socket_test_util.h (right):

http://codereview.chromium.org/6580006/diff/5004/net/socket/socket_test_util....
net/socket/socket_test_util.h:549: virtual void ClearSSLSessionCache() {}
On 2011/02/24 22:49:37, wtc wrote:
> Nit: please define these two ClearSSLSessionCache() methods
> in socket_test_util.cc, next to the corresponding
> CreateSSLClientSocket() method definitions.

Done.

http://codereview.chromium.org/6580006/diff/5004/net/socket/tcp_client_socket...
File net/socket/tcp_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/6580006/diff/5004/net/socket/tcp_client_socket...
net/socket/tcp_client_socket_pool_unittest.cc:262: virtual void
ClearSSLSessionCache() {}
On 2011/02/24 22:49:37, wtc wrote:
> Consider adding NOTIMPLEMENTED().  See the
> CreateSSLClientSocket() method above.

Done.

[wtc, 2011-02-25 16:20:10]

LGTM!