Add a new SpdyStream::Delegate method to handle trailers

Add a new SpdyStream::Delegate method to handle trailers

This CL adds a new SpdyStream::Delegate method to surface trailers to
Delegate's implementations. Before this CL, receiving trailers will trigger
a spdy protocol error. However, SpdyHttpStream does not implement
this delegate method as of this CL, so trailers are ignored.

BUG=481033
Committed: https://crrev.com/294da720bb313d48fdba567f192850c9b36b0650
Cr-Commit-Position: refs/heads/master@{#342859}

[xunjieli, 2015-08-05 23:25:19]

Patchset #1 (id:1) has been deleted

[Bence, 2015-08-06 15:02:40]

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

[Bence, 2015-08-06 15:02:41]

I think this in the right direction.  Thank you.

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

https://codereview.chromium.org/1272283003/diff/20001/net/spdy/spdy_stream.cc...
net/spdy/spdy_stream.cc:454: } else if (type_ == SPDY_PUSH_STREAM &&
Once you are here, you might want to consider removing "else" to change to the
early return style so prevalent in net/.

https://codereview.chromium.org/1272283003/diff/20001/net/spdy/spdy_stream_un...
File net/spdy/spdy_stream_unittest.cc (right):

https://codereview.chromium.org/1272283003/diff/20001/net/spdy/spdy_stream_un...
net/spdy/spdy_stream_unittest.cc:178: // Delegate that receives trailers.
Please confirm (at least using a local build) that this test fails without the
rest of the CL.  You can also consider adding the following comment:
// Regression test for https://crbug.com/481033.

[xunjieli, 2015-08-06 16:10:13]

Thanks for the review! PTAL.

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

https://codereview.chromium.org/1272283003/diff/20001/net/spdy/spdy_stream.cc...
net/spdy/spdy_stream.cc:454: } else if (type_ == SPDY_PUSH_STREAM &&
On 2015/08/06 15:02:41, Bence wrote:
> Once you are here, you might want to consider removing "else" to change to the
> early return style so prevalent in net/.

Done.

https://codereview.chromium.org/1272283003/diff/20001/net/spdy/spdy_stream_un...
File net/spdy/spdy_stream_unittest.cc (right):

https://codereview.chromium.org/1272283003/diff/20001/net/spdy/spdy_stream_un...
net/spdy/spdy_stream_unittest.cc:178: // Delegate that receives trailers.
On 2015/08/06 15:02:41, Bence wrote:
> Please confirm (at least using a local build) that this test fails without the
> rest of the CL.  You can also consider adding the following comment:
> // Regression test for https://crbug.com/481033.

Confirmed that the test failes without the rest of the CL with a
ERR_SPDY_PROTOCOL_ERROR reported in
SpdyStream::OnAdditionalResponseHeadersReceived. I also tested against the
nghttpd server mentioned in the bug tracker.

[Bence, 2015-08-06 18:26:25]

LGTM.

Note that you'll need l-g-t-m from an owner.

https://codereview.chromium.org/1272283003/diff/40001/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (right):

https://codereview.chromium.org/1272283003/diff/40001/net/spdy/spdy_stream.h#...
net/spdy/spdy_stream.h:153: // have END_STREAM flag set, so this will be follwed
by OnDataReceived with
Nit: s/follwed/followed/.

https://codereview.chromium.org/1272283003/diff/40001/net/spdy/spdy_test_util...
File net/spdy/spdy_test_util_common.cc (right):

https://codereview.chromium.org/1272283003/diff/40001/net/spdy/spdy_test_util...
net/spdy/spdy_test_util_common.cc:1110:
spdy_headers.mutable_name_value_block());
You'll need to rebase on https://crrev.com/1263803002 and change this to
spdy_headers.mutable_header_block().

[xunjieli, 2015-08-06 21:02:00]

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

[xunjieli, 2015-08-06 21:02:00]

+rch
Ryan, could you take a look?

Thanks for the review! I ran all the tests, and realized that we also need to
reset the stream if we receive DATA after trailers. I added that check. Now all
tests are passing. 
Btw, you should probably add yourself to net/OWNERS. I don't see why you
shouldn't be there :)

https://codereview.chromium.org/1272283003/diff/40001/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (right):

https://codereview.chromium.org/1272283003/diff/40001/net/spdy/spdy_stream.h#...
net/spdy/spdy_stream.h:153: // have END_STREAM flag set, so this will be follwed
by OnDataReceived with
On 2015/08/06 18:26:25, Bence wrote:
> Nit: s/follwed/followed/.

Done.

https://codereview.chromium.org/1272283003/diff/40001/net/spdy/spdy_test_util...
File net/spdy/spdy_test_util_common.cc (right):

https://codereview.chromium.org/1272283003/diff/40001/net/spdy/spdy_test_util...
net/spdy/spdy_test_util_common.cc:1110:
spdy_headers.mutable_name_value_block());
On 2015/08/06 18:26:25, Bence wrote:
> You'll need to rebase on https://crrev.com/1263803002 and change this to
> spdy_headers.mutable_header_block().

Done.

https://codereview.chromium.org/1272283003/diff/60001/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (right):

https://codereview.chromium.org/1272283003/diff/60001/net/spdy/spdy_stream.h#...
net/spdy/spdy_stream.h:153: // have END_STREAM flag set, so this will be
followed by OnDataReceived with
Sorry this comment isn't quite right. SpdyStream::OnDataReceived will be invoked
with an empty buffer, but SpdyStream::Delegate::OnDataReceived won't be invoked.
I updated it and also moved the method above OnClose to match the execution
flow.

[xunjieli, 2015-08-10 14:56:24]

Pinging rch@.

[Ryan Hamilton, 2015-08-10 17:23:25]

Thanks for doing this! Sorry for the delay. We had a CAM+MTV quic team meeting
last week and I kinda dropped of the face of the earth.

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_network_t...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:5470:
TEST_P(SpdyNetworkTransactionTest, SynReplyWithLateHeaders) {
Can you add a comment which explains what this test is testing for? In
particular, I think the idea is that trailers are now supported so this should
not be an error. As such, it seems like it would be good if the stream was not
RST but was instead closed normally. Is there a way to make that happen?

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

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_stream.cc...
net/spdy/spdy_stream.cc:497: "Data received after trailers");
Should this only be checked for SPDY_REQUEST_RESPONSE_STREAM?  Also, why do we
need to check buffer? Even an empty data frame seems like it would violate
8.1...

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (right):

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_stream.h#...
net/spdy/spdy_stream.h:57: // whether the current response headers are complete
or not.
Please update this comment to reflect the meaning of the new TRAILERS_RECEIVED
state. Also, it would be great to explain here (or elsewhere) when it's possible
to received HEADERS. It looks like only before any DATA or after all DATA. Is
that right?

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_stream.h#...
net/spdy/spdy_stream.h:145: virtual void OnTrailers(const SpdyHeaderBlock&
trailers);
Is it required by the spec that trailers have the END_STREAM flag set, or is
this convention? Looks like section 8.1 of the HTTP/2 RFC requires END_STREAM be
set?

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_stream.h#...
net/spdy/spdy_stream.h:145: virtual void OnTrailers(const SpdyHeaderBlock&
trailers);
I think this should be = 0 (pure virtual), shouldn't it?

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_stream_un...
File net/spdy/spdy_stream_unittest.cc (right):

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_stream_un...
net/spdy/spdy_stream_unittest.cc:191: const SpdyHeaderBlock& GetTrailers() {
return trailers_; }
nit: the style guide would suggest this be called trailers(). Might as well also
be a const method.

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_stream_un...
net/spdy/spdy_stream_unittest.cc:252: EXPECT_EQ(ERR_CONNECTION_CLOSED,
delegate.WaitForClose());
If the last IO state is the read of EOF, it would be great to run for 1 fewer
steps so that the following blocks of checks happen while the connection is
still alive. Would that be easy?

[xunjieli, 2015-08-10 20:31:07]

