Ignore 1xx informational headers.

Ignore 1xx informational headers.

Also correct response headers in a number of pre-existing unittests:
status text is not allowed in HTTP/2.

BUG=662197
Committed: https://crrev.com/c9f762a0a84c859d1e57352637d4a06e1b2b4081
Cr-Commit-Position: refs/heads/master@{#436701}

[Bence, 2016-12-06 01:21:10]

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

[commit-bot: I haz the power, 2016-12-06 01:21:30]

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-12-06 02:21:03]

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

[commit-bot: I haz the power, 2016-12-06 02:21:06]

Dry run: This issue passed the CQ dry run.

[Bence, 2016-12-06 13:11:44]

Description was changed from

==========
Ignore 1xx informational headers.

BUG=662197
==========

to

==========
Ignore 1xx informational headers.

Also correct response headers in a number of pre-existing unittests:
status text is not allowed in HTTP/2.

BUG=662197
==========

[Bence, 2016-12-06 13:11:59]

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

[Bence, 2016-12-06 13:12:00]

Ryan: PTAL.  Thank you.

[Ryan Hamilton, 2016-12-06 20:02:18]

lgtm

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

https://codereview.chromium.org/2555563003/diff/1/net/spdy/spdy_stream.cc#new...
net/spdy/spdy_stream.cc:394: const std::string error("Cannot parse :status.");
I wonder if there are any servers spitting out old spdy-style, aka http/1, style
status headers. Do we already require this in SpdyHttpStream (or somewhere
else?)

https://codereview.chromium.org/2555563003/diff/1/net/spdy/spdy_stream.cc#new...
net/spdy/spdy_stream.cc:405: }
This is surprisingly clean. nice.

[Bence, 2016-12-06 20:10:03]

The CQ bit was checked by bnc@chromium.org

[Bence, 2016-12-06 20:10:13]

Thank you.

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

https://codereview.chromium.org/2555563003/diff/1/net/spdy/spdy_stream.cc#new...
net/spdy/spdy_stream.cc:394: const std::string error("Cannot parse :status.");
On 2016/12/06 20:02:18, Ryan Hamilton wrote:
> I wonder if there are any servers spitting out old spdy-style, aka http/1,
style
> status headers. Do we already require this in SpdyHttpStream (or somewhere
> else?)

No, we do not.  That's why our unittests producing these incorrect headers did
not fail before this CL.

[commit-bot: I haz the power, 2016-12-06 20:10:31]

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-12-06 20:38:27]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1481055003979680, "parent_rev":
"a4a0c1a915abc1af955e171aa92cb0a10a416f9e", "commit_rev":
"3baf9197efe8658d95cd461ca946ff953eeb87b2"}

[commit-bot: I haz the power, 2016-12-06 20:38:56]

Description was changed from

==========
Ignore 1xx informational headers.

Also correct response headers in a number of pre-existing unittests:
status text is not allowed in HTTP/2.

BUG=662197
==========

to

==========
Ignore 1xx informational headers.

Also correct response headers in a number of pre-existing unittests:
status text is not allowed in HTTP/2.

BUG=662197
==========

[commit-bot: I haz the power, 2016-12-06 20:38:57]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-12-06 20:40:35]

Description was changed from

==========
Ignore 1xx informational headers.

Also correct response headers in a number of pre-existing unittests:
status text is not allowed in HTTP/2.

BUG=662197
==========

to

==========
Ignore 1xx informational headers.

Also correct response headers in a number of pre-existing unittests:
status text is not allowed in HTTP/2.

BUG=662197

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

[commit-bot: I haz the power, 2016-12-06 20:40:35]

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

[Ryan Hamilton, 2016-12-06 23:48:14]

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

https://codereview.chromium.org/2555563003/diff/1/net/spdy/spdy_stream.cc#new...
net/spdy/spdy_stream.cc:394: const std::string error("Cannot parse :status.");
On 2016/12/06 20:10:13, Bence wrote:
> On 2016/12/06 20:02:18, Ryan Hamilton wrote:
> > I wonder if there are any servers spitting out old spdy-style, aka http/1,
> style
> > status headers. Do we already require this in SpdyHttpStream (or somewhere
> > else?)
> 
> No, we do not.  That's why our unittests producing these incorrect headers did
> not fail before this CL.

