Implement PUSH_PROMISE handling in spdy_session

net/spdy/spdy_network_transaction_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/file_util.h"
@@ skipping 3073 matching lines @@
   EXPECT_TRUE(response.headers.get() != NULL);
   EXPECT_EQ("HTTP/1.1 200 OK", response.headers->GetStatusLine());
 
   // Verify the pushed stream.
   EXPECT_TRUE(response2.headers.get() != NULL);
   EXPECT_EQ("HTTP/1.1 200 OK", response2.headers->GetStatusLine());
 }
 
 TEST_P(SpdyNetworkTransactionTest, ServerPushInvalidAssociatedStreamID0) {
   if (spdy_util_.spdy_version() == SPDY4) {
-    // TODO(jgraettinger): We don't support associated stream
-    // checks in SPDY4 yet.
+    // PUSH_PROMISE with stream id 0 is connection-level error.

[baranovich, 2014/06/13 11:42:46]

Should I write a test about sending goaway? I think no, since it is
framer-initiated goaway, and is tested as every goaway is. But I may be wrong=)

[Johnny, 2014/06/17 04:01:32]

It'd be good for completeness, and regression testing that the invalid stream
maps to a GOAWAY / PROTOCOL_ERROR as required by the spec. Can be done later (a
TODO is fine).

[baranovich, 2014/06/17 21:33:42]

Done.

     return;
   }
+
   scoped_ptr<SpdyFrame> stream1_syn(
       spdy_util_.ConstructSpdyGet(NULL, 0, false, 1, LOWEST, true));
   scoped_ptr<SpdyFrame> stream1_body(
       spdy_util_.ConstructSpdyBodyFrame(1, true));
   scoped_ptr<SpdyFrame> stream2_rst(
       spdy_util_.ConstructSpdyRstStream(2, RST_STREAM_REFUSED_STREAM));
   MockWrite writes[] = {
     CreateMockWrite(*stream1_syn, 1),
     CreateMockWrite(*stream2_rst, 4),
   };
@@ skipping 41 matching lines @@
                                     << " Write index: "
                                     << data.write_index();
 
   // Verify the SYN_REPLY.
   HttpResponseInfo response = *trans->GetResponseInfo();
   EXPECT_TRUE(response.headers.get() != NULL);
   EXPECT_EQ("HTTP/1.1 200 OK", response.headers->GetStatusLine());
 }
 
 TEST_P(SpdyNetworkTransactionTest, ServerPushInvalidAssociatedStreamID9) {
-  if (spdy_util_.spdy_version() == SPDY4) {
-    // TODO(jgraettinger): We don't support associated stream
-    // checks in SPDY4 yet.
-    return;
-  }
   scoped_ptr<SpdyFrame> stream1_syn(
       spdy_util_.ConstructSpdyGet(NULL, 0, false, 1, LOWEST, true));
   scoped_ptr<SpdyFrame> stream1_body(
       spdy_util_.ConstructSpdyBodyFrame(1, true));
   scoped_ptr<SpdyFrame> stream2_rst(
       spdy_util_.ConstructSpdyRstStream(2, RST_STREAM_INVALID_STREAM));

[Johnny, 2014/06/17 04:01:32]

My read of the spec is that invalid stream identifiers are a
GOAWAY/PROTOCOL_ERROR (under "promised stream ID" in section 6.6, and then
section 5.1.1).

[baranovich, 2014/06/17 21:33:42]

True, but that's true for all frame types.
But AFAIU this test is not about promised stream id, but about associated stream
id. We'd better not go away on PUSH_PROMISE with invalid (associated)stream id,
because of races (stream is locally cancelled and deleted but lots of data from
the server (including push promises) may be still in the wire). And we still
should send RST_STREAM for promised-stream-id to the server. So to my mind, this
behavior is correct although it is not directly in spec.

For unexpected promised stream id (odd id, less than last seen id, duplicate
id), I agree, we should go away.
Although from SPDY 3 spec
(http://www.chromium.org/spdy/spdy-protocol/spdy-protocol-draft3-1#TOC-2.3.2-S...):
The stream-id MUST increase with each new stream. If an endpoint receives a
SYN_STREAM with a stream id which is less than any previously received
SYN_STREAM, it MUST issue a session error (Section 2.4.1) with the status
PROTOCOL_ERROR.

It is a protocol error to send two SYN_STREAMs with the same stream-id. If a
recipient receives a second SYN_STREAM for the same stream, it MUST issue a
stream error Section (2.4.2) with the status code PROTOCOL_ERROR.

Not a word about what to do with odd ids, but still... this is not implemented,
we just do nothing silently.

Should I implement all this for SPDY4 and SPDY3 then?

On 2014/06/17 04:01:32, Johnny wrote:

[Johnny, 2014/06/19 16:58:51]

That's an excellent point. I'm convinced. So long as we immediately "forget"
about streams which we've cancelled, we need this behavior.
Yep, I put another comment on the spot where we do this in parallel, before
reading this. It'd be good to do this later, but this CL is ready to land as-is.

   MockWrite writes[] = {
     CreateMockWrite(*stream1_syn, 1),
     CreateMockWrite(*stream2_rst, 4),
   };
 
   scoped_ptr<SpdyFrame>
       stream1_reply(spdy_util_.ConstructSpdyGetSynReply(NULL, 0, 1));
   scoped_ptr<SpdyFrame>
       stream2_syn(spdy_util_.ConstructSpdyPush(NULL,
                                     0,
@@ skipping 52 matching lines @@
     CreateMockWrite(*stream1_syn, 1),
     CreateMockWrite(*stream2_rst, 4),
   };
 
   scoped_ptr<SpdyFrame>
       stream1_reply(spdy_util_.ConstructSpdyGetSynReply(NULL, 0, 1));
   scoped_ptr<SpdyHeaderBlock> incomplete_headers(new SpdyHeaderBlock());
   (*incomplete_headers)["hello"] = "bye";
   (*incomplete_headers)[spdy_util_.GetStatusKey()] = "200 OK";
   (*incomplete_headers)[spdy_util_.GetVersionKey()] = "HTTP/1.1";
-  scoped_ptr<SpdyFrame> stream2_syn(
-      spdy_util_.ConstructSpdyControlFrame(incomplete_headers.Pass(),
-                                           false,
-                                           2,  // Stream ID
-                                           LOWEST,
-                                           SYN_STREAM,
-                                           CONTROL_FLAG_NONE,
-                                           // Associated stream ID
-                                           1));
+  scoped_ptr<SpdyFrame> stream2_syn(spdy_util_.ConstructInitialSpdyPushFrame(
+      incomplete_headers.Pass(), 2, 1));
   MockRead reads[] = {
     CreateMockRead(*stream1_reply, 2),
     CreateMockRead(*stream2_syn, 3),
     CreateMockRead(*stream1_body, 4),
     MockRead(ASYNC, ERR_IO_PENDING, 5)  // Force a pause
   };
 
   OrderedSocketData data(reads, arraysize(reads),
                          writes, arraysize(writes));
   NormalSpdyTransactionHelper helper(CreateGetRequest(), DEFAULT_PRIORITY,
@@ skipping 1937 matching lines @@
   scoped_ptr<SpdyFrame> stream1_body(
       spdy_util_.ConstructSpdyBodyFrame(1, true));
   MockWrite writes[] = {
     CreateMockWrite(*stream1_syn, 1),
   };
 
   scoped_ptr<SpdyHeaderBlock> initial_headers(new SpdyHeaderBlock());
   spdy_util_.AddUrlToHeaderBlock(
       "http://www.google.com/foo.dat", initial_headers.get());
   scoped_ptr<SpdyFrame> stream2_syn(
-      spdy_util_.ConstructSpdyControlFrame(initial_headers.Pass(),
-                                           false,
-                                           2,
-                                           LOWEST,
-                                           SYN_STREAM,
-                                           CONTROL_FLAG_NONE,
-                                           1));
+      spdy_util_.ConstructInitialSpdyPushFrame(initial_headers.Pass(), 2, 1));
 
   scoped_ptr<SpdyHeaderBlock> late_headers(new SpdyHeaderBlock());
   (*late_headers)["hello"] = "bye";
   (*late_headers)[spdy_util_.GetStatusKey()] = "200";
   (*late_headers)[spdy_util_.GetVersionKey()] = "HTTP/1.1";
   scoped_ptr<SpdyFrame> stream2_headers(
       spdy_util_.ConstructSpdyControlFrame(late_headers.Pass(),
                                            false,
                                            2,
                                            LOWEST,
@@ skipping 42 matching lines @@
   scoped_ptr<SpdyFrame> stream1_body(
       spdy_util_.ConstructSpdyBodyFrame(1, true));
   MockWrite writes[] = {
     CreateMockWrite(*stream1_syn, 0, SYNCHRONOUS),
   };
 
   scoped_ptr<SpdyHeaderBlock> initial_headers(new SpdyHeaderBlock());
   spdy_util_.AddUrlToHeaderBlock(
       "http://www.google.com/foo.dat", initial_headers.get());
   scoped_ptr<SpdyFrame> stream2_syn(
-      spdy_util_.ConstructSpdyControlFrame(initial_headers.Pass(),
-                                           false,
-                                           2,
-                                           LOWEST,
-                                           SYN_STREAM,
-                                           CONTROL_FLAG_NONE,
-                                           1));
+      spdy_util_.ConstructInitialSpdyPushFrame(initial_headers.Pass(), 2, 1));
 
   scoped_ptr<SpdyHeaderBlock> late_headers(new SpdyHeaderBlock());
   (*late_headers)["hello"] = "bye";
   (*late_headers)[spdy_util_.GetStatusKey()] = "200";
   (*late_headers)[spdy_util_.GetVersionKey()] = "HTTP/1.1";
   scoped_ptr<SpdyFrame> stream2_headers(
       spdy_util_.ConstructSpdyControlFrame(late_headers.Pass(),
                                            false,
                                            2,
                                            LOWEST,
@@ skipping 96 matching lines @@
   // We push a stream and attempt to claim it before the headers come down.
   scoped_ptr<SpdyFrame> stream1_syn(
       spdy_util_.ConstructSpdyGet(NULL, 0, false, 1, LOWEST, true));
   scoped_ptr<SpdyFrame> stream1_body(
       spdy_util_.ConstructSpdyBodyFrame(1, true));
   MockWrite writes[] = {
     CreateMockWrite(*stream1_syn, 0, SYNCHRONOUS),
   };
 
   scoped_ptr<SpdyHeaderBlock> initial_headers(new SpdyHeaderBlock());
-  (*initial_headers)["alpha"] = "beta";
+  if (spdy_util_.spdy_version() < SPDY4) {
+    // In SPDY4 PUSH_PROMISE headers won't show up in the response headers.
+    (*initial_headers)["alpha"] = "beta";
+  }
   spdy_util_.AddUrlToHeaderBlock(
       "http://www.google.com/foo.dat", initial_headers.get());
   scoped_ptr<SpdyFrame> stream2_syn(
-      spdy_util_.ConstructSpdyControlFrame(initial_headers.Pass(),
-                                           false,
-                                           2,
-                                           LOWEST,
-                                           SYN_STREAM,
-                                           CONTROL_FLAG_NONE,
-                                           1));
+      spdy_util_.ConstructInitialSpdyPushFrame(initial_headers.Pass(), 2, 1));
 
   scoped_ptr<SpdyHeaderBlock> middle_headers(new SpdyHeaderBlock());
   (*middle_headers)["hello"] = "bye";
   scoped_ptr<SpdyFrame> stream2_headers1(
       spdy_util_.ConstructSpdyControlFrame(middle_headers.Pass(),
                                            false,
                                            2,
                                            LOWEST,
                                            HEADERS,
                                            CONTROL_FLAG_NONE,
@@ skipping 87 matching lines @@
 
   // Verify the SYN_REPLY.
   EXPECT_TRUE(response.headers.get() != NULL);
   EXPECT_EQ("HTTP/1.1 200 OK", response.headers->GetStatusLine());
 
   // Verify the pushed stream.
   EXPECT_TRUE(response2.headers.get() != NULL);
   EXPECT_EQ("HTTP/1.1 200 OK", response2.headers->GetStatusLine());
 
   // Verify we got all the headers from all header blocks.
-  EXPECT_TRUE(response2.headers->HasHeaderValue("alpha", "beta"));
+  if (spdy_util_.spdy_version() < SPDY4)
+    EXPECT_TRUE(response2.headers->HasHeaderValue("alpha", "beta"));
   EXPECT_TRUE(response2.headers->HasHeaderValue("hello", "bye"));
   EXPECT_TRUE(response2.headers->HasHeaderValue("status", "200"));
 
   // Read the final EOF (which will close the session)
   data.RunFor(1);
 
   // Verify that we consumed all test data.
   EXPECT_TRUE(data.at_read_eof());
   EXPECT_TRUE(data.at_write_eof());
 }
 
 TEST_P(SpdyNetworkTransactionTest, ServerPushWithNoStatusHeaderFrames) {
   // We push a stream and attempt to claim it before the headers come down.
   scoped_ptr<SpdyFrame> stream1_syn(
       spdy_util_.ConstructSpdyGet(NULL, 0, false, 1, LOWEST, true));
   scoped_ptr<SpdyFrame> stream1_body(
       spdy_util_.ConstructSpdyBodyFrame(1, true));
   MockWrite writes[] = {
     CreateMockWrite(*stream1_syn, 0, SYNCHRONOUS),
   };
 
   scoped_ptr<SpdyHeaderBlock> initial_headers(new SpdyHeaderBlock());
   spdy_util_.AddUrlToHeaderBlock(
       "http://www.google.com/foo.dat", initial_headers.get());
   scoped_ptr<SpdyFrame> stream2_syn(
-      spdy_util_.ConstructSpdyControlFrame(initial_headers.Pass(),
-                                           false,
-                                           2,
-                                           LOWEST,
-                                           SYN_STREAM,
-                                           CONTROL_FLAG_NONE,
-                                           1));
+      spdy_util_.ConstructInitialSpdyPushFrame(initial_headers.Pass(), 2, 1));
 
   scoped_ptr<SpdyHeaderBlock> middle_headers(new SpdyHeaderBlock());
   (*middle_headers)["hello"] = "bye";
   scoped_ptr<SpdyFrame> stream2_headers1(
       spdy_util_.ConstructSpdyControlFrame(middle_headers.Pass(),
                                            false,
                                            2,
                                            LOWEST,
                                            HEADERS,
                                            CONTROL_FLAG_NONE,
@@ skipping 176 matching lines @@
   DelayedSocketData data(1, reads, arraysize(reads),
                          writes, arraysize(writes));
   NormalSpdyTransactionHelper helper(CreateGetRequest(), DEFAULT_PRIORITY,
                                      BoundNetLog(), GetParam(), NULL);
   helper.RunToCompletion(&data);
   TransactionHelperResult out = helper.output();
   EXPECT_EQ(ERR_SPDY_PROTOCOL_ERROR, out.rv);
 }
 
 TEST_P(SpdyNetworkTransactionTest, ServerPushCrossOriginCorrectness) {
-  if (spdy_util_.spdy_version() == SPDY4) {
-    // TODO(jgraettinger): We don't support associated stream
-    // checks in SPDY4 yet.
-    return;
-  }
   // In this test we want to verify that we can't accidentally push content
   // which can't be pushed by this content server.
   // This test assumes that:
   //   - if we're requesting http://www.foo.com/barbaz
   //   - the browser has made a connection to "www.foo.com".
 
   // A list of the URL to fetch, followed by the URL being pushed.
   static const char* const kTestCases[] = {
     "http://www.google.com/foo.html",
     "http://www.google.com:81/foo.js",     // Bad port
@@ skipping 977 matching lines @@
 TEST_P(SpdyNetworkTransactionTLSUsageCheckTest, TLSCipherSuiteSucky) {
   scoped_ptr<SSLSocketDataProvider> ssl_provider(
       new SSLSocketDataProvider(ASYNC, OK));
   // Set to TLS_RSA_WITH_NULL_MD5
   SSLConnectionStatusSetCipherSuite(0x1, &ssl_provider->connection_status);
 
   RunTLSUsageCheckTest(ssl_provider.Pass());
 }
 
 }  // namespace net