Closed Bug 1130874 Opened 6 years ago Closed 6 years ago

h2 alt-svc extension update to -06

Categories

(Core :: Networking: HTTP, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: mcmanus, Assigned: mcmanus)

References

Details

Attachments

(3 files, 1 obsolete file)

changes from 04 to 06

* alt-used contents
* h2 frame format

also added a test for alt-svc frame on stream 0

also added node-http2 support for generating -06 compatible altsvc frames to enable the test. This isn't really suitable for upstream as it doesn't do frame reciept or the node-http2 internal framing tests correctly. But it should be fine as a placeholder in our CI.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff8bdd58e8d5
/r/3543 - bug 1130874 - update h2 alternate service extension to draft-06 r=hurley
/r/3545 - bug 1130874 - h2 test server altsvc 06 r=hurley
/r/3547 - bug 1130874 - test for alt-svc -06 h2 extension r=hurley

Pull down these commits:

hg pull review -r 2f4920057e25f1b60864ab573fc6b6bad1169563
Attachment #8561067 - Flags: review?(hurley)
https://reviewboard.mozilla.org/r/3547/#review2829

Looks pretty good to me. Just one comment that I would like clarification on, and some fist-shaking at your editor :)

FYI, I filed https://github.com/molnarg/node-http2/issues/95 to update node-http2 appropriately (we can update CI to use the new full version whenever I get to writing those bits).

::: netwerk/test/unit/test_http2.js
(Diff revision 1)
> +            , test_http2_continuations

Why was this moved?

::: testing/xpcshell/moz-http2/moz-http2.js
(Diff revision 1)
> +	req.headers['alt-used'] != ("localhost:" + serverPort)) {

... and here ...

::: testing/xpcshell/moz-http2/moz-http2.js
(Diff revision 1)
> +	req.scheme != "http" ||

... and here ...

::: testing/xpcshell/moz-http2/moz-http2.js
(Diff revision 1)
> +	req.headers['alt-used'] != ("localhost:" + serverPort)) {

... and here.

::: testing/xpcshell/moz-http2/moz-http2.js
(Diff revision 1)
> +	req.scheme != "http" ||

Your editor once again did leading tabs instead of spaces here...
Comment on attachment 8561067 [details]
MozReview Request: bz://1130874/mcmanus

Awwww, reviewboard posted the comments on moz-http2.js in reverse order, totally ruining my extended review sentence :(

It also didn't set r+ for some reason beyond understanding, even though I said "ship it!".
Attachment #8561067 - Flags: review?(hurley) → review+
> 
> ::: netwerk/test/unit/test_http2.js
> (Diff revision 1)
> > +            , test_http2_continuations
> 
> Why was this moved?
> 

    when running the test locally I would periodically see this test fail on a node-http2 exception that "res didn't have a method push".. and it was apparent that the transaction was using h1. It think perhaps the wrongsuite test occasionally managed to make an extra connection (via preconnect?) that negotiated down to h1. Anyhow, moving the test up to the point in the manifest where there is less h1 traffic resolved it.
Oh, joy. Well, WFM, then :)
https://hg.mozilla.org/mozilla-central/rev/f0e2f7d00b82
https://hg.mozilla.org/mozilla-central/rev/eb0c935a8fda
https://hg.mozilla.org/mozilla-central/rev/717a6673bc18
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Attachment #8561067 - Attachment is obsolete: true
Attachment #8619406 - Flags: review+
Attachment #8619407 - Flags: review+
Attachment #8619408 - Flags: review+
Regressions: 1566465
You need to log in before you can comment on or make changes to this bug.