Attempting to re-land a small portion of this change... Simply add links from

Attempting to re-land a small portion of this change...  Simply add links from 
lower layer pools to higher layer pool.

Revert 10006036 - Revert 130129 - Revert 129034 - Revert 127893 - Revert 127730 - Revert 127717 - Revert 118788 - Revert 113405 - Revert 113305 - Revert 113300 - Revert 112134 - Revert 112130 - Close idle connections / SPDY sessions when needed

Due to the idle connection state being held by different socket pools, it's possible for one socket pool to hold an idle socket in a lower layer socket pool. From the lower level socket pool's perspective, the socket is being "actively" used. From the higher socket pool's (including SpdySession, which is more of a connection manager) perspective, the connection is idle and can be closed if we have hit a limit.

Normally this isn't a big deal, except when we have a lot of idle SPDY connections and are connecting via a proxy, so we have low connection limits through the proxy server. We address this problem by allowing lower-level socket pools to tell higher level socket pools to close a socket.

Fixed ASAN test failures by removing .Times(1) and .Times(2) from CloseMultipleIdleSocketsHeldByLayeredPoolWhenNeeded unittest (this removes the tests relying on the order of std::set in CloseOneIdleConnectionInLayeredPool). ASAN is prob
ably causing the memory allocator to allocate the pools differently. The std::set is ordered by LayeredPool* which is the address of the LayeredPool (willchan).

Added NET_EXPORT for layered_pool class defintion to fix windows shared compile.

BUG=62364, 92244, 109876, 110368, 119847
TEST=


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

[Ryan Hamilton, 2012-04-09 21:08:10]

eroman: ok, this CL is a small subset of the initial CL.  Can you take a look? 
I'd also be happy to show you the diffs if that would be helpful...

[eroman, 2012-04-09 21:14:23]

Could you have willchan or mmenke review this instead, since they did the
earlier review already :)

[Ryan Hamilton, 2012-04-09 21:23:59]

Will, can you take a look at this?  eroman is justifiably afraid :>

[willchan no longer on Chromium, 2012-04-10 10:06:42]

Adding Matt too. What's your strategy here? Are you going to try to break up the
change into chunks for landing to see what breaks it?

[Ryan Hamilton, 2012-04-10 15:40:55]

On 2012/04/10 10:06:42, willchan wrote:
> Adding Matt too. What's your strategy here? Are you going to try to break up
the
> change into chunks for landing to see what breaks it?

Yes, precisely.  I want to do it in three parts:

1) Basic Layered pool functionality - Define LayeredPool, and have all
ClientSocketPools implement it.  This means that we have circular pointers,
which, hopefully, we are handling correctly. (this CL)
2) SPDY support - Make SpdySession implement LayeredPool
3) CloseOneIdleConnection - Make ClientSocketPoolBaseHelper (I think) attempt to
actually close idle connections in layered socket pools.

This accomplishes two thing.  First, it breaks the code into smaller chunks. 
With luck, we'll see a problem in one of the first two CLs.  Secondly, if don't
see a problem in the first CL, then we can keep that code in.  This makes the
reverts easier to manage.

[willchan no longer on Chromium, 2012-04-10 15:46:18]

OK, LGTM

http://codereview.chromium.org/10026024/diff/5016/net/socket/client_socket_po...
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/10026024/diff/5016/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:935: it != group_map_.end(); it++) {
++it :)

[mmenke, 2012-04-10 15:52:21]

On 2012/04/10 15:40:55, Ryan Hamilton wrote:
> On 2012/04/10 10:06:42, willchan wrote:
> > Adding Matt too. What's your strategy here? Are you going to try to break up
> the
> > change into chunks for landing to see what breaks it?
> 
> Yes, precisely.  I want to do it in three parts:
> 
> 1) Basic Layered pool functionality - Define LayeredPool, and have all
> ClientSocketPools implement it.  This means that we have circular pointers,
> which, hopefully, we are handling correctly. (this CL)
> 2) SPDY support - Make SpdySession implement LayeredPool
> 3) CloseOneIdleConnection - Make ClientSocketPoolBaseHelper (I think) attempt
to
> actually close idle connections in layered socket pools.
> 
> This accomplishes two thing.  First, it breaks the code into smaller chunks. 
> With luck, we'll see a problem in one of the first two CLs.  Secondly, if
don't
> see a problem in the first CL, then we can keep that code in.  This makes the
> reverts easier to manage.

