1130874 - h2 alt-svc extension update to -06

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

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

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: MozReview Request: bz://1130874/mcmanus

/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

Comment 2

[u408661]

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 3

[u408661]

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!".

Comment 4

[Patrick McManus [:mcmanus]]

> 
> ::: 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.

Comment 5

[u408661]

Oh, joy. Well, WFM, then :)

Comment 6

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0e2f7d00b82
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb0c935a8fda
https://hg.mozilla.org/integration/mozilla-inbound/rev/717a6673bc18

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f0e2f7d00b82
https://hg.mozilla.org/mozilla-central/rev/eb0c935a8fda
https://hg.mozilla.org/mozilla-central/rev/717a6673bc18

Comment 9

[Patrick McManus [:mcmanus]]

Attachment: MozReview Request: bug 1130874 - h2 test server altsvc 06 r=hurley

Comment 10

[Patrick McManus [:mcmanus]]

Attachment: MozReview Request: bug 1130874 - test for alt-svc -06 h2 extension r=hurley

Comment 11

[Patrick McManus [:mcmanus]]

Attachment: MozReview Request: bug 1130874 - update h2 alternate service extension to draft-06 r=hurley