Fix SPDY error-handling if the connection gets closed just after use.

Fix SPDY error-handling if the connection gets closed just after use.

- Make SpdySession::IsReused() return true if the underlying socket was
  UNUSED_IDLE. This makes the HttpNetworkTransaction-level retry try a fresh
  socket in case a preconnected socket was stale.

- If the SpdySession closes in an event loop iteration between
  HttpStreamFactoryImplJob::DoCreateStream and OnNewSpdySessionReadyCallback,
  propogate the error to the request to prevent it from hanging. Do so by
  creating the originating request's SpdyHttpStream as soon as the SpdySession
  is created so it can sample SpdySession::IsReused() and advise
  HttpNetworkTransaction on error.

- Delay pumping the SpdySession read loop by an event loop iteration. This
  simplifies some logic and ensures that HttpNetworkTransaction receives a
  SpdyHttpStream to advise retry logic. This does mean we lose the error code;
  it now follows the asynchronous case and turns into ERR_CONNECTION_CLOSED in
  SpdyHttpStream::InitializeStream.

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

[davidben, 2014-03-18 23:02:33]

Split off from https://codereview.chromium.org/169643006/.

rch: For review. It contains the one-line change you reviewed in the parent CL,
tests for it, and stuff that came up while writing those tests.

mmenke: FYI since you're on the other CL.

----

Making sure the retry works properly in all these cases is actually kinda
screwy. It's possibly we want to go about this another way? This seems to work
though and it's reasonably non-invasive. HttpNetworkTransaction gets socket
reuse state from the HttpStream. This means we need to ensure that, if a
REUSED_IDLE or UNUSED_IDLE socket is used, the HttpStreamRequest succeeds.
Actually using the stream can than immediately fail. So that's what this CL
tries to ensure happens in the SPDY case.

(If this isn't a good way to resolve that, we'll still want part of this CL; if
SpdySession is gone by
HttpStreamFactoryImpl::Job::OnNewSpdySessionReadyCallback, the request hangs.)

I'll work on a CL later this week to add histograms for the retry code
(https://code.google.com/p/chromium/issues/detail?id=353239) because we don't
seem to have any visibility into this weirdo logic.

----

Relevant comments from other CL:

https://codereview.chromium.org/169643006/diff/330001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/169643006/diff/330001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1683: return buffered_spdy_framer_->frames_received() >
0 ||
On 2014/03/18 15:16:55, mmenke wrote:
> Hrm...  If we always received a settings frame first thing, would this always
be
> true?

SETTINGS frames aren't counted in this. It's oddly named and a somewhat odd
check, but I think it works? It counts:

SYN_STREAM
HEADERS
SYN_REPLY
data frames

https://codereview.chromium.org/169643006/diff/330001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1684: connection_->reuse_type() ==
ClientSocketHandle::UNUSED_IDLE;
On 2014/03/18 15:16:55, mmenke wrote:
> can we just use connection->is_reused()?  Yes, it will never be REUSED_IDLE,
> anyways, but still seems weird to just be checking for UNUSED_IDLE.

is_reused() is actually only REUSED_IDLE, not UNUSED_IDLE, which is sort of
bizarre. I haven't looked at all the callers to see if checking both would be
more correct or anything.

[Ryan Hamilton, 2014-03-21 00:39:12]

Sorry for the delay!

This change looks promising, but I'm not sure I understand why the current
implementation causes a hang.  If the SpdySession closes in an event loop
iteration between
HttpStreamFactoryImplJob::DoCreateStream and OnNewSpdySessionReadyCallback, what
happens which causes the hang?

[davidben, 2014-03-21 00:51:51]

On 2014/03/21 00:39:12, Ryan Hamilton wrote:
> Sorry for the delay!
> 
> This change looks promising, but I'm not sure I understand why the current
> implementation causes a hang.  If the SpdySession closes in an event loop
> iteration between
> HttpStreamFactoryImplJob::DoCreateStream and OnNewSpdySessionReadyCallback,
what
> happens which causes the hang?

Nothing happens. :-) That's why it hangs; the request that initiated the
SpdySession never finds out it fails and stays pending until some other
SpdySession happens to succeed and notify it through the
OnNewSpdySessionReadyCallback logic.

[Ryan Hamilton, 2014-03-21 15:49:02]

LGTM. After discussing this offline, I think this approach is good. I'm nervous
about changing the async behavior of SpdySession because we've been burned
before, so stay alert for bug/crash reports.  But LGTM!

[davidben, 2014-03-21 15:54:42]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-03-21 15:54:46]

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

[commit-bot: I haz the power, 2014-03-21 19:35:32]

Change committed as 258647

[vandebo (ex-Chrome), 2014-03-21 20:15:46]

A revert of this CL has been created in
https://codereview.chromium.org/208663003/ by vandebo@chromium.org.

The reason for reverting is: Linux LSan isn't happy: AddressSanitizer:
stack-buffer-overflow

http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%2BLSan%20Te....