Fix crash in GetSpdySessionFromSSLSocket in the case where the

server immediately hangs up after negotiating SPDY via NPN.

BUG=46369
TEST=SpdyNPNServerHangup
Review URL: http://codereview.chromium.org/2862027

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

6 files changed, 84 insertions, 19 deletions

diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 3269f42..72b1b1b 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -1515,8 +1515,10 @@ int HttpNetworkTransaction::DoSpdySendRequest() {
     // extension, so |connection_| must contain an SSLClientSocket.
     DCHECK(using_ssl_);
     CHECK(connection_->socket());
-    spdy_session = spdy_pool->GetSpdySessionFromSSLSocket(
-        endpoint_, session_, connection_.release(), net_log_);
+    int error = spdy_pool->GetSpdySessionFromSSLSocket(
+        endpoint_, session_, connection_.release(), net_log_, spdy_session);
+    if (error != OK)
+      return error;
   }
 
   CHECK(spdy_session.get());
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index f8a3a5e..449a966 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -6042,4 +6042,53 @@ TEST_F(HttpNetworkTransactionTest, NpnWithHttpOverSSL) {
   HttpNetworkTransaction::SetNextProtos("");
   HttpNetworkTransaction::SetUseAlternateProtocols(false);
 }
+
+TEST_F(HttpNetworkTransactionTest, SpdyPostNPNServerHangup) {
+  // Simulate the SSL handshake completing with an NPN negotiation
+  // followed by an immediate server closing of the socket.
+  // Fix crash:  http://crbug.com/46369
+  HttpNetworkTransaction::SetUseAlternateProtocols(true);
+  HttpNetworkTransaction::SetNextProtos(
+      "\x08http/1.1\x07http1.1\x06spdy/1\x04spdy");
+  SessionDependencies session_deps;
+
+  HttpRequestInfo request;
+  request.method = "GET";
+  request.url = GURL("https://www.google.com/");
+  request.load_flags = 0;
+
+  SSLSocketDataProvider ssl(true, OK);
+  ssl.next_proto_status = SSLClientSocket::kNextProtoNegotiated;
+  ssl.next_proto = "spdy/1";
+  ssl.was_npn_negotiated = true;
+  session_deps.socket_factory.AddSSLSocketDataProvider(&ssl);
+
+  MockWrite spdy_writes[] = {
+    MockWrite(true, reinterpret_cast<const char*>(kGetSyn),
+              arraysize(kGetSyn)),
+  };
+
+  MockRead spdy_reads[] = {
+    MockRead(false, 0, 0)   // Not async - return 0 immediately.
+  };
+
+  scoped_refptr<DelayedSocketData> spdy_data(
+      new DelayedSocketData(
+          0,  // don't wait in this case, immediate hangup.
+          spdy_reads, arraysize(spdy_reads),
+          spdy_writes, arraysize(spdy_writes)));
+  session_deps.socket_factory.AddSocketDataProvider(spdy_data);
+
+  TestCompletionCallback callback;
+
+  scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps));
+  scoped_ptr<HttpNetworkTransaction> trans(new HttpNetworkTransaction(session));
+
+  int rv = trans->Start(&request, &callback, BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  EXPECT_EQ(ERR_CONNECTION_CLOSED, callback.WaitForResult());
+
+  HttpNetworkTransaction::SetNextProtos("");
+  HttpNetworkTransaction::SetUseAlternateProtocols(false);
+}
 }  // namespace net
diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc
index 9e474b4..2e771e7 100644
--- a/net/spdy/spdy_session.cc
+++ b/net/spdy/spdy_session.cc
@@ -300,7 +300,8 @@ SpdySession::~SpdySession() {
   net_log_.EndEvent(NetLog::TYPE_SPDY_SESSION, NULL);
 }
 
