[SPDY] Fix SpdySession's handling of SYN_REPLY frames

It sent the wrong error code for duplicate SYN_REPLY frames. Also,
it didn't trigger an error for data frames sent before SYN_REPLY.

Update tests to match.

Add {Active,Pushed}StreamInfo structs and use them in
{Active,Pushed}StreamMap.

Be defensive when closing expired push streams.

Give the correct name to SpdyHttpStream tests.

BUG=252556
R=rch@chromium.org

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

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

net/spdy/spdy_http_stream_unittest.cc

@@ -63,10 +63,10 @@ void TestLoadTimingNotReused(const HttpStream& stream) {
 
 }  // namespace
 
-class SpdyHttpStreamSpdy2Test : public testing::Test,
-                                public testing::WithParamInterface<NextProto> {
+class SpdyHttpStreamTest : public testing::Test,
+                           public testing::WithParamInterface<NextProto> {
  public:
-  SpdyHttpStreamSpdy2Test()
+  SpdyHttpStreamTest()
       : spdy_util_(GetParam()),
         session_deps_(GetParam()) {
     session_deps_.net_log = &net_log_;
@@ -164,7 +164,7 @@ class SpdyHttpStreamSpdy2Test : public testing::Test,
 
 INSTANTIATE_TEST_CASE_P(
     NextProto,
-    SpdyHttpStreamSpdy2Test,
+    SpdyHttpStreamTest,
     testing::Values(kProtoSPDY2, kProtoSPDY3, kProtoSPDY31, kProtoSPDY4a2));
 
 // TODO(akalin): Don't early-exit in the tests below for values >
@@ -172,7 +172,7 @@ INSTANTIATE_TEST_CASE_P(
 
 // SpdyHttpStream::GetUploadProgress() should still work even before the
 // stream is initialized.
-TEST_P(SpdyHttpStreamSpdy2Test, GetUploadProgressBeforeInitialization) {
+TEST_P(SpdyHttpStreamTest, GetUploadProgressBeforeInitialization) {
   if (GetParam() > kProtoSPDY3)
     return;
 
@@ -182,7 +182,7 @@ TEST_P(SpdyHttpStreamSpdy2Test, GetUploadProgressBeforeInitialization) {
   EXPECT_EQ(0u, progress.position());
 }
 
-TEST_P(SpdyHttpStreamSpdy2Test, SendRequest) {
+TEST_P(SpdyHttpStreamTest, SendRequest) {
   if (GetParam() > kProtoSPDY3)
     return;
 
@@ -249,7 +249,7 @@ TEST_P(SpdyHttpStreamSpdy2Test, SendRequest) {
   TestLoadTimingNotReused(*http_stream);
 }
 
-TEST_P(SpdyHttpStreamSpdy2Test, LoadTimingTwoRequests) {
+TEST_P(SpdyHttpStreamTest, LoadTimingTwoRequests) {
   if (GetParam() > kProtoSPDY3)
     return;
 
@@ -350,7 +350,7 @@ TEST_P(SpdyHttpStreamSpdy2Test, LoadTimingTwoRequests) {
   TestLoadTimingReused(*http_stream2);
 }
 
-TEST_P(SpdyHttpStreamSpdy2Test, SendChunkedPost) {
+TEST_P(SpdyHttpStreamTest, SendChunkedPost) {
   BufferedSpdyFramer framer(spdy_util_.spdy_version(), false);
 
   scoped_ptr<SpdyFrame> initial_window_update(
@@ -427,7 +427,7 @@ TEST_P(SpdyHttpStreamSpdy2Test, SendChunkedPost) {
 
 // Test to ensure the SpdyStream state machine does not get confused when a
 // chunk becomes available while a write is pending.
-TEST_P(SpdyHttpStreamSpdy2Test, DelayedSendChunkedPost) {
+TEST_P(SpdyHttpStreamTest, DelayedSendChunkedPost) {
   if (GetParam() > kProtoSPDY3)
     return;
 
@@ -537,7 +537,7 @@ TEST_P(SpdyHttpStreamSpdy2Test, DelayedSendChunkedPost) {
 
 // Test the receipt of a WINDOW_UPDATE frame while waiting for a chunk to be
 // made available is handled correctly.
-TEST_P(SpdyHttpStreamSpdy2Test, DelayedSendChunkedPostWithWindowUpdate) {
+TEST_P(SpdyHttpStreamTest, DelayedSendChunkedPostWithWindowUpdate) {
   if (GetParam() != kProtoSPDY3)
     return;
 
@@ -654,7 +654,7 @@ TEST_P(SpdyHttpStreamSpdy2Test, DelayedSendChunkedPostWithWindowUpdate) {
 }
 
 // Test case for bug: http://code.google.com/p/chromium/issues/detail?id=50058
-TEST_P(SpdyHttpStreamSpdy2Test, SpdyURLTest) {
+TEST_P(SpdyHttpStreamTest, SpdyURLTest) {
   if (GetParam() > kProtoSPDY3)
     return;
 
@@ -772,7 +772,7 @@ SpdyFrame* ConstructCredentialRequestFrame(NextProto next_proto,
 // Test that if we request a resource for a new origin on a session that
 // used domain bound certificates, that we send a CREDENTIAL frame for
 // the new domain before we send the new request.
-void SpdyHttpStreamSpdy2Test::TestSendCredentials(
+void SpdyHttpStreamTest::TestSendCredentials(
     ServerBoundCertService* server_bound_cert_service,
     const std::string& cert,
     const std::string& proof) {
@@ -896,7 +896,7 @@ void SpdyHttpStreamSpdy2Test::TestSendCredentials(
   ASSERT_EQ(200, response.headers->response_code());
 }
 
-TEST_P(SpdyHttpStreamSpdy2Test, SendCredentialsEC) {
+TEST_P(SpdyHttpStreamTest, SendCredentialsEC) {
   if (GetParam() != kProtoSPDY3)
     return;
 
@@ -916,7 +916,7 @@ TEST_P(SpdyHttpStreamSpdy2Test, SendCredentialsEC) {
   sequenced_worker_pool->Shutdown();
 }
 
-TEST_P(SpdyHttpStreamSpdy2Test, DontSendCredentialsForHttpUrlsEC) {
+TEST_P(SpdyHttpStreamTest, DontSendCredentialsForHttpUrlsEC) {
   if (GetParam() != kProtoSPDY3)
     return;
 

net/spdy/spdy_network_transaction_unittest.cc

@@ -2195,7 +2195,7 @@ TEST_P(SpdyNetworkTransactionTest, ResponseWithoutSynReply) {
                                      BoundNetLog(), GetParam(), NULL);
   helper.RunToCompletion(&data);
   TransactionHelperResult out = helper.output();
-  EXPECT_EQ(ERR_SYN_REPLY_NOT_RECEIVED, out.rv);
+  EXPECT_EQ(ERR_SPDY_PROTOCOL_ERROR, out.rv);
 }
 
 // Test that the transaction doesn't crash when we get two replies on the same
@@ -2206,7 +2206,12 @@ TEST_P(SpdyNetworkTransactionTest, ResponseWithTwoSynReplies) {
 
   scoped_ptr<SpdyFrame> req(
       spdy_util_.ConstructSpdyGet(NULL, 0, false, 1, LOWEST, true));
-  MockWrite writes[] = { CreateMockWrite(*req) };
+  scoped_ptr<SpdyFrame> rst(
+      spdy_util_.ConstructSpdyRstStream(1, RST_STREAM_STREAM_IN_USE));
+  MockWrite writes[] = {
+    CreateMockWrite(*req),
+    CreateMockWrite(*rst),
+  };
 
   scoped_ptr<SpdyFrame> resp(spdy_util_.ConstructSpdyGetSynReply(NULL, 0, 1));
   scoped_ptr<SpdyFrame> body(spdy_util_.ConstructSpdyBodyFrame(1, true));

net/spdy/spdy_session.h

@@ -483,9 +483,25 @@ class NET_EXPORT SpdySession : public base::RefCounted<SpdySession>,
   typedef std::deque<SpdyStreamRequest*> PendingStreamRequestQueue;
   typedef std::set<SpdyStreamRequest*> PendingStreamRequestCompletionSet;
 
-  typedef std::map<SpdyStreamId, SpdyStream*> ActiveStreamMap;
-  typedef std::map<std::string, std::pair<SpdyStream*, base::TimeTicks> >
-      PushedStreamMap;
+  struct ActiveStreamInfo {
+    ActiveStreamInfo();
+    explicit ActiveStreamInfo(SpdyStream* stream);
+    ~ActiveStreamInfo();
+
+    SpdyStream* stream;
+    bool waiting_for_syn_reply;
+  };
+  typedef std::map<SpdyStreamId, ActiveStreamInfo> ActiveStreamMap;
+
+  struct PushedStreamInfo {
+    PushedStreamInfo();
+    PushedStreamInfo(SpdyStreamId stream_id, base::TimeTicks creation_time);
+    ~PushedStreamInfo();
+
+    SpdyStreamId stream_id;
+    base::TimeTicks creation_time;
+  };
+  typedef std::map<std::string, PushedStreamInfo> PushedStreamMap;
 
   typedef std::set<SpdyStream*> CreatedStreamSet;
 

net/spdy/spdy_stream.h

@@ -129,6 +129,7 @@ class NET_EXPORT_PRIVATE SpdyStream {
   SpdyStreamId stream_id() const { return stream_id_; }
   void set_stream_id(SpdyStreamId stream_id) { stream_id_ = stream_id; }
 
+  // TODO(akalin): Remove these two functions.
   bool response_received() const { return response_received_; }
   void set_response_received() { response_received_ = true; }