Thanks for the review! I wasn't sure if you saw the email coz the original CL
title has the word "TEMP" in it and rietveld didn't change it when sending out
the email :)

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_network_t...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:5470:
TEST_P(SpdyNetworkTransactionTest, SynReplyWithLateHeaders) {
On 2015/08/10 17:23:25, Ryan Hamilton wrote:
> Can you add a comment which explains what this test is testing for? In
> particular, I think the idea is that trailers are now supported so this should
> not be an error. As such, it seems like it would be good if the stream was not
> RST but was instead closed normally. Is there a way to make that happen?

Done. This test sends data again after sending late headers. From the RFC,
trailers must not be followed by data, that's why we need to RST this stream
with error. I added comment.

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

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_stream.cc...
net/spdy/spdy_stream.cc:497: "Data received after trailers");
On 2015/08/10 17:23:25, Ryan Hamilton wrote:
> Should this only be checked for SPDY_REQUEST_RESPONSE_STREAM? 
I added a DCHECK. Since TRAILERS_RECEIVED is only used for type
SPDY_REQUEST_RESPONSE_STREAM. DCHECKing here seems a bit safer. Let me know if
you'd like me to do a if conditional instead.

> Also, why do we
> need to check buffer? Even an empty data frame seems like it would violate
> 8.1...

When END_STREAM flag is set, SpdyStream::OnDataReceived is invoked with an empty
buffer, even though END_STREAM isn't in a DATA frame. The name of the method is
quite misleading unfortunately :\

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (right):

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_stream.h#...
net/spdy/spdy_stream.h:57: // whether the current response headers are complete
or not.
On 2015/08/10 17:23:25, Ryan Hamilton wrote:
> Please update this comment to reflect the meaning of the new TRAILERS_RECEIVED
> state. Also, it would be great to explain here (or elsewhere) when it's
possible
> to received HEADERS. It looks like only before any DATA or after all DATA. Is
> that right?

Done. Good catch! I also updated comments below.

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_stream.h#...
net/spdy/spdy_stream.h:145: virtual void OnTrailers(const SpdyHeaderBlock&
trailers);
On 2015/08/10 17:23:25, Ryan Hamilton wrote:
> Is it required by the spec that trailers have the END_STREAM flag set, or is
> this convention? Looks like section 8.1 of the HTTP/2 RFC requires END_STREAM
be
> set?

The RFC says "The HEADERS frame starting the trailers header block has the
END_STREAM flag set." I think it is required that trailers header block has
END_STREAM flag. I edited the comment to make it clearer.

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_stream.h#...
net/spdy/spdy_stream.h:145: virtual void OnTrailers(const SpdyHeaderBlock&
trailers);
On 2015/08/10 17:23:25, Ryan Hamilton wrote:
> I think this should be = 0 (pure virtual), shouldn't it?

Done. I was not sure if all implementations should care about trailers.

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_stream_un...
File net/spdy/spdy_stream_unittest.cc (right):

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_stream_un...
net/spdy/spdy_stream_unittest.cc:191: const SpdyHeaderBlock& GetTrailers() {
return trailers_; }
On 2015/08/10 17:23:25, Ryan Hamilton wrote:
> nit: the style guide would suggest this be called trailers(). Might as well
also
> be a const method.

Done.

[Ryan Hamilton, 2015-08-10 22:37:39]

