Change the SPDY4 maximum frame size to 16K-1

This lands server change 62231065 by hkhalil.

(Note Chromium uses SpdyFramer::GetControlFrameBufferMaxSize(), rather
than SpdyFramer::kMaxControlFrameSize, to control this).

Additional enforcement of this limit to come in follow-up CLs.
Also adds some DCHECKs for better debugability of problematic test code.

Adds some clamping to the maximum frame size to DATA frame size calculation in
SpdyWriter. This has no effect for SPDY < 4, since maximum DATA frame payload
there is huge.

This lands server change 62302279 by hkhalil.

BUG=345769

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

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

5 files changed, 67 insertions, 46 deletions

diff --git a/net/spdy/spdy_frame_builder.cc b/net/spdy/spdy_frame_builder.cc
index 9e779ff..5194c11 100644
--- a/net/spdy/spdy_frame_builder.cc
+++ b/net/spdy/spdy_frame_builder.cc
@@ -102,7 +102,9 @@ bool SpdyFrameBuilder::WriteFramePrefix(const SpdyFramer& framer,
   DCHECK_EQ(0u, stream_id & ~kStreamIdMask);
   DCHECK_LE(4, framer.protocol_version());
   bool success = true;
-  DCHECK_GT(1u<<16, capacity_);  // Make sure length fits in 2B.
+  // Upstream DCHECK's that capacity_ is under the maximum frame size at this
+  // point. Chromium does not, because of the large additional zlib inflation
+  // factor we use. (Frame size is is still checked by OverwriteLength() below).
   success &= WriteUInt16(capacity_);
   success &= WriteUInt8(type);
   success &= WriteUInt8(flags);
@@ -153,6 +155,12 @@ bool SpdyFrameBuilder::RewriteLength(const SpdyFramer& framer) {
 
 bool SpdyFrameBuilder::OverwriteLength(const SpdyFramer& framer,
                                        size_t length) {
+  if (framer.protocol_version() < 4) {
+    DCHECK_GT(framer.GetFrameMaximumSize() - framer.GetFrameMinimumSize(),
+              length);
+  } else {
+    DCHECK_GE(framer.GetFrameMaximumSize(), length);
+  }
   bool success = false;
   const size_t old_length = length_;
 
diff --git a/net/spdy/spdy_framer.cc b/net/spdy/spdy_framer.cc
index 132a555..1c0ee75 100644
--- a/net/spdy/spdy_framer.cc
+++ b/net/spdy/spdy_framer.cc
@@ -324,7 +324,13 @@ size_t SpdyFramer::GetFrameMinimumSize() const {
 }
 
 size_t SpdyFramer::GetFrameMaximumSize() const {
-  return (protocol_version() < 4) ? 0xffffff : 0xffff;
+  if (protocol_version() < 4) {
+    // 24-bit length field plus eight-byte frame header.
+    return ((1<<24) - 1) + 8;
+  } else {
+    // 14-bit length field.
+    return (1<<14) - 1;
+  }
 }
 
 size_t SpdyFramer::GetDataFrameMaximumPayload() const {
@@ -693,6 +699,12 @@ size_t SpdyFramer::ProcessCommonHeader(const char* data, size_t len) {
 
   // if we're here, then we have the common header all received.
   if (!is_control_frame) {
+    if (protocol_version() >= 4) {
+      // Catch bogus tests sending oversized DATA frames.
+      DCHECK_GE(GetFrameMaximumSize(), current_frame_length_)
+          << "DATA frame too large for SPDY >= 4.";
+    }
+
     if (current_frame_flags_ & ~DATA_FLAG_FIN) {
       set_error(SPDY_INVALID_DATA_FRAME_FLAGS);
     } else {
diff --git a/net/spdy/spdy_framer.h b/net/spdy/spdy_framer.h
index 59b3d6f..f3cd98f 100644
--- a/net/spdy/spdy_framer.h
+++ b/net/spdy/spdy_framer.h
@@ -615,9 +615,8 @@ class NET_EXPORT_PRIVATE SpdyFramer {
     if (spdy_version_ == SPDY3) {
       return 16 * 1024 * 1024;
     }
-    // The theoretical maximum for SPDY4 is 2^16 - 1, as the length
-    // field does count the size of the header.
-    return 16 * 1024;
+    // Absolute maximum size of HTTP2 frame payload (section 4.2 "Frame size").
+    return (1<<14) - 1;
   }
 
   // The size of the control frame buffer.
diff --git a/net/spdy/spdy_framer_test.cc b/net/spdy/spdy_framer_test.cc
index 8f5b63b..30db38d 100644
--- a/net/spdy/spdy_framer_test.cc
+++ b/net/spdy/spdy_framer_test.cc
@@ -2922,7 +2922,11 @@ TEST_P(SpdyFramerTest, ControlFrameTooLarge) {
   // Create a frame at exatly that size.
   string big_value(kBigValueSize, 'x');
   syn_stream.SetHeader("aa", big_value.c_str());
+  // Upstream branches here and wraps SPDY4 with EXPECT_DEBUG_DFATAL. We
+  // neither support that in Chromium, nor do we use the same DFATAL (see
+  // SpdyFrameBuilder::WriteFramePrefix()).
   control_frame.reset(framer.SerializeSynStream(syn_stream));
+
   EXPECT_TRUE(control_frame.get() != NULL);
   EXPECT_EQ(framer.GetControlFrameBufferMaxSize() + 1,
             control_frame->size());
@@ -3725,8 +3729,8 @@ TEST_P(SpdyFramerTest, SizesTest) {
     EXPECT_EQ(8u, framer.GetBlockedSize());
     EXPECT_EQ(12u, framer.GetPushPromiseMinimumSize());
     EXPECT_EQ(8u, framer.GetFrameMinimumSize());
-    EXPECT_EQ(65535u, framer.GetFrameMaximumSize());
-    EXPECT_EQ(65527u, framer.GetDataFrameMaximumPayload());
+    EXPECT_EQ(16383u, framer.GetFrameMaximumSize());
+    EXPECT_EQ(16375u, framer.GetDataFrameMaximumPayload());
   } else {
     EXPECT_EQ(8u, framer.GetControlFrameHeaderSize());
     EXPECT_EQ(18u, framer.GetSynStreamMinimumSize());
@@ -3738,8 +3742,8 @@ TEST_P(SpdyFramerTest, SizesTest) {
     EXPECT_EQ(IsSpdy2() ? 14u : 12u, framer.GetHeadersMinimumSize());
     EXPECT_EQ(16u, framer.GetWindowUpdateSize());
     EXPECT_EQ(8u, framer.GetFrameMinimumSize());
-    EXPECT_EQ(16777215u, framer.GetFrameMaximumSize());
-    EXPECT_EQ(16777207u, framer.GetDataFrameMaximumPayload());
+    EXPECT_EQ(16777223u, framer.GetFrameMaximumSize());
+    EXPECT_EQ(16777215u, framer.GetDataFrameMaximumPayload());
   }
 }
 
diff --git a/net/spdy/spdy_network_transaction_unittest.cc b/net/spdy/spdy_network_transaction_unittest.cc
index a307b42..c80efe8 100644
--- a/net/spdy/spdy_network_transaction_unittest.cc
+++ b/net/spdy/spdy_network_transaction_unittest.cc
@@ -12,6 +12,7 @@
 #include "base/memory/scoped_vector.h"
 #include "base/run_loop.h"
 #include "base/stl_util.h"
+#include "base/strings/string_piece.h"
 #include "base/test/test_file_util.h"
 #include "net/base/auth.h"
 #include "net/base/net_log_unittest.h"
@@ -33,6 +34,7 @@
 #include "net/spdy/spdy_test_util_common.h"
 #include "net/spdy/spdy_test_utils.h"
 #include "net/url_request/url_request_test_util.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/platform_test.h"
 
 //-----------------------------------------------------------------------------
@@ -40,6 +42,10 @@
 namespace net {
 
 namespace {
+
+using testing::Each;
+using testing::Eq;
+
 const char kRequestUrl[] = "http://www.google.com/";
 
 enum SpdyNetworkTransactionTestSSLType {
@@ -6038,16 +6044,15 @@ TEST_P(SpdyNetworkTransactionTest, WindowUpdateSent) {
   if (GetParam().protocol < kProtoSPDY3)
     return;
 
-  // Set the data in the body frame large enough to trigger sending a
-  // WINDOW_UPDATE by the stream.
-  const std::string body_data(kSpdyStreamInitialWindowSize / 2 + 1, 'x');
+  // Amount of body required to trigger a sent window update.
+  const size_t kTargetSize = kSpdyStreamInitialWindowSize / 2 + 1;
 
   scoped_ptr<SpdyFrame> req(
       spdy_util_.ConstructSpdyGet(NULL, 0, false, 1, LOWEST, true));
   scoped_ptr<SpdyFrame> session_window_update(
-      spdy_util_.ConstructSpdyWindowUpdate(0, body_data.size()));
+      spdy_util_.ConstructSpdyWindowUpdate(0, kTargetSize));
   scoped_ptr<SpdyFrame> window_update(
-      spdy_util_.ConstructSpdyWindowUpdate(1, body_data.size()));
+      spdy_util_.ConstructSpdyWindowUpdate(1, kTargetSize));
 
   std::vector<MockWrite> writes;
   writes.push_back(CreateMockWrite(*req));
@@ -6055,23 +6060,23 @@ TEST_P(SpdyNetworkTransactionTest, WindowUpdateSent) {
     writes.push_back(CreateMockWrite(*session_window_update));
   writes.push_back(CreateMockWrite(*window_update));
 
+  std::vector<MockRead> reads;
   scoped_ptr<SpdyFrame> resp(
       spdy_util_.ConstructSpdyGetSynReply(NULL, 0, 1));
-  scoped_ptr<SpdyFrame> body_no_fin(
-      spdy_util_.ConstructSpdyBodyFrame(
-          1, body_data.data(), body_data.size(), false));
-  scoped_ptr<SpdyFrame> body_fin(
-      spdy_util_.ConstructSpdyBodyFrame(1, NULL, 0, true));
-  MockRead reads[] = {
-    CreateMockRead(*resp),
-    CreateMockRead(*body_no_fin),
-    MockRead(ASYNC, ERR_IO_PENDING, 0),  // Force a pause
-    CreateMockRead(*body_fin),
-    MockRead(ASYNC, ERR_IO_PENDING, 0),  // Force a pause
-    MockRead(ASYNC, 0, 0)  // EOF
-  };
+  reads.push_back(CreateMockRead(*resp));
+
+  ScopedVector<SpdyFrame> body_frames;
+  const std::string body_data(4096, 'x');
+  for (size_t remaining = kTargetSize; remaining != 0;) {
+    size_t frame_size = std::min(remaining, body_data.size());
+    body_frames.push_back(spdy_util_.ConstructSpdyBodyFrame(
+        1, body_data.data(), frame_size, false));
+    reads.push_back(CreateMockRead(*body_frames.back()));
+    remaining -= frame_size;
+  }
+  reads.push_back(MockRead(ASYNC, ERR_IO_PENDING, 0)); // Yield.
 
-  DelayedSocketData data(1, reads, arraysize(reads),
+  DelayedSocketData data(1, vector_as_array(&reads), reads.size(),
                          vector_as_array(&writes), writes.size());
 
   NormalSpdyTransactionHelper helper(CreateGetRequest(), DEFAULT_PRIORITY,
@@ -6092,9 +6097,9 @@ TEST_P(SpdyNetworkTransactionTest, WindowUpdateSent) {
   ASSERT_TRUE(stream != NULL);
   ASSERT_TRUE(stream->stream() != NULL);
 
-  EXPECT_EQ(
-      static_cast<int>(kSpdyStreamInitialWindowSize - body_data.size()),
-      stream->stream()->recv_window_size());
+  // All data has been read, but not consumed. The window reflects this.
+  EXPECT_EQ(static_cast<int>(kSpdyStreamInitialWindowSize - kTargetSize),
+            stream->stream()->recv_window_size());
 
   const HttpResponseInfo* response = trans->GetResponseInfo();
   ASSERT_TRUE(response != NULL);
@@ -6104,22 +6109,15 @@ TEST_P(SpdyNetworkTransactionTest, WindowUpdateSent) {
 
   // Issue a read which will cause a WINDOW_UPDATE to be sent and window
   // size increased to default.
-  scoped_refptr<net::IOBuffer> buf(new net::IOBuffer(body_data.size()));
-  rv = trans->Read(buf.get(), body_data.size(), CompletionCallback());
-  EXPECT_EQ(static_cast<int>(body_data.size()), rv);
-  std::string content(buf->data(), buf->data() + body_data.size());
-  EXPECT_EQ(body_data, content);
-
-  // Schedule the reading of empty data frame with FIN
-  data.CompleteRead();
-
-  // Force write of WINDOW_UPDATE which was scheduled during the above
-  // read.
+  scoped_refptr<net::IOBuffer> buf(new net::IOBuffer(kTargetSize));
+  EXPECT_EQ(static_cast<int>(kTargetSize),
+            trans->Read(buf.get(), kTargetSize, CompletionCallback()));
+  EXPECT_EQ(static_cast<int>(kSpdyStreamInitialWindowSize),
+            stream->stream()->recv_window_size());
+  EXPECT_THAT(base::StringPiece(buf->data(), kTargetSize), Each(Eq('x')));
+
+  // Allow scheduled WINDOW_UPDATE frames to write.
   base::RunLoop().RunUntilIdle();
-
-  // Read EOF.
-  data.CompleteRead();
-
   helper.VerifyDataConsumed();
 }