SPDY sends RST_STREAM upon cancelling request, or bad header parse data.

Also fix tsan failure for spdy_http_stream_unittest.
Attempted to commit in http://codereview.chromium.org/3014030/show, ran into
different tsan failure.
TEST=net_unittests, tsan on windows for SpdyHttpStreamTest.
BUG=46589, 47478, 50198

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

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

6 files changed, 81 insertions, 19 deletions

diff --git a/net/data/valgrind/net_unittests.gtest-tsan_win32.txt b/net/data/valgrind/net_unittests.gtest-tsan_win32.txt
index 6b2663a..0fc6d3c 100644
--- a/net/data/valgrind/net_unittests.gtest-tsan_win32.txt
+++ b/net/data/valgrind/net_unittests.gtest-tsan_win32.txt
@@ -14,9 +14,6 @@ DiskCacheBackendTest.*
 # See http://crbug.com/47836
 ClientSocketPoolBaseTest.CancelPendingSocketAtSocketLimit
 
-# See http://crbug.com/50198
-SpdyHttpStreamTest.SendRequest
-
 #########################################
 # These tests fail if you don't have our SSL certificate installed.
 # Please see http://dev.chromium.org/developers/testing#TOC-SSL-tests
diff --git a/net/spdy/spdy_http_stream_unittest.cc b/net/spdy/spdy_http_stream_unittest.cc
index 9c23b54..ab4c606 100644
--- a/net/spdy/spdy_http_stream_unittest.cc
+++ b/net/spdy/spdy_http_stream_unittest.cc
@@ -10,6 +10,8 @@
 namespace net {
 
 class SpdyHttpStreamTest : public testing::Test {
+ public:
+  OrderedSocketData* data() { return data_; }
  protected:
   SpdyHttpStreamTest() {}
 
@@ -49,8 +51,11 @@ TEST_F(SpdyHttpStreamTest, SendRequest) {
   MockWrite writes[] = {
     CreateMockWrite(*req.get(), 1),
   };
+  scoped_ptr<spdy::SpdyFrame> resp(ConstructSpdyGetSynReply(NULL, 0, 1));
   MockRead reads[] = {
-    MockRead(false, 0, 2)  // EOF
+    CreateMockRead(*resp, 2),
+    MockRead(true, ERR_IO_PENDING, 3),  // Force a pause
+    MockRead(false, 0, 4)  // EOF
   };
 
   HostPortPair host_port_pair("www.google.com", 80);
@@ -70,9 +75,17 @@ TEST_F(SpdyHttpStreamTest, SendRequest) {
 
   EXPECT_EQ(ERR_IO_PENDING,
             http_stream->SendRequest(&response, &callback));
-  MessageLoop::current()->RunAllPending();
   EXPECT_TRUE(http_session_->spdy_session_pool()->HasSession(host_port_pair));
-  http_session_->spdy_session_pool()->Remove(session_);
+
+  // This triggers the MockWrite and reads 2 & 3
+  callback.WaitForResult();
+
+  // This triggers read 4. The empty read causes the session to shut down.
+  data()->CompleteRead();
+
+  EXPECT_TRUE(!http_session_->spdy_session_pool()->HasSession(host_port_pair));
+  EXPECT_TRUE(data()->at_read_eof());
+  EXPECT_TRUE(data()->at_write_eof());
 }
 
 // Test case for bug: http://code.google.com/p/chromium/issues/detail?id=50058
@@ -80,16 +93,12 @@ TEST_F(SpdyHttpStreamTest, SpdyURLTest) {
   EnableCompression(false);
   SpdySession::SetSSLMode(false);
 
-  scoped_ptr<spdy::SpdyFrame> req(ConstructSpdyGet(NULL, 0, false, 1, LOWEST));
-  MockWrite writes[] = {
-    CreateMockWrite(*req.get(), 1),
-  };
   MockRead reads[] = {
     MockRead(false, 0, 2),  // EOF
   };
 
   HostPortPair host_port_pair("www.google.com", 80);
-  EXPECT_EQ(OK, InitSession(reads, arraysize(reads), writes, arraysize(writes),
+  EXPECT_EQ(OK, InitSession(reads, arraysize(reads), NULL, 0,
       host_port_pair));
 
   HttpRequestInfo request;
@@ -113,7 +122,8 @@ TEST_F(SpdyHttpStreamTest, SpdyURLTest) {
 
   MessageLoop::current()->RunAllPending();
   EXPECT_TRUE(http_session_->spdy_session_pool()->HasSession(host_port_pair));
-  http_session_->spdy_session_pool()->Remove(session_);
+  http_session_->spdy_session_pool()->CloseAllSessions();
+  EXPECT_TRUE(!data()->at_read_eof());
 }
 
 // TODO(willchan): Write a longer test for SpdyStream that exercises all
diff --git a/net/spdy/spdy_network_transaction_unittest.cc b/net/spdy/spdy_network_transaction_unittest.cc
index bce3feb..3ac38cb 100644
--- a/net/spdy/spdy_network_transaction_unittest.cc
+++ b/net/spdy/spdy_network_transaction_unittest.cc
@@ -1128,6 +1128,46 @@ TEST_P(SpdyNetworkTransactionTest, CancelledTransaction) {
   helper.VerifyDataNotConsumed();
 }
 
+// Verify that the client sends a Rst Frame upon cancelling the stream.
+TEST_P(SpdyNetworkTransactionTest, CancelledTransactionSendRst) {
+  scoped_ptr<spdy::SpdyFrame> req(ConstructSpdyGet(NULL, 0, false, 1, LOWEST));
+  scoped_ptr<spdy::SpdyFrame> rst(
+      ConstructSpdyRstStream(1, spdy::CANCEL));
+  MockWrite writes[] = {
+    CreateMockWrite(*req, 1),
+    CreateMockWrite(*rst, 3),
+  };
+
+  scoped_ptr<spdy::SpdyFrame> resp(ConstructSpdyGetSynReply(NULL, 0, 1));
+  MockRead reads[] = {
+    CreateMockRead(*resp, 2),
+    MockRead(true, 0, 0, 4)  // EOF
+  };
+
+  scoped_refptr<OrderedSocketData> data(
+      new OrderedSocketData(reads, arraysize(reads),
+                            writes, arraysize(writes)));
+
+  NormalSpdyTransactionHelper helper(CreateGetRequest(),
+                                     BoundNetLog(),
+                                     GetParam());
+  helper.AddData(data.get());
+  helper.RunPreTestSetup();
+  HttpNetworkTransaction* trans = helper.trans();
+
+  TestCompletionCallback callback;
+
+  int rv = trans->Start(&CreateGetRequest(), &callback, BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  rv = callback.WaitForResult();
+  helper.ResetTrans();  // Cancel the transaction.
+
+  // Finish running rest of tasks.
+  MessageLoop::current()->RunAllPending();
+  data->CompleteRead();
+  helper.VerifyDataConsumed();
+}
+
 class SpdyNetworkTransactionTest::StartTransactionCallback
     : public CallbackRunner< Tuple1<int> > {
  public:
@@ -1696,8 +1736,11 @@ TEST_P(SpdyNetworkTransactionTest, DecompressFailureOnSynReply) {
 
   scoped_ptr<spdy::SpdyFrame> compressed(
       ConstructSpdyGet(NULL, 0, true, 1, LOWEST));
+  scoped_ptr<spdy::SpdyFrame> rst(
+      ConstructSpdyRstStream(1, spdy::PROTOCOL_ERROR));
   MockWrite writes[] = {
     CreateMockWrite(*compressed),
+    CreateMockWrite(*rst),
   };
 
   scoped_ptr<spdy::SpdyFrame> resp(ConstructSpdyGetSynReply(NULL, 0, 1));
@@ -1705,7 +1748,6 @@ TEST_P(SpdyNetworkTransactionTest, DecompressFailureOnSynReply) {
   MockRead reads[] = {
     CreateMockRead(*resp),
     CreateMockRead(*body),
-    MockRead(true, 0, 0)  // EOF
   };
 
   scoped_refptr<DelayedSocketData> data(
@@ -1715,7 +1757,7 @@ TEST_P(SpdyNetworkTransactionTest, DecompressFailureOnSynReply) {
                                      BoundNetLog(), GetParam());
   helper.RunToCompletion(data.get());
   TransactionHelperResult out = helper.output();
-  EXPECT_EQ(ERR_SYN_REPLY_NOT_RECEIVED, out.rv);
+  EXPECT_EQ(ERR_SPDY_PROTOCOL_ERROR, out.rv);
   data->Reset();
 
   EnableCompression(false);
diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc
index 9ceb951..5d6fb02 100644
--- a/net/spdy/spdy_session.cc
+++ b/net/spdy/spdy_session.cc
@@ -1122,8 +1122,16 @@ void SpdySession::OnControl(const spdy::SpdyControlFrame* frame) {
   uint32 type = frame->type();
   if (type == spdy::SYN_STREAM || type == spdy::SYN_REPLY) {
     if (!spdy_framer_.ParseHeaderBlock(frame, headers.get())) {
-      LOG(WARNING) << "Could not parse Spdy Control Frame Header";
-      // TODO(mbelshe):  Error the session?
+      LOG(WARNING) << "Could not parse Spdy Control Frame Header.";
+      int stream_id = 0;
+      if (type == spdy::SYN_STREAM)
+        stream_id = (reinterpret_cast<const spdy::SpdySynStreamControlFrame*>
+                     (frame))->stream_id();
+      if (type == spdy::SYN_REPLY)
+        stream_id = (reinterpret_cast<const spdy::SpdySynReplyControlFrame*>
+                     (frame))->stream_id();
+      if(IsStreamActive(stream_id))
+        ResetStream(stream_id, spdy::PROTOCOL_ERROR);
       return;
     }
   }
diff --git a/net/spdy/spdy_session_pool.h b/net/spdy/spdy_session_pool.h
index 362fdc9..1c18438 100644
--- a/net/spdy/spdy_session_pool.h
+++ b/net/spdy/spdy_session_pool.h
@@ -71,7 +71,8 @@ class SpdySessionPool
   // Close all Spdy Sessions; used for debugging.
   void CloseAllSessions();
 
-  // Removes a SpdySession from the SpdySessionPool.
+  // Removes a SpdySession from the SpdySessionPool. This should only be called
+  // by SpdySession, because otherwise session->state_ is not set to CLOSED.
   void Remove(const scoped_refptr<SpdySession>& session);
 
   // NetworkChangeNotifier::Observer methods:
diff --git a/net/spdy/spdy_stream.cc b/net/spdy/spdy_stream.cc
index 72a1a4f..145141e 100644
--- a/net/spdy/spdy_stream.cc
+++ b/net/spdy/spdy_stream.cc
@@ -53,7 +53,7 @@ void SpdyStream::SetDelegate(Delegate* delegate) {
 
 void SpdyStream::DetachDelegate() {
   delegate_ = NULL;
-  if (!response_complete_ && !cancelled())
+  if (!response_complete_)
     Cancel();
 }
 
@@ -216,8 +216,12 @@ void SpdyStream::OnClose(int status) {
 }
 
 void SpdyStream::Cancel() {
+  if (cancelled())
+    return;
+
   cancelled_ = true;
-  session_->CloseStream(stream_id_, ERR_ABORTED);
+  if(session_->IsStreamActive(stream_id_))
+    session_->ResetStream(stream_id_, spdy::CANCEL);
 }
 
 int SpdyStream::DoSendRequest(bool has_upload_data) {