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

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

Allow response headers up to 256 kB to be processed when speaking HTTP/2
(increased from earlier 32 kB limit).  Instead of allocating a 256 kB buffer at
once, start with a 8 kB buffer and grow exponentially to limit memory usage.

Also increase string literal (header key and value) limit to 256 kB (from
earlier 16 kB limit).

BUG=535629
Committed: https://crrev.com/d6738334bdb481e0c5d0303269c55049a904773e
Cr-Commit-Position: refs/heads/master@{#368442}

[Bence, 2016-01-07 12:08:13]

bnc@chromium.org changed reviewers:
+ rch@chromium.org

[Bence, 2016-01-07 12:08:14]

Ryan: PTAL.  Thanks.

[Ryan Hamilton, 2016-01-08 00:16:30]

https://codereview.chromium.org/1566703002/diff/1/net/spdy/buffered_spdy_fram...
File net/spdy/buffered_spdy_framer.cc (right):

https://codereview.chromium.org/1566703002/diff/1/net/spdy/buffered_spdy_fram...
net/spdy/buffered_spdy_framer.cc:48: header_buffer_.clear();
Does this do anything since the buffer was newly created?

https://codereview.chromium.org/1566703002/diff/1/net/spdy/buffered_spdy_fram...
net/spdy/buffered_spdy_framer.cc:187: size_t new_capacity = std::max(new_size,
kHeaderBufferInitialSize);
Can new_size ever be < kHeaderBufferInitialSize, since it starts at that value
and only increases.

https://codereview.chromium.org/1566703002/diff/1/net/spdy/buffered_spdy_fram...
net/spdy/buffered_spdy_framer.cc:193: return true;
Should we have a unit test of this new behavior?

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

Ryan: PTAL.  Thanks.

https://codereview.chromium.org/1566703002/diff/1/net/spdy/buffered_spdy_fram...
File net/spdy/buffered_spdy_framer.cc (right):

https://codereview.chromium.org/1566703002/diff/1/net/spdy/buffered_spdy_fram...
net/spdy/buffered_spdy_framer.cc:48: header_buffer_.clear();
On 2016/01/08 00:16:29, Ryan Hamilton wrote:
> Does this do anything since the buffer was newly created?

Hm, I guess not.  Thanks for catching it.

https://codereview.chromium.org/1566703002/diff/1/net/spdy/buffered_spdy_fram...
net/spdy/buffered_spdy_framer.cc:187: size_t new_capacity = std::max(new_size,
kHeaderBufferInitialSize);
On 2016/01/08 00:16:29, Ryan Hamilton wrote:
> Can new_size ever be < kHeaderBufferInitialSize, since it starts at that value
> and only increases.

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.

https://codereview.chromium.org/1566703002/diff/1/net/spdy/buffered_spdy_fram...
net/spdy/buffered_spdy_framer.cc:193: return true;
On 2016/01/08 00:16:29, Ryan Hamilton wrote:
> Should we have a unit test of this new behavior?

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.

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

lgtm

https://codereview.chromium.org/1566703002/diff/1/net/spdy/buffered_spdy_fram...
File net/spdy/buffered_spdy_framer.cc (right):

https://codereview.chromium.org/1566703002/diff/1/net/spdy/buffered_spdy_fram...
net/spdy/buffered_spdy_framer.cc:187: size_t new_capacity = std::max(new_size,
kHeaderBufferInitialSize);
On 2016/01/08 18:58:24, Bence wrote:
> On 2016/01/08 00:16:29, Ryan Hamilton wrote:
> > Can new_size ever be < kHeaderBufferInitialSize, since it starts at that
value
> > and only increases.
> 
> 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.

Hah, I missed that. Good point :>

> 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.

I think your approach sounds good. I just missed it. Thanks.

[Bence, 2016-01-08 21:49:32]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2016-01-08 21:50:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1566703002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1566703002/20001

[commit-bot: I haz the power, 2016-01-08 21:58:49]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-08 22:00:56]

Description was changed from

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

Allow response headers up to 256 kB to be processed when speaking HTTP/2
(increased from earlier 32 kB limit).  Instead of allocating a 256 kB buffer at
once, start with a 8 kB buffer and grow exponentially to limit memory usage.

Also increase string literal (header key and value) limit to 256 kB (from
earlier 16 kB limit).

BUG=535629
==========

to

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

Allow response headers up to 256 kB to be processed when speaking HTTP/2
(increased from earlier 32 kB limit).  Instead of allocating a 256 kB buffer at
once, start with a 8 kB buffer and grow exponentially to limit memory usage.

Also increase string literal (header key and value) limit to 256 kB (from
earlier 16 kB limit).

BUG=535629

Committed: https://crrev.com/d6738334bdb481e0c5d0303269c55049a904773e
Cr-Commit-Position: refs/heads/master@{#368442}
==========

[commit-bot: I haz the power, 2016-01-08 22:00:57]

Patchset 2 (id:??) landed as
https://crrev.com/d6738334bdb481e0c5d0303269c55049a904773e
Cr-Commit-Position: refs/heads/master@{#368442}