Fix preconnect crash when we hit max socket limit.

Fix preconnect crash when we hit max socket limit.

When we hit the max socket limit, we close an idle socket in order to make space for the new preconnecting socket.  It's possible for the selected socket to belong to the same connection group as the one we're preconnecting a socket for.  This is obviously broken.  The bug currently results in us potentially deleting the ClientSocketPoolHelper::Group associated with that connection group, so the |Group* group| local variable in RequestSocketInternal() is now invalid.  Any access to that variable later on in the function results in badness.

This was safe before because we would never try to close an idle socket in the connection group we're request a socket for, because the first condition in RequestSocketInternal() checks to see if we can reuse an idle socket.  In the preconnect case, since we want to warm up the number of sockets in the connection group, we bypass idle sockets.

The solution is to create a new function: CloseOneIdleSocketExceptInGroup(const Group*).  This way we avoid this problem.  I provide a return value so that the caller can tell whether or not an idle socket was closed.  If it was not closed (it's possible that the connection group we're requesting a socket for is the only one with idle sockets), then the caller can tell, so it can pass up the failure so RequestSockets() knows that we've hit the max socket limit and there's no point in trying to preconnect more sockets.

BUG=64940
TEST=New unittest.

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

[willchan no longer on Chromium, 2010-12-02 01:58:07]

mbelshe: You're primary reviewer here.
vandebo: More of an FYI for you, so you can stay abreast of
ClientSocketPoolBaseHelper changes.

http://codereview.chromium.org/5549001/diff/1/net/socket/client_socket_pool_b...
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/5549001/diff/1/net/socket/client_socket_pool_b...
net/socket/client_socket_pool_base.cc:249: int rv =
RequestSocketInternal(group_name, &request);
Should I special case ERR_PRECONNECT_MAX_SOCKET_LIMIT here so I can add a new
NetLog EventParameter for TYPE_SOCKET_POOL_CONNECTING_N_SOCKETS so we can tell
when preconnect is not warming up any more sockets due to the max socket limit? 
Will anyone read this?

[Mike Belshe, 2010-12-02 02:46:48]

LGTM.  

Why is preconnect able to boot a socket for its own group?

http://codereview.chromium.org/5549001/diff/1/net/socket/client_socket_pool_b...
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/5549001/diff/1/net/socket/client_socket_pool_b...
net/socket/client_socket_pool_base.cc:249: int rv =
RequestSocketInternal(group_name, &request);
On 2010/12/02 01:58:07, willchan wrote:
> Should I special case ERR_PRECONNECT_MAX_SOCKET_LIMIT here so I can add a new
> NetLog EventParameter for TYPE_SOCKET_POOL_CONNECTING_N_SOCKETS so we can tell
> when preconnect is not warming up any more sockets due to the max socket
limit? 
> Will anyone read this?

It's up to you.  Maybe add a TODO

[willchan no longer on Chromium, 2010-12-02 02:58:23]

On 2010/12/02 02:46:48, Mike Belshe wrote:
> LGTM.  
> 
> Why is preconnect able to boot a socket for its own group?

It was previously able to.  The new CloseOneIdleSocketExceptInGroup() function
makes it possible not to do this.  I've used this in RequestSocketInternal() to
prevent booting from its own group.

> 
>
http://codereview.chromium.org/5549001/diff/1/net/socket/client_socket_pool_b...
> File net/socket/client_socket_pool_base.cc (right):
> 
>
http://codereview.chromium.org/5549001/diff/1/net/socket/client_socket_pool_b...
> net/socket/client_socket_pool_base.cc:249: int rv =
> RequestSocketInternal(group_name, &request);
> On 2010/12/02 01:58:07, willchan wrote:
> > Should I special case ERR_PRECONNECT_MAX_SOCKET_LIMIT here so I can add a
new
> > NetLog EventParameter for TYPE_SOCKET_POOL_CONNECTING_N_SOCKETS so we can
tell
> > when preconnect is not warming up any more sockets due to the max socket
> limit? 
> > Will anyone read this?
> 
> It's up to you.  Maybe add a TODO

OK.