So if there are servers out there which return "200 OK", then Chrome will stop
working to them, right?

[Bence, 2016-12-07 00:22:12]

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

https://codereview.chromium.org/2555563003/diff/1/net/spdy/spdy_stream.cc#new...
net/spdy/spdy_stream.cc:394: const std::string error("Cannot parse :status.");
On 2016/12/06 23:48:14, Ryan Hamilton wrote:
> On 2016/12/06 20:10:13, Bence wrote:
> > On 2016/12/06 20:02:18, Ryan Hamilton wrote:
> > > I wonder if there are any servers spitting out old spdy-style, aka http/1,
> > style
> > > status headers. Do we already require this in SpdyHttpStream (or somewhere
> > > else?)
> > 
> > No, we do not.  That's why our unittests producing these incorrect headers
did
> > not fail before this CL.
> 
> So if there are servers out there which return "200 OK", then Chrome will stop
> working to them, right?

That is correct, and this is a functional change introduced by this CL.  8.1.2.4
of the HTTP/2 spec says ":status pseudo-header field is defined that carries the
HTTP status code field [...] HTTP/2 does not define a way to carry the version
or reason phrase that is included in an HTTP/1.1 status line".  That is, such a
server would not be in compliance with the spec.

We can (a) leave it as is, and still be able to relax the requirement before
branch point if users see this in the wild, or (b) relax the requirement and not
worry about it, or (c) relax the requirement, gather UMA data, and get back to
this later.  WDYT?

[Ryan Hamilton, 2016-12-07 01:26:09]

On 2016/12/07 00:22:12, Bence wrote:
> https://codereview.chromium.org/2555563003/diff/1/net/spdy/spdy_stream.cc
> File net/spdy/spdy_stream.cc (right):
> 
>
https://codereview.chromium.org/2555563003/diff/1/net/spdy/spdy_stream.cc#new...
> net/spdy/spdy_stream.cc:394: const std::string error("Cannot parse :status.");
> On 2016/12/06 23:48:14, Ryan Hamilton wrote: 
> > 
> > So if there are servers out there which return "200 OK", then Chrome will
stop
> > working to them, right?
> 
> That is correct, and this is a functional change introduced by this CL. 
8.1.2.4
> of the HTTP/2 spec says ":status pseudo-header field is defined that carries
the
> HTTP status code field [...] HTTP/2 does not define a way to carry the version
> or reason phrase that is included in an HTTP/1.1 status line".  That is, such
a
> server would not be in compliance with the spec.
> 
> We can (a) leave it as is, and still be able to relax the requirement before
> branch point if users see this in the wild, or (b) relax the requirement and
not
> worry about it, or (c) relax the requirement, gather UMA data, and get back to
> this later.  WDYT?

For next time, when we make changes which might cause breakage, we should
probably mention that in the CL description. The current description suggests
that nothing should change in the case of non-1xx responses. But that ship has
sailed.

I'm happy to adhere to the spec, but we should probably make sure we understand
how often this is happening so that if it's common we can figure out what to do
next. Perhaps land a follow up to add a histogram when the :status header is not
parsable as an int?

[Bence, 2016-12-07 15:51:06]

On 2016/12/07 01:26:09, Ryan Hamilton wrote:
> 
> For next time, when we make changes which might cause breakage, we should
> probably mention that in the CL description. The current description suggests
> that nothing should change in the case of non-1xx responses. But that ship has
> sailed.
> 
> I'm happy to adhere to the spec, but we should probably make sure we
understand
> how often this is happening so that if it's common we can figure out what to
do
> next. Perhaps land a follow up to add a histogram when the :status header is
not
> parsable as an int?

I am sorry for missing this.  Follow-up at https://crrev.com/2555183002.