Retry requests on reused sockets that receive ERR_EMPTY_RESPONSE.

Retry requests on reused sockets that receive ERR_EMPTY_RESPONSE.

We retry requests on ERR_CONNECTION_CLOSED in case of a close/reuse race, but
ERR_CONNECTION_CLOSED is converted to ERR_EMPTY_RESPONSE if this is a socket's
first request. Such a socket is normally not reused unless it was a preconnect
miss.

To avoid test flakiness, make the UNUSED vs UNUSED_IDLE determination not
timing-sensitive. The existing logic is compares idle_time to 0, so it's
dependent on clock granularity rather than any intentional timeout.

Add equivalent tests to HttpNetworkTransactionTest.KeepAliveConnection for
preconnect misses.

BUG=352156
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257748

[davidben, 2014-03-14 16:16:22]

(I believe the red trybot runs are from it testing off too old a revision. I've
sent out new ones.)

[mmenke, 2014-03-14 16:38:06]

LGTM

https://codereview.chromium.org/197283012/diff/1/net/http/http_network_transa...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/197283012/diff/1/net/http/http_network_transa...
net/http/http_network_transaction_unittest.cc:1309: void
HttpNetworkTransactionTest::PreconnectMissResendRequestTest(
Not sure if "PreconnectMiss" is quite the right name for these.

Maybe "PreconnectErrorResendRequestTest" (And similar changes for the test
names)

https://codereview.chromium.org/197283012/diff/1/net/http/http_network_transa...
net/http/http_network_transaction_unittest.cc:1340: ASSERT_TRUE(!read_failure);
Hrm...ASSERT_FALSE?  Guess this was just copied from the others, and should
stick with being consistent

https://codereview.chromium.org/197283012/diff/1/net/http/http_stream_factory...
File net/http/http_stream_factory_test_util.cc (right):

https://codereview.chromium.org/197283012/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_test_util.cc:22:
base::MessageLoop::current()->Run();
optional:  Maybe switch to using a RunLoop, while you're mucking with this?

https://codereview.chromium.org/197283012/diff/1/net/http/http_stream_factory...
File net/http/http_stream_factory_test_util.h (right):

https://codereview.chromium.org/197283012/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_test_util.h:15:
MockHttpStreamFactoryImplForPreconnect(HttpNetworkSession* session,
Should probably forward declare HttpNetworkSession.

https://codereview.chromium.org/197283012/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_test_util.h:22: virtual void
OnPreconnectsCompleteInternal() OVERRIDE;
include macros.h

https://codereview.chromium.org/197283012/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_test_util.h:25: bool waiting_for_preconnect_;
DISALLOW_COPY_AND_ASSIGN?

[davidben, 2014-03-14 18:35:02]

https://codereview.chromium.org/197283012/diff/1/net/http/http_network_transa...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/197283012/diff/1/net/http/http_network_transa...
net/http/http_network_transaction_unittest.cc:1309: void
HttpNetworkTransactionTest::PreconnectMissResendRequestTest(
On 2014/03/14 16:38:06, mmenke wrote:
> Not sure if "PreconnectMiss" is quite the right name for these.
> 
> Maybe "PreconnectErrorResendRequestTest" (And similar changes for the test
> names)

Done.

https://codereview.chromium.org/197283012/diff/1/net/http/http_network_transa...
net/http/http_network_transaction_unittest.cc:1340: ASSERT_TRUE(!read_failure);
On 2014/03/14 16:38:06, mmenke wrote:
> Hrm...ASSERT_FALSE?  Guess this was just copied from the others, and should
> stick with being consistent

Eh. Flipped both of them.

https://codereview.chromium.org/197283012/diff/1/net/http/http_stream_factory...
File net/http/http_stream_factory_test_util.cc (right):

https://codereview.chromium.org/197283012/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_test_util.cc:22:
base::MessageLoop::current()->Run();
On 2014/03/14 16:38:06, mmenke wrote:
> optional:  Maybe switch to using a RunLoop, while you're mucking with this?

Done.

https://codereview.chromium.org/197283012/diff/1/net/http/http_stream_factory...
File net/http/http_stream_factory_test_util.h (right):

https://codereview.chromium.org/197283012/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_test_util.h:15:
MockHttpStreamFactoryImplForPreconnect(HttpNetworkSession* session,
On 2014/03/14 16:38:06, mmenke wrote:
> Should probably forward declare HttpNetworkSession.

Done.

https://codereview.chromium.org/197283012/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_test_util.h:22: virtual void
OnPreconnectsCompleteInternal() OVERRIDE;
On 2014/03/14 16:38:06, mmenke wrote:
> include macros.h

Done.

https://codereview.chromium.org/197283012/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_test_util.h:25: bool waiting_for_preconnect_;
On 2014/03/14 16:38:06, mmenke wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

[davidben, 2014-03-14 22:00:40]

Okay, apparently OnPreconnectsCompleteInternal is a lie[0], so I swapped it out
for RunUntilIdle(), annoying as that is.

[0] https://codereview.chromium.org/169643006/#msg11

[mmenke, 2014-03-17 17:48:10]

Still LGTM

[davidben, 2014-03-17 19:58:18]

The win_swarm_triggered bot didn't like the test. Current theory is that it
thought the connection was UNUSED rather than UNUSED_IDLE. Change that logic to
be deterministic; we currently compare to base::TimeDelta() so it's dependent on
clock granularity.

It's possible we do want to treat, e.g., preconnect hits as UNUSED rather than
UNUSED_IDLE, but then we should use an explicit timeout. I've added a TODO and
filed http://crbug.com/353239 so we can get histograms as to whether this is
desirable.

(Note that preconnect hits that occur before the socket has finished connecting
will continue to be UNUSED. This is only if the preconnect hits after the socket
is connected but before $CLOCK_GRANULARITY has passed.)

[mmenke, 2014-03-18 14:20:48]

And still LGTM (I do think it's kinda odd that a REUSED_IDLE socket may never
have actually been idle, but that's not an issue with this CL...)

[mmenke, 2014-03-18 14:29:17]

Oops...forgot to publish my nit.

https://codereview.chromium.org/197283012/diff/60001/net/socket/client_socket...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/197283012/diff/60001/net/socket/client_socket...
net/socket/client_socket_pool_base.cc:484: ClientSocketHandle::UNUSED_IDLE;
optional:  May be easier to read if the last two lines are indented 4 more.

[davidben, 2014-03-18 15:08:12]

https://codereview.chromium.org/197283012/diff/60001/net/socket/client_socket...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/197283012/diff/60001/net/socket/client_socket...
net/socket/client_socket_pool_base.cc:484: ClientSocketHandle::UNUSED_IDLE;
On 2014/03/18 14:29:17, mmenke wrote:
> optional:  May be easier to read if the last two lines are indented 4 more.

Done.

[davidben, 2014-03-18 15:09:26]

On 2014/03/18 14:20:48, mmenke wrote:
> And still LGTM (I do think it's kinda odd that a REUSED_IDLE socket may never
> have actually been idle, but that's not an issue with this CL...)

The terminology is also kind of weird. UNUSED and UNUSED_IDLE describes the
state of the socket just before (re-)use, while REUSED_IDLE describes the state
just after (REUSE) and just before (IDLE).

[davidben, 2014-03-18 15:09:36]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-03-18 15:10:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidben@chromium.org/197283012/80001

[commit-bot: I haz the power, 2014-03-18 20:50:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-18 20:50:02]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[davidben, 2014-03-18 20:51:32]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-03-18 20:56:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidben@chromium.org/197283012/80001

[commit-bot: I haz the power, 2014-03-18 20:59:57]

Change committed as 257748