Closed Bug 1352146 Opened 5 years ago Closed 5 years ago
Fail stream on :status with reason phrase
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 firstname.lastname@example.org: 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
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
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
(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 email@example.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
You need to log in before you can comment on or make changes to this bug.