Disallow multiple HEADERS frames on pushed streams.

Disallow multiple HEADERS frames on pushed streams.

Functional:

* Send RST_STREAM if HEADERS do not contain ":status" on pushed stream.
* Log HTTP2_STREAM_ERROR if response is received before request is sent.
* Log HTTP2_STREAM_ERROR if trailers are received on a pushed stream.
* Log HTTP2_STREAM_ERROR if trailers are received after trailers.
* Log HTTP2_STREAM_ERROR if data are received after trailers.

Refactoring:

* Add plenty of unittests.
* Move all header block checks from SpdySession and from SpdyStream::Delegate to
  SpdyStream.
* Change SpdyStream::Delegate::OnResponseHeadersUpdated type to void.
* Make enum SpdyResponseHeaderStatus private to SpdyStream, rename entries.
* Rename SpdyStream::Delegate::OnResponseHeadersUpdated() to
  OnHeadersReceived(), since update is not allowed.
* Change SpdyHttpStream::response_headers_status_ type to bool, rename.
* Remove tests that rely on the net stack incorrectly accepting invalid
  responses.
* Remove SpdySession::ActiveStreamInfo::waiting_for_reply_headers_frame,
  inline now trivial SpdySession::ActiveStreamInfo struct.
* Rename SpdyStream::Delegate::OnRequestHeadersSent() to OnHeadersSent() to
  match OnData{Received,Sent} methods.
* Make TryCreatePushStream() void as return value was dropped on the floor
  anyway.
* Combine SpdyStream::OnInitialResponseHeadersReceived() and
  OnAdditionalResponseHeadersReceived() into a single OnHeadersReceived()
  method.
* Rename SpdyStream::MergeWithResponseHeaders() to SaveResponseHeaders() as no
  merging is done any more.

BUG=666383
Committed: https://crrev.com/4c21431268a20459813ce06a503300db42d2337f
Cr-Commit-Position: refs/heads/master@{#434669}

[Bence, 2016-11-23 18:46:25]

The CQ bit was checked by bnc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 18:47:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 20:39:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 20:39:11]

Dry run: This issue passed the CQ dry run.

[Bence, 2016-11-23 20:41:57]

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

[Bence, 2016-11-23 20:41:58]

Ryan: PTAL.  Thank you, and have a nice weekend!

[Ryan Hamilton, 2016-11-23 20:58:43]

NICE!

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

https://codereview.chromium.org/2526003002/diff/1/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:325: DCHECK(headers_valid);
Why is this a DCHECK as opposed to an error? If the peer sends us bogus headers,
could we hit this?

https://codereview.chromium.org/2526003002/diff/1/net/spdy/spdy_proxy_client_...
File net/spdy/spdy_proxy_client_socket.cc (right):

https://codereview.chromium.org/2526003002/diff/1/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket.cc:450: DCHECK(headers_valid);
ditto.

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

https://codereview.chromium.org/2526003002/diff/1/net/spdy/spdy_stream.cc#new...
net/spdy/spdy_stream.cc:383: const std::string error("Response headers do not
include :status.");
It's not obvious to me that this check belongs here and not in HTTP-land. What
would happen if this check were removed?

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

https://codereview.chromium.org/2526003002/diff/1/net/spdy/spdy_stream.h#newc...
net/spdy/spdy_stream.h:84: // Called when headers have been received.
This comment is far to easy to understand compared to the previous comment!

:)

https://codereview.chromium.org/2526003002/diff/1/net/spdy/spdy_stream.h#newc...
net/spdy/spdy_stream.h:403: READY_FOR_DATA,
nit: I think this should be renamed to:
READY_FOR_TRAILERS
if it's just the headers status, or
READY_FOR_DATA_OR_TRAILERS
if this enum is about both body and headers (in which case, perhaps the enum
should be renamed).  Also, since this is an inner-calls, I think you can drop
the "Spdy" prefix.

[Bence, 2016-11-23 21:15:59]

PTAL at response to comments.  I'll do the enum rename next week.  Thank you.

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

https://codereview.chromium.org/2526003002/diff/1/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:325: DCHECK(headers_valid);
On 2016/11/23 20:58:43, Ryan Hamilton wrote:
> Why is this a DCHECK as opposed to an error? If the peer sends us bogus
headers,
> could we hit this?

Nope.  SpdyHeadersToHttpResponse() only fails if there is no ":status", and now
SpdyStream checks this before calling OnHeadersReceived.

Also, what kind of error would it be?  Should we generate a RST_STREAM here by
calling back to |stream_|?  Or should this method have a return value?  In the
latter case, would we give up surfacing an error string in NetLog?

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

https://codereview.chromium.org/2526003002/diff/1/net/spdy/spdy_stream.cc#new...
net/spdy/spdy_stream.cc:383: const std::string error("Response headers do not
include :status.");
On 2016/11/23 20:58:43, Ryan Hamilton wrote:
> It's not obvious to me that this check belongs here and not in HTTP-land. What
> would happen if this check were removed?

I agree that this feels more like HTTP layer stuff.  However, advantages for the
check being here:
* This is enforcing stuff that's in the HTTP/2 specs, so it is uniform for all
Delegates.  Moving this logic to each Delegate would result in code duplication.
* SpdyHttpStream can conveniently reset the stream with a useful error string
that's logged in NetLog, Delegate would need some indirection for that.
* Soon when I add support for informational (status = 1xx) headers, it will be
necessary to parse :status in order to distinguish headers from trailers (note
that there does not have to be any DATA frames).  If we refuse to do that in
SpdyHttpStream, then Delegates cannot have separate OnHeaders() and OnTrailers()
calls, and will need to implement this :status logic and keep state themselves
to distinguish headers from trailers

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

https://codereview.chromium.org/2526003002/diff/1/net/spdy/spdy_stream.h#newc...
net/spdy/spdy_stream.h:403: READY_FOR_DATA,
On 2016/11/23 20:58:43, Ryan Hamilton wrote:
> nit: I think this should be renamed to:
> READY_FOR_TRAILERS
> if it's just the headers status, or
> READY_FOR_DATA_OR_TRAILERS
> if this enum is about both body and headers (in which case, perhaps the enum
> should be renamed).  Also, since this is an inner-calls, I think you can drop
> the "Spdy" prefix.

Okay, I'll do this.

[Ryan Hamilton, 2016-11-23 22:02:49]

fair enough. LGTM!

[Bence, 2016-11-28 13:47:47]

Patchset #2 (id:20001) has been deleted

[Bence, 2016-11-28 14:07:42]

The CQ bit was checked by bnc@chromium.org

[Bence, 2016-11-28 14:07:42]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/2526003002/#ps40001
(title: "Rename enum, enum entry, and member.")

[commit-bot: I haz the power, 2016-11-28 14:07:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-28 16:49:20]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480342062360030,
"parent_rev": "b08745c1cde0a13ca535a69c33b77968c1e2d987", "commit_rev":
"d8e7080016ec2c019df5b1440ffed5dcd24aa06c"}

[commit-bot: I haz the power, 2016-11-28 16:49:47]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-11-28 16:54:55]

Description was changed from

==========
Disallow multiple HEADERS frames on pushed streams.

Functional:

* Send RST_STREAM if HEADERS do not contain ":status" on pushed stream.
* Log HTTP2_STREAM_ERROR if response is received before request is sent.
* Log HTTP2_STREAM_ERROR if trailers are received on a pushed stream.
* Log HTTP2_STREAM_ERROR if trailers are received after trailers.
* Log HTTP2_STREAM_ERROR if data are received after trailers.

Refactoring:

* Add plenty of unittests.
* Move all header block checks from SpdySession and from SpdyStream::Delegate to
  SpdyStream.
* Change SpdyStream::Delegate::OnResponseHeadersUpdated type to void.
* Make enum SpdyResponseHeaderStatus private to SpdyStream, rename entries.
* Rename SpdyStream::Delegate::OnResponseHeadersUpdated() to
  OnHeadersReceived(), since update is not allowed.
* Change SpdyHttpStream::response_headers_status_ type to bool, rename.
* Remove tests that rely on the net stack incorrectly accepting invalid
  responses.
* Remove SpdySession::ActiveStreamInfo::waiting_for_reply_headers_frame,
  inline now trivial SpdySession::ActiveStreamInfo struct.
* Rename SpdyStream::Delegate::OnRequestHeadersSent() to OnHeadersSent() to
  match OnData{Received,Sent} methods.
* Make TryCreatePushStream() void as return value was dropped on the floor
  anyway.
* Combine SpdyStream::OnInitialResponseHeadersReceived() and
  OnAdditionalResponseHeadersReceived() into a single OnHeadersReceived()
  method.
* Rename SpdyStream::MergeWithResponseHeaders() to SaveResponseHeaders() as no
  merging is done any more.

BUG=666383
==========

to

==========
Disallow multiple HEADERS frames on pushed streams.

Functional:

* Send RST_STREAM if HEADERS do not contain ":status" on pushed stream.
* Log HTTP2_STREAM_ERROR if response is received before request is sent.
* Log HTTP2_STREAM_ERROR if trailers are received on a pushed stream.
* Log HTTP2_STREAM_ERROR if trailers are received after trailers.
* Log HTTP2_STREAM_ERROR if data are received after trailers.

Refactoring:

* Add plenty of unittests.
* Move all header block checks from SpdySession and from SpdyStream::Delegate to
  SpdyStream.
* Change SpdyStream::Delegate::OnResponseHeadersUpdated type to void.
* Make enum SpdyResponseHeaderStatus private to SpdyStream, rename entries.
* Rename SpdyStream::Delegate::OnResponseHeadersUpdated() to
  OnHeadersReceived(), since update is not allowed.
* Change SpdyHttpStream::response_headers_status_ type to bool, rename.
* Remove tests that rely on the net stack incorrectly accepting invalid
  responses.
* Remove SpdySession::ActiveStreamInfo::waiting_for_reply_headers_frame,
  inline now trivial SpdySession::ActiveStreamInfo struct.
* Rename SpdyStream::Delegate::OnRequestHeadersSent() to OnHeadersSent() to
  match OnData{Received,Sent} methods.
* Make TryCreatePushStream() void as return value was dropped on the floor
  anyway.
* Combine SpdyStream::OnInitialResponseHeadersReceived() and
  OnAdditionalResponseHeadersReceived() into a single OnHeadersReceived()
  method.
* Rename SpdyStream::MergeWithResponseHeaders() to SaveResponseHeaders() as no
  merging is done any more.

BUG=666383

Committed: https://crrev.com/4c21431268a20459813ce06a503300db42d2337f
Cr-Commit-Position: refs/heads/master@{#434669}
==========

[commit-bot: I haz the power, 2016-11-28 16:54:56]

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