Fixes for OpenSSL

Fixes for OpenSSL

A couple of nit fixes following:
+ http://codereview.chromium.org/7247005
+ http://codereview.chromium.org/6990036

+ Fixes a typo (ctx / context)
+ Adds new abstract methods from stream_socket.h

BUG=
TEST=


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

[bulach, 2011-06-28 10:31:27]

Hi agl, gagan,

I bundled a couple of unrelated fixes in this patch, would you mind taking a
look please?

[agl, 2011-06-28 11:17:46]

LGTM

[agl, 2011-06-28 11:19:19]

LGTM

[gagansingh, 2011-06-28 11:32:03]

http://codereview.chromium.org/7253038/diff/1/net/socket/ssl_client_socket_op...
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/7253038/diff/1/net/socket/ssl_client_socket_op...
net/socket/ssl_client_socket_openssl.cc:1102: return 0;
maybe return -1 as 0 might mean no content (empty response)

http://codereview.chromium.org/7253038/diff/1/net/socket/ssl_client_socket_op...
net/socket/ssl_client_socket_openssl.cc:1110: return base::TimeDelta();
maybe -1 here as well?

[bulach, 2011-06-28 13:49:52]

thanks both! Gagan, addressed your comments. another look, please?

http://codereview.chromium.org/7253038/diff/1/net/socket/ssl_client_socket_op...
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/7253038/diff/1/net/socket/ssl_client_socket_op...
net/socket/ssl_client_socket_openssl.cc:1102: return 0;
On 2011/06/28 11:32:04, gagansingh wrote:
> maybe return -1 as 0 might mean no content (empty response)

Done.

http://codereview.chromium.org/7253038/diff/1/net/socket/ssl_client_socket_op...
net/socket/ssl_client_socket_openssl.cc:1110: return base::TimeDelta();
On 2011/06/28 11:32:04, gagansingh wrote:
> maybe -1 here as well?

Done.

[gagansingh, 2011-06-28 13:59:29]

LGTM

[commit-bot: I haz the power, 2011-06-29 09:15:13]

Presubmit check for 7253038-2003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
net/socket/ssl_client_socket_openssl.cc,net/socket/ssl_client_socket_openssl.h

[bulach, 2011-06-29 09:25:24]

Hi Mike,

I need your OWNERS power for this :) would you mind taking a look please?

Thanks!

[bulach, 2011-07-01 09:38:41]

+wtc for owners approval, please.

[wtc, 2011-07-01 16:41:17]

LGTM.

http://codereview.chromium.org/7253038/diff/2003/crypto/openpgp_symmetric_enc...
File crypto/openpgp_symmetric_encryption_openssl.cc (right):

http://codereview.chromium.org/7253038/diff/2003/crypto/openpgp_symmetric_enc...
crypto/openpgp_symmetric_encryption_openssl.cc:141: EVP_MD_CTX_cleanup(&ctx);
I think this file has been removed.

http://codereview.chromium.org/7253038/diff/2003/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/7253038/diff/2003/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1102: return -1;
I think returning 0 here and in GetConnectTimeMicros
is better.  0 is the initial values used by
TCPClientSocketWin and TCPClientSocketLibevent.

I am especially worried about returning a TimeDelta of -1.
That may confuse code.

I know you copied the -1 value from ssl_client_socket_nss.cc.
So this is fine for now.

[bulach, 2011-07-05 10:06:54]

thanks wtc!
explanations inline, I'll land to get the bot green but happy to follow up. (I
also squeezed in a minor fix for cert_database_openssl.cc).

http://codereview.chromium.org/7253038/diff/2003/crypto/openpgp_symmetric_enc...
File crypto/openpgp_symmetric_encryption_openssl.cc (right):

http://codereview.chromium.org/7253038/diff/2003/crypto/openpgp_symmetric_enc...
crypto/openpgp_symmetric_encryption_openssl.cc:141: EVP_MD_CTX_cleanup(&ctx);
On 2011/07/01 16:41:17, wtc wrote:
> I think this file has been removed.

looks like the deletion was reverted.. :)

http://codereview.chromium.org/7253038/diff/2003/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/7253038/diff/2003/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1102: return -1;
I'll keep this as is now for consistency.
joth pointed out that it'd be cleaner to have:

CHECK(transport_.get() && transport_->socked());
return transport_->socket()->FooBar();

or even better, have some adapter / base class that we could re-use across all
platform rather than duplicate this code..
if you agree with either suggestions, I'm happy to follow up!

On 2011/07/01 16:41:17, wtc wrote:
> I think returning 0 here and in GetConnectTimeMicros
> is better.  0 is the initial values used by
> TCPClientSocketWin and TCPClientSocketLibevent.
> 
> I am especially worried about returning a TimeDelta of -1.
> That may confuse code.
> 
> I know you copied the -1 value from ssl_client_socket_nss.cc.
> So this is fine for now.

[commit-bot: I haz the power, 2011-07-05 11:34:48]

Change committed as 91498

[joth, 2011-07-05 11:42:24]

On 2011/07/05 10:06:54, bulach wrote:
> joth pointed out that it'd be cleaner to have:
> 
> CHECK(transport_.get() && transport_->socked());
> return transport_->socket()->FooBar();
> 
> or even better, have some adapter / base class that we could re-use across all
> platform rather than duplicate this code..
> if you agree with either suggestions, I'm happy to follow up!
> 

Just to clarify these points
1/ using CHECK for pre-conditions is far more in line with the style guide than
coding up NOTREACHED impossible return paths.

2/ As a rough estimate, we have 10 unique socket types that just delegate many
of the API methods to an underlying transport socket. (see grep below). It seems
there should be someway to factor out a delegate socket base class that means
these methods can just be implemented once each time a new one is added.
(Or maybe, extract a seperate interface for the connection status query methods,
and then there's just one GetStreamStatusProvider() virtual that needs to
delegate to the underlying transport)

$ git gs ">GetConnectTimeMicros" 
jingle/notifier/base/fake_ssl_client_socket.cc:344:  return
transport_socket_->GetConnectTimeMicros();
jingle/notifier/base/proxy_resolving_client_socket.cc:354:    return
transport_->socket()->GetConnectTimeMicros();
net/http/http_basic_stream.cc:138:  double rtt =
connection_->socket()->GetConnectTimeMicros().ToInternalValue();
net/http/http_proxy_client_socket.cc:178:    return
transport_->socket()->GetConnectTimeMicros();
net/socket/socks5_client_socket.cc:155:    return
transport_->socket()->GetConnectTimeMicros();
net/socket/socks_client_socket.cc:182:    return
transport_->socket()->GetConnectTimeMicros();
net/socket/ssl_client_socket_mac.cc:671:    return
transport_->socket()->GetConnectTimeMicros();
net/socket/ssl_client_socket_nss.cc:740:    return
transport_->socket()->GetConnectTimeMicros();
net/socket/ssl_client_socket_win.cc:744:    return
transport_->socket()->GetConnectTimeMicros();
net/socket/ssl_server_socket_nss.cc:234:  return
transport_socket_->GetConnectTimeMicros();