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

Review URL: https://codereview.chromium.org/200723004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@258647 0039d316-1c4b-4281-b951-d872f2087c98

12 files changed, 213 insertions, 144 deletions

diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 7dda13e..ca06785 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -289,7 +289,8 @@ class HttpNetworkTransactionTest
   // failure should cause the network transaction to resend the request, and the
   // other argument should be NULL.
   void PreconnectErrorResendRequestTest(const MockWrite* write_failure,
-                                        const MockRead* read_failure);
+                                        const MockRead* read_failure,
+                                        bool use_spdy);
 
   SimpleGetHelperResult SimpleGetHelperForData(StaticSocketDataProvider* data[],
                                                size_t data_count) {
@@ -1308,46 +1309,81 @@ void HttpNetworkTransactionTest::KeepAliveConnectionResendRequestTest(
 
 void HttpNetworkTransactionTest::PreconnectErrorResendRequestTest(
     const MockWrite* write_failure,
-    const MockRead* read_failure) {
+    const MockRead* read_failure,
+    bool use_spdy) {
   HttpRequestInfo request;
   request.method = "GET";
-  request.url = GURL("http://www.foo.com/");
+  request.url = GURL("https://www.foo.com/");
   request.load_flags = 0;
 
   CapturingNetLog net_log;
   session_deps_.net_log = &net_log;
   scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps_));
 
-  // Written data for successfully sending a request.
-  MockWrite data1_writes[] = {
-    MockWrite("GET / HTTP/1.1\r\n"
-              "Host: www.foo.com\r\n"
-              "Connection: keep-alive\r\n\r\n"),
-  };
+  SSLSocketDataProvider ssl1(ASYNC, OK);
+  SSLSocketDataProvider ssl2(ASYNC, OK);
+  if (use_spdy) {
+    ssl1.SetNextProto(GetParam());
+    ssl2.SetNextProto(GetParam());
+  }
+  session_deps_.socket_factory->AddSSLSocketDataProvider(&ssl1);
+  session_deps_.socket_factory->AddSSLSocketDataProvider(&ssl2);
 
-  // Read results for the first request.
-  MockRead data1_reads[] = {
-    MockRead(ASYNC, OK),
-  };
+  // SPDY versions of the request and response.
+  scoped_ptr<SpdyFrame> spdy_request(spdy_util_.ConstructSpdyGet(
+      request.url.spec().c_str(), false, 1, DEFAULT_PRIORITY));
+  scoped_ptr<SpdyFrame> spdy_response(
+      spdy_util_.ConstructSpdyGetSynReply(NULL, 0, 1));
+  scoped_ptr<SpdyFrame> spdy_data(
+      spdy_util_.ConstructSpdyBodyFrame(1, "hello", 5, true));
 
+  // HTTP/1.1 versions of the request and response.
+  const char kHttpRequest[] = "GET / HTTP/1.1\r\n"
+      "Host: www.foo.com\r\n"
+      "Connection: keep-alive\r\n\r\n";
+  const char kHttpResponse[] = "HTTP/1.1 200 OK\r\nContent-Length: 5\r\n\r\n";
+  const char kHttpData[] = "hello";
+
+  std::vector<MockRead> data1_reads;
+  std::vector<MockWrite> data1_writes;
   if (write_failure) {
     ASSERT_FALSE(read_failure);
-    data1_writes[0] = *write_failure;
+    data1_writes.push_back(*write_failure);
+    data1_reads.push_back(MockRead(ASYNC, OK));
   } else {
     ASSERT_TRUE(read_failure);
-    data1_reads[0] = *read_failure;
+    if (use_spdy) {
+      data1_writes.push_back(CreateMockWrite(*spdy_request));
+    } else {
+      data1_writes.push_back(MockWrite(kHttpRequest));
+    }
+    data1_reads.push_back(*read_failure);
   }
 
-  StaticSocketDataProvider data1(data1_reads, arraysize(data1_reads),
-                                 data1_writes, arraysize(data1_writes));
+  StaticSocketDataProvider data1(&data1_reads[0], data1_reads.size(),
+                                 &data1_writes[0], data1_writes.size());
   session_deps_.socket_factory->AddSocketDataProvider(&data1);
 
-  MockRead data2_reads[] = {
-    MockRead("HTTP/1.1 200 OK\r\nContent-Length: 5\r\n\r\n"),
-    MockRead("hello"),
-    MockRead(ASYNC, OK),
-  };
-  StaticSocketDataProvider data2(data2_reads, arraysize(data2_reads), NULL, 0);
+  std::vector<MockRead> data2_reads;
+  std::vector<MockWrite> data2_writes;
+
+  if (use_spdy) {
+    data2_writes.push_back(CreateMockWrite(*spdy_request, 0, ASYNC));
+
+    data2_reads.push_back(CreateMockRead(*spdy_response, 1, ASYNC));
+    data2_reads.push_back(CreateMockRead(*spdy_data, 2, ASYNC));
+    data2_reads.push_back(MockRead(ASYNC, OK, 3));
+  } else {
+    data2_writes.push_back(
+        MockWrite(ASYNC, kHttpRequest, strlen(kHttpRequest), 0));
+
+    data2_reads.push_back(
+        MockRead(ASYNC, kHttpResponse, strlen(kHttpResponse), 1));
+    data2_reads.push_back(MockRead(ASYNC, kHttpData, strlen(kHttpData), 2));
+    data2_reads.push_back(MockRead(ASYNC, OK, 3));
+  }
+  OrderedSocketData data2(&data2_reads[0], data2_reads.size(),
+                          &data2_writes[0], data2_writes.size());
   session_deps_.socket_factory->AddSocketDataProvider(&data2);
 
   // Preconnect a socket.
@@ -1360,7 +1396,7 @@ void HttpNetworkTransactionTest::PreconnectErrorResendRequestTest(
   // Wait for the preconnect to complete.
   // TODO(davidben): Some way to wait for an idle socket count might be handy.
   base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(1, GetIdleSocketCountInTransportSocketPool(session.get()));
+  EXPECT_EQ(1, GetIdleSocketCountInSSLSocketPool(session.get()));
 
   // Make the request.
   TestCompletionCallback callback;
@@ -1376,7 +1412,9 @@ void HttpNetworkTransactionTest::PreconnectErrorResendRequestTest(
 
   LoadTimingInfo load_timing_info;
   EXPECT_TRUE(trans->GetLoadTimingInfo(&load_timing_info));
-  TestLoadTimingNotReused(load_timing_info, CONNECT_TIMING_HAS_DNS_TIMES);
+  TestLoadTimingNotReused(
+      load_timing_info,
+      CONNECT_TIMING_HAS_DNS_TIMES|CONNECT_TIMING_HAS_SSL_TIMES);
 
   const HttpResponseInfo* response = trans->GetResponseInfo();
   ASSERT_TRUE(response != NULL);
@@ -1387,7 +1425,7 @@ void HttpNetworkTransactionTest::PreconnectErrorResendRequestTest(
   std::string response_data;
   rv = ReadTransaction(trans.get(), &response_data);
   EXPECT_EQ(OK, rv);
-  EXPECT_EQ("hello", response_data);
+  EXPECT_EQ(kHttpData, response_data);
 }
 
 TEST_P(HttpNetworkTransactionTest,
@@ -1409,17 +1447,43 @@ TEST_P(HttpNetworkTransactionTest, KeepAliveConnectionEOF) {
 TEST_P(HttpNetworkTransactionTest,
        PreconnectErrorNotConnectedOnWrite) {
   MockWrite write_failure(ASYNC, ERR_SOCKET_NOT_CONNECTED);
-  PreconnectErrorResendRequestTest(&write_failure, NULL);
+  PreconnectErrorResendRequestTest(&write_failure, NULL, false);
 }
 
 TEST_P(HttpNetworkTransactionTest, PreconnectErrorReset) {
   MockRead read_failure(ASYNC, ERR_CONNECTION_RESET);
-  PreconnectErrorResendRequestTest(NULL, &read_failure);
+  PreconnectErrorResendRequestTest(NULL, &read_failure, false);
 }
 
 TEST_P(HttpNetworkTransactionTest, PreconnectErrorEOF) {
   MockRead read_failure(SYNCHRONOUS, OK);  // EOF
-  PreconnectErrorResendRequestTest(NULL, &read_failure);
+  PreconnectErrorResendRequestTest(NULL, &read_failure, false);
+}
+
+TEST_P(HttpNetworkTransactionTest, PreconnectErrorAsyncEOF) {
+  MockRead read_failure(ASYNC, OK);  // EOF
+  PreconnectErrorResendRequestTest(NULL, &read_failure, false);
+}
+
+TEST_P(HttpNetworkTransactionTest,
+       SpdyPreconnectErrorNotConnectedOnWrite) {
+  MockWrite write_failure(ASYNC, ERR_SOCKET_NOT_CONNECTED);
+  PreconnectErrorResendRequestTest(&write_failure, NULL, true);
+}
+
+TEST_P(HttpNetworkTransactionTest, SpdyPreconnectErrorReset) {
+  MockRead read_failure(ASYNC, ERR_CONNECTION_RESET);
+  PreconnectErrorResendRequestTest(NULL, &read_failure, true);
+}
+
+TEST_P(HttpNetworkTransactionTest, SpdyPreconnectErrorEOF) {
+  MockRead read_failure(SYNCHRONOUS, OK);  // EOF
+  PreconnectErrorResendRequestTest(NULL, &read_failure, true);
+}
+
+TEST_P(HttpNetworkTransactionTest, SpdyPreconnectErrorAsyncEOF) {
+  MockRead read_failure(ASYNC, OK);  // EOF
+  PreconnectErrorResendRequestTest(NULL, &read_failure, true);
 }
 
 TEST_P(HttpNetworkTransactionTest, NonKeepAliveConnectionReset) {
diff --git a/net/http/http_proxy_client_socket_pool.cc b/net/http/http_proxy_client_socket_pool.cc
index 8d691b7..e2529f8 100644
--- a/net/http/http_proxy_client_socket_pool.cc
+++ b/net/http/http_proxy_client_socket_pool.cc
@@ -315,11 +315,11 @@ int HttpProxyConnectJob::DoSpdyProxyCreateStream() {
     }
   } else {
     // Create a session direct to the proxy itself
-    int rv = spdy_pool->CreateAvailableSessionFromSocket(
-        key, transport_socket_handle_.Pass(),
-        net_log(), OK, &spdy_session, /*using_ssl_*/ true);
-    if (rv < 0)
-      return rv;
+    spdy_session =
+        spdy_pool->CreateAvailableSessionFromSocket(
+            key, transport_socket_handle_.Pass(),
+            net_log(), OK, /*using_ssl_*/ true);
+    DCHECK(spdy_session);
   }
 
   next_state_ = STATE_SPDY_PROXY_CREATE_STREAM_COMPLETE;
diff --git a/net/http/http_stream_factory_impl_job.cc b/net/http/http_stream_factory_impl_job.cc
index 960bc58..d595f2a 100644
--- a/net/http/http_stream_factory_impl_job.cc
+++ b/net/http/http_stream_factory_impl_job.cc
@@ -334,20 +334,23 @@ void HttpStreamFactoryImpl::Job::OnWebSocketHandshakeStreamReadyCallback() {
 }
 
 void HttpStreamFactoryImpl::Job::OnNewSpdySessionReadyCallback() {
-  DCHECK(!stream_.get());
+  DCHECK(stream_.get());
   DCHECK(!IsPreconnecting());
   DCHECK(using_spdy());
-  if (!new_spdy_session_)
-    return;
+  // Note: an event loop iteration has passed, so |new_spdy_session_| may be
+  // NULL at this point if the SpdySession closed immediately after creation.
   base::WeakPtr<SpdySession> spdy_session = new_spdy_session_;
   new_spdy_session_.reset();
   if (IsOrphaned()) {
-    stream_factory_->OnNewSpdySessionReady(
-        spdy_session, spdy_session_direct_, server_ssl_config_, proxy_info_,
-        was_npn_negotiated(), protocol_negotiated(), using_spdy(), net_log_);
+    if (spdy_session) {
+      stream_factory_->OnNewSpdySessionReady(
+          spdy_session, spdy_session_direct_, server_ssl_config_, proxy_info_,
+          was_npn_negotiated(), protocol_negotiated(), using_spdy(), net_log_);
+    }
     stream_factory_->OnOrphanedJobComplete(this);
   } else {
-    request_->OnNewSpdySessionReady(this, spdy_session, spdy_session_direct_);
+    request_->OnNewSpdySessionReady(
+        this, stream_.Pass(), spdy_session, spdy_session_direct_);
   }
   // |this| may be deleted after this call.
 }
@@ -1104,21 +1107,27 @@ int HttpStreamFactoryImpl::Job::DoCreateStream() {
     SpdySessionPool* spdy_pool = session_->spdy_session_pool();
     spdy_session = spdy_pool->FindAvailableSession(spdy_session_key, net_log_);
     if (!spdy_session) {
-      int error =
+      new_spdy_session_ =
           spdy_pool->CreateAvailableSessionFromSocket(spdy_session_key,
                                                       connection_.Pass(),
                                                       net_log_,
                                                       spdy_certificate_error_,
-                                                      &new_spdy_session_,
                                                       using_ssl_);
-      if (error != OK)
-        return error;
       const HostPortPair& host_port_pair = spdy_session_key.host_port_pair();
       base::WeakPtr<HttpServerProperties> http_server_properties =
           session_->http_server_properties();
       if (http_server_properties)
         http_server_properties->SetSupportsSpdy(host_port_pair, true);
       spdy_session_direct_ = direct;
+
+      // Create a SpdyHttpStream attached to the session;
+      // OnNewSpdySessionReadyCallback is not called until an event loop
+      // iteration later, so if the SpdySession is closed between then, allow
+      // reuse state from the underlying socket, sampled by SpdyHttpStream,
+      // bubble up to the request.
+      bool use_relative_url = direct || request_info_.url.SchemeIs("https");
+      stream_.reset(new SpdyHttpStream(new_spdy_session_, use_relative_url));
+
       return OK;
     }
   }
diff --git a/net/http/http_stream_factory_impl_request.cc b/net/http/http_stream_factory_impl_request.cc
index ff66f14..5f129ac 100644
--- a/net/http/http_stream_factory_impl_request.cc
+++ b/net/http/http_stream_factory_impl_request.cc
@@ -291,11 +291,16 @@ HttpStreamFactoryImpl::Request::RemoveRequestFromHttpPipeliningRequestMap() {
 
 void HttpStreamFactoryImpl::Request::OnNewSpdySessionReady(
     Job* job,
+    scoped_ptr<HttpStream> stream,
     const base::WeakPtr<SpdySession>& spdy_session,
     bool direct) {
   DCHECK(job);
   DCHECK(job->using_spdy());
 
+  // Note: |spdy_session| may be NULL. In that case, |delegate_| should still
+  // receive |stream| so the error propogates up correctly, however there is no
+  // point in broadcasting |spdy_session| to other requests.
+
   // The first case is the usual case.
   if (!bound_job_.get()) {
     OrphanJobsExcept(job);
@@ -322,21 +327,20 @@ void HttpStreamFactoryImpl::Request::OnNewSpdySessionReady(
     // implemented.
     NOTREACHED();
   } else {
-    bool use_relative_url = direct || url().SchemeIs("https");
-    delegate_->OnStreamReady(
-        job->server_ssl_config(),
-        job->proxy_info(),
-        new SpdyHttpStream(spdy_session, use_relative_url));
+    delegate_->OnStreamReady(job->server_ssl_config(), job->proxy_info(),
+                             stream.release());
   }
   // |this| may be deleted after this point.
-  factory->OnNewSpdySessionReady(spdy_session,
-                                 direct,
-                                 used_ssl_config,
-                                 used_proxy_info,
-                                 was_npn_negotiated,
-                                 protocol_negotiated,
-                                 using_spdy,
-                                 net_log);
+  if (spdy_session) {
+    factory->OnNewSpdySessionReady(spdy_session,
+                                   direct,
+                                   used_ssl_config,
+                                   used_proxy_info,
+                                   was_npn_negotiated,
+                                   protocol_negotiated,
+                                   using_spdy,
+                                   net_log);
+  }
 }
 
 void HttpStreamFactoryImpl::Request::OrphanJobsExcept(Job* job) {
diff --git a/net/http/http_stream_factory_impl_request.h b/net/http/http_stream_factory_impl_request.h
index 3d3b2c8..22c8ff2 100644
--- a/net/http/http_stream_factory_impl_request.h
+++ b/net/http/http_stream_factory_impl_request.h
@@ -16,6 +16,7 @@
 namespace net {
 
 class ClientSocketHandle;
+class HttpStream;
 class SpdySession;
 
 class HttpStreamFactoryImpl::Request : public HttpStreamRequest {
@@ -64,6 +65,7 @@ class HttpStreamFactoryImpl::Request : public HttpStreamRequest {
 
   // Called by an attached Job if it sets up a SpdySession.
   void OnNewSpdySessionReady(Job* job,
+                             scoped_ptr<HttpStream> stream,
                              const base::WeakPtr<SpdySession>& spdy_session,
                              bool direct);
 
diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc
index c20d89e..5f0e285 100644
--- a/net/spdy/spdy_session.cc
+++ b/net/spdy/spdy_session.cc
@@ -532,7 +532,7 @@ SpdySession::~SpdySession() {
   net_log_.EndEvent(NetLog::TYPE_SPDY_SESSION);
 }
 
-Error SpdySession::InitializeWithSocket(
+void SpdySession::InitializeWithSocket(
     scoped_ptr<ClientSocketHandle> connection,
     SpdySessionPool* pool,
     bool is_secure,
@@ -593,19 +593,17 @@ Error SpdySession::InitializeWithSocket(
       NetLog::TYPE_SPDY_SESSION_INITIALIZED,
       connection_->socket()->NetLog().source().ToEventParametersCallback());
 
-  int error = DoReadLoop(READ_STATE_DO_READ, OK);
-  if (error == ERR_IO_PENDING)
-    error = OK;
-  if (error == OK) {
-    DCHECK_NE(availability_state_, STATE_CLOSED);
-    connection_->AddHigherLayeredPool(this);
-    if (enable_sending_initial_data_)
-      SendInitialData();
-    pool_ = pool;
-  } else {
-    DcheckClosed();
-  }
-  return static_cast<Error>(error);
+  DCHECK_NE(availability_state_, STATE_CLOSED);
+  connection_->AddHigherLayeredPool(this);
+  if (enable_sending_initial_data_)
+    SendInitialData();
+  pool_ = pool;
+
+  // Bootstrap the read loop.
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&SpdySession::PumpReadLoop,
+                 weak_factory_.GetWeakPtr(), READ_STATE_DO_READ, OK));
 }
 
 bool SpdySession::VerifyDomainAuthentication(const std::string& domain) {
@@ -1554,9 +1552,8 @@ SpdySession::CloseSessionResult SpdySession::DoCloseSession(
   UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SpdySession.BytesRead.OtherErrors",
                               total_bytes_received_, 1, 100000000, 50);
 
-  // |pool_| will be NULL when |InitializeWithSocket()| is in the
-  // call stack.
-  if (pool_ && availability_state_ != STATE_GOING_AWAY)
+  DCHECK(pool_);
+  if (availability_state_ != STATE_GOING_AWAY)
     pool_->MakeSessionUnavailable(GetWeakPtr());
 
   availability_state_ = STATE_CLOSED;
@@ -1628,10 +1625,8 @@ void SpdySession::CloseSessionOnError(Error err,
 void SpdySession::MakeUnavailable() {
   if (availability_state_ < STATE_GOING_AWAY) {
     availability_state_ = STATE_GOING_AWAY;
-    // |pool_| will be NULL when |InitializeWithSocket()| is in the
-    // call stack.
-    if (pool_)
-      pool_->MakeSessionUnavailable(GetWeakPtr());
+    DCHECK(pool_);
+    pool_->MakeSessionUnavailable(GetWeakPtr());
   }
 }
 
@@ -1686,7 +1681,8 @@ base::Value* SpdySession::GetInfoAsValue() const {
 }
 
 bool SpdySession::IsReused() const {
-  return buffered_spdy_framer_->frames_received() > 0;
+  return buffered_spdy_framer_->frames_received() > 0 ||
+      connection_->reuse_type() == ClientSocketHandle::UNUSED_IDLE;
 }
 
 bool SpdySession::GetLoadTimingInfo(SpdyStreamId stream_id,
diff --git a/net/spdy/spdy_session.h b/net/spdy/spdy_session.h
index cbb87bf..203f1ef 100644
--- a/net/spdy/spdy_session.h
+++ b/net/spdy/spdy_session.h
@@ -256,13 +256,13 @@ class NET_EXPORT SpdySession : public BufferedSpdyFramerVisitorInterface,
   // |certificate_error_code| must either be OK or less than
   // ERR_IO_PENDING.
   //
-  // Returns OK on success, or an error on failure. Never returns
-  // ERR_IO_PENDING. If an error is returned, the session must be
-  // destroyed immediately.
-  Error InitializeWithSocket(scoped_ptr<ClientSocketHandle> connection,
-                             SpdySessionPool* pool,
-                             bool is_secure,
-                             int certificate_error_code);
+  // The session begins reading from |connection| on a subsequent event loop
+  // iteration, so the SpdySession may close immediately afterwards if the first
+  // read of |connection| fails.
+  void InitializeWithSocket(scoped_ptr<ClientSocketHandle> connection,
+                            SpdySessionPool* pool,
+                            bool is_secure,
+                            int certificate_error_code);
 
   // Returns the protocol used by this session. Always between
   // kProtoSPDYMinimumVersion and kProtoSPDYMaximumVersion.
@@ -367,7 +367,8 @@ class NET_EXPORT SpdySession : public BufferedSpdyFramerVisitorInterface,
   base::Value* GetInfoAsValue() const;
 
   // Indicates whether the session is being reused after having successfully
-  // used to send/receive data in the past.
+  // used to send/receive data in the past or if the underlying socket was idle
+  // before being used for a SPDY session.
   bool IsReused() const;
 
   // Returns true if the underlying transport socket ever had any reads or
@@ -617,8 +618,7 @@ class NET_EXPORT SpdySession : public BufferedSpdyFramerVisitorInterface,
   // Advance the ReadState state machine. |expected_read_state| is the
   // expected starting read state.
   //
-  // This function must always be called via PumpReadLoop() except for
-  // from InitializeWithSocket().
+  // This function must always be called via PumpReadLoop().
   int DoReadLoop(ReadState expected_read_state, int result);
   // The implementations of the states of the ReadState state machine.
   int DoRead();
diff --git a/net/spdy/spdy_session_pool.cc b/net/spdy/spdy_session_pool.cc
index 093db1e..6ce0a93 100644
--- a/net/spdy/spdy_session_pool.cc
+++ b/net/spdy/spdy_session_pool.cc
@@ -77,12 +77,11 @@ SpdySessionPool::~SpdySessionPool() {
   CertDatabase::GetInstance()->RemoveObserver(this);
 }
 
-net::Error SpdySessionPool::CreateAvailableSessionFromSocket(
+base::WeakPtr<SpdySession> SpdySessionPool::CreateAvailableSessionFromSocket(
     const SpdySessionKey& key,
     scoped_ptr<ClientSocketHandle> connection,
     const BoundNetLog& net_log,
     int certificate_error_code,
-    base::WeakPtr<SpdySession>* available_session,
     bool is_secure) {
   DCHECK_GE(default_protocol_, kProtoSPDYMinimumVersion);
   DCHECK_LE(default_protocol_, kProtoSPDYMaximumVersion);
@@ -105,22 +104,16 @@ net::Error SpdySessionPool::CreateAvailableSessionFromSocket(
                       trusted_spdy_proxy_,
                       net_log.net_log()));
 
-  Error error =  new_session->InitializeWithSocket(
+  new_session->InitializeWithSocket(
       connection.Pass(), this, is_secure, certificate_error_code);
-  DCHECK_NE(error, ERR_IO_PENDING);
 
-  if (error != OK) {
-    available_session->reset();
-    return error;
-  }
-
-  *available_session = new_session->GetWeakPtr();
+  base::WeakPtr<SpdySession> available_session = new_session->GetWeakPtr();
   sessions_.insert(new_session.release());
-  MapKeyToAvailableSession(key, *available_session);
+  MapKeyToAvailableSession(key, available_session);
 
   net_log.AddEvent(
       NetLog::TYPE_SPDY_SESSION_POOL_IMPORTED_SESSION_FROM_SOCKET,
-      (*available_session)->net_log().source().ToEventParametersCallback());
+      available_session->net_log().source().ToEventParametersCallback());
 
   // Look up the IP address for this session so that we can match
   // future sessions (potentially to different domains) which can
@@ -129,11 +122,11 @@ net::Error SpdySessionPool::CreateAvailableSessionFromSocket(
   // to see if this is a direct connection.
   if (key.proxy_server().is_direct()) {
     IPEndPoint address;
-    if ((*available_session)->GetPeerAddress(&address) == OK)
+    if (available_session->GetPeerAddress(&address) == OK)
       aliases_[address] = key;
   }
 
-  return error;
+  return available_session;
 }
 
 base::WeakPtr<SpdySession> SpdySessionPool::FindAvailableSession(
diff --git a/net/spdy/spdy_session_pool.h b/net/spdy/spdy_session_pool.h
index c44e9de..0fad5d2 100644
--- a/net/spdy/spdy_session_pool.h
+++ b/net/spdy/spdy_session_pool.h
@@ -79,15 +79,14 @@ class NET_EXPORT SpdySessionPool
   // encountered when connecting the SSL socket, with OK meaning there
   // was no error.
   //
-  // If successful, OK is returned and |available_session| will be
-  // non-NULL and available. Otherwise, an error is returned and
-  // |available_session| will be NULL.
-  net::Error CreateAvailableSessionFromSocket(
+  // Returns the new SpdySession. Note that the SpdySession begins reading from
+  // |connection| on a subsequent event loop iteration, so it may be closed
+  // immediately afterwards if the first read of |connection| fails.
+  base::WeakPtr<SpdySession> CreateAvailableSessionFromSocket(
       const SpdySessionKey& key,
       scoped_ptr<ClientSocketHandle> connection,
       const BoundNetLog& net_log,
       int certificate_error_code,
-      base::WeakPtr<SpdySession>* available_session,
       bool is_secure);
 
   // Find an available session for the given key, or NULL if there isn't one.
diff --git a/net/spdy/spdy_session_unittest.cc b/net/spdy/spdy_session_unittest.cc
index 0e8b370..a5d9292 100644
--- a/net/spdy/spdy_session_unittest.cc
+++ b/net/spdy/spdy_session_unittest.cc
@@ -190,8 +190,12 @@ INSTANTIATE_TEST_CASE_P(
 TEST_P(SpdySessionTest, InitialReadError) {
   CreateDeterministicNetworkSession();
 
-  TryCreateFakeSpdySessionExpectingFailure(
+  base::WeakPtr<SpdySession> session = TryCreateFakeSpdySessionExpectingFailure(
       spdy_session_pool_, key_, ERR_FAILED);
+  EXPECT_TRUE(session);
+  // Flush the read.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(session);
 }
 
 namespace {
@@ -277,8 +281,6 @@ TEST_P(SpdySessionTest, PendingStreamCancellingAnother) {
   session->CloseSessionOnError(ERR_ABORTED, "Aborting session");
 
   EXPECT_EQ(ERR_ABORTED, callback1.WaitForResult());
-
-  data.RunFor(1);
 }
 
 // A session receiving a GOAWAY frame with no active streams should
@@ -330,9 +332,12 @@ TEST_P(SpdySessionTest, GoAwayImmediatelyWithNoActiveStreams) {
 
   data.StopAfter(1);
 
-  TryCreateInsecureSpdySessionExpectingFailure(
-      http_session_, key_, ERR_CONNECTION_CLOSED, BoundNetLog());
+  base::WeakPtr<SpdySession> session =
+      TryCreateInsecureSpdySessionExpectingFailure(
+          http_session_, key_, ERR_CONNECTION_CLOSED, BoundNetLog());
+  base::RunLoop().RunUntilIdle();
 
+  EXPECT_FALSE(session);
   EXPECT_FALSE(HasSpdySession(spdy_session_pool_, key_));
 }
 
@@ -1045,8 +1050,6 @@ TEST_P(SpdySessionTest, FailedPing) {
 
   EXPECT_TRUE(session == NULL);
   EXPECT_FALSE(HasSpdySession(spdy_session_pool_, key_));
-
-  data.RunFor(1);
   EXPECT_EQ(NULL, spdy_stream1.get());
 }
 
diff --git a/net/spdy/spdy_test_util_common.cc b/net/spdy/spdy_test_util_common.cc
index 66a6f8c3..22ff5f6 100644
--- a/net/spdy/spdy_test_util_common.cc
+++ b/net/spdy/spdy_test_util_common.cc
@@ -540,15 +540,12 @@ base::WeakPtr<SpdySession> CreateSpdySessionHelper(
 
   EXPECT_EQ(OK, rv);
 
-  base::WeakPtr<SpdySession> spdy_session;
-  EXPECT_EQ(
-      expected_status,
+  base::WeakPtr<SpdySession> spdy_session =
       http_session->spdy_session_pool()->CreateAvailableSessionFromSocket(
-          key, connection.Pass(), net_log, OK, &spdy_session,
-          is_secure));
-  EXPECT_EQ(expected_status == OK, spdy_session != NULL);
-  EXPECT_EQ(expected_status == OK,
-            HasSpdySession(http_session->spdy_session_pool(), key));
+          key, connection.Pass(), net_log, OK, is_secure);
+  // Failure is reported asynchronously.
+  EXPECT_TRUE(spdy_session != NULL);
+  EXPECT_TRUE(HasSpdySession(http_session->spdy_session_pool(), key));
   return spdy_session;
 }
 
@@ -562,14 +559,14 @@ base::WeakPtr<SpdySession> CreateInsecureSpdySession(
                                  OK, false /* is_secure */);
 }
 
-void TryCreateInsecureSpdySessionExpectingFailure(
+base::WeakPtr<SpdySession> TryCreateInsecureSpdySessionExpectingFailure(
     const scoped_refptr<HttpNetworkSession>& http_session,
     const SpdySessionKey& key,
     Error expected_error,
     const BoundNetLog& net_log) {
   DCHECK_LT(expected_error, ERR_IO_PENDING);
-  CreateSpdySessionHelper(http_session, key, net_log,
-                          expected_error, false /* is_secure */);
+  return CreateSpdySessionHelper(http_session, key, net_log,
+                                 expected_error, false /* is_secure */);
 }
 
 base::WeakPtr<SpdySession> CreateSecureSpdySession(
@@ -643,17 +640,15 @@ base::WeakPtr<SpdySession> CreateFakeSpdySessionHelper(
     Error expected_status) {
   EXPECT_NE(expected_status, ERR_IO_PENDING);
   EXPECT_FALSE(HasSpdySession(pool, key));
-  base::WeakPtr<SpdySession> spdy_session;
   scoped_ptr<ClientSocketHandle> handle(new ClientSocketHandle());
   handle->SetSocket(scoped_ptr<StreamSocket>(new FakeSpdySessionClientSocket(
       expected_status == OK ? ERR_IO_PENDING : expected_status)));
-  EXPECT_EQ(
-      expected_status,
+  base::WeakPtr<SpdySession> spdy_session =
       pool->CreateAvailableSessionFromSocket(
-          key, handle.Pass(), BoundNetLog(), OK, &spdy_session,
-          true /* is_secure */));
-  EXPECT_EQ(expected_status == OK, spdy_session != NULL);
-  EXPECT_EQ(expected_status == OK, HasSpdySession(pool, key));
+          key, handle.Pass(), BoundNetLog(), OK, true /* is_secure */);
+  // Failure is reported asynchronously.
+  EXPECT_TRUE(spdy_session != NULL);
+  EXPECT_TRUE(HasSpdySession(pool, key));
   return spdy_session;
 }
 
@@ -664,11 +659,12 @@ base::WeakPtr<SpdySession> CreateFakeSpdySession(SpdySessionPool* pool,
   return CreateFakeSpdySessionHelper(pool, key, OK);
 }
 
-void TryCreateFakeSpdySessionExpectingFailure(SpdySessionPool* pool,
-                                              const SpdySessionKey& key,
-                                              Error expected_error) {
+base::WeakPtr<SpdySession> TryCreateFakeSpdySessionExpectingFailure(
+    SpdySessionPool* pool,
+    const SpdySessionKey& key,
+    Error expected_error) {
   DCHECK_LT(expected_error, ERR_IO_PENDING);
-  CreateFakeSpdySessionHelper(pool, key, expected_error);
+  return CreateFakeSpdySessionHelper(pool, key, expected_error);
 }
 
 SpdySessionPoolPeer::SpdySessionPoolPeer(SpdySessionPool* pool) : pool_(pool) {
diff --git a/net/spdy/spdy_test_util_common.h b/net/spdy/spdy_test_util_common.h
index 42b595b..668a7e8 100644
--- a/net/spdy/spdy_test_util_common.h
+++ b/net/spdy/spdy_test_util_common.h
@@ -247,8 +247,9 @@ base::WeakPtr<SpdySession> CreateInsecureSpdySession(
 
 // Tries to create a SPDY session for the given key but expects the
 // attempt to fail with the given error. A SPDY session for |key| must
-// not already exist.
-void TryCreateInsecureSpdySessionExpectingFailure(
+// not already exist. The session will be created but close in the
+// next event loop iteration.
+base::WeakPtr<SpdySession> TryCreateInsecureSpdySessionExpectingFailure(
     const scoped_refptr<HttpNetworkSession>& http_session,
     const SpdySessionKey& key,
     Error expected_error,
@@ -269,10 +270,12 @@ base::WeakPtr<SpdySession> CreateFakeSpdySession(SpdySessionPool* pool,
 // Tries to create an insecure SPDY session for the given key but
 // expects the attempt to fail with the given error. The session will
 // neither receive nor send any data. A SPDY session for |key| must
-// not already exist.
-void TryCreateFakeSpdySessionExpectingFailure(SpdySessionPool* pool,
-                                              const SpdySessionKey& key,
-                                              Error expected_error);
+// not already exist. The session will be created but close in the
+// next event loop iteration.
+base::WeakPtr<SpdySession> TryCreateFakeSpdySessionExpectingFailure(
+    SpdySessionPool* pool,
+    const SpdySessionKey& key,
+    Error expected_error);
 
 class SpdySessionPoolPeer {
  public: