Close idle connections / SPDY sessions when needed

Attempting to re-land this feature with instrumentation to track down the use-after-free.

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=130129

[Ryan Hamilton, 2012-03-27 22:43:09]

eroman:  I've added some use-after-free instrumentation to my earlier CL.  I've
also added a possibly "fix" for the CHECK() failure during the pool destructor
by explicitly removing the spdy session from the pool when the session is closed
by an error. I'm curious if this has any effect.  Can you take a look?  The
first patch set is the initial CL, the second has the new changes, so you might
need to only look there.

[Ryan Hamilton, 2012-03-28 17:58:18]

FYI: I removed the logic in SpdySession after realizing that the behavior I
attributed to SpdySession can be explained by the use-after-free problem.  So
it's probably better to just track that down first.

[eroman, 2012-03-28 18:02:49]

https://chromiumcodereview.appspot.com/9861032/diff/2001/net/socket/client_so...
File net/socket/client_socket_pool.cc (right):

https://chromiumcodereview.appspot.com/9861032/diff/2001/net/socket/client_so...
net/socket/client_socket_pool.cc:29: magic_value_ = 0;
Two comments:
  (1) Define the magic values as an enum. This will simplify things for you/me
when looking at minidumps, since the debugger can resolve to the symbolic name.
For instance:

enum MagicValue {
  ALIVE = 0xCA11AB1E,
  DEAD = 0xDEADBEEF;
};

(2) You should absolutely not use 0 for your magic value on deletion. Zero tends
to be far too common of a value, which means there are more ways for this slot
to become zero than just destruction. Something with more bits of randomness
(like 0xDEADBEFF) would be preferable.

https://chromiumcodereview.appspot.com/9861032/diff/2001/net/socket/client_so...
net/socket/client_socket_pool.cc:39: crash(stack_trace_);
OK, so you will want to do some extra work here. Structure it like this:


if (magic_value_ != ALIVE) {
  MagicValue magic_value = magic_value_;
  base::debug::StackTrace deletion_callstack = stack_trace_;

  base::debug::Alias(&magic_value);
  base::debug::Alias(&deletion_callstack);

  CHECK(false);
}

We must explicitly copy the data we want onto the stack, otherwise there is no
guarantee that the minidump's memory will include it (since it is going to be in
the heap, somewhere off of the |this| pointer, which may not be recoverable).

Simply copying into local variables is insufficient since the compiler will
optimize it away. That is where the base::debug::Alias() comes it. It makes the
variable non-eliminatable by virtue of taking its address.

https://chromiumcodereview.appspot.com/9861032/diff/2001/net/socket/client_so...
File net/socket/client_socket_pool.h (right):

https://chromiumcodereview.appspot.com/9861032/diff/2001/net/socket/client_so...
net/socket/client_socket_pool.h:46: private:
indentation

https://chromiumcodereview.appspot.com/9861032/diff/2001/net/socket/client_so...
net/socket/client_socket_pool.h:48: base::debug::StackTrace stack_trace_;
Can you add corresponding TODOs to the code for later removal? (eventually we
will want to clean this out).

[Ryan Hamilton, 2012-03-28 18:15:44]

Thanks for the review and all the help with this!

https://chromiumcodereview.appspot.com/9861032/diff/2001/net/socket/client_so...
File net/socket/client_socket_pool.cc (right):

https://chromiumcodereview.appspot.com/9861032/diff/2001/net/socket/client_so...
net/socket/client_socket_pool.cc:29: magic_value_ = 0;
On 2012/03/28 18:02:50, eroman wrote:
> Two comments:
>   (1) Define the magic values as an enum. This will simplify things for you/me
> when looking at minidumps, since the debugger can resolve to the symbolic
name.
> For instance:
> 
> enum MagicValue {
>   ALIVE = 0xCA11AB1E,
>   DEAD = 0xDEADBEEF;
> };

Done!

> (2) You should absolutely not use 0 for your magic value on deletion. Zero
tends
> to be far too common of a value, which means there are more ways for this slot
> to become zero than just destruction. Something with more bits of randomness
> (like 0xDEADBEFF) would be preferable.

Good point!  I wondered about this but since I thought we were merely checking
alive vs. !alive 0 would be sufficient.  But i like your approach which can help
us differentiate between a freed object and reused memory.