I'm still highly suspicions of SPDY.  I'd suggest either doing 3) before 2), or
adding SPDY support for CloseOneIdleConnection in a step 4)

[Ryan Hamilton, 2012-04-10 16:01:26]

http://codereview.chromium.org/10026024/diff/5016/net/socket/client_socket_po...
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/10026024/diff/5016/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:935: it != group_map_.end(); it++) {
On 2012/04/10 15:46:18, willchan wrote:
> ++it :)

Hah, done.

[Ryan Hamilton, 2012-04-10 16:08:19]

On 2012/04/10 15:52:21, Matt Menke wrote:
> On 2012/04/10 15:40:55, Ryan Hamilton wrote:
> > On 2012/04/10 10:06:42, willchan wrote:
> > > Adding Matt too. What's your strategy here? Are you going to try to break
up
> > the
> > > change into chunks for landing to see what breaks it?
> > 
> > Yes, precisely.  I want to do it in three parts:
> > 
> > 1) Basic Layered pool functionality - Define LayeredPool, and have all
> > ClientSocketPools implement it.  This means that we have circular pointers,
> > which, hopefully, we are handling correctly. (this CL)
> > 2) SPDY support - Make SpdySession implement LayeredPool
> > 3) CloseOneIdleConnection - Make ClientSocketPoolBaseHelper (I think)
attempt
> to
> > actually close idle connections in layered socket pools.
> > 
> > This accomplishes two thing.  First, it breaks the code into smaller chunks.

> > With luck, we'll see a problem in one of the first two CLs.  Secondly, if
> don't
> > see a problem in the first CL, then we can keep that code in.  This makes
the
> > reverts easier to manage.
> 
> I'm still highly suspicions of SPDY.  I'd suggest either doing 3) before 2),
or
> adding SPDY support for CloseOneIdleConnection in a step 4)

*nod*  I share your suspicion.  Do you have a preference for 3,2 vs 2,3,4?

[commit-bot: I haz the power, 2012-04-10 16:09:07]

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

[mmenke, 2012-04-10 16:11:10]

On 2012/04/10 16:08:19, Ryan Hamilton wrote:
> On 2012/04/10 15:52:21, Matt Menke wrote:
> > On 2012/04/10 15:40:55, Ryan Hamilton wrote:
> > > On 2012/04/10 10:06:42, willchan wrote:
> > > > Adding Matt too. What's your strategy here? Are you going to try to
break
> up
> > > the
> > > > change into chunks for landing to see what breaks it?
> > > 
> > > Yes, precisely.  I want to do it in three parts:
> > > 
> > > 1) Basic Layered pool functionality - Define LayeredPool, and have all
> > > ClientSocketPools implement it.  This means that we have circular
pointers,
> > > which, hopefully, we are handling correctly. (this CL)
> > > 2) SPDY support - Make SpdySession implement LayeredPool
> > > 3) CloseOneIdleConnection - Make ClientSocketPoolBaseHelper (I think)
> attempt
> > to
> > > actually close idle connections in layered socket pools.
> > > 
> > > This accomplishes two thing.  First, it breaks the code into smaller
chunks.
> 
> > > With luck, we'll see a problem in one of the first two CLs.  Secondly, if
> > don't
> > > see a problem in the first CL, then we can keep that code in.  This makes
> the
> > > reverts easier to manage.
> > 
> > I'm still highly suspicions of SPDY.  I'd suggest either doing 3) before 2),
> or
> > adding SPDY support for CloseOneIdleConnection in a step 4)
> 
> *nod*  I share your suspicion.  Do you have a preference for 3,2 vs 2,3,4?

I'd go with 2,3,4 (Or maybe 3,2,4, so we actually have most of the CL working on
trunk - SPDY is only a fairly small portion of sites), though I leave the
decision up to you.

[commit-bot: I haz the power, 2012-04-10 16:16:10]

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

[commit-bot: I haz the power, 2012-04-10 19:19:51]

Change committed as 131604