Move header-ordering validation from hpack decoder to header validator.

Move header-ordering validation from hpack decoder to header validator.

The HTTP/2 spec (not the HPACK spec) demands that pseudoheaders appearing after
regular headers in a header block should result in a RST_STREAM with
PROTOCOL_ERROR.

This CL lands server change 123039286 by dahollings.

BUG=488484
Committed: https://crrev.com/3992fd06cd7ff06861ccee38a02bd568f61e4d52
Cr-Commit-Position: refs/heads/master@{#400824}

[yinjie, 2016-05-30 21:31:06]

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

[commit-bot: I haz the power, 2016-05-30 21:31:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2028473003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2028473003/1

[commit-bot: I haz the power, 2016-05-30 22:51:25]

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

[commit-bot: I haz the power, 2016-05-30 22:51:26]

Dry run: This issue passed the CQ dry run.

[yinjie, 2016-05-30 22:52:11]

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

[Bence, 2016-05-31 15:57:02]

This validation should happen somewhere in Chromium.  I think you should
implement it in HeaderCoalescer: add |regular_header_seen_| member, and set
|error_seen_| if pseudo-headers follow regular headers.

https://codereview.chromium.org/2028473003/diff/1/net/spdy/hpack/hpack_decode...
File net/spdy/hpack/hpack_decoder.cc (left):

https://codereview.chromium.org/2028473003/diff/1/net/spdy/hpack/hpack_decode...
net/spdy/hpack/hpack_decoder.cc:107: if (regular_header_seen_) {
Please also remove regular_header_seen_ member from header file.

https://codereview.chromium.org/2028473003/diff/1/net/spdy/hpack/hpack_decode...
File net/spdy/hpack/hpack_decoder_test.cc (right):

https://codereview.chromium.org/2028473003/diff/1/net/spdy/hpack/hpack_decode...
net/spdy/hpack/hpack_decoder_test.cc:342: TEST_P(HpackDecoderTest,
InvalidPseudoHeaderPositionStatic) {
Please remove these two tests entirely.

[yinjie, 2016-06-19 06:41:12]

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

[commit-bot: I haz the power, 2016-06-19 06:41:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2028473003/40001

[commit-bot: I haz the power, 2016-06-19 07:03:23]

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

[commit-bot: I haz the power, 2016-06-19 07:03:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Bence, 2016-06-20 12:11:27]

LGTM with nit.  Thank you.

https://codereview.chromium.org/2028473003/diff/100001/net/spdy/header_coales...
File net/spdy/header_coalescer.cc (right):

https://codereview.chromium.org/2028473003/diff/100001/net/spdy/header_coales...
net/spdy/header_coalescer.cc:11: void
HeaderCoalescer::OnHeader(base::StringPiece key, base::StringPiece value) {
Please return if |error_seen_| first thing in this function: there is no point
in doing any more work.

[yinjie, 2016-06-20 18:26:53]

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

[commit-bot: I haz the power, 2016-06-20 18:27:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2028473003/120001

[commit-bot: I haz the power, 2016-06-20 18:29:31]

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

[commit-bot: I haz the power, 2016-06-20 18:29:32]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[yinjie, 2016-06-20 18:34:45]

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

[commit-bot: I haz the power, 2016-06-20 18:35:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2028473003/140001

[commit-bot: I haz the power, 2016-06-20 18:37:28]

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

[commit-bot: I haz the power, 2016-06-20 18:37:29]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yinjie, 2016-06-20 20:28:58]

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

[commit-bot: I haz the power, 2016-06-20 20:29:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2028473003/180001

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

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

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

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[yinjie, 2016-06-20 20:57:11]

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

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

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2028473003/220001

[commit-bot: I haz the power, 2016-06-20 21:29:11]

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

[commit-bot: I haz the power, 2016-06-20 21:29:13]

Dry run: Try jobs failed on following builders:
  win_clang on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[yinjie, 2016-06-20 22:58:25]

On 2016/06/20 12:11:27, Bence wrote:
> LGTM with nit.  Thank you.
> 
>
https://codereview.chromium.org/2028473003/diff/100001/net/spdy/header_coales...
> File net/spdy/header_coalescer.cc (right):
> 
>
https://codereview.chromium.org/2028473003/diff/100001/net/spdy/header_coales...
> net/spdy/header_coalescer.cc:11: void
> HeaderCoalescer::OnHeader(base::StringPiece key, base::StringPiece value) {
> Please return if |error_seen_| first thing in this function: there is no point
> in doing any more work.

Thanks for the review and thank you for helping me debugging test failures.

[yinjie, 2016-06-20 22:58:33]

The CQ bit was checked by yinjie@chromium.org

[yinjie, 2016-06-20 22:58:33]

The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org
Link to the patchset: https://codereview.chromium.org/2028473003/#ps220001
(title: "Fix lint errors.")

[commit-bot: I haz the power, 2016-06-20 22:59:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2028473003/220001

[commit-bot: I haz the power, 2016-06-20 23:06:22]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-06-20 23:07:48]

Description was changed from

==========
Move header-ordering validation from hpack decoder to header validator.

The HTTP/2 spec (not the HPACK spec) demands that pseudoheaders appearing after
regular headers in a header block should result in a RST_STREAM with
PROTOCOL_ERROR.

This CL lands server change 123039286 by dahollings.

BUG=488484
==========

to

==========
Move header-ordering validation from hpack decoder to header validator.

The HTTP/2 spec (not the HPACK spec) demands that pseudoheaders appearing after
regular headers in a header block should result in a RST_STREAM with
PROTOCOL_ERROR.

This CL lands server change 123039286 by dahollings.

BUG=488484

Committed: https://crrev.com/3992fd06cd7ff06861ccee38a02bd568f61e4d52
Cr-Commit-Position: refs/heads/master@{#400824}
==========

[commit-bot: I haz the power, 2016-06-20 23:07:49]

Patchset 12 (id:??) landed as
https://crrev.com/3992fd06cd7ff06861ccee38a02bd568f61e4d52
Cr-Commit-Position: refs/heads/master@{#400824}