Client attempts to start a new spdy transaction with a session that is closing down.

TEST=net_unittests
BUG=47455,48503

Review URL: http://codereview.chromium.org/2841029

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

7 files changed, 119 insertions, 21 deletions

diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index ea30552..fed8af8 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -1482,6 +1482,8 @@ int HttpNetworkTransaction::DoSpdySendRequest() {
   }
 
   CHECK(spdy_session.get());
+  if(spdy_session->IsClosed())
+    return ERR_CONNECTION_CLOSED;
 
   UploadDataStream* upload_data = NULL;
   if (request_->upload_data) {
diff --git a/net/spdy/spdy_network_transaction.cc b/net/spdy/spdy_network_transaction.cc
index ce52518..9a3049f 100644
--- a/net/spdy/spdy_network_transaction.cc
+++ b/net/spdy/spdy_network_transaction.cc
@@ -249,6 +249,12 @@ int SpdyNetworkTransaction::DoInitConnectionComplete(int result) {
 int SpdyNetworkTransaction::DoSendRequest() {
   next_state_ = STATE_SEND_REQUEST_COMPLETE;
   CHECK(!stream_.get());
+
+  // It is possible that the spdy session was shut down while it was
+  // asynchronously waiting to connect.
+  if(spdy_->IsClosed())
+    return ERR_CONNECTION_CLOSED;
+
   UploadDataStream* upload_data = NULL;
   if (request_->upload_data) {
     int error_code;
diff --git a/net/spdy/spdy_network_transaction_unittest.cc b/net/spdy/spdy_network_transaction_unittest.cc
index 35756d0..7f48a60 100644
--- a/net/spdy/spdy_network_transaction_unittest.cc
+++ b/net/spdy/spdy_network_transaction_unittest.cc
@@ -420,14 +420,93 @@ TEST_F(SpdyNetworkTransactionTest, CancelledTransaction) {
   MessageLoop::current()->RunAllPending();
 }
 
+class StartTransactionCallback : public CallbackRunner< Tuple1<int> > {
+ public:
+  explicit StartTransactionCallback(
+      const scoped_refptr<HttpNetworkSession>& session)
+      : session_(session) {}
+
+  // We try to start another transaction, which should succeed.
+  virtual void RunWithParams(const Tuple1<int>& params) {
+    scoped_ptr<HttpTransaction> trans(new SpdyNetworkTransaction(session_));
+    TestCompletionCallback callback;
+    HttpRequestInfo request;
+    request.method = "GET";
+    request.url = GURL("http://www.google.com/");
+    request.load_flags = 0;
+    int rv = trans->Start(&request, &callback, BoundNetLog());
+    EXPECT_EQ(ERR_IO_PENDING, rv);
+  }
+
+ private:
+  const scoped_refptr<HttpNetworkSession>& session_;
+};
+
+// Verify that the client can correctly deal with the user callback attempting
+// to start another transaction on a session that is closing down. See
+// http://crbug.com/47455
+TEST_F(SpdyNetworkTransactionTest, StartTransactionOnReadCallback) {
+  scoped_ptr<spdy::SpdyFrame> req(ConstructSpdyGet(NULL, 0));
+  MockWrite writes[] = { CreateMockWrite(*req) };
+  MockWrite writes2[] = { CreateMockWrite(*req) };
+
+  // The indicated length of this packet is longer than its actual length. When
+  // the session receives an empty packet after this one, it shuts down the
+  // session, and calls the read callback with the incomplete data.
+  const uint8 kGetBodyFrame2[] = {
+    0x00, 0x00, 0x00, 0x01,
+    0x01, 0x00, 0x00, 0x07,
+    'h', 'e', 'l', 'l', 'o', '!',
+  };
+
+  scoped_ptr<spdy::SpdyFrame> resp(ConstructSpdyGetSynReply(NULL, 0));
+  MockRead reads[] = {
+    CreateMockRead(*resp, 2),
+    MockRead(true, ERR_IO_PENDING, 3),  // Force a pause
+    MockRead(true, reinterpret_cast<const char*>(kGetBodyFrame2),
+             arraysize(kGetBodyFrame2), 4),
+    MockRead(true, 0, 0, 5),  // EOF
+  };
+  MockRead reads2[] = {
+    CreateMockRead(*resp, 2),
+    MockRead(true, 0, 0, 3),  // EOF
+  };
+
+  // We disable SSL for this test.
+  SpdySession::SetSSLMode(false);
+
+  SessionDependencies session_deps;
+  scoped_refptr<HttpNetworkSession> session = CreateSession(&session_deps);
+  scoped_ptr<HttpTransaction> trans(new SpdyNetworkTransaction(session));
+  scoped_refptr<OrderedSocketData> data(
+      new OrderedSocketData(reads, arraysize(reads),
+                            writes, arraysize(writes)));
+  scoped_refptr<OrderedSocketData> data2(
+      new OrderedSocketData(reads2, arraysize(reads2),
+                            writes2, arraysize(writes2)));
+  session_deps.socket_factory.AddSocketDataProvider(data);
+  session_deps.socket_factory.AddSocketDataProvider(data2);
+
+  // Start the transaction with basic parameters.
+  TestCompletionCallback callback;
+  int rv = trans->Start(&CreateGetRequest(), &callback, BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  rv = callback.WaitForResult();
+
+  StartTransactionCallback callback2(session);
+  const int kSize = 3000;
+  scoped_refptr<net::IOBuffer> buf = new net::IOBuffer(kSize);
+  rv = trans->Read(buf, kSize, &callback2);
+  data->CompleteRead();
+  data2->CompleteRead();
+}
+
 class DeleteSessionCallback : public CallbackRunner< Tuple1<int> > {
  public:
   explicit DeleteSessionCallback(SpdyNetworkTransaction* trans1) :
       trans(trans1) {}
 
-  // We kill the transaction, which deletes the session and stream. However, the
-  // memory is still accessible, so we also have to zero out the memory of the
-  // stream. This is not a well defined operation, and can cause failures.
+  // We kill the transaction, which deletes the session and stream.
   virtual void RunWithParams(const Tuple1<int>& params) {
     delete trans;
   }
diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc
index bc9be5e..b1ba0f1 100644
--- a/net/spdy/spdy_session.cc
+++ b/net/spdy/spdy_session.cc
@@ -178,6 +178,8 @@ SpdySession::SpdySession(const HostPortPair& host_port_pair,
 }
 
 SpdySession::~SpdySession() {
+  state_ = CLOSED;
+
   // Cleanup all the streams.
   CloseAllStreams(net::ERR_ABORTED);
 
@@ -763,8 +765,8 @@ void SpdySession::CloseSessionOnError(net::Error err) {
   if (state_ != CLOSED) {
     state_ = CLOSED;
     error_ = err;
-    CloseAllStreams(err);
     RemoveFromPool();
+    CloseAllStreams(err);
   }
 }
 
diff --git a/net/spdy/spdy_session.h b/net/spdy/spdy_session.h
index 8a23168..d128661 100644
--- a/net/spdy/spdy_session.h
+++ b/net/spdy/spdy_session.h
@@ -101,9 +101,6 @@ class SpdySession : public base::RefCounted<SpdySession>,
   // status, such as "resolving host", "connecting", etc.
   LoadState GetLoadState() const;
 
-  // Closes all streams.  Used as part of shutdown.
-  void CloseAllStreams(net::Error status);
-
   // Fills SSL info in |ssl_info| and returns true when SSL is in use.
   bool GetSSLInfo(SSLInfo* ssl_info, bool* was_npn_negotiated);
 
@@ -111,6 +108,15 @@ class SpdySession : public base::RefCounted<SpdySession>,
   static void SetSSLMode(bool enable) { use_ssl_ = enable; }
   static bool SSLMode() { return use_ssl_; }
 
+  // If session is closed, no new streams/transactions should be created.
+  bool IsClosed() const { return state_ == CLOSED; }
+
+  // Closes this session.  This will close all active streams and mark
+  // the session as permanently closed.
+  // |err| should not be OK; this function is intended to be called on
+  // error.
+  void CloseSessionOnError(net::Error err);
+
  private:
   friend class base::RefCounted<SpdySession>;
   FRIEND_TEST(SpdySessionTest, GetActivePushStream);
@@ -173,12 +179,6 @@ class SpdySession : public base::RefCounted<SpdySession>,
   void QueueFrame(spdy::SpdyFrame* frame, spdy::SpdyPriority priority,
                   SpdyStream* stream);
 
-  // Closes this session.  This will close all active streams and mark
-  // the session as permanently closed.
-  // |err| should not be OK; this function is intended to be called on
-  // error.
-  void CloseSessionOnError(net::Error err);
-
   // Track active streams in the active stream list.
   void ActivateStream(SpdyStream* stream);
   void DeleteStream(spdy::SpdyStreamId id, int status);
@@ -198,6 +198,9 @@ class SpdySession : public base::RefCounted<SpdySession>,
 
   void RecordHistograms();
 
+  // Closes all streams.  Used as part of shutdown.
+  void CloseAllStreams(net::Error status);
+
   // Callbacks for the Spdy session.
   CompletionCallbackImpl<SpdySession> connect_callback_;
   CompletionCallbackImpl<SpdySession> ssl_connect_callback_;
diff --git a/net/spdy/spdy_session_pool.cc b/net/spdy/spdy_session_pool.cc
index 21fa0ac..8caa040 100644
--- a/net/spdy/spdy_session_pool.cc
+++ b/net/spdy/spdy_session_pool.cc
@@ -122,19 +122,26 @@ void SpdySessionPool::RemoveSessionList(const HostPortPair& host_port_pair) {
   }
 }
 
-void SpdySessionPool::RemoveAllSessions(bool close) {
-  while (sessions_.size()) {
+void SpdySessionPool::ClearSessions() {
+  while (!sessions_.empty()) {
     SpdySessionList* list = sessions_.begin()->second;
     DCHECK(list);
     sessions_.erase(sessions_.begin()->first);
     while (list->size()) {
-      scoped_refptr<SpdySession> session = list->front();
       list->pop_front();
-      if (close)
-        session->CloseAllStreams(net::ERR_ABORTED);
     }
     delete list;
   }
 }
 
+void SpdySessionPool::CloseAllSessions() {
+  while (!sessions_.empty()) {
+    SpdySessionList* list = sessions_.begin()->second;
+    DCHECK(list);
+    const scoped_refptr<SpdySession>& session = list->front();
+    DCHECK(session);
+    session->CloseSessionOnError(net::ERR_ABORTED);
+  }
+}
+
 }  // namespace net
diff --git a/net/spdy/spdy_session_pool.h b/net/spdy/spdy_session_pool.h
index 321f55c..7574154 100644
--- a/net/spdy/spdy_session_pool.h
+++ b/net/spdy/spdy_session_pool.h
@@ -64,7 +64,7 @@ class SpdySessionPool
   bool HasSession(const HostPortPair& host_port_pair)const;
 
   // Close all Spdy Sessions; used for debugging.
-  void CloseAllSessions() { RemoveAllSessions(true); }
+  void CloseAllSessions();
 
   // Removes a SpdySession from the SpdySessionPool.
   void Remove(const scoped_refptr<SpdySession>& session);
@@ -96,8 +96,7 @@ class SpdySessionPool
   // idle sessions being deleted, and the active sessions from being reused, so
   // they will be deleted once all active streams belonging to that session go
   // away.
-  void ClearSessions() { RemoveAllSessions(false); }
-  void RemoveAllSessions(bool close);
+  void ClearSessions();
 
   // This is our weak session pool - one session per domain.
   SpdySessionsMap sessions_;