https://chromiumcodereview.appspot.com/9861032/diff/2001/net/socket/client_so...
net/socket/client_socket_pool.cc:39: crash(stack_trace_);
On 2012/03/28 18:02:50, eroman wrote:
> OK, so you will want to do some extra work here. Structure it like this:
> 
> 
> if (magic_value_ != ALIVE) {
>   MagicValue magic_value = magic_value_;
>   base::debug::StackTrace deletion_callstack = stack_trace_;
> 
>   base::debug::Alias(&magic_value);
>   base::debug::Alias(&deletion_callstack);
> 
>   CHECK(false);
> }
> 
> We must explicitly copy the data we want onto the stack, otherwise there is no
> guarantee that the minidump's memory will include it (since it is going to be
in
> the heap, somewhere off of the |this| pointer, which may not be recoverable).
> 
> Simply copying into local variables is insufficient since the compiler will
> optimize it away. That is where the base::debug::Alias() comes it. It makes
the
> variable non-eliminatable by virtue of taking its address.

AH!  Awesome.  I kinda thought that adding the StackTrace as an argument to 
method might not be sufficient.  Thanks!

https://chromiumcodereview.appspot.com/9861032/diff/2001/net/socket/client_so...
File net/socket/client_socket_pool.h (right):

https://chromiumcodereview.appspot.com/9861032/diff/2001/net/socket/client_so...
net/socket/client_socket_pool.h:46: private:
On 2012/03/28 18:02:50, eroman wrote:
> indentation

Done.

https://chromiumcodereview.appspot.com/9861032/diff/2001/net/socket/client_so...
net/socket/client_socket_pool.h:48: base::debug::StackTrace stack_trace_;
On 2012/03/28 18:02:50, eroman wrote:
> Can you add corresponding TODOs to the code for later removal? (eventually we
> will want to clean this out).

Done.

[eroman, 2012-03-28 18:25:59]

I only reviewed the deltas since patchset 1 -- I am assuming everything prior to
that is the code already reviewed during your previous review cycle.

LGTM

http://codereview.chromium.org/9861032/diff/7026/net/socket/client_socket_poo...
File net/socket/client_socket_pool.cc (right):

http://codereview.chromium.org/9861032/diff/7026/net/socket/client_socket_poo...
net/socket/client_socket_pool.cc:26: magic_value_ = ALIVE;
nit: i suggest putting this in initializer list (i.e after the ":")

http://codereview.chromium.org/9861032/diff/7026/net/socket/client_socket_poo...
net/socket/client_socket_pool.cc:29: LayeredPool::~LayeredPool() {
I suggest calling CrashIfFreed() first.
Double-frees tend to be a surprisingly common bug.

http://codereview.chromium.org/9861032/diff/7026/net/socket/client_socket_pool.h
File net/socket/client_socket_pool.h (right):

http://codereview.chromium.org/9861032/diff/7026/net/socket/client_socket_poo...
net/socket/client_socket_pool.h:34: enum MagicValue {
nit: Please put this inside the private section of the LayeredPool class, to
avoid namespace pollution.

[Ryan Hamilton, 2012-03-28 18:35:28]

> I only reviewed the deltas since patchset 1 -- I am assuming everything prior
> to that is the code already reviewed during your previous review cycle.

Yup!

Thanks for the lgtm.  I'll hold off on landing this until the M19 branch point
is clear.  (Don't want to revert this from M19 branch *again*)

http://codereview.chromium.org/9861032/diff/7026/net/socket/client_socket_poo...
File net/socket/client_socket_pool.cc (right):

http://codereview.chromium.org/9861032/diff/7026/net/socket/client_socket_poo...
net/socket/client_socket_pool.cc:26: magic_value_ = ALIVE;
On 2012/03/28 18:25:59, eroman wrote:
> nit: i suggest putting this in initializer list (i.e after the ":")

Duh!  Good point.

http://codereview.chromium.org/9861032/diff/7026/net/socket/client_socket_poo...
net/socket/client_socket_pool.cc:29: LayeredPool::~LayeredPool() {
On 2012/03/28 18:25:59, eroman wrote:
> I suggest calling CrashIfFreed() first.
> Double-frees tend to be a surprisingly common bug.

Done.

http://codereview.chromium.org/9861032/diff/7026/net/socket/client_socket_pool.h
File net/socket/client_socket_pool.h (right):

http://codereview.chromium.org/9861032/diff/7026/net/socket/client_socket_poo...
net/socket/client_socket_pool.h:34: enum MagicValue {
On 2012/03/28 18:25:59, eroman wrote:
> nit: Please put this inside the private section of the LayeredPool class, to
> avoid namespace pollution.

Done.

[commit-bot: I haz the power, 2012-04-02 01:33:50]

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

[commit-bot: I haz the power, 2012-04-02 01:45:37]

Try job failure for 9861032-7051 (retry) on android for steps "Compile, build".
It's a second try, previously, steps "Compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-04-02 02:27:01]

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

[commit-bot: I haz the power, 2012-04-02 05:36:26]

Try job failure for 9861032-11002 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-02 13:38:22]

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

[commit-bot: I haz the power, 2012-04-02 15:22:45]

Change committed as 130129