[SPDY] Replace SpdyIOBuffer with new SpdyBuffer class

[SPDY] Replace SpdyIOBuffer with new SpdyBuffer class

Use SpdyBuffer for both SPDY reads and writes. A future
CL will add hooks to SpdyBuffer so that we keep track of
flow control windows properly.

Replace SpdyFrameProducer with SpdyBufferProducer.

Also introduce new SpdyReadQueue class for delegates
of SpdyStream to use.

BUG=176592
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194560

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194564

[akalin, 2013-04-16 06:06:13]

+rch for review

[Ryan Hamilton, 2013-04-16 18:44:25]

looks good, just a few small comments.

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_buffer.h
File net/spdy/spdy_buffer.h (right):

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_buffer.h#newcode20
net/spdy/spdy_buffer.h:20: // SPDY connection. It is similar to a
DrainableIOBuffer.
nit: Can you add a comment which explains why it is different (and why it is not
an IOBuffer subclass)?

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_buffer.h#newcode42
net/spdy/spdy_buffer.h:42: // TODO(akalin): Add a way to get called back when
Consume() is
Who should be called back when Consume is called?  The session, I guess?

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_buffer.h#newcode50
net/spdy/spdy_buffer.h:50: IOBuffer* GetIOBufferForRemainingData();
Interesting.  IOBuffers are refcounted, right?  It seems dangerous to stick a
refcounted buffer on top of a non-refcounted buffer.

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_http_stream.cc#...
net/spdy/spdy_http_stream.cc:139: size_t bytes_consumed =
response_body_queue_.Dequeue(buf->data(), buf_len);
nice cleanup

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_io_buffer.h
File net/spdy/spdy_io_buffer.h (left):

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_io_buffer.h#old...
net/spdy/spdy_io_buffer.h:47: const scoped_refptr<SpdyStream>& stream() const {
return stream_; }
Interesting.  Because this class keeps around a ref_ptr to the stream, we ensure
that the stream is not destroyed until the buffer is destroyed (and probably
consumed).

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_stream.cc#newco...
net/spdy/spdy_stream.cc:489: if (buffer) {
Looks like you return on line 486 if !buffer, so no need to check if (buffer)
again, right?

[akalin, 2013-04-16 23:01:46]

PTAL

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_buffer.h
File net/spdy/spdy_buffer.h (right):

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_buffer.h#newcode20
net/spdy/spdy_buffer.h:20: // SPDY connection. It is similar to a
DrainableIOBuffer.
On 2013/04/16 18:44:25, Ryan Hamilton wrote:
> nit: Can you add a comment which explains why it is different (and why it is
not
> an IOBuffer subclass)?

Done.

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_buffer.h#newcode42
net/spdy/spdy_buffer.h:42: // TODO(akalin): Add a way to get called back when
Consume() is
On 2013/04/16 18:44:25, Ryan Hamilton wrote:
> Who should be called back when Consume is called?  The session, I guess?

The session and the stream. CL coming!

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_buffer.h#newcode50
net/spdy/spdy_buffer.h:50: IOBuffer* GetIOBufferForRemainingData();
On 2013/04/16 18:44:25, Ryan Hamilton wrote:
> Interesting.  IOBuffers are refcounted, right?  It seems dangerous to stick a
> refcounted buffer on top of a non-refcounted buffer.

It is, but it's unavoidable. :/ It's only used in one place; the call to
socket->Write() in SpdySession. We keep around the buffer (in
in_flight_write_buffer_) until OnWriteComplete is called, so I think it's safe
enough to assume that the socket doesn't hold onto the IOBuffer past that.

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_io_buffer.h
File net/spdy/spdy_io_buffer.h (left):

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_io_buffer.h#old...
net/spdy/spdy_io_buffer.h:47: const scoped_refptr<SpdyStream>& stream() const {
return stream_; }
On 2013/04/16 18:44:25, Ryan Hamilton wrote:
> Interesting.  Because this class keeps around a ref_ptr to the stream, we
ensure
> that the stream is not destroyed until the buffer is destroyed (and probably
> consumed).

Yeah. I think it's better to move the stream out to be a member of SpdySession,
though. For example, we should really clear the in-flight write and its stream
if we delete the stream, but we don't (and it's not obvious because it's hidden
in this class).

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_stream.cc#newco...
net/spdy/spdy_stream.cc:489: if (buffer) {
On 2013/04/16 18:44:25, Ryan Hamilton wrote:
> Looks like you return on line 486 if !buffer, so no need to check if (buffer)
> again, right?

Done.

[Ryan Hamilton, 2013-04-16 23:34:11]

lgtm

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_buffer.h
File net/spdy/spdy_buffer.h (right):

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_buffer.h#newcode50
net/spdy/spdy_buffer.h:50: IOBuffer* GetIOBufferForRemainingData();
On 2013/04/16 23:01:46, akalin wrote:
> On 2013/04/16 18:44:25, Ryan Hamilton wrote:
> > Interesting.  IOBuffers are refcounted, right?  It seems dangerous to stick
a
> > refcounted buffer on top of a non-refcounted buffer.
> 
> It is, but it's unavoidable. :/ It's only used in one place; the call to
> socket->Write() in SpdySession. We keep around the buffer (in
> in_flight_write_buffer_) until OnWriteComplete is called, so I think it's safe
> enough to assume that the socket doesn't hold onto the IOBuffer past that.

Sounds good.  Can you add a comment in SpdySession where
GetIOBufferForRemainingData is called that make it clear how long the buffer
needs to live?

[akalin, 2013-04-16 23:48:05]

On 2013/04/16 23:34:11, Ryan Hamilton wrote:
> lgtm
> 
> https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_buffer.h
> File net/spdy/spdy_buffer.h (right):
> 
>
https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_buffer.h#newcode50
> net/spdy/spdy_buffer.h:50: IOBuffer* GetIOBufferForRemainingData();
> On 2013/04/16 23:01:46, akalin wrote:
> > On 2013/04/16 18:44:25, Ryan Hamilton wrote:
> > > Interesting.  IOBuffers are refcounted, right?  It seems dangerous to
stick
> a
> > > refcounted buffer on top of a non-refcounted buffer.
> > 
> > It is, but it's unavoidable. :/ It's only used in one place; the call to
> > socket->Write() in SpdySession. We keep around the buffer (in
> > in_flight_write_buffer_) until OnWriteComplete is called, so I think it's
safe
> > enough to assume that the socket doesn't hold onto the IOBuffer past that.
> 
> Sounds good.  Can you add a comment in SpdySession where
> GetIOBufferForRemainingData is called that make it clear how long the buffer
> needs to live?

Done.

[akalin, 2013-04-16 23:48:15]

Committing as soon as trybots pass

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_io_buffer.h
File net/spdy/spdy_io_buffer.h (left):

https://codereview.chromium.org/13990005/diff/1/net/spdy/spdy_io_buffer.h#old...
net/spdy/spdy_io_buffer.h:47: const scoped_refptr<SpdyStream>& stream() const {
return stream_; }
On 2013/04/16 23:01:46, akalin wrote:
> On 2013/04/16 18:44:25, Ryan Hamilton wrote:
> > Interesting.  Because this class keeps around a ref_ptr to the stream, we
> ensure
> > that the stream is not destroyed until the buffer is destroyed (and probably
> > consumed).
> 
> Yeah. I think it's better to move the stream out to be a member of
SpdySession,
> though. For example, we should really clear the in-flight write and its stream
> if we delete the stream, but we don't (and it's not obvious because it's
hidden
> in this class).

Actually, per our discussion about GetIOBufferForRemainingData(), we need to
clear the in-flight stream but *not* the SpdyBuffer object itself. Tricky!

[akalin, 2013-04-17 04:10:59]

The sync_integration_test failures are legit, which is strange since the test
sync server doesn't use SPDY, but it looks like the memory stomper is coming
from safe browsing, which does use SPDY. (Although it's surprising that the sync
integration tests call out to Google services.)

Looks like there is indeed a memory stomper in the new read queue that the tests
didn't catch.  Looking...

[akalin, 2013-04-17 05:10:24]

Fixed memory stomper, committing after trybots

[akalin, 2013-04-17 09:59:42]

Committed patchset #5 manually as r194560 (presubmit successful).

[akalin, 2013-04-17 10:38:48]

Committed patchset #6 manually as r194564 (presubmit successful).