Implement SSLClientSocketPool.

Implement SSLClientSocketPool.

To support SSLClientSocketPool, ClientSocketPoolBase and ClientSocketHandle require a notion of additional error state reported from the pool.  Overtime the error handling may get become more integrated, alleviating the need for some of the additional error state.
To support getting Http Proxy credentials from the user, the SSLClientSocketPool will release unauthenticated HttpProxyClientSocket's into the pool as idle.  However, it checks their authentication status when receiving one, completing the authentication once the user has provided the credentials.

BUG=30357
TEST=existing unit tests, ClientSocketPoolBaseTest.AdditionalErrorState*, SSLClientSocketPoolTest.*

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

[vandebo (ex-Chrome), 2010-06-28 09:27:36]

FYI

[Mike Belshe, 2010-06-29 22:29:46]

Wow - this is a lot of work!

Made it through one pass.  Here are some quick comments.  Now I need to read
again!

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

http://codereview.chromium.org/2870030/diff/1/6#newcode745
net/http/http_network_transaction.cc:745: connection_group =
StringPrintf("spdy/%s", connection_group.c_str());
Do we need spdy and ssl in isolated pools anymore?  I think we can eliminate
lines 744-6.

http://codereview.chromium.org/2870030/diff/1/6#newcode850
net/http/http_network_transaction.cc:850: // logic is used when determining how
to interpret an error.
These return codes are really complicated and subtle.  I don't have an answer on
how to manage them better.

http://codereview.chromium.org/2870030/diff/1/28
File net/socket/ssl_client_socket_pool.h (right):

http://codereview.chromium.org/2870030/diff/1/28#newcode3
net/socket/ssl_client_socket_pool.h:3: // found in the LICENSE file.
Any way we could build a unit test for this class?

http://codereview.chromium.org/2870030/diff/1/28#newcode32
net/socket/ssl_client_socket_pool.h:32: // that will be used (denoted by
|proxy|).
I don't understand this comment?

[vandebo (ex-Chrome), 2010-06-30 07:46:47]

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

http://codereview.chromium.org/2870030/diff/1/6#newcode745
net/http/http_network_transaction.cc:745: connection_group =
StringPrintf("spdy/%s", connection_group.c_str());
On 2010/06/29 22:29:46, Mike Belshe wrote:
> Do we need spdy and ssl in isolated pools anymore?  I think we can eliminate
> lines 744-6.

They'll be in the same socket pool, but with isolated connection groups...  As I
understand it, SPDY is an incompatible protocol. If we have an idle SSL socket
we shouldn't talk SPDY on it and vice-a-versa.   If that is incorrect, then, yes
we can remove this if-else

http://codereview.chromium.org/2870030/diff/1/6#newcode850
net/http/http_network_transaction.cc:850: // logic is used when determining how
to interpret an error.
On 2010/06/29 22:29:46, Mike Belshe wrote:
> These return codes are really complicated and subtle.  I don't have an answer
on
> how to manage them better.

Yes. I plan to (hopefully Wednesday) step through the old and new code together
to a) make sure it's right and b) see if there's anyway to make this function
less subtle.

http://codereview.chromium.org/2870030/diff/1/28
File net/socket/ssl_client_socket_pool.h (right):

http://codereview.chromium.org/2870030/diff/1/28#newcode3
net/socket/ssl_client_socket_pool.h:3: // found in the LICENSE file.
On 2010/06/29 22:29:46, Mike Belshe wrote:
> Any way we could build a unit test for this class?

I assume you mean for SSLConnectJob, the Pool itself is pretty boilerplate.  And
there's actually not a lot going on in the ConnectJob - transport connect, ssl
connect, check for a couple error codes.

The reason that it actually is complicated is because of the multiple of error
conditions that can get returned from below - though most of them are handled
above, in HttpNetworkTransaction.  The HttpNetworkTransaction unit tests test
that integration with the pool pretty well.

Of course if there's something specific, I'll happily look into a test for it.

http://codereview.chromium.org/2870030/diff/1/28#newcode32
net/socket/ssl_client_socket_pool.h:32: // that will be used (denoted by
|proxy|).
On 2010/06/29 22:29:46, Mike Belshe wrote:
> I don't understand this comment?

You only need to pass in one of |tcp_params|, |http_proxy_params|, or
|socks_params|, and which one should match up with |proxy|.  This is checked on
line 51.  I will try to make the comment better.

[willchan no longer on Chromium, 2010-07-01 23:05:29]

Wow, there's a LOT going on here.  Here are some initial comments.  I'm quite
worried that I won't be able to finish the whole review before I leave for
Africa.  I recommend looping in eroman to start taking a look, unless mbelshe
feels comfortable enough with the whole socket pool architecture too.  There is
a lot happening.

http://codereview.chromium.org/2870030/diff/1/4
File net/http/http_network_session.cc (right):

http://codereview.chromium.org/2870030/diff/1/4#newcode59
net/http/http_network_session.cc:59: tcp_for_ssl_socket_pool_(new
TCPClientSocketPool(
Why does this have to be a member?
I think the only reason I didn't re-use the main pool for the proxy pools is
because of wanting to make sure we could get more than 6 connections to a proxy
server.  This doesn't apply to the ssl_socket_pool_.  So I think
ssl_socket_pool_ should re-use the main tcp socket pool.

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

http://codereview.chromium.org/2870030/diff/1/6#oldcode378
net/http/http_network_transaction.cc:378: // Are we using SPDY or HTTP?
It's not clear to me what problem you fixed by moving this afterward.  I trust
that it's probably correct, but for my own personal clarification, can you
explain why this had to happen afterward?  And if it's required, is there a test
to enforce this?

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

http://codereview.chromium.org/2870030/diff/1/6#newcode408
net/http/http_network_transaction.cc:408: return 0;  // connection_->has been
reset.  Treat like EOF.
The comment reads weird.  Perhaps it'd be better if you did |*connection_|
instead.

http://codereview.chromium.org/2870030/diff/1/6#newcode745
net/http/http_network_transaction.cc:745: connection_group =
StringPrintf("spdy/%s", connection_group.c_str());
On 2010/06/30 07:46:47, vandebo wrote:
> On 2010/06/29 22:29:46, Mike Belshe wrote:
> > Do we need spdy and ssl in isolated pools anymore?  I think we can eliminate
> > lines 744-6.
> 
> They'll be in the same socket pool, but with isolated connection groups...  As
I
> understand it, SPDY is an incompatible protocol. If we have an idle SSL socket
> we shouldn't talk SPDY on it and vice-a-versa.   If that is incorrect, then,
yes
> we can remove this if-else

Currently, the SpdySession only releases the socket after it's been
disconnected, so there is never any danger of re-use.

[Mike Belshe, 2010-07-06 17:49:09]

still looking.  a few more thoughts.  will review more tonight.  sorry for being
slow.

http://codereview.chromium.org/2870030/diff/1/13
File net/socket/client_socket_handle.h (right):

http://codereview.chromium.org/2870030/diff/1/13#newcode98
net/socket/client_socket_handle.h:98: void set_tunnel_auth_response_info(
Would this be a good case for implementing a derived class?  e.g. a
SSLClientSocketHandle?

http://codereview.chromium.org/2870030/diff/1/13#newcode114
net/socket/client_socket_handle.h:114: bool is_ssl_error() { return
is_ssl_error_; }
DCHECK(socket_.get())?

http://codereview.chromium.org/2870030/diff/1/13#newcode150
net/socket/client_socket_handle.h:150: // reset the supplemental error state.
"supplemental" error state basically means SSL tunnel state, right?  (part of
why it seems like a subclass would be better to me)

http://codereview.chromium.org/2870030/diff/1/13#newcode165
net/socket/client_socket_handle.h:165: HttpResponseInfo 
tunnel_auth_response_info_;
nit: you have an extra space in there.

http://codereview.chromium.org/2870030/diff/1/27
File net/socket/ssl_client_socket_pool.cc (right):

http://codereview.chromium.org/2870030/diff/1/27#newcode134
net/socket/ssl_client_socket_pool.cc:134: next_state_ =
STATE_TCP_CONNECT_COMPLETE;
DCHECK(tcp_pool_.get());

http://codereview.chromium.org/2870030/diff/1/28
File net/socket/ssl_client_socket_pool.h (right):

http://codereview.chromium.org/2870030/diff/1/28#newcode3
net/socket/ssl_client_socket_pool.h:3: // found in the LICENSE file.
It's hard to tell what is covered and what is not covered in test cases.  Do we
test basic-auth over SSL?

I believe Wan-Teh doesn't yet have client-auth tests in the tree; so those will
need to be tested manually if so.

I was thinking of tests which just do the basics of pulling sockets from the
right pools.  So, create a SSLClientSocketPool, grab a socket from it, release
it back to the pool, then verify you can grab it back from the pool.

Do the same for the SSLClientSocketPoolWithProxy.

Then cover the case where you need to use the RETRY_CONNECTION error case. 


On 2010/06/30 07:46:47, vandebo wrote:
> On 2010/06/29 22:29:46, Mike Belshe wrote:
> > Any way we could build a unit test for this class?
> 
> I assume you mean for SSLConnectJob, the Pool itself is pretty boilerplate. 
And
> there's actually not a lot going on in the ConnectJob - transport connect, ssl
> connect, check for a couple error codes.
> 
> The reason that it actually is complicated is because of the multiple of error
> conditions that can get returned from below - though most of them are handled
> above, in HttpNetworkTransaction.  The HttpNetworkTransaction unit tests test
> that integration with the pool pretty well.
> 
> Of course if there's something specific, I'll happily look into a test for it.

http://codereview.chromium.org/2870030/diff/1/28#newcode184
net/socket/ssl_client_socket_pool.h:184: const
scoped_refptr<TCPClientSocketPool>& tcp_pool,
We pass in 3 different pools here; and sometimes these can be NULL (e.g. when
creating a pool for a proxy, the tcp_pool will be NULL).  Some more docs about
the bi-modal use of this class might be good.

[Mike Belshe, 2010-07-07 17:28:38]

Overall, I'm okay with this.  I'd like to see better docs and tests around the
partially-errored sockets coming out of the socket pool.  Other than that, the
change is so big, I think it needs to land so we can start working on it and
testing it as a team.

http://codereview.chromium.org/2870030/diff/1/13
File net/socket/client_socket_handle.h (right):

http://codereview.chromium.org/2870030/diff/1/13#newcode30
net/socket/client_socket_handle.h:30: //
BTW - at this point, the difference between a ClientSocketHandle and a
ClientSocket are lost on me - functionality is buried in both places somewhat
arbitrarily? I guess when you're using a pool, you use a Handle?  And when
you're not you'd use a ClientSocket?

http://codereview.chromium.org/2870030/diff/1/13#newcode57
net/socket/client_socket_handle.h:57: //
This comment needs to be updated to reflect the new partial-failure state that
can be returned.

[vandebo (ex-Chrome), 2010-07-08 00:12:17]

Rebase and address comments so far.

I still need to check that DoInitConnectComplete is functionally equivalent to
the previous set of functions and add tests for GetAdditionalErrorState and
basic SSLPool operations.  Addressing comments on the dependent changes and
sheriff duty last week delayed these tasks.

http://codereview.chromium.org/2870030/diff/1/4
File net/http/http_network_session.cc (right):

http://codereview.chromium.org/2870030/diff/1/4#newcode59
net/http/http_network_session.cc:59: tcp_for_ssl_socket_pool_(new
TCPClientSocketPool(
On 2010/07/01 23:05:29, willchan wrote:
> Why does this have to be a member?
> I think the only reason I didn't re-use the main pool for the proxy pools is
> because of wanting to make sure we could get more than 6 connections to a
proxy
> server.  This doesn't apply to the ssl_socket_pool_.  So I think
> ssl_socket_pool_ should re-use the main tcp socket pool.

IIRC, the other reason was to get a separate set of histograms.  We don't have a
particular use for that data right now, so I'm happy to use the main tcp socket
pool.

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

http://codereview.chromium.org/2870030/diff/1/6#oldcode378
net/http/http_network_transaction.cc:378: // Are we using SPDY or HTTP?
On 2010/07/01 23:05:29, willchan wrote:
> It's not clear to me what problem you fixed by moving this afterward.  I trust
> that it's probably correct, but for my own personal clarification, can you
> explain why this had to happen afterward?  And if it's required, is there a
test
> to enforce this?

With tunnel setup in a pool, connection_->is_initialized() is now false in the
proxy auth case.  So the test that verifies that we get
ERR_TUNNEL_CONNECTION_FAILED if we try to Read() after a 407 would get OK
instead.

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

http://codereview.chromium.org/2870030/diff/1/6#newcode408
net/http/http_network_transaction.cc:408: return 0;  // connection_->has been
reset.  Treat like EOF.
On 2010/07/01 23:05:29, willchan wrote:
> The comment reads weird.  Perhaps it'd be better if you did |*connection_|
> instead.

Done.

http://codereview.chromium.org/2870030/diff/1/6#newcode745
net/http/http_network_transaction.cc:745: connection_group =
StringPrintf("spdy/%s", connection_group.c_str());
On 2010/07/01 23:05:29, willchan wrote:
> On 2010/06/30 07:46:47, vandebo wrote:
> > On 2010/06/29 22:29:46, Mike Belshe wrote:
> > > Do we need spdy and ssl in isolated pools anymore?  I think we can
eliminate
> > > lines 744-6.
> > 
> > They'll be in the same socket pool, but with isolated connection groups... 
As
> I
> > understand it, SPDY is an incompatible protocol. If we have an idle SSL
socket
> > we shouldn't talk SPDY on it and vice-a-versa.   If that is incorrect, then,
> yes
> > we can remove this if-else
> 
> Currently, the SpdySession only releases the socket after it's been
> disconnected, so there is never any danger of re-use.

If that changed for some reason, would the server (spec, not necessarily the
current implementation) allow transitioning back and forth between http and
spdy?  If so, this should stay in as defensive programming. Removed assuming the
spec allows switching between http and spdy.

http://codereview.chromium.org/2870030/diff/1/13
File net/socket/client_socket_handle.h (right):

http://codereview.chromium.org/2870030/diff/1/13#newcode30
net/socket/client_socket_handle.h:30: //
On 2010/07/07 17:28:38, Mike Belshe wrote:
> BTW - at this point, the difference between a ClientSocketHandle and a
> ClientSocket are lost on me - functionality is buried in both places somewhat
> arbitrarily? I guess when you're using a pool, you use a Handle?  And when
> you're not you'd use a ClientSocket?

That is the distinction.

http://codereview.chromium.org/2870030/diff/1/13#newcode57
net/socket/client_socket_handle.h:57: //
On 2010/07/07 17:28:38, Mike Belshe wrote:
> This comment needs to be updated to reflect the new partial-failure state that
> can be returned.

Done.

http://codereview.chromium.org/2870030/diff/1/13#newcode98
net/socket/client_socket_handle.h:98: void set_tunnel_auth_response_info(
On 2010/07/06 17:49:09, Mike Belshe wrote:
> Would this be a good case for implementing a derived class?  e.g. a
> SSLClientSocketHandle?

It's unfortunate that this is the cleanest way to return extra state from the
pools, but I don't think a derived class would make things any cleaner.  It
would probably just complicate matters because then the ssl pool would need to
make sure the caller passed in the right kind of ClientSocketHandle, but the
pool doesn't get the handle directly...

http://codereview.chromium.org/2870030/diff/1/13#newcode114
net/socket/client_socket_handle.h:114: bool is_ssl_error() { return
is_ssl_error_; }
On 2010/07/06 17:49:09, Mike Belshe wrote:
> DCHECK(socket_.get())?

DCHECK_EQ(socket_.get(), NULL) Done.

http://codereview.chromium.org/2870030/diff/1/13#newcode150
net/socket/client_socket_handle.h:150: // reset the supplemental error state.
On 2010/07/06 17:49:09, Mike Belshe wrote:
> "supplemental" error state basically means SSL tunnel state, right?  (part of
> why it seems like a subclass would be better to me)

The SSL pool is the only producer of supplemental error state at this time, yes.

http://codereview.chromium.org/2870030/diff/1/13#newcode165
net/socket/client_socket_handle.h:165: HttpResponseInfo 
tunnel_auth_response_info_;
On 2010/07/06 17:49:09, Mike Belshe wrote:
> nit: you have an extra space in there.

Done.

http://codereview.chromium.org/2870030/diff/1/27
File net/socket/ssl_client_socket_pool.cc (right):

http://codereview.chromium.org/2870030/diff/1/27#newcode134
net/socket/ssl_client_socket_pool.cc:134: next_state_ =
STATE_TCP_CONNECT_COMPLETE;
On 2010/07/06 17:49:09, Mike Belshe wrote:
> DCHECK(tcp_pool_.get());

Done.

http://codereview.chromium.org/2870030/diff/1/28
File net/socket/ssl_client_socket_pool.h (right):

http://codereview.chromium.org/2870030/diff/1/28#newcode3
net/socket/ssl_client_socket_pool.h:3: // found in the LICENSE file.
On 2010/07/06 17:49:09, Mike Belshe wrote:
> It's hard to tell what is covered and what is not covered in test cases.  Do
we
> test basic-auth over SSL?

I'm not sure, but quite likely.  In some sense this is just a refactoring of
existing code, so the code coverage isn't substantially changing.
 
> I believe Wan-Teh doesn't yet have client-auth tests in the tree; so those
will
> need to be tested manually if so.
> 
> I was thinking of tests which just do the basics of pulling sockets from the
> right pools.  So, create a SSLClientSocketPool, grab a socket from it, release
> it back to the pool, then verify you can grab it back from the pool.
> 
> Do the same for the SSLClientSocketPoolWithProxy.
> 
> Then cover the case where you need to use the RETRY_CONNECTION error case. 

Seems reasonable.

> On 2010/06/30 07:46:47, vandebo wrote:
> > On 2010/06/29 22:29:46, Mike Belshe wrote:
> > > Any way we could build a unit test for this class?
> > 
> > I assume you mean for SSLConnectJob, the Pool itself is pretty boilerplate. 
> And
> > there's actually not a lot going on in the ConnectJob - transport connect,
ssl
> > connect, check for a couple error codes.
> > 
> > The reason that it actually is complicated is because of the multiple of
error
> > conditions that can get returned from below - though most of them are
handled
> > above, in HttpNetworkTransaction.  The HttpNetworkTransaction unit tests
test
> > that integration with the pool pretty well.
> > 
> > Of course if there's something specific, I'll happily look into a test for
it.
> 
>

http://codereview.chromium.org/2870030/diff/1/28#newcode184
net/socket/ssl_client_socket_pool.h:184: const
scoped_refptr<TCPClientSocketPool>& tcp_pool,
On 2010/07/06 17:49:09, Mike Belshe wrote:
> We pass in 3 different pools here; and sometimes these can be NULL (e.g. when
> creating a pool for a proxy, the tcp_pool will be NULL).  Some more docs about
> the bi-modal use of this class might be good.

The SSL pool itself is not specific to this bi-modal use, that's just how we
currently use the SSL pool.  I added a comment indicating that only the pools
that will be used need to be passed in.

[willchan no longer on Chromium, 2010-07-08 20:23:40]

I think I've gotten through any major issue I have now.  Hopefully followups
should have a shorter turnaround time now.

http://codereview.chromium.org/2870030/diff/1/4
File net/http/http_network_session.cc (right):

http://codereview.chromium.org/2870030/diff/1/4#newcode59
net/http/http_network_session.cc:59: tcp_for_ssl_socket_pool_(new
TCPClientSocketPool(
On 2010/07/08 00:12:17, vandebo wrote:
> On 2010/07/01 23:05:29, willchan wrote:
> > Why does this have to be a member?
> > I think the only reason I didn't re-use the main pool for the proxy pools is
> > because of wanting to make sure we could get more than 6 connections to a
> proxy
> > server.  This doesn't apply to the ssl_socket_pool_.  So I think
> > ssl_socket_pool_ should re-use the main tcp socket pool.
> 
> IIRC, the other reason was to get a separate set of histograms.  We don't have
a
> particular use for that data right now, so I'm happy to use the main tcp
socket
> pool.

Ok, then I'd rather do that so we can enforce the max socket limit better.

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

http://codereview.chromium.org/2870030/diff/1/6#newcode745
net/http/http_network_transaction.cc:745: connection_group =
StringPrintf("spdy/%s", connection_group.c_str());
On 2010/07/08 00:12:17, vandebo wrote:
> On 2010/07/01 23:05:29, willchan wrote:
> > On 2010/06/30 07:46:47, vandebo wrote:
> > > On 2010/06/29 22:29:46, Mike Belshe wrote:
> > > > Do we need spdy and ssl in isolated pools anymore?  I think we can
> eliminate
> > > > lines 744-6.
> > > 
> > > They'll be in the same socket pool, but with isolated connection groups...

> As
> > I
> > > understand it, SPDY is an incompatible protocol. If we have an idle SSL
> socket
> > > we shouldn't talk SPDY on it and vice-a-versa.   If that is incorrect,
then,
> > yes
> > > we can remove this if-else
> > 
> > Currently, the SpdySession only releases the socket after it's been
> > disconnected, so there is never any danger of re-use.
> 
> If that changed for some reason, would the server (spec, not necessarily the
> current implementation) allow transitioning back and forth between http and
> spdy?  If so, this should stay in as defensive programming. Removed assuming
the
> spec allows switching between http and spdy.

No, we are not allowed to switch between http and spdy.  spdy is session-based,
so it always stays alive until the client/server terminates the connection.  I'm
not quite sure if we can get away with the separate connection_group for spdy
connections, because it might be possible that we won't know if we are doing
standard https or spdy until after the SSL connect completes (we choose the
protocol via a TLS extension - Next Protocol Negotion [NPN]).  I think I'm fine
without being as defensive, because it's a bad bug if we ever release a
connected spdy session back to the socket pool.

http://codereview.chromium.org/2870030/diff/35001/6003
File chrome/browser/net/preconnect.cc (right):

http://codereview.chromium.org/2870030/diff/35001/6003#newcode39
chrome/browser/net/preconnect.cc:39: // TODO(jar): This does not handle proxies
currently.
At some point we're going to need to create a function / class to request a
socket and return a handle.  It should have all the logic for building the
connection groups, selecting the right socket pool, and constructing the
relevant SocketParams.  Perhaps we need a SocketPools class or something.

http://codereview.chromium.org/2870030/diff/35001/6003#newcode76
chrome/browser/net/preconnect.cc:76: scoped_refptr<net::SSLClientSocketPool>
pool = session->ssl_socket_pool();
minor nit, but this can be a const scoped_refptr<>& instead, which will reduce
the unnecessary AddRef()/Release() pair.

http://codereview.chromium.org/2870030/diff/35001/6008
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/2870030/diff/35001/6008#newcode818
net/http/http_network_transaction.cc:818: if (proxy_info_.is_http())
Nit: I feel like I never see this in chromium code.  Usually if the statement
spans multiple lines, we wrap it in braces.  No style mandate here, but since I
feel this is slightly out of our norm, I'd recommend braces.

http://codereview.chromium.org/2870030/diff/35001/6008#newcode1611
net/http/http_network_transaction.cc:1611: // FIXME
What's going on here?

http://codereview.chromium.org/2870030/diff/35001/6010
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/2870030/diff/35001/6010#newcode305
net/http/http_network_transaction_unittest.cc:305:
CaptureGroupNameSocketPool<EmulatedClientSocketPool>::CaptureGroupNameSocketPool
Formatting is off here.  The '(' needs to be on the same line as the function
name.  Perhaps the solution is to replace EmulatedClientSocketPool with a
shorter name.  BaseClientSocketPool?  I dunno.

http://codereview.chromium.org/2870030/diff/35001/6013
File net/http/http_proxy_client_socket_pool.h (right):

http://codereview.chromium.org/2870030/diff/35001/6013#newcode17
net/http/http_proxy_client_socket_pool.h:17: #include "net/http/http_auth.h"
out of order

http://codereview.chromium.org/2870030/diff/35001/6016
File net/socket/client_socket_factory.h (right):

http://codereview.chromium.org/2870030/diff/35001/6016#newcode10
net/socket/client_socket_factory.h:10: #include
"net/socket/client_socket_handle.h"
Remember to ditch this when you fix 37810.  Or move the deprecated function
definition to the .cc file.

http://codereview.chromium.org/2870030/diff/35001/6018
File net/socket/client_socket_handle.h (right):

http://codereview.chromium.org/2870030/diff/35001/6018#newcode108
net/socket/client_socket_handle.h:108: scoped_refptr<HttpResponseHeaders>
headers,
I'd make these const scoped_refptr<>& to prevent unnecessary AddRef()/Release()
calls.

http://codereview.chromium.org/2870030/diff/35001/6018#newcode123
net/socket/client_socket_handle.h:123: bool is_ssl_error() {
const

http://codereview.chromium.org/2870030/diff/35001/6020
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/2870030/diff/35001/6020#newcode655
net/socket/client_socket_pool_base.cc:655: RemoveConnectJob(job, &group);
I think this is a bug.  Let's say that we were at 5 sockets out of 6 in a group.
 Since we HandoutSocket() before we call RemoveConnectJob() here, we are double
counting a socket.  This gets us to 6 sockets out of 6.  If
r->callback()->Run(result) tries to request a socket, it will fail instead of
succeed.

Please write a test for this case.

http://codereview.chromium.org/2870030/diff/35001/6025
File net/socket/ssl_client_socket.h (right):

http://codereview.chromium.org/2870030/diff/35001/6025#newcode76
net/socket/ssl_client_socket.h:76: static bool IgnoreCertError(int error, int
load_flags) {
I'm not sure that I think this is the right place for this function.  I'm at a
loss to pick a better place though.  Maybe you should consult wtc.

http://codereview.chromium.org/2870030/diff/35001/6026
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/2870030/diff/35001/6026#newcode564
net/socket/ssl_client_socket_mac.cc:564: transport_->socket()->Disconnect();
Should we also reset the handle so the socket gets released to the pool?

http://codereview.chromium.org/2870030/diff/35001/6028
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/2870030/diff/35001/6028#newcode552
net/socket/ssl_client_socket_nss.cc:552: transport_->socket()->Disconnect();
Same comment about resetting the transport handle.

http://codereview.chromium.org/2870030/diff/35001/6032
File net/socket/ssl_client_socket_pool.cc (right):

http://codereview.chromium.org/2870030/diff/35001/6032#newcode48
net/socket/ssl_client_socket_pool.cc:48: NOTREACHED() << "Unkown proxy type";
LOG(DFATAL)
unknown

http://codereview.chromium.org/2870030/diff/35001/6032#newcode118
net/socket/ssl_client_socket_pool.cc:118: NOTREACHED() << "Unkown proxy type";
LOG(DFATAL)
unknown

http://codereview.chromium.org/2870030/diff/35001/6032#newcode166
net/socket/ssl_client_socket_pool.cc:166: NOTREACHED() << "bad state";
LOG(DFATAL)

http://codereview.chromium.org/2870030/diff/35001/6032#newcode215
net/socket/ssl_client_socket_pool.cc:215: return
transport_socket_handle_->Init(group_name(), http_proxy_params,
Parameters must align with the first one, should you should move the first param
here to the next line.

http://codereview.chromium.org/2870030/diff/35001/6033
File net/socket/ssl_client_socket_pool.h (right):

http://codereview.chromium.org/2870030/diff/35001/6033#newcode69
net/socket/ssl_client_socket_pool.h:69: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/2870030/diff/35001/6033#newcode80
net/socket/ssl_client_socket_pool.h:80: const
scoped_refptr<HttpProxyClientSocketPool>& http_proxy__pool,
extra '_' in param name

http://codereview.chromium.org/2870030/diff/35001/6033#newcode82
net/socket/ssl_client_socket_pool.h:82: ClientSocketFactory*
client_socket_factory_,
extra trailing '_'

http://codereview.chromium.org/2870030/diff/35001/6033#newcode83
net/socket/ssl_client_socket_pool.h:83: const scoped_refptr<HostResolver>
&host_resolver,
We always put '&' next to type in chromium code

http://codereview.chromium.org/2870030/diff/35001/6033#newcode131
net/socket/ssl_client_socket_pool.h:131: ClientSocketFactory*
client_socket_factory_;
ClientSocketFactory* const

http://codereview.chromium.org/2870030/diff/35001/6034
File net/socket/ssl_client_socket_win.cc (right):

http://codereview.chromium.org/2870030/diff/35001/6034#newcode527
net/socket/ssl_client_socket_win.cc:527: transport_->socket()->Disconnect();
Same comment elsewhere about resetting the handle.

[vandebo (ex-Chrome), 2010-07-10 02:17:32]

I compared the error/completion procedure of this final CL with the code from
before the HttpProxy changes and after a few minor corrections, found them to
have the same effective result. I also noted one place where we may want to
change the behavior, now that it is easier to do so.
I've added tests for the additional error state mechanism and for the total
SSLClientSocketPool.  Let me know if you see further cases that should be
tested, otherwise this change is ready for final review.

http://codereview.chromium.org/2870030/diff/1/4
File net/http/http_network_session.cc (right):

http://codereview.chromium.org/2870030/diff/1/4#newcode59
net/http/http_network_session.cc:59: tcp_for_ssl_socket_pool_(new
TCPClientSocketPool(
On 2010/07/08 20:23:42, willchan wrote:
> On 2010/07/08 00:12:17, vandebo wrote:
> > On 2010/07/01 23:05:29, willchan wrote:
> > > Why does this have to be a member?
> > > I think the only reason I didn't re-use the main pool for the proxy pools
is
> > > because of wanting to make sure we could get more than 6 connections to a
> > proxy
> > > server.  This doesn't apply to the ssl_socket_pool_.  So I think
> > > ssl_socket_pool_ should re-use the main tcp socket pool.
> > 
> > IIRC, the other reason was to get a separate set of histograms.  We don't
have
> a
> > particular use for that data right now, so I'm happy to use the main tcp
> socket
> > pool.
> 
> Ok, then I'd rather do that so we can enforce the max socket limit better.

Done.

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

http://codereview.chromium.org/2870030/diff/1/6#newcode745
net/http/http_network_transaction.cc:745: connection_group =
StringPrintf("spdy/%s", connection_group.c_str());
On 2010/07/08 20:23:42, willchan wrote:
> On 2010/06/29 22:29:46, Mike Belshe wrote:
> > Do we need spdy and ssl in isolated pools anymore?  I think we can
> > eliminate lines 744-6.
> 
> No, we are not allowed to switch between http and spdy.  spdy is
session-based,
> so it always stays alive until the client/server terminates the connection. 
I'm
> not quite sure if we can get away with the separate connection_group for spdy
> connections, because it might be possible that we won't know if we are doing
> standard https or spdy until after the SSL connect completes (we choose the
> protocol via a TLS extension - Next Protocol Negotion [NPN]).  I think I'm
fine
> without being as defensive, because it's a bad bug if we ever release a
> connected spdy session back to the socket pool.

Done.  Though, as I read the code, if we want a spdy connection, but fail to get
one, we disconnect the SSL socket before trying again for plain SSL.

http://codereview.chromium.org/2870030/diff/35001/6003
File chrome/browser/net/preconnect.cc (right):

http://codereview.chromium.org/2870030/diff/35001/6003#newcode39
chrome/browser/net/preconnect.cc:39: // TODO(jar): This does not handle proxies
currently.
On 2010/07/08 20:23:42, willchan wrote:
> At some point we're going to need to create a function / class to request a
> socket and return a handle.  It should have all the logic for building the
> connection groups, selecting the right socket pool, and constructing the
> relevant SocketParams.  Perhaps we need a SocketPools class or something.

Indeed.  With the SSLPool, we'll finally be at a state where we can do that
easily.  I was thinking a method in HttpNetworkSession.  There will likely be
some work to figure out what error handling to do inside the method vs. not.

http://codereview.chromium.org/2870030/diff/35001/6003#newcode76
chrome/browser/net/preconnect.cc:76: scoped_refptr<net::SSLClientSocketPool>
pool = session->ssl_socket_pool();
On 2010/07/08 20:23:42, willchan wrote:
> minor nit, but this can be a const scoped_refptr<>& instead, which will reduce
> the unnecessary AddRef()/Release() pair.

Done.

http://codereview.chromium.org/2870030/diff/35001/6008
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/2870030/diff/35001/6008#newcode818
net/http/http_network_transaction.cc:818: if (proxy_info_.is_http())
On 2010/07/08 20:23:42, willchan wrote:
> Nit: I feel like I never see this in chromium code.  Usually if the statement
> spans multiple lines, we wrap it in braces.  No style mandate here, but since
I
> feel this is slightly out of our norm, I'd recommend braces.

Done.

http://codereview.chromium.org/2870030/diff/35001/6008#newcode1611
net/http/http_network_transaction.cc:1611: // FIXME
On 2010/07/08 20:23:42, willchan wrote:
> What's going on here?

Done.  Was a note to self about a merge, thought I already removed it.

http://codereview.chromium.org/2870030/diff/35001/6010
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/2870030/diff/35001/6010#newcode305
net/http/http_network_transaction_unittest.cc:305:
CaptureGroupNameSocketPool<EmulatedClientSocketPool>::CaptureGroupNameSocketPool
On 2010/07/08 20:23:42, willchan wrote:
> Formatting is off here.  The '(' needs to be on the same line as the function
> name.  Perhaps the solution is to replace EmulatedClientSocketPool with a
> shorter name.  BaseClientSocketPool?  I dunno.

Done.

http://codereview.chromium.org/2870030/diff/35001/6013
File net/http/http_proxy_client_socket_pool.h (right):

http://codereview.chromium.org/2870030/diff/35001/6013#newcode17
net/http/http_proxy_client_socket_pool.h:17: #include "net/http/http_auth.h"
On 2010/07/08 20:23:42, willchan wrote:
> out of order

Done.

http://codereview.chromium.org/2870030/diff/35001/6016
File net/socket/client_socket_factory.h (right):

http://codereview.chromium.org/2870030/diff/35001/6016#newcode10
net/socket/client_socket_factory.h:10: #include
"net/socket/client_socket_handle.h"
On 2010/07/08 20:23:42, willchan wrote:
> Remember to ditch this when you fix 37810.  Or move the deprecated function
> definition to the .cc file.

Done.

http://codereview.chromium.org/2870030/diff/35001/6018
File net/socket/client_socket_handle.h (right):

http://codereview.chromium.org/2870030/diff/35001/6018#newcode108
net/socket/client_socket_handle.h:108: scoped_refptr<HttpResponseHeaders>
headers,
On 2010/07/08 20:23:42, willchan wrote:
> I'd make these const scoped_refptr<>& to prevent unnecessary
AddRef()/Release()
> calls.

Done.

http://codereview.chromium.org/2870030/diff/35001/6018#newcode123
net/socket/client_socket_handle.h:123: bool is_ssl_error() {
On 2010/07/08 20:23:42, willchan wrote:
> const

Done.

http://codereview.chromium.org/2870030/diff/35001/6020
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/2870030/diff/35001/6020#newcode655
net/socket/client_socket_pool_base.cc:655: RemoveConnectJob(job, &group);
On 2010/07/08 20:23:42, willchan wrote:
> I think this is a bug.  Let's say that we were at 5 sockets out of 6 in a
group.
>  Since we HandoutSocket() before we call RemoveConnectJob() here, we are
double
> counting a socket.  This gets us to 6 sockets out of 6.  If
> r->callback()->Run(result) tries to request a socket, it will fail instead of
> succeed.
> 
> Please write a test for this case.

New test shows it is a bug, good catch.  Done.

http://codereview.chromium.org/2870030/diff/35001/6026
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/2870030/diff/35001/6026#newcode564
net/socket/ssl_client_socket_mac.cc:564: transport_->socket()->Disconnect();
On 2010/07/08 20:23:42, willchan wrote:
> Should we also reset the handle so the socket gets released to the pool?

I'm not sure it's worth it to follow all transport->socket()->Disconnect()'s
with a handle->Reset().  In most (all?) cases, the high level socket will be
destroyed shortly there after, destroying transport, which does the Reset.

http://codereview.chromium.org/2870030/diff/35001/6032
File net/socket/ssl_client_socket_pool.cc (right):

http://codereview.chromium.org/2870030/diff/35001/6032#newcode48
net/socket/ssl_client_socket_pool.cc:48: NOTREACHED() << "Unkown proxy type";
On 2010/07/08 20:23:42, willchan wrote:
> LOG(DFATAL)
> unknown

Done.

http://codereview.chromium.org/2870030/diff/35001/6032#newcode118
net/socket/ssl_client_socket_pool.cc:118: NOTREACHED() << "Unkown proxy type";
On 2010/07/08 20:23:42, willchan wrote:
> LOG(DFATAL)
> unknown

Done.

http://codereview.chromium.org/2870030/diff/35001/6032#newcode166
net/socket/ssl_client_socket_pool.cc:166: NOTREACHED() << "bad state";
On 2010/07/08 20:23:42, willchan wrote:
> LOG(DFATAL)

Other state machines used NOTREACHED() here, so I'm going to leave this for
consistency.

http://codereview.chromium.org/2870030/diff/35001/6032#newcode215
net/socket/ssl_client_socket_pool.cc:215: return
transport_socket_handle_->Init(group_name(), http_proxy_params,
On 2010/07/08 20:23:42, willchan wrote:
> Parameters must align with the first one, should you should move the first
param
> here to the next line.

Done.

http://codereview.chromium.org/2870030/diff/35001/6033
File net/socket/ssl_client_socket_pool.h (right):

http://codereview.chromium.org/2870030/diff/35001/6033#newcode69
net/socket/ssl_client_socket_pool.h:69: };
On 2010/07/08 20:23:42, willchan wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/2870030/diff/35001/6033#newcode80
net/socket/ssl_client_socket_pool.h:80: const
scoped_refptr<HttpProxyClientSocketPool>& http_proxy__pool,
On 2010/07/08 20:23:42, willchan wrote:
> extra '_' in param name

Done.

http://codereview.chromium.org/2870030/diff/35001/6033#newcode82
net/socket/ssl_client_socket_pool.h:82: ClientSocketFactory*
client_socket_factory_,
On 2010/07/08 20:23:42, willchan wrote:
> extra trailing '_'

Done.

http://codereview.chromium.org/2870030/diff/35001/6033#newcode83
net/socket/ssl_client_socket_pool.h:83: const scoped_refptr<HostResolver>
&host_resolver,
On 2010/07/08 20:23:42, willchan wrote:
> We always put '&' next to type in chromium code

Done.

http://codereview.chromium.org/2870030/diff/35001/6033#newcode131
net/socket/ssl_client_socket_pool.h:131: ClientSocketFactory*
client_socket_factory_;
On 2010/07/08 20:23:42, willchan wrote:
> ClientSocketFactory* const

Done.

[Mike Belshe, 2010-07-10 04:59:48]

lgtm; looks like there might be one last mac failure to fix.

[willchan no longer on Chromium, 2010-07-13 00:02:17]

LGTM.  I didn't take a close look again at the end.  But structurally I'm fine
with this change.  We need to get this in and get some ToT testing on it before
next week's dev channel.  I'm fairly scared of this change.  Hopefully things
won't break.  Good luck!

[willchan no longer on Chromium, 2010-07-15 00:52:23]

http://codereview.chromium.org/2870030/diff/55001/45020
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/2870030/diff/55001/45020#newcode634
net/socket/client_socket_pool_base.cc:634: base::TimeDelta(), &group,
r->net_log());
Is this a bad merge?  Did this get copied twice?

[Nico, 2015-03-08 02:31:17]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-03-08 02:31:19]

https://codereview.chromium.org/2870030/diff/55001/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_pool.cc (right):

https://codereview.chromium.org/2870030/diff/55001/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_pool.cc:76: resolver_(host_resolver),
This constructor sets all fields except next_state_. Is that intended?

(collecting data for a potential compiler warning…)

[wrong vandebo, 2015-03-08 03:57:15]

https://codereview.chromium.org/2870030/diff/55001/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_pool.cc (right):

https://codereview.chromium.org/2870030/diff/55001/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_pool.cc:76: resolver_(host_resolver),
On 2015/03/08 02:31:18, Nico wrote:
> This constructor sets all fields except next_state_. Is that intended?
> 
> (collecting data for a potential compiler warning…)

Looks like it could have been set to STATE_NONE

[wtc, 2015-03-10 22:59:31]

On 2015/03/08 03:57:15, wrong vandebo wrote:
>
> Looks like it could have been set to STATE_NONE

I agree. We should initialize the next_state_ member to STATE_NONE.

I don't think we will use next_state_ uninitialized. In the current
code, next_state_ is initialized in SSLConnectJob::ConnectInternal().
But it is good for a constructor to initialize every data member,
and STATE_NONE is a reasonable dummy/empty value for next_state_.