-void SpdySession::InitializeWithSSLSocket(ClientSocketHandle* connection) {
+net::Error SpdySession::InitializeWithSSLSocket(
+    ClientSocketHandle* connection) {
   static StatsCounter spdy_sessions("spdy.sessions");
   spdy_sessions.Increment();
 
@@ -313,7 +314,10 @@ void SpdySession::InitializeWithSSLSocket(ClientSocketHandle* connection) {
   // This is a newly initialized session that no client should have a handle to
   // yet, so there's no need to start writing data as in OnTCPConnect(), but we
   // should start reading data.
-  ReadSocket();
+  net::Error error = ReadSocket();
+  if (error == ERR_IO_PENDING)
+    return OK;
+  return error;
 }
 
 net::Error SpdySession::Connect(const std::string& group_name,
@@ -347,6 +351,7 @@ scoped_refptr<SpdyHttpStream> SpdySession::GetOrCreateStream(
     const HttpRequestInfo& request,
     const UploadDataStream* upload_data,
     const BoundNetLog& stream_net_log) {
+  CHECK_NE(state_, CLOSED);
   const GURL& url = request.url;
   const std::string& path = url.PathForRequest();
 
@@ -666,13 +671,13 @@ void SpdySession::OnWriteComplete(int result) {
   }
 }
 
-void SpdySession::ReadSocket() {
+net::Error SpdySession::ReadSocket() {
   if (read_pending_)
-    return;
+    return OK;
 
   if (state_ == CLOSED) {
     NOTREACHED();
-    return;
+    return ERR_UNEXPECTED;
   }
 
   CHECK(connection_.get());
@@ -684,11 +689,11 @@ void SpdySession::ReadSocket() {
     case 0:
       // Socket is closed!
       CloseSessionOnError(ERR_CONNECTION_CLOSED);
-      return;
+      return ERR_CONNECTION_CLOSED;
     case net::ERR_IO_PENDING:
       // Waiting for data.  Nothing to do now.
       read_pending_ = true;
-      return;
+      return ERR_IO_PENDING;
     default:
       // Data was read, process it.
       // Schedule the work through the message loop to avoid recursive
@@ -698,6 +703,7 @@ void SpdySession::ReadSocket() {
           this, &SpdySession::OnReadComplete, bytes_read));
       break;
   }
+  return OK;
 }
 
 void SpdySession::WriteSocketLater() {
diff --git a/net/spdy/spdy_session.h b/net/spdy/spdy_session.h
index 4d70d22..588b7fb 100644
--- a/net/spdy/spdy_session.h
+++ b/net/spdy/spdy_session.h
@@ -71,7 +71,8 @@ class SpdySession : public base::RefCounted<SpdySession>,
       const BoundNetLog& stream_net_log);
 
   // Used by SpdySessionPool to initialize with a pre-existing SSL socket.
-  void InitializeWithSSLSocket(ClientSocketHandle* connection);
+  // Returns OK on success, or an error on failure.
+  net::Error InitializeWithSSLSocket(ClientSocketHandle* connection);
 
   // Write a data frame to the stream.
   // Used to create and queue a data frame for the given stream.
@@ -141,7 +142,8 @@ class SpdySession : public base::RefCounted<SpdySession>,
   void SendSettings();
 
   // Start reading from the socket.
-  void ReadSocket();
+  // Returns OK on success, or an error on failure.
+  net::Error ReadSocket();
 
   // Write current data to the socket.
   void WriteSocketLater();
diff --git a/net/spdy/spdy_session_pool.cc b/net/spdy/spdy_session_pool.cc
index 40557f2..c7e2e79 100644
--- a/net/spdy/spdy_session_pool.cc
+++ b/net/spdy/spdy_session_pool.cc
@@ -51,20 +51,22 @@ scoped_refptr<SpdySession> SpdySessionPool::Get(
   return spdy_session;
 }
 
-scoped_refptr<SpdySession> SpdySessionPool::GetSpdySessionFromSSLSocket(
+net::Error SpdySessionPool::GetSpdySessionFromSSLSocket(
     const HostPortPair& host_port_pair,
     HttpNetworkSession* session,
     ClientSocketHandle* connection,
-    const BoundNetLog& net_log) {
+    const BoundNetLog& net_log,
+    scoped_refptr<SpdySession>& spdy_session) {
+  // Create the SPDY session and add it to the pool.
+  spdy_session = (new SpdySession(host_port_pair, session, net_log.net_log()));
   SpdySessionList* list = GetSessionList(host_port_pair);
   if (!list)
     list = AddSessionList(host_port_pair);
   DCHECK(list->empty());
-  scoped_refptr<SpdySession> spdy_session(
-      new SpdySession(host_port_pair, session, net_log.net_log()));
-  spdy_session->InitializeWithSSLSocket(connection);
   list->push_back(spdy_session);
-  return spdy_session;
+
+  // Now we can initialize the session with the SSL socket.
+  return spdy_session->InitializeWithSSLSocket(connection);
 }
 
 bool SpdySessionPool::HasSession(const HostPortPair& host_port_pair) const {
diff --git a/net/spdy/spdy_session_pool.h b/net/spdy/spdy_session_pool.h
index fa02121e..08d01e0 100644
--- a/net/spdy/spdy_session_pool.h
+++ b/net/spdy/spdy_session_pool.h
@@ -13,6 +13,7 @@
 #include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
 #include "net/base/host_port_pair.h"
+#include "net/base/net_errors.h"
 #include "net/base/network_change_notifier.h"
 
 namespace net {
@@ -47,11 +48,14 @@ class SpdySessionPool
   // calling Get() first to use an existing SpdySession so we don't get
   // multiple SpdySessions per domain.  Note that ownership of |connection| is
   // transferred from the caller to the SpdySession.
-  scoped_refptr<SpdySession> GetSpdySessionFromSSLSocket(
+  // Returns OK on success, and the |spdy_session| will be provided.
+  // Returns an error on failure, and |spdy_session| will be NULL.
+  net::Error GetSpdySessionFromSSLSocket(
       const HostPortPair& host_port_pair,
       HttpNetworkSession* session,
       ClientSocketHandle* connection,
-      const BoundNetLog& net_log);
+      const BoundNetLog& net_log,
+      scoped_refptr<SpdySession>& spdy_session);
 
   // TODO(willchan): Consider renaming to HasReusableSession, since perhaps we
   // should be creating a new session.