Change SpdySession::GetSSLInfo to get the SSLInfo from the underlying socket

Change SpdySession::GetSSLInfo to get the SSLInfo from the underlying socket
even if the session is not "secure".  This required refactoring StreamSocket
to add WasNpnNegotiated() and GetSSLInfo() methods.

This allows for a change to SpdySession::GetSSLInfo to accurately return the correct SSLInfo in the case of SPDY Proxy sessions.

BUG=134690
TEST=\*DoNotUseSpdySessionIfCertDoesNotMatch\*



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

[Ryan Sleevi, 2012-07-11 22:50:42]

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/http/http_netwo...
File net/http/http_network_transaction_spdy2_unittest.cc (right):

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/http/http_netwo...
net/http/http_network_transaction_spdy2_unittest.cc:10079:
TEST_F(HttpNetworkTransactionSpdy2Test, DoNotUseSpdySessionIfCertDoesNotMatch) {
Can you add a comment describing exactly what this test is meant to be testing.

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/http/http_netwo...
net/http/http_network_transaction_spdy2_unittest.cc:10112: MockRead(ASYNC,
ERR_IO_PENDING, 3)
Could you add a comment about this? Normally I'd expect to see an EOF here, but
I suspect you're hanging an ERR_IO_PENDING so that the first connection doesn't
disappear. Same on 10137

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/http/http_netwo...
net/http/http_network_transaction_spdy2_unittest.cc:10182:
EXPECT_EQ(ERR_IO_PENDING,
ASSERT_EQ

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/http/http_netwo...
net/http/http_network_transaction_spdy2_unittest.cc:10186: EXPECT_EQ(OK,
callback1.WaitForResult());
ASSERT_TRUE(callback1.have_result());
EXPECT_EQ(OK, callback1.WaitForResult())

Same below

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/socket/socket_t...
File net/socket/socket_test_util.h (right):

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/socket/socket_t...
net/socket/socket_test_util.h:765: virtual bool WasNpnNegotiated() const
OVERRIDE;
This belongs on line 753, right?

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/socket/ssl_clie...
File net/socket/ssl_client_socket.h (right):

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/socket/ssl_clie...
net/socket/ssl_client_socket.h:79: virtual bool WasNpnNegotiated() const
OVERRIDE;
Move 78+79 to 72

Put the interface implementations before this classes implementation

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/socket/ssl_clie...
net/socket/ssl_client_socket.h:110: virtual bool was_npn_negotiated() const;
How is this different than WasNpnNegotiated?

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/socket/stream_s...
File net/socket/stream_socket.h (right):

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/socket/stream_s...
net/socket/stream_socket.h:92: virtual bool WasNpnNegotiated() const = 0;
How is this different than GetNegotiatedProtocol?

Based on the documentation, it WasNpnNegotiated == false is the same as
GetNegotiatedProtocol == kProtoUnkown, and vice versa.

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/spdy/spdy_proxy...
File net/spdy/spdy_proxy_client_socket.cc (right):

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/spdy/spdy_proxy...
net/spdy/spdy_proxy_client_socket.cc:183: &protocol_negotiated);
nit: alignment

[Ryan Hamilton, 2012-07-11 23:37:25]

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/http/http_netwo...
File net/http/http_network_transaction_spdy2_unittest.cc (right):

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/http/http_netwo...
net/http/http_network_transaction_spdy2_unittest.cc:10079:
TEST_F(HttpNetworkTransactionSpdy2Test, DoNotUseSpdySessionIfCertDoesNotMatch) {
On 2012/07/11 22:50:43, Ryan Sleevi wrote:
> Can you add a comment describing exactly what this test is meant to be
testing.

Done.  Good point.

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/http/http_netwo...
net/http/http_network_transaction_spdy2_unittest.cc:10112: MockRead(ASYNC,
ERR_IO_PENDING, 3)
On 2012/07/11 22:50:43, Ryan Sleevi wrote:
> Could you add a comment about this? Normally I'd expect to see an EOF here,
but
> I suspect you're hanging an ERR_IO_PENDING so that the first connection
doesn't
> disappear. Same on 10137

This is common in SPDY tests for the reason you stated.  (I started this test by
copying an existing test and modifying it)  However, since this test uses a 
DeterministicSocket, I think we can use the normal convention.  I've modified
the tests to use it.

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/http/http_netwo...
net/http/http_network_transaction_spdy2_unittest.cc:10182:
EXPECT_EQ(ERR_IO_PENDING,
On 2012/07/11 22:50:43, Ryan Sleevi wrote:
> ASSERT_EQ

Done.

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/http/http_netwo...
net/http/http_network_transaction_spdy2_unittest.cc:10186: EXPECT_EQ(OK,
callback1.WaitForResult());
On 2012/07/11 22:50:43, Ryan Sleevi wrote:
> ASSERT_TRUE(callback1.have_result());
> EXPECT_EQ(OK, callback1.WaitForResult())
> 
> Same below

Done.

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/socket/socket_t...
File net/socket/socket_test_util.h (right):

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/socket/socket_t...
net/socket/socket_test_util.h:765: virtual bool WasNpnNegotiated() const
OVERRIDE;
On 2012/07/11 22:50:43, Ryan Sleevi wrote:
> This belongs on line 753, right?

Done.

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/socket/ssl_clie...
File net/socket/ssl_client_socket.h (right):

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/socket/ssl_clie...
net/socket/ssl_client_socket.h:79: virtual bool WasNpnNegotiated() const
OVERRIDE;
On 2012/07/11 22:50:43, Ryan Sleevi wrote:
> Move 78+79 to 72
> 
> Put the interface implementations before this classes implementation

Done.

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/socket/ssl_clie...
net/socket/ssl_client_socket.h:110: virtual bool was_npn_negotiated() const;
On 2012/07/11 22:50:43, Ryan Sleevi wrote:
> How is this different than WasNpnNegotiated?

*whistles innocently*

Done. :>

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/socket/stream_s...
File net/socket/stream_socket.h (right):

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/socket/stream_s...
net/socket/stream_socket.h:92: virtual bool WasNpnNegotiated() const = 0;
On 2012/07/11 22:50:43, Ryan Sleevi wrote:
> How is this different than GetNegotiatedProtocol?
> 
> Based on the documentation, it WasNpnNegotiated == false is the same as
> GetNegotiatedProtocol == kProtoUnkown, and vice versa.

I believe there is a distinction, though perhaps it is not an important
distinction.  But I think that WasNpnNegotiated can return true even if both
sides did not agree on protocol, and hence kProtoUnknow will be returned from
GetNegotiatedProtocol.

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/spdy/spdy_proxy...
File net/spdy/spdy_proxy_client_socket.cc (right):

https://chromiumcodereview.appspot.com/10690122/diff/3001/net/spdy/spdy_proxy...
net/spdy/spdy_proxy_client_socket.cc:183: &protocol_negotiated);
On 2012/07/11 22:50:43, Ryan Sleevi wrote:
> nit: alignment

Done.

[Ryan Hamilton, 2012-07-13 17:49:42]

Attempted to elaborate on the test comment.

[Ryan Sleevi, 2012-07-13 18:32:03]

Close.

I just realized you haven't touched the other SSLClientSocket* classes, which
you'll need to do because you're changing the SSLClientSocket interface ("void
GetSSLInfo(SSLInfo* ssl_info)" to "bool GetSSLInfo(SSLInfo* ssl_info) const")

Other than that, the changes between patchsets 2 & 4 look good, and just need to
finish out the platform bits (which will cause a compile failure otherwise)

I'm also not sure if GetSSLInfo() can be const'd on the other platforms, but I
imagine you'll find out quickly once you implement and run try jobs.

https://chromiumcodereview.appspot.com/10690122/diff/3004/net/http/http_netwo...
File net/http/http_network_transaction_spdy2_unittest.cc (right):

https://chromiumcodereview.appspot.com/10690122/diff/3004/net/http/http_netwo...
net/http/http_network_transaction_spdy2_unittest.cc:10087:
ASSERT_NE(static_cast<X509Certificate*>(NULL), server_cert);
nit: I would move this block to line 10160, close to where you actually use it

https://chromiumcodereview.appspot.com/10690122/diff/3004/net/http/http_netwo...
net/http/http_network_transaction_spdy2_unittest.cc:10162: ssl1.cert =
server_cert;
nit: It would be good to mention here that |server_cert| does not have to be
valid for |proxy| (because you're able to lie in the SSLSocketDataProvider and
say cert verification was OK), but /does/ have to be valid for
"mail.google.com", because of the name matching SPDY does.

Just seems to be a possibly confusing edge.

https://chromiumcodereview.appspot.com/10690122/diff/3004/net/socket/ssl_clie...
File net/socket/ssl_client_socket.h (right):

https://chromiumcodereview.appspot.com/10690122/diff/3004/net/socket/ssl_clie...
net/socket/ssl_client_socket.h:76: virtual NextProto GetNegotiatedProtocol()
const OVERRIDE;
If this is part of the StreamSocket impl, then there should be no line break on
line 75

[Ryan Hamilton, 2012-07-16 19:24:08]

Good point about the other platforms.  I've done the needful.  As you
anticipated, I was unable to make GetSSLInfo const.  bummer :>  It seems to
build on win, mac, android, and linux

https://chromiumcodereview.appspot.com/10690122/diff/3004/net/http/http_netwo...
File net/http/http_network_transaction_spdy2_unittest.cc (right):

https://chromiumcodereview.appspot.com/10690122/diff/3004/net/http/http_netwo...
net/http/http_network_transaction_spdy2_unittest.cc:10087:
ASSERT_NE(static_cast<X509Certificate*>(NULL), server_cert);
On 2012/07/13 18:32:03, Ryan Sleevi wrote:
> nit: I would move this block to line 10160, close to where you actually use it

Done.

https://chromiumcodereview.appspot.com/10690122/diff/3004/net/http/http_netwo...
net/http/http_network_transaction_spdy2_unittest.cc:10162: ssl1.cert =
server_cert;
On 2012/07/13 18:32:03, Ryan Sleevi wrote:
> nit: It would be good to mention here that |server_cert| does not have to be
> valid for |proxy| (because you're able to lie in the SSLSocketDataProvider and
> say cert verification was OK), but /does/ have to be valid for
> "mail.google.com", because of the name matching SPDY does.
> 
> Just seems to be a possibly confusing edge.

Done.

https://chromiumcodereview.appspot.com/10690122/diff/3004/net/socket/ssl_clie...
File net/socket/ssl_client_socket.h (right):

https://chromiumcodereview.appspot.com/10690122/diff/3004/net/socket/ssl_clie...
net/socket/ssl_client_socket.h:76: virtual NextProto GetNegotiatedProtocol()
const OVERRIDE;
On 2012/07/13 18:32:03, Ryan Sleevi wrote:
> If this is part of the StreamSocket impl, then there should be no line break
on
> line 75

Done.

[Ryan Sleevi, 2012-07-17 01:22:43]

Just reviewed the SSLClientSocket* changes - I assume everything else was
un-consting.

LGTM.

[Ryan Hamilton, 2012-07-17 22:04:57]

ajwong: can you look at the changes to remoting and jingle
jam: can look at the single change to a content unit test
sleevi: thanks for the lgtm :>

[awong, 2012-07-17 22:11:04]

Punting remoting + jingle to sergeyu@ as I am no longer working on Chromoting.

[Sergey Ulanov, 2012-07-17 22:13:07]

remoting and jingle - LGTM

[jam, 2012-07-18 05:17:05]

sergeyu is an owner for the content file

[Ryan Hamilton, 2012-07-18 15:27:25]

On 2012/07/18 05:17:05, John Abd-El-Malek wrote:
> sergeyu is an owner for the content file

Whoops, I'm so sorry.  I completely forgot that content is not like net/ in that
is has multiple OWNERS files.

sergeyu: you've already lgtm'd this for jingle and remoting, could you also look
at the file in content.  sorry for the headache.

[Sergey Ulanov, 2012-07-18 18:15:46]

On 2012/07/18 15:27:25, Ryan Hamilton wrote:
> On 2012/07/18 05:17:05, John Abd-El-Malek wrote:
> > sergeyu is an owner for the content file
> 
> Whoops, I'm so sorry.  I completely forgot that content is not like net/ in
that
> is has multiple OWNERS files.
> 
> sergeyu: you've already lgtm'd this for jingle and remoting, could you also
look
> at the file in content.  sorry for the headache.

content/browser/renderer_host/p2p - LGTM

[commit-bot: I haz the power, 2012-07-18 22:14:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/10690122/15003

[commit-bot: I haz the power, 2012-07-18 22:34:04]

Failed to request the patch to try. Please note that binary filesare still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[commit-bot: I haz the power, 2012-07-19 15:23:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/10690122/15003

[commit-bot: I haz the power, 2012-07-19 15:47:33]

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

[commit-bot: I haz the power, 2012-07-19 16:08:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/10690122/31004

[commit-bot: I haz the power, 2012-07-19 17:55:19]

Change committed as 147479