1352146 - Fail stream on :status with reason phrase

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[u408661]

RFC 7540 requires no reason phrase in the :status pseudo-header - it should only be :status 200 (or 400, or 500, or whatever http status code) without the " OK" at the end. Up until now, both chrome and us have been tolerant of reason phrases. According to measurements, 1 in every 4e-8 responses has a :status with reason phrase. That's low enough to get strict on this. So, let's do it.

For reference, chrome bug for the same thing: https://bugs.chromium.org/p/chromium/issues/detail?id=672065

Comment 1

[u408661]

Attachment: Bug 1352146 - fix whitespace in moz-http2.js.

Review commit: https://reviewboard.mozilla.org/r/127816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127816/

Comment 2

[u408661]

Attachment: Bug 1352146 - Don't allow status phrases in http/2.

Review commit: https://reviewboard.mozilla.org/r/127818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127818/

Comment 3

[Patrick McManus [:mcmanus]]

Comment on attachment 8855937 [details]
Bug 1352146 - fix whitespace in moz-http2.js.

https://reviewboard.mozilla.org/r/127816/#review130508

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 8855938 [details]
Bug 1352146 - Don't allow status phrases in http/2.

https://reviewboard.mozilla.org/r/127818/#review130510

thanks

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/11b27254f152
fix whitespace in moz-http2.js. r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/c8199958741a
Don't allow status phrases in http/2. r=mcmanus

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for leaks detected by Linux x64 asan:

https://hg.mozilla.org/integration/autoland/rev/9204aed2836c9700fefe19463d0fbe8054be5266
https://hg.mozilla.org/integration/autoland/rev/b5c4d21732ce1901a055fe964fe91d83bf7378c3

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c8199958741afef261c03b81e5b4e89cb7918f38&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=89712650&repo=autoland

Comment 7

[Patrick McManus [:mcmanus]]

head scratching emoji

Comment 8

[u408661]

Uhhhhhhhh... WAT? Mochitest b-c shouldn't be touching http/2 code, at all (we don't even start the h2 test server in mochitests). So I'm not seeing how this could leak there. This is also, like, the simplest patch I've written in ages. So of course it would leak in an impossible manner.

:aryx, are we certain this is the cause of the leak? (See also comment 7 for more of my reaction)

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Nicholas, good news. This can reland. I checked the pushes up to the backout and there is one which finished later but got pushed before the backout and doesn't have these failures (there was definitely something wrong, might have been the test environment):
https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=linux%20asan%20tc-m-e10s&tochange=9204aed2836c9700fefe19463d0fbe8054be5266&fromchange=52ebd9950a44ef2a4c2c5d67f1f2a1fd8792fd72&selectedJob=89737020

Comment 10

[u408661]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #9)
> Nicholas, good news. This can reland. I checked the pushes up to the backout
> and there is one which finished later but got pushed before the backout and
> doesn't have these failures (there was definitely something wrong, might
> have been the test environment):
> https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-
> searchStr=linux%20asan%20tc-m-
> e10s&tochange=9204aed2836c9700fefe19463d0fbe8054be5266&fromchange=52ebd9950a4
> 4ef2a4c2c5d67f1f2a1fd8792fd72&selectedJob=89737020

Excellent, thanks!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b1879a044bc
fix whitespace in moz-http2.js. r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/409b7edf344a
Don't allow status phrases in http/2. r=mcmanus

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5b1879a044bc
https://hg.mozilla.org/mozilla-central/rev/409b7edf344a