Looks great. Just a few minor comments.

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_network_t...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:5470:
TEST_P(SpdyNetworkTransactionTest, SynReplyWithLateHeaders) {
On 2015/08/10 20:31:06, xunjieli wrote:
> On 2015/08/10 17:23:25, Ryan Hamilton wrote:
> > Can you add a comment which explains what this test is testing for? In
> > particular, I think the idea is that trailers are now supported so this
should
> > not be an error. As such, it seems like it would be good if the stream was
not
> > RST but was instead closed normally. Is there a way to make that happen?
> 
> Done. This test sends data again after sending late headers. From the RFC,
> trailers must not be followed by data, that's why we need to RST this stream
> with error. I added comment.

Ah, I missed that. Cool, thanks! 

Perhaps that name should be changed from "WithLateHeaders" to
"DataAfterTrailers" or some such?

https://codereview.chromium.org/1272283003/diff/100001/net/spdy/spdy_http_str...
File net/spdy/spdy_http_stream.cc (right):

https://codereview.chromium.org/1272283003/diff/100001/net/spdy/spdy_http_str...
net/spdy/spdy_http_stream.cc:368: void SpdyHttpStream::OnTrailers(const
SpdyHeaderBlock& trailers) {}
nit: Can you add a TODO here to do something with them? Does chrome support
trailers for normal HTTP/1 responses? I assume so, but maybe not?

https://codereview.chromium.org/1272283003/diff/100001/net/spdy/spdy_proxy_cl...
File net/spdy/spdy_proxy_client_socket.cc (right):

https://codereview.chromium.org/1272283003/diff/100001/net/spdy/spdy_proxy_cl...
net/spdy/spdy_proxy_client_socket.cc:499: void
SpdyProxyClientSocket::OnTrailers(const SpdyHeaderBlock& trailers) {}
Ignoring headers is fine, though you could also close the connection with some
sort of error, if you wish. I can't imagine trailers make any sense for a
CONNECT tunnel.

https://codereview.chromium.org/1272283003/diff/100001/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (right):

https://codereview.chromium.org/1272283003/diff/100001/net/spdy/spdy_stream.h...
net/spdy/spdy_stream.h:60: // response headers are complete.
It looks like (from comments in the .cc file) that the TRAILERS_RECEIVED state
is only for REQUEST_RESPONSE sockets. Can you mention that here too?

[xunjieli, 2015-08-11 15:07:14]

Thanks for the reviews! PTAL.

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_network_t...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1272283003/diff/80001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:5470:
TEST_P(SpdyNetworkTransactionTest, SynReplyWithLateHeaders) {
On 2015/08/10 22:37:38, Ryan Hamilton wrote:
> On 2015/08/10 20:31:06, xunjieli wrote:
> > On 2015/08/10 17:23:25, Ryan Hamilton wrote:
> > > Can you add a comment which explains what this test is testing for? In
> > > particular, I think the idea is that trailers are now supported so this
> should
> > > not be an error. As such, it seems like it would be good if the stream was
> not
> > > RST but was instead closed normally. Is there a way to make that happen?
> > 
> > Done. This test sends data again after sending late headers. From the RFC,
> > trailers must not be followed by data, that's why we need to RST this stream
> > with error. I added comment.
> 
> Ah, I missed that. Cool, thanks! 
> 
> Perhaps that name should be changed from "WithLateHeaders" to
> "DataAfterTrailers" or some such?

Done. Good idea. Thanks!

https://codereview.chromium.org/1272283003/diff/100001/net/spdy/spdy_http_str...
File net/spdy/spdy_http_stream.cc (right):

https://codereview.chromium.org/1272283003/diff/100001/net/spdy/spdy_http_str...
net/spdy/spdy_http_stream.cc:368: void SpdyHttpStream::OnTrailers(const
SpdyHeaderBlock& trailers) {}
On 2015/08/10 22:37:38, Ryan Hamilton wrote:
> nit: Can you add a TODO here to do something with them? Does chrome support
> trailers for normal HTTP/1 responses? I assume so, but maybe not?

Done. Good idea! I added the bug number as well. According to crbug.com/422958,
Chrome does not support trailers for HTTP/1.

https://codereview.chromium.org/1272283003/diff/100001/net/spdy/spdy_proxy_cl...
File net/spdy/spdy_proxy_client_socket.cc (right):

https://codereview.chromium.org/1272283003/diff/100001/net/spdy/spdy_proxy_cl...
net/spdy/spdy_proxy_client_socket.cc:499: void
SpdyProxyClientSocket::OnTrailers(const SpdyHeaderBlock& trailers) {}
On 2015/08/10 22:37:38, Ryan Hamilton wrote:
> Ignoring headers is fine, though you could also close the connection with some
> sort of error, if you wish. I can't imagine trailers make any sense for a
> CONNECT tunnel.

I added a comment. I am not sure how to close the connection with an error.
There's a Disconnect() method, but it only resets the stream with
STATUS_CODE_CANCEL. Should we do that here?

https://codereview.chromium.org/1272283003/diff/100001/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (right):

https://codereview.chromium.org/1272283003/diff/100001/net/spdy/spdy_stream.h...
net/spdy/spdy_stream.h:60: // response headers are complete.
On 2015/08/10 22:37:38, Ryan Hamilton wrote:
> It looks like (from comments in the .cc file) that the TRAILERS_RECEIVED state
> is only for REQUEST_RESPONSE sockets. Can you mention that here too?

Done.

[Ryan Hamilton, 2015-08-11 16:30:10]

lgtm

thanks for doing this!

https://codereview.chromium.org/1272283003/diff/100001/net/spdy/spdy_proxy_cl...
File net/spdy/spdy_proxy_client_socket.cc (right):

https://codereview.chromium.org/1272283003/diff/100001/net/spdy/spdy_proxy_cl...
net/spdy/spdy_proxy_client_socket.cc:499: void
SpdyProxyClientSocket::OnTrailers(const SpdyHeaderBlock& trailers) {}
On 2015/08/11 15:07:14, xunjieli wrote:
> On 2015/08/10 22:37:38, Ryan Hamilton wrote:
> > Ignoring headers is fine, though you could also close the connection with
some
> > sort of error, if you wish. I can't imagine trailers make any sense for a
> > CONNECT tunnel.
> 
> I added a comment. I am not sure how to close the connection with an error.
> There's a Disconnect() method, but it only resets the stream with
> STATUS_CODE_CANCEL. Should we do that here? 

Hm. Good question :> Perhaps leaving it alone is fine. Now that I think about
it, I don't that the proxy client socket's SpdyStream is a
REQUEST_RESPONSE_STREAM, so perhaps you can just put NOTREACHED() here, but
OnTrailers() will never be called?

[xunjieli, 2015-08-11 17:58:12]

Thanks for the feedback!

https://codereview.chromium.org/1272283003/diff/100001/net/spdy/spdy_proxy_cl...
File net/spdy/spdy_proxy_client_socket.cc (right):

https://codereview.chromium.org/1272283003/diff/100001/net/spdy/spdy_proxy_cl...
net/spdy/spdy_proxy_client_socket.cc:499: void
SpdyProxyClientSocket::OnTrailers(const SpdyHeaderBlock& trailers) {}
On 2015/08/11 16:30:09, Ryan Hamilton wrote:
> On 2015/08/11 15:07:14, xunjieli wrote:
> > On 2015/08/10 22:37:38, Ryan Hamilton wrote:
> > > Ignoring headers is fine, though you could also close the connection with
> some
> > > sort of error, if you wish. I can't imagine trailers make any sense for a
> > > CONNECT tunnel.
> > 
> > I added a comment. I am not sure how to close the connection with an error.
> > There's a Disconnect() method, but it only resets the stream with
> > STATUS_CODE_CANCEL. Should we do that here? 
> 
> Hm. Good question :> Perhaps leaving it alone is fine. Now that I think about
> it, I don't that the proxy client socket's SpdyStream is a
> REQUEST_RESPONSE_STREAM, so perhaps you can just put NOTREACHED() here, but
> OnTrailers() will never be called?

Done. You are right -- the proxy client socket's SpdyStream is of type
SPDY_BIDIRECTIONAL_STREAM, so trailers will be combined with response headers.
NOTREACHED here makes sense.

[xunjieli, 2015-08-11 17:59:19]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2015-08-11 17:59:20]

The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org,
rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1272283003/#ps140001
(title: "Add NOTREACHED")

[commit-bot: I haz the power, 2015-08-11 17:59:56]

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

[commit-bot: I haz the power, 2015-08-11 19:15:11]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-08-11 19:15:55]

Patchset 7 (id:??) landed as
https://crrev.com/294da720bb313d48fdba567f192850c9b36b0650
Cr-Commit-Position: refs/heads/master@{#342859}