Introduce STATE_RESERVED_REMOTE. No behavioral changes

Introduce STATE_RESERVED_REMOTE. No behavioral changes

Just to keep changes small.
Divide OnSynStream headers into request/response part.
Request headers are now delivered to stream via
OnPushPromiseHeadersReceived to ensure correct stream state.

BUG=377538
R=jgraettinger@chromium.org

TEST=All current push-related SpdyStream/SpdySession tests

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

[baranovich, 2014-05-31 16:23:12]

BTW, checking pushed headers using GetUrlFromHeaderBlock does not conform the
spec of SPDY3/HTTP2. GURL::is_valid() will be true even if :path is missing.
Should we change it for SPDY3 as well as HTTP2 or some spdy servers may already
rely on this?

[baranovich, 2014-05-31 19:45:18]

bad, I forgot casing/duplication headers checks

[baranovich, 2014-05-31 20:07:39]

On 2014/05/31 19:45:18, baranovich wrote:
> bad, I forgot casing/duplication headers checks
 Umm, no, not really... everything works luckily (headers are std::map=)), but
may be broken for SPDY: for pushed stream, server may send more of
:scheme/:host/:path in the next HEADERS frame, and they will pass, since
MergeWithResponseHeaders() does not check |request_headers_|.
I'm not quite sure, is it OK, or we should treat this as error? Technically,
that does not lead to anything catastrophic, but changes the current logic.
Should I make some additional checks about it?

[Johnny, 2014-06-03 03:38:26]

Hi Alexey,

Wanted to apologies for not getting to this yet. It's in my queue. I'm traveling
at the moment. I should get back to you in the next couple days.

cheers,
-johnny

[Johnny, 2014-06-05 02:56:03]

https://codereview.chromium.org/304353012/diff/40001/net/spdy/spdy_network_tr...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/304353012/diff/40001/net/spdy/spdy_network_tr...
net/spdy/spdy_network_transaction_unittest.cc:5342: // TODO(baranovich): HTTP 2
does not allow multiple HEADERS frames
This isn't quite right. There's no restriction on multiple HEADERS frames in
HTTP/2 that I'm aware of.

They are disallowed by [1], and I think it's fine to leave them as disallowed,
but we should probably avoid adding further places which restrict this or
comment it as such.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_stre...

https://codereview.chromium.org/304353012/diff/40001/net/spdy/spdy_network_tr...
net/spdy/spdy_network_transaction_unittest.cc:5470: // Verify we got all the
headers from both header blocks.
Expose and verify request headers on the transaction? Or add a TODO to do this
later?

https://codereview.chromium.org/304353012/diff/40001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/304353012/diff/40001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:286: static const char* kRequestHeaders[] = {
":scheme", ":host", ":path" };
Also :authority (instead of :host) for HTTP/2.

nit: Use git cl format for things like the extra spaces within {}.

https://codereview.chromium.org/304353012/diff/40001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:294: }
Can we add unit testing on this in SpdySessionTest?

https://codereview.chromium.org/304353012/diff/40001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:2163:
GetPushPromiseHeadersFromSynStream(&response_headers,
Since we're copying here anyway, how about passing in headers as a const
reference, and request & response headers as output arguments?

Consider a name for the method which more concisely conveys it's function
(extracting request vs response headers), rather than it's use (for push-promise
vs syn-stream frames).

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

https://codereview.chromium.org/304353012/diff/40001/net/spdy/spdy_stream.h#n...
net/spdy/spdy_stream.h:302: // Called by the SpdySession when PUSH_PROMISE is
received. This is the
I'm worried about the use of exact frame names in comments & code names, here
and elsewhere. I think that'll confuse readers as to when a literal SYN_STREAM
or PUSH_PROMISE frame is meant, vs when we just mean "a frame carrying request
headers opening a stream".

[baranovich, 2014-06-05 19:16:58]

https://codereview.chromium.org/304353012/diff/40001/net/spdy/spdy_network_tr...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/304353012/diff/40001/net/spdy/spdy_network_tr...
net/spdy/spdy_network_transaction_unittest.cc:5342: // TODO(baranovich): HTTP 2
does not allow multiple HEADERS frames
http://tools.ietf.org/html/draft-ietf-httpbis-http2-12#section-8.1 states that:

An HTTP message (request or response) consists of:

   1.  one HEADERS frame, followed by zero or more CONTINUATION frames
       (containing the message headers; see [HTTP-p1], Section 3.2), and

   2.  zero or more DATA frames (containing the message payload; see
       [HTTP-p1], Section 3.3), and

   3.  optionally, one HEADERS frame, followed by zero or more
       CONTINUATION frames (containing the trailer-part, if present; see
       [HTTP-p1], Section 4.1.2).

And unless we support trailing headers, I think this means that only one HEADERS
frame is allowed.

On 2014/06/05 02:56:04, Johnny wrote:
> This isn't quite right. There's no restriction on multiple HEADERS frames in
> HTTP/2 that I'm aware of.
> 
> They are disallowed by [1], and I think it's fine to leave them as disallowed,
> but we should probably avoid adding further places which restrict this or
> comment it as such.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_stre...

