Fix SPDY crash where we receive an early SYN_REPLY.

Added a test case which simulates this by doing a POST and scheduling
the SYN_REPLY to arrive before we've finished sending.

Also improved the error checking in SpdyStream a bit.

BUG=43133
TEST=SpdyNetworkTransactionTest.PostWithEarlySynReply


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

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

4 files changed, 79 insertions, 24 deletions

diff --git a/net/spdy/spdy_network_transaction_unittest.cc b/net/spdy/spdy_network_transaction_unittest.cc
index a025ce9..add273b 100644
--- a/net/spdy/spdy_network_transaction_unittest.cc
+++ b/net/spdy/spdy_network_transaction_unittest.cc
@@ -569,21 +569,21 @@ TEST_F(SpdyNetworkTransactionTest, Post) {
 
 // Test that a simple POST works.
 TEST_F(SpdyNetworkTransactionTest, EmptyPost) {
-static const unsigned char kEmptyPostSyn[] = {
-  0x80, 0x01, 0x00, 0x01,                                      // header
-  0x01, 0x00, 0x00, 0x4a,                                      // flags, len
-  0x00, 0x00, 0x00, 0x01,                                      // stream id
-  0x00, 0x00, 0x00, 0x00,                                      // associated
-  0xc0, 0x00, 0x00, 0x03,                                      // 4 headers
-  0x00, 0x06, 'm', 'e', 't', 'h', 'o', 'd',
-  0x00, 0x04, 'P', 'O', 'S', 'T',
-  0x00, 0x03, 'u', 'r', 'l',
-  0x00, 0x16, 'h', 't', 't', 'p', ':', '/', '/', 'w', 'w', 'w',
-              '.', 'g', 'o', 'o', 'g', 'l', 'e', '.', 'c', 'o',
-              'm', '/',
-  0x00, 0x07, 'v', 'e', 'r', 's', 'i', 'o', 'n',
-  0x00, 0x08, 'H', 'T', 'T', 'P', '/', '1', '.', '1',
-};
+  static const unsigned char kEmptyPostSyn[] = {
+    0x80, 0x01, 0x00, 0x01,                                      // header
+    0x01, 0x00, 0x00, 0x4a,                                      // flags, len
+    0x00, 0x00, 0x00, 0x01,                                      // stream id
+    0x00, 0x00, 0x00, 0x00,                                      // associated
+    0xc0, 0x00, 0x00, 0x03,                                      // 4 headers
+    0x00, 0x06, 'm', 'e', 't', 'h', 'o', 'd',
+    0x00, 0x04, 'P', 'O', 'S', 'T',
+    0x00, 0x03, 'u', 'r', 'l',
+    0x00, 0x16, 'h', 't', 't', 'p', ':', '/', '/', 'w', 'w', 'w',
+                '.', 'g', 'o', 'o', 'g', 'l', 'e', '.', 'c', 'o',
+                'm', '/',
+    0x00, 0x07, 'v', 'e', 'r', 's', 'i', 'o', 'n',
+    0x00, 0x08, 'H', 'T', 'T', 'P', '/', '1', '.', '1',
+  };
 
   // Setup the request
   HttpRequestInfo request;
@@ -615,6 +615,40 @@ static const unsigned char kEmptyPostSyn[] = {
   EXPECT_EQ("hello!", out.response_data);
 }
 
+// While we're doing a post, the server sends back a SYN_REPLY.
+TEST_F(SpdyNetworkTransactionTest, PostWithEarlySynReply) {
+  static const char upload[] = { "hello world" };
+
+  // Setup the request
+  HttpRequestInfo request;
+  request.method = "POST";
+  request.url = GURL("http://www.google.com/");
+  request.upload_data = new UploadData();
+  request.upload_data->AppendBytes(upload, sizeof(upload));
+
+  MockWrite writes[] = {
+    MockWrite(true, reinterpret_cast<const char*>(kPostSyn),
+              arraysize(kPostSyn), 2),
+    MockWrite(true, reinterpret_cast<const char*>(kPostUploadFrame),
+              arraysize(kPostUploadFrame), 3),
+  };
+
+  MockRead reads[] = {
+    MockRead(true, reinterpret_cast<const char*>(kPostSynReply),
+             arraysize(kPostSynReply), 2),
+    MockRead(true, reinterpret_cast<const char*>(kPostBodyFrame),
+             arraysize(kPostBodyFrame), 3),
+    MockRead(false, 0, 0)  // EOF
+  };
+
+  scoped_refptr<DelayedSocketData> data(
+      new DelayedSocketData(0, reads, arraysize(reads),
+                            writes, arraysize(writes)));
+  TransactionHelperResult out = TransactionHelper(request, data.get(),
+                                                  BoundNetLog());
+  EXPECT_EQ(ERR_SPDY_PROTOCOL_ERROR, out.rv);
+}
+
 // Test that the transaction doesn't crash when we don't have a reply.
 TEST_F(SpdyNetworkTransactionTest, ResponseWithoutSynReply) {
   MockRead reads[] = {
diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc
index 12b3d42..fc20bd0 100644
--- a/net/spdy/spdy_session.cc
+++ b/net/spdy/spdy_session.cc
@@ -900,14 +900,21 @@ bool SpdySession::Respond(const spdy::SpdyHeaderBlock& headers,
   // TODO(ahendrickson): This is recorded after the entire SYN_STREAM control
   // frame has been received and processed.  Move to framer?
   response.response_time = base::Time::Now();
+  int rv = OK;
+
   if (SpdyHeadersToHttpResponse(headers, &response)) {
     GetSSLInfo(&response.ssl_info);
     response.request_time = stream->GetRequestTime();
     response.vary_data.Init(*stream->GetRequestInfo(), *response.headers);
-    stream->OnResponseReceived(response);
+    rv = stream->OnResponseReceived(response);
   } else {
+    rv = ERR_INVALID_RESPONSE;
+  }
+
+  if (rv < 0) {
+    DCHECK_NE(rv, ERR_IO_PENDING);
     const spdy::SpdyStreamId stream_id = stream->stream_id();
-    stream->OnClose(ERR_INVALID_RESPONSE);
+    stream->OnClose(rv);
     DeactivateStream(stream_id);
     return false;
   }
diff --git a/net/spdy/spdy_stream.cc b/net/spdy/spdy_stream.cc
index 678b0c4..022ccd2 100644
--- a/net/spdy/spdy_stream.cc
+++ b/net/spdy/spdy_stream.cc
@@ -44,8 +44,10 @@ SpdyStream::~SpdyStream() {
   // inactive pending/pushed streams will still have stream_id_ set.
   if (stream_id_) {
     session_->CancelStream(stream_id_);
-  } else if (!response_complete_) {
-    NOTREACHED();
+  } else {
+    // When the stream_id_ is 0, we expect that it is because
+    // we've cancelled or closed the stream and set the stream_id to 0.
+    DCHECK(response_complete_);
   }
 }
 
@@ -191,7 +193,9 @@ void SpdyStream::Cancel() {
   session_->CancelStream(stream_id_);
 }
 
-void SpdyStream::OnResponseReceived(const HttpResponseInfo& response) {
+int SpdyStream::OnResponseReceived(const HttpResponseInfo& response) {
+  int rv = OK;
+
   metrics_.StartStream();
 
   CHECK(!response_->headers);
@@ -213,13 +217,16 @@ void SpdyStream::OnResponseReceived(const HttpResponseInfo& response) {
     // client requests an X-Associated-Content piece of content prior
     // to when the server push happens.
   } else {
-    NOTREACHED();
+    // We're not expecting a response while in this state.  Error!
+    rv = ERR_SPDY_PROTOCOL_ERROR;
   }
 
-  int rv = DoLoop(OK);
+  rv = DoLoop(rv);
 
   if (user_callback_)
     DoCallback(rv);
+
+  return rv;
 }
 
 bool SpdyStream::OnDataReceived(const char* data, int length) {
@@ -276,6 +283,10 @@ void SpdyStream::OnWriteComplete(int status) {
   // TODO(mbelshe): Check for cancellation here.  If we're cancelled, we
   // should discontinue the DoLoop.
 
+  // It is possible that this stream was closed while we had a write pending.
+  if (response_complete_)
+    return;
+
   if (status > 0)
     send_bytes_ += status;
 
@@ -456,6 +467,8 @@ int SpdyStream::DoSendBody() {
   // data portion, of course.
   io_state_ = STATE_SEND_BODY_COMPLETE;
   int buf_len = static_cast<int>(request_body_stream_->buf_len());
+  if (!buf_len)
+    return OK;
   return session_->WriteStreamData(stream_id_,
                                    request_body_stream_->buf(),
                                    buf_len);
@@ -499,7 +512,7 @@ int SpdyStream::DoReadBody() {
 int SpdyStream::DoReadBodyComplete(int result) {
   // TODO(mbelshe): merge SpdyStreamParser with SpdyStream and then this
   // makes sense.
-  return result;
+  return OK;
 }
 
 void SpdyStream::UpdateHistograms() {
diff --git a/net/spdy/spdy_stream.h b/net/spdy/spdy_stream.h
index cf7ea4a..235e645 100644
--- a/net/spdy/spdy_stream.h
+++ b/net/spdy/spdy_stream.h
@@ -99,7 +99,8 @@ class SpdyStream : public base::RefCounted<SpdyStream> {
   // Called by the SpdySession when a response (e.g. a SYN_REPLY) has been
   // received for this stream.  |path| is the path of the URL for a server
   // initiated stream, otherwise is empty.
-  void OnResponseReceived(const HttpResponseInfo& response);
+  // Returns a status code.
+  int OnResponseReceived(const HttpResponseInfo& response);
 
   // Called by the SpdySession when response data has been received for this
   // stream.  This callback may be called multiple times as data arrives