Fix Server Push bug; we properly detected the duplicate SYN, but we forgot to

break out, so we leaked the stream.  Also add a netlog entry for the case where
we max out the active stream limit.

BUG=none
TEST=SpdyNetworkTransactionTest.ServerPushDuplicate

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

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

3 files changed, 72 insertions, 15 deletions

diff --git a/net/base/net_log_event_type_list.h b/net/base/net_log_event_type_list.h
index dd71c1c..35159d4 100644
--- a/net/base/net_log_event_type_list.h
+++ b/net/base/net_log_event_type_list.h
@@ -638,6 +638,10 @@ EVENT_TYPE(SPDY_SESSION_STALLED_ON_SEND_WINDOW)
 //   }
 EVENT_TYPE(SPDY_SESSION_CLOSE)
 
+// Event when the creation of a stream is stalled because we're at
+// the maximum number of concurrent streams.
+EVENT_TYPE(SPDY_SESSION_STALLED_MAX_STREAMS)
+
 // ------------------------------------------------------------------------
 // SpdySessionPool
 // ------------------------------------------------------------------------
diff --git a/net/spdy/spdy_network_transaction_unittest.cc b/net/spdy/spdy_network_transaction_unittest.cc
index 884ec06..6780b6a 100644
--- a/net/spdy/spdy_network_transaction_unittest.cc
+++ b/net/spdy/spdy_network_transaction_unittest.cc
@@ -2351,7 +2351,6 @@ TEST_P(SpdyNetworkTransactionTest, ServerPushSingleDataFrame2) {
   EXPECT_EQ("HTTP/1.1 200 OK", response2.headers->GetStatusLine());
 }
 
-
 TEST_P(SpdyNetworkTransactionTest, ServerPushServerAborted) {
   scoped_ptr<spdy::SpdyFrame>
       stream1_syn(ConstructSpdyGet(NULL, 0, false, 1, LOWEST));
@@ -2409,6 +2408,57 @@ TEST_P(SpdyNetworkTransactionTest, ServerPushServerAborted) {
   EXPECT_EQ("HTTP/1.1 200 OK", response.headers->GetStatusLine());
 }
 
+TEST_P(SpdyNetworkTransactionTest, ServerPushDuplicate) {
+  // Verify that we don't leak streams and that we properly send a reset
+  // if the server pushes the same stream twice.
+  static const unsigned char kPushBodyFrame[] = {
+    0x00, 0x00, 0x00, 0x02,                                      // header, ID
+    0x01, 0x00, 0x00, 0x06,                                      // FIN, length
+    'p', 'u', 's', 'h', 'e', 'd'                                 // "pushed"
+  };
+
+  scoped_ptr<spdy::SpdyFrame>
+      stream1_syn(ConstructSpdyGet(NULL, 0, false, 1, LOWEST));
+  scoped_ptr<spdy::SpdyFrame>
+      stream1_body(ConstructSpdyBodyFrame(1, true));
+  scoped_ptr<spdy::SpdyFrame>
+      stream3_rst(ConstructSpdyRstStream(4, spdy::PROTOCOL_ERROR));
+  MockWrite writes[] = {
+    CreateMockWrite(*stream1_syn, 1),
+    CreateMockWrite(*stream3_rst, 5),
+  };
+
+  scoped_ptr<spdy::SpdyFrame>
+      stream1_reply(ConstructSpdyGetSynReply(NULL, 0, 1));
+  scoped_ptr<spdy::SpdyFrame>
+      stream2_syn(ConstructSpdyPush(NULL, 0, 2, 1, "/foo.dat"));
+  scoped_ptr<spdy::SpdyFrame>
+      stream3_syn(ConstructSpdyPush(NULL, 0, 4, 1, "/foo.dat"));
+  MockRead reads[] = {
+    CreateMockRead(*stream1_reply, 2),
+    CreateMockRead(*stream2_syn, 3),
+    CreateMockRead(*stream3_syn, 4),
+    CreateMockRead(*stream1_body, 6, false),
+    MockRead(true, reinterpret_cast<const char*>(kPushBodyFrame),
+             arraysize(kPushBodyFrame), 7),
+    MockRead(true, ERR_IO_PENDING, 8),  // Force a pause
+  };
+
+  HttpResponseInfo response;
+  HttpResponseInfo response2;
+  std::string expected_push_result("pushed");
+  RunServerPushTest(writes, arraysize(writes), reads, arraysize(reads),
+                    &response, &response2, expected_push_result);
+
+  // Verify the SYN_REPLY.
+  EXPECT_TRUE(response.headers != NULL);
+  EXPECT_EQ("HTTP/1.1 200 OK", response.headers->GetStatusLine());
+
+  // Verify the pushed stream.
+  EXPECT_TRUE(response2.headers != NULL);
+  EXPECT_EQ("HTTP/1.1 200 OK", response2.headers->GetStatusLine());
+}
+
 TEST_P(SpdyNetworkTransactionTest, ServerPushMultipleDataFrame) {
   static const unsigned char kPushBodyFrame1[] = {
     0x00, 0x00, 0x00, 0x02,                                      // header, ID
diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc
index 23dcbbb..ba8755c 100644
--- a/net/spdy/spdy_session.cc
+++ b/net/spdy/spdy_session.cc
@@ -383,6 +383,7 @@ int SpdySession::CreateStream(
     return CreateStreamImpl(url, priority, spdy_stream, stream_net_log);
   }
 
+  net_log().AddEvent(NetLog::TYPE_SPDY_SESSION_STALLED_MAX_STREAMS, NULL);
   create_stream_queues_[priority].push(
       PendingCreateStream(url, priority, spdy_stream,
                           stream_net_log, callback));
@@ -1050,6 +1051,15 @@ void SpdySession::OnSyn(const spdy::SpdySynStreamControlFrame& frame,
                         const linked_ptr<spdy::SpdyHeaderBlock>& headers) {
   spdy::SpdyStreamId stream_id = frame.stream_id();
   spdy::SpdyStreamId associated_stream_id = frame.associated_stream_id();
+
+  if (net_log_.IsLoggingAll()) {
+    net_log_.AddEvent(
+        NetLog::TYPE_SPDY_SESSION_PUSHED_SYN_STREAM,
+        new NetLogSpdySynParameter(
+            headers, static_cast<spdy::SpdyControlFlags>(frame.flags()),
+            stream_id));
+  }
+
   // Server-initiated streams should have even sequence numbers.
   if ((stream_id & 0x1) != 0) {
     LOG(ERROR) << "Received invalid OnSyn stream id " << stream_id;
@@ -1090,28 +1100,21 @@ void SpdySession::OnSyn(const spdy::SpdySynStreamControlFrame& frame,
     return;
   }
 
-  scoped_refptr<SpdyStream> stream;
-
-  stream = new SpdyStream(this, stream_id, true, net_log_);
-
-  if (net_log_.IsLoggingAll()) {
-    net_log_.AddEvent(
-        NetLog::TYPE_SPDY_SESSION_PUSHED_SYN_STREAM,
-        new NetLogSpdySynParameter(
-            headers, static_cast<spdy::SpdyControlFlags>(frame.flags()),
-            stream_id));
-  }
-
   // TODO(erikchen): Actually do something with the associated id.
 
-  stream->set_path(path);
-
   // There should not be an existing pushed stream with the same path.
   PushedStreamMap::iterator it = unclaimed_pushed_streams_.find(path);
   if (it != unclaimed_pushed_streams_.end()) {
     LOG(ERROR) << "Received duplicate pushed stream with path: " << path;
     ResetStream(stream_id, spdy::PROTOCOL_ERROR);
+    return;
   }
+
+  scoped_refptr<SpdyStream> stream =
+      new SpdyStream(this, stream_id, true, net_log_);
+
+  stream->set_path(path);
+
   unclaimed_pushed_streams_[path] = stream;
 
   ActivateStream(stream);