https://codereview.chromium.org/304353012/diff/40001/net/spdy/spdy_network_tr...
net/spdy/spdy_network_transaction_unittest.cc:5470: // Verify we got all the
headers from both header blocks.
I'm not quite sure what and how should be checked then. Request headers are
available for transaction, but these are headers set by the client. We need
something like "server implied request headers", but that would be a kind of
abstraction leak.
Do we really need to check it there?
Technically, we check here that final response has headers from SYN_STREAM and
all HEADERS frames. Facts about correct push stream routing are checked in other
tests and getting 200 OK status line for a given URI implies that everything
worked. For this test, scheme/path/host headers are no more then just headers
imo, so if they're unavailable, we may check some other =) Maybe I'd better add
some headers to |initial_headers| block (which is used for SYN_STREAM
construction) which will go to response section instead?
PS Did that in the patch #4.
On 2014/06/05 02:56:04, Johnny wrote:
> Expose and verify request headers on the transaction? Or add a TODO to do this
> later?

https://codereview.chromium.org/304353012/diff/40001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/304353012/diff/40001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:286: static const char* kRequestHeaders[] = {
":scheme", ":host", ":path" };
On 2014/06/05 02:56:04, Johnny wrote:
> Also :authority (instead of :host) for HTTP/2.
> 
> nit: Use git cl format for things like the extra spaces within {}.

Done.

https://codereview.chromium.org/304353012/diff/40001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:294: }
On 2014/06/05 02:56:04, Johnny wrote:
> Can we add unit testing on this in SpdySessionTest?

Done.

https://codereview.chromium.org/304353012/diff/40001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:2163:
GetPushPromiseHeadersFromSynStream(&response_headers,
On 2014/06/05 02:56:04, Johnny wrote:
> Since we're copying here anyway, how about passing in headers as a const
> reference, and request & response headers as output arguments?
> 
> Consider a name for the method which more concisely conveys it's function
> (extracting request vs response headers), rather than it's use (for
push-promise
> vs syn-stream frames).

Done.
That's the best name I've come up to;) maybe you'll suggest better one, my
english is poor=)

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

https://codereview.chromium.org/304353012/diff/40001/net/spdy/spdy_stream.h#n...
net/spdy/spdy_stream.h:302: // Called by the SpdySession when PUSH_PROMISE is
received. This is the
On 2014/06/05 02:56:04, Johnny wrote:
> I'm worried about the use of exact frame names in comments & code names, here
> and elsewhere. I think that'll confuse readers as to when a literal SYN_STREAM
> or PUSH_PROMISE frame is meant, vs when we just mean "a frame carrying request
> headers opening a stream".

Done.

[Johnny, 2014-06-09 18:44:16]

LGTM. I have nits about a TODO, but that's it :)

> HTTP 2 does not allow multiple HEADERS frames
> http://tools.ietf.org/html/draft-ietf-httpbis-http2-12#section-8.1
> states that:
>
> <snip>
>
> And unless we support trailing headers, I think this means that only one
> HEADERS frame is allowed.

Yep. The distinction is in the HTTP semantic layer, vs HTTP/2 framing layer. In
practical terms, that means that enforcement is ideally restricted to the
SpdyHttpStream delegate, but not other SpdyStream delegates layered upon HTTP/2
framing (eg, SpdyWebSocketStream, or SpdyProxyClientSocketStream), and not in
SpdyStream & SpdySession itself.

See the inline comment. I realize this is all pedantic right now, as
SpdyHttpStream is the only kind of delegate expected to handle push streams.
It's also a bit muddied already, as
SpdyStream::OnAdditionalResponseHeadersRecieved() does some enforcement.

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

https://codereview.chromium.org/304353012/diff/60001/net/spdy/spdy_stream.cc#...
net/spdy/spdy_stream.cc:425: // Set |response_headers_status_| to
RESPONSE_HEADERS_ARE_COMPLETE.
Current behavior is that we'll accept & merge headers without question until a
delegate is set, and a push stream will have response_headers_status_ set to
RESPONSE_HEADERS_ARE_COMPLETE only after a delegate has been set and the
delegate's OnResponseHeadersUpdated() indicated that all response headers were
received.

The problem with trying to set RESPONSE_HEADERS_ARE_COMPLETE beforehand is that
we don't know what kind of delegate will eventually arrive, and we don't know
what "complete" means in the stream context. That's up to the delegate.

[baranovich, 2014-06-10 10:28:07]

The CQ bit was checked by baranovich@yandex-team.ru

[commit-bot: I haz the power, 2014-06-10 10:31:43]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/baranovich@yandex-team.ru/304353012/60001

[commit-bot: I haz the power, 2014-06-10 15:18:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[commit-bot: I haz the power, 2014-06-10 15:26:04]

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

[commit-bot: I haz the power, 2014-06-10 15:26:05]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)

[baranovich, 2014-06-11 09:50:13]

The CQ bit was checked by baranovich@yandex-team.ru

[commit-bot: I haz the power, 2014-06-11 09:52:33]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/baranovich@yandex-team.ru/304353012/80001

[baranovich, 2014-06-11 11:59:52]

The CQ bit was checked by baranovich@yandex-team.ru

[commit-bot: I haz the power, 2014-06-11 12:01:30]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/baranovich@yandex-team.ru/304353012/10...

[baranovich, 2014-06-11 13:00:40]

The CQ bit was checked by baranovich@yandex-team.ru

[commit-bot: I haz the power, 2014-06-11 13:01:43]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/baranovich@yandex-team.ru/304353012/12...

[commit-bot: I haz the power, 2014-06-11 20:59:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 21:18:40]

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

[commit-bot: I haz the power, 2014-06-11 21:18:41]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[baranovich, 2014-06-12 10:02:40]

The CQ bit was checked by baranovich@yandex-team.ru

[commit-bot: I haz the power, 2014-06-12 10:03:57]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/baranovich@yandex-team.ru/304353012/12...

[commit-bot: I haz the power, 2014-06-12 16:15:02]

Change committed as 276698