Closed Bug 1352146 Opened 3 years ago Closed 3 years ago

Fail stream on :status with reason phrase

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: u408661, Assigned: u408661)

Details

(Whiteboard: [necko-active][spdy])

Attachments

(2 files)

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 on attachment 8855937 [details]
Bug 1352146 - fix whitespace in moz-http2.js.

https://reviewboard.mozilla.org/r/127816/#review130508
Attachment #8855937 - Flags: review?(mcmanus) → review+
Comment on attachment 8855938 [details]
Bug 1352146 - Don't allow status phrases in http/2.

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

thanks
Attachment #8855938 - Flags: review?(mcmanus) → review+
Pushed by hurley@mozilla.com:
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
head scratching emoji
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)
Flags: needinfo?(hurley) → needinfo?(aryx.bugmail)
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
Flags: needinfo?(aryx.bugmail)
(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!
Pushed by hurley@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/5b1879a044bc
https://hg.mozilla.org/mozilla-central/rev/409b7edf344a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.