SPDY: Fix Alternate-Protocol.

SPDY: Fix Alternate-Protocol.

(1) In DoInitConnection() we do the existing spdy session check.  If it exists there, then we assuem it exists in DoSpdySendRequest().  Unfortunately, we didn't do the same check.  Use a member variable to store the HostPortPair.
(2) In DoInitConnection(), we used the scheme://urlhost:urlport as the connection group.  With Alternate-Protocol, we used the scheme://urlhost:urlport even though we were connecting to a different port, with a different protocol (TLS).  This means we would mix conflicting sockets in the ClientSocketPool.  I fix this by dropping scheme://, since it's unnecessary, and would cause us not to share SSL sockets in different connection groups (since the specified scheme might be http://, but due to Alternate-Protocol, we actually do an SSL connect).  I also don't use the urlhost:urlport, but use the host:port that we actually connect to.

TODO(willchan):
Fix Alternate-Protocol so it works properly with proxies.  I need to change CONNECT for http proxies and patch the SOCKs connects.

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

[wtc, 2010-04-22 00:42:34]

http://codereview.chromium.org/1755005/diff/1/2
File net/base/host_port_pair.cc (right):

http://codereview.chromium.org/1755005/diff/1/2#newcode11
net/base/host_port_pair.cc:11: return StringPrintf("%s:%u", host.c_str(), port);
Please make sure if |host| is an IPv6 address literal, it
already contains the square brackets [].

http://codereview.chromium.org/1755005/diff/1/3
File net/http/http_network_transaction.cc (left):

http://codereview.chromium.org/1755005/diff/1/3#oldcode1253
net/http/http_network_transaction.cc:1253: HostPortPair
host_port_pair(request_->url.HostNoBrackets(),
This code shows that the |host| in a HostPortPair does not
include the square brackets [] for IPv6 address literals.
So the ToString method needs to add [] if necessary.

http://codereview.chromium.org/1755005/diff/1/3
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1755005/diff/1/3#newcode763
net/http/http_network_transaction.cc:763:
connection_group.append(peer_.ToString());
This change needs to be reviewed carefully.
It changes two things.
1. The URL scheme (http, https) is dropped from the connection
group name.
2. The host may not be the host in the URL any more.

[willchan no longer on Chromium, 2010-04-22 01:05:21]

http://codereview.chromium.org/1755005/diff/1/3
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1755005/diff/1/3#newcode763
net/http/http_network_transaction.cc:763:
connection_group.append(peer_.ToString());
On 2010/04/22 00:42:34, wtc wrote:
> This change needs to be reviewed carefully.
> It changes two things.
> 1. The URL scheme (http, https) is dropped from the connection
> group name.
> 2. The host may not be the host in the URL any more.

Please comment what you think about (1).  I know when I talked to jar, he
disliked having the scheme.  It doesn't seem to really add anything, and creates
more of a hassle for clients to construct the correct scheme.  It's not like
ports 80 and 443 can handle both http and https.  The host port pair usually
implies a protocol.

About (2): I think the host is still the same.  Alternate-Protocol does not let
you change the host, just the port.  Unless you are talking about proxies.  This
code is broken for socks and ssl over http tunnels.  I need to fix that
(although I think that the existing connection limits we have for those cases
seems broken).

[Mike Belshe, 2010-04-22 02:26:27]

lgtm

http://codereview.chromium.org/1755005/diff/1/3
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1755005/diff/1/3#newcode897
net/http/http_network_transaction.cc:897: CHECK(connection_->socket());
nit: I would make this a DCHECK.

http://codereview.chromium.org/1755005/diff/1/3#newcode1266
net/http/http_network_transaction.cc:1266: CHECK(connection_->socket());
nit: I would make this a DCHECK too.  I guess we have style differences around
CHECK/DCHECK.

[willchan no longer on Chromium, 2010-04-22 23:47:47]

I've updated the changelist.  I also noted in the changelist description that I
have to fix Alternate-Protocol to work with proxies.  I left a TODO for that
since I didn't want this changelist to grow too large.

[Mike Belshe, 2010-04-23 21:56:12]

lgtm

http://codereview.chromium.org/1755005/diff/11001/12001
File net/base/host_port_pair.cc (right):

http://codereview.chromium.org/1755005/diff/11001/12001#newcode15
net/base/host_port_pair.cc:15: if (host.find(':') != std::string::npos)
nit:  I'd put a comment that the ':' is checking for IPv6 addrs.

http://codereview.chromium.org/1755005/diff/11001/12010
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/1755005/diff/11001/12010#newcode197
net/spdy/spdy_network_transaction_unittest.cc:197: static const unsigned char
kGetSynCompressed[] = {
nit: any reason we shouldn't move all of these to spdy_test_util?  Seems like
they should all go together?

[wtc, 2010-04-23 22:53:48]

I only reviewed your changes to host_port_pair.* and
http_network_transaction.*.  They look good to me.  I
have some suggested changes below.

http://codereview.chromium.org/1755005/diff/11001/12002
File net/base/host_port_pair.h (right):

http://codereview.chromium.org/1755005/diff/11001/12002#newcode31
net/base/host_port_pair.h:31: std::string ToStringNoBrackets() const;
Please remove the ToStringNoBrackets() method.  We don't need
it.

http://codereview.chromium.org/1755005/diff/11001/12003
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1755005/diff/11001/12003#newcode714
net/http/http_network_transaction.cc:714: HostPortPair endpoint;
Nit: please document what "endpoint" means (ultimate destination).

http://codereview.chromium.org/1755005/diff/11001/12003#newcode762
net/http/http_network_transaction.cc:762: if (proxy_mode_ == kDirectConnection)
Please merge these four lines (762-765) into the
if-else statement at lines 718-740 above.

Alternatively, keep the original code's structure:
  if (proxy_mode_ != kHTTPProxy)
    connection_group.append(endpoint.ToString());
which is equivalent to your code because if
proxy_mode_ == kDirectConnection, then connection_group
is empty, and peer_ is equal to endpoint.

[willchan no longer on Chromium, 2010-04-23 23:30:58]

http://codereview.chromium.org/1755005/diff/11001/12002
File net/base/host_port_pair.h (right):

http://codereview.chromium.org/1755005/diff/11001/12002#newcode31
net/base/host_port_pair.h:31: std::string ToStringNoBrackets() const;
On 2010/04/23 22:53:48, wtc wrote:
> Please remove the ToStringNoBrackets() method.  We don't need
> it.

Done.

http://codereview.chromium.org/1755005/diff/11001/12003
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1755005/diff/11001/12003#newcode714
net/http/http_network_transaction.cc:714: HostPortPair endpoint;
On 2010/04/23 22:53:48, wtc wrote:
> Nit: please document what "endpoint" means (ultimate destination).

Done.

http://codereview.chromium.org/1755005/diff/11001/12003#newcode762
net/http/http_network_transaction.cc:762: if (proxy_mode_ == kDirectConnection)
On 2010/04/23 22:53:48, wtc wrote:
> Please merge these four lines (762-765) into the
> if-else statement at lines 718-740 above.
> 
> Alternatively, keep the original code's structure:
>   if (proxy_mode_ != kHTTPProxy)
>     connection_group.append(endpoint.ToString());
> which is equivalent to your code because if
> proxy_mode_ == kDirectConnection, then connection_group
> is empty, and peer_ is equal to endpoint.

I don't believe this is true, because of lines 743-748.

[willchan no longer on Chromium, 2010-04-26 20:55:05]

Updated, merged, and fixed Mike's comments too.  I'm going to land this today
unless anyone else wants to chime in too.

http://codereview.chromium.org/1755005/diff/11001/12001
File net/base/host_port_pair.cc (right):

http://codereview.chromium.org/1755005/diff/11001/12001#newcode15
net/base/host_port_pair.cc:15: if (host.find(':') != std::string::npos)
On 2010/04/23 21:56:12, Mike Belshe wrote:
> nit:  I'd put a comment that the ':' is checking for IPv6 addrs.  

Done.

http://codereview.chromium.org/1755005/diff/11001/12010
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/1755005/diff/11001/12010#newcode197
net/spdy/spdy_network_transaction_unittest.cc:197: static const unsigned char
kGetSynCompressed[] = {
On 2010/04/23 21:56:12, Mike Belshe wrote:
> nit: any reason we shouldn't move all of these to spdy_test_util?  Seems like
> they should all go together?

Done.

[wtc, 2010-04-27 00:00:10]

Will, thanks for your reply.

http://codereview.chromium.org/1755005/diff/11001/12003
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1755005/diff/11001/12003#newcode762
net/http/http_network_transaction.cc:762: if (proxy_mode_ == kDirectConnection)
On 2010/04/23 23:30:58, willchan wrote:
>
> I don't believe this is true, because of lines 743-748.

That's right.  But if a fixed http or https port
is specified, don't you think the connection group
should use the original port so that they don't all
end up in the same connection group?  Just wondering.

[willchan no longer on Chromium, 2010-04-27 00:21:24]

http://codereview.chromium.org/1755005/diff/11001/12003
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1755005/diff/11001/12003#newcode762
net/http/http_network_transaction.cc:762: if (proxy_mode_ == kDirectConnection)
On 2010/04/27 00:00:10, wtc wrote:
> On 2010/04/23 23:30:58, willchan wrote:
> >
> > I don't believe this is true, because of lines 743-748.
> 
> That's right.  But if a fixed http or https port
> is specified, don't you think the connection group
> should use the original port so that they don't all
> end up in the same connection group?  Just wondering.
> 
> 

Oh I see.  I think they'd all end up in the same connection group anyway, right?
 It's not like a host usually has multiple http{s} ports.  I wouldn't worry
about that too much.  I think whatever makes the code easiest to read is best. 
Your suggestion is more readable except for the testing fixed http/https port
case.  But we'd have to add comments here explaining why it's ok to ignore this
case.

I don't think either way is a net win.  Sorry, I didn't wait for your comment
earlier and already landed this.  If you disagree and think that your earlier
suggestion is still a net win, I'm happy to make the change and send it to you
for review.

[wtc, 2010-04-27 17:18:43]

http://codereview.chromium.org/1755005/diff/11001/12003
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1755005/diff/11001/12003#newcode762
net/http/http_network_transaction.cc:762: if (proxy_mode_ == kDirectConnection)
Will: you did the right thing to not wait for my comment.
This is a minor issue.  I just wanted to make sure I understood
the code.

I think the DoInitConnection function has grown too long.
I'm considering turning the middle section of the function
into a ComputeConnectionInfo function.  If you agree, I'll
create a changelist.

[willchan no longer on Chromium, 2010-04-27 17:27:08]

http://codereview.chromium.org/1755005/diff/11001/12003
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1755005/diff/11001/12003#newcode762
net/http/http_network_transaction.cc:762: if (proxy_mode_ == kDirectConnection)
On 2010/04/27 17:18:43, wtc wrote:
> Will: you did the right thing to not wait for my comment.
> This is a minor issue.  I just wanted to make sure I understood
> the code.
> 
> I think the DoInitConnection function has grown too long.
> I'm considering turning the middle section of the function
> into a ComputeConnectionInfo function.  If you agree, I'll
> create a changelist.

I agree with this.  I was actually going to wait for jar to do this, because he
needs to compute this for his TCP prewarming change.  It sounds to me that
perhaps it needs to be extracted to be a free / class static function.  I think
that we should perhaps put it in client_socket_pool.h or somewhere, since Jim's
code theoretically doesn't need to depend on http_network_transaction.h.

I was actually thinking that in the future, the connection groups could be
HostPortPairs instead of strings, but that doesn't work right now because of how
our proxy connection groups work.  I was thinking of fixing
http://code.google.com/p/chromium/issues/detail?id=12066 (fixing our proxy
connection limits), which might let me fix this too.  This is a moot point for
now, so splitting out the current code into a separate function is strictly a
good thing.