Increase HTTP/2 response header buffer limit to 256 kB.

net/spdy/buffered_spdy_framer.cc

Index: net/spdy/buffered_spdy_framer.cc
diff --git a/net/spdy/buffered_spdy_framer.cc b/net/spdy/buffered_spdy_framer.cc
index 2372aff1a56ce65f05df688c7a87bf410fcd66b6..0f90c20b1e23c8a35a37bae5aacd31bea25c63e9 100644
--- a/net/spdy/buffered_spdy_framer.cc
+++ b/net/spdy/buffered_spdy_framer.cc
@@ -13,6 +13,10 @@ namespace {
 // GOAWAY frame debug data is only buffered up to this many bytes.
 size_t kGoAwayDebugDataMaxSize = 1024;
 
+// Initial and maximum sizes for header block buffer.
+size_t kHeaderBufferInitialSize = 8 * 1024;
+size_t kHeaderBufferMaxSize = 256 * 1024;
+
 }  // namespace
 
 SpdyMajorVersion NextProtoToSpdyMajorVersion(NextProto next_proto) {
@@ -37,12 +41,11 @@ BufferedSpdyFramer::BufferedSpdyFramer(SpdyMajorVersion version,
                                        bool enable_compression)
     : spdy_framer_(version),
       visitor_(NULL),
-      header_buffer_used_(0),
       header_buffer_valid_(false),
       header_stream_id_(SpdyFramer::kInvalidStream),
       frames_received_(0) {
   spdy_framer_.set_enable_compression(enable_compression);
-  memset(header_buffer_, 0, sizeof(header_buffer_));
+  header_buffer_.clear();

[Ryan Hamilton, 2016/01/08 00:16:29]

Does this do anything since the buffer was newly created?

[Bence, 2016/01/08 18:58:24]

Hm, I guess not.  Thanks for catching it.

 }
 
 BufferedSpdyFramer::~BufferedSpdyFramer() {
@@ -127,8 +130,8 @@ bool BufferedSpdyFramer::OnControlFrameHeaderData(SpdyStreamId stream_id,
     CHECK(header_buffer_valid_);
 
     SpdyHeaderBlock headers;
-    if (!spdy_framer_.ParseHeaderBlockInBuffer(header_buffer_,
-                                               header_buffer_used_, &headers)) {
+    if (!spdy_framer_.ParseHeaderBlockInBuffer(
+            header_buffer_.data(), header_buffer_.size(), &headers)) {
       visitor_->OnStreamError(
           stream_id, "Could not parse Spdy Control Frame Header.");
       return false;
@@ -171,15 +174,22 @@ bool BufferedSpdyFramer::OnControlFrameHeaderData(SpdyStreamId stream_id,
     return true;
   }
 
-  const size_t available = kHeaderBufferSize - header_buffer_used_;
-  if (len > available) {
+  const size_t new_size = header_buffer_.size() + len;
+  if (new_size > kHeaderBufferMaxSize) {
     header_buffer_valid_ = false;
-    visitor_->OnStreamError(
-        stream_id, "Received more data than the allocated size.");
+    visitor_->OnStreamError(stream_id, "Received too much header data.");
     return false;
   }
-  memcpy(header_buffer_ + header_buffer_used_, header_data, len);
-  header_buffer_used_ += len;
+
+  if (new_size > header_buffer_.capacity()) {
+    // Grow |header_buffer_| exponentially to reduce memory allocations and
+    // copies.
+    size_t new_capacity = std::max(new_size, kHeaderBufferInitialSize);

[Ryan Hamilton, 2016/01/08 00:16:29]

Can new_size ever be < kHeaderBufferInitialSize, since it starts at that value
and only increases.

[Bence, 2016/01/08 18:58:24]

Yes, new_size can be smaller.  This is the only place in the code where I'm
using kHeaderBufferInitialSize, this is where it starts at that value.  So when
this code gets executed for the first time, header_buffer_.size() is zero, so
new_size == len, which can be much smaller than kHeaderBufferInitialSize.

I was also contemplating reserve()-ing kHeaderBufferInitialSize bytes in the
constructor above instead of enforcing the minimum size here, but decided to do
allocations as late as possible instead.  I can still do that if you want me to.
 By the way, what is the maximum value of |len|?  In case this method is called
with len == 16k, then it's useless to reserve 8k in the constructor.

[Ryan Hamilton, 2016/01/08 21:20:46]

Hah, I missed that. Good point :>
I think your approach sounds good. I just missed it. Thanks.

+    new_capacity = std::max(new_capacity, 2 * header_buffer_.capacity());
+    new_capacity = std::min(new_capacity, kHeaderBufferMaxSize);
+    header_buffer_.reserve(new_capacity);
+  }
+  header_buffer_.append(header_data, len);
   return true;

[Ryan Hamilton, 2016/01/08 00:16:29]

Should we have a unit test of this new behavior?

[Bence, 2016/01/08 18:58:24]

The behavior of larger buffer limit?  There is one.  The behavior of exponential
growth of the buffer?  No, that is an implementation detail, I don't believe
there should be a unit test for that.

 }
 
@@ -442,8 +452,7 @@ SpdyPriority BufferedSpdyFramer::GetHighestPriority() const {
 }
 
 void BufferedSpdyFramer::InitHeaderStreaming(SpdyStreamId stream_id) {
-  memset(header_buffer_, 0, kHeaderBufferSize);
-  header_buffer_used_ = 0;
+  header_buffer_.clear();
   header_buffer_valid_ = true;
   header_stream_id_ = stream_id;
   DCHECK_NE(header_stream_id_, SpdyFramer::kInvalidStream);