1581637 - Add Http3 support

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement, P2)

Description

[Dragana Damjanovic [:dragana]]

We will add http3 support behind a pref.

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: Add Http3 prefs. r=mayhemer

Comment 2

[Dragana Damjanovic [:dragana]]

Attachment: Part 0 - Make necko recognize Http3. r=mayhemer

Comment 3

[Dragana Damjanovic [:dragana]]

Attachment: Part 1 - Make Dashboard knows http3.r=mayhemer

Comment 4

[Dragana Damjanovic [:dragana]]

Attachment: Part 2 - Add NS_ERROR_NET_HTTP3_PROTOCOL_ERROR error. r=mayhemer

Comment 5

[Dragana Damjanovic [:dragana]]

Attachment: Part 3 - Add Http3 to nsHttpConnectionInfo. r=mayhemer

Comment 6

[Dragana Damjanovic [:dragana]]

Attachment: Part 4 - Add AltSvc support for Http3. r=mayhemer

Comment 7

[Dragana Damjanovic [:dragana]]

Attachment: Part 5 - Add Http3 prefs. r=mayhemer

Comment 8

[Dragana Damjanovic [:dragana]]

Attachment: Part 8 - Add Http3Session/Http3Stream. r=mayhemer

Comment 9

[Dragana Damjanovic [:dragana]]

Attachment: Part 9 - Make Http3 connection and include http3 in nsHttpConnectionMgr. r=mayhemer

Comment 10

[Dragana Damjanovic [:dragana]]

Attachment: Part 10 - Implement a posibility to use alt-svc on transaction restart in some cases. r= mayhemer

Comment 11

[Dragana Damjanovic [:dragana]]

Attachment: Part 11 - Missing includes. r=mayhemer

Comment 12

[Dragana Damjanovic [:dragana]]

Attachment: Part 6 - Import neqo.

Comment 13

[Dragana Damjanovic [:dragana]]

Attachment: Part 6 - Add neqo-necko API.

Comment 14

[Dragana Damjanovic [:dragana]]

Attachment: Part 7 - Add new vendored crates required by neqo and neqo_glue.

Comment 15

[Dragana Damjanovic [:dragana]]

Attachment: neqo-crypto/build.rs changes.

Comment 16

[Dragana Damjanovic [:dragana]]

Attachment: Part 12 - Add extra-bindgen-flags

Comment 17

[Dragana Damjanovic [:dragana]]

Happy try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=abdb3b3506a193de8b9325bc93480964a75254ee

Comment 18

[Dragana Damjanovic [:dragana]]

Patch D47228 neqo-crypto/build.rs changes. will be integrated into neqo and review there.
Therefore I am not added reviews for "Part 6 - Import neqo" yet

Comment 19

[Nicholas Nethercote [inactive]]

Dragana: I don't know anything about Http3. Can you clarify why you have asked me to review? I guess it's somehow related to the fact that this is mostly Rust code, but I'd like a clearer understanding of what kind of review you want me to do, especially given how much code there is here.

Comment 20

[Dragana Damjanovic [:dragana]]

(In reply to Nicholas Nethercote [:njn] from comment #19)

Dragana: I don't know anything about Http3. Can you clarify why you have asked me to review? I guess it's somehow related to the fact that this is mostly Rust code, but I'd like a clearer understanding of what kind of review you want me to do, especially given how much code there is here.

Http3 is implemented in rust. The neqo repo is https://github.com/mozilla/neqo.The neqo code is reviewed on its own so you do not need to review it here.

Patch "Part 6 - Import neqo" copies the neqo repo into gecko(necko). It uses update.sh script for that and it applies gecko.patch. It also updates README_MOZILLA. floydnj suggested to do it this way. For this patch I would like you only to review how we import neqo, (i.e. files update.sh, gecko.patch and README_MOZILLA) and if the procedure corresponds to how we import rust code.

Patch "Part 7- Add neqo-necko API" is the rust-c++ interface for neqo. I would like you to review this one. Necko developers do not have a lot of experience with rust-c++ interfaces. NeqoHttp3Conn is only used by Http3Session (Http3Session is added in patch "Part 9 - Add Http3Session/Http3Stream. r=mayhemer"). Only Http3Session creates it, accesses it and it is destroyed when Http3Session is destroyed. Http3Session only connects neqo to specifics of necko. I am happy to give you more details. I will also share with you a doc that I wrote about how neqo-necko should work. The doc is written more for a necko developer (some necko details are not explain, but I am happy to answer any questions).

"Part 8 - Add new vendored crates required by neqo and neqo_glue." this changes are made by running "mach vendor rust".

About "Part 13 - Add extra-bindgen-flags": neqo-crypto uses nss/nspr, the same nss firefox uses, and neqo-crypto/build.rs needs extra-bindgen-flags to properly compile. I was told that using such a file is the preferred way to configure rust compiler.

Comment 21

[Nicholas Nethercote [inactive]]

I have very little experience with importaing and vendoring Rust code into mozilla-central, bindgen, and such things. heycam has kindly offered to take a look, so I will forward the reviews to him, except for part 13 which feels more appropriate for glandium.

Comment 22

[Cameron McCormack (:heycam)]

Dragana, I don't wish to second guess Nathan's suggestion, but can you explain why we should vendor in the code using an update script like this, as opposed to adding them as dependencies to the neqo_glue crate, and vendoring them in with mach vendor rust? Is it that you will soon be moving towards having the mozilla-central copy of the code be the authoritative source (at which point you'll remove update.sh)?

Comment 23

[Dragana Damjanovic [:dragana]]

(In reply to Nicholas Nethercote [:njn] from comment #21)

I have very little experience with importaing and vendoring Rust code into mozilla-central, bindgen, and such things. heycam has kindly offered to take a look, so I will forward the reviews to him, except for part 13 which feels more appropriate for glandium.

Thanks!

Comment 24

[Dragana Damjanovic [:dragana]]

(In reply to Cameron McCormack (:heycam) from comment #22)

Dragana, I don't wish to second guess Nathan's suggestion, but can you explain why we should vendor in the code using an update script like this, as opposed to adding them as dependencies to the neqo_glue crate, and vendoring them in with mach vendor rust? Is it that you will soon be moving towards having the mozilla-central copy of the code be the authoritative source (at which point you'll remove update.sh)?

Nathan and I did not discussed this for very long.
There are other code that is pulled in in this way, e.g. https://searchfox.org/mozilla-central/source/media/audioipc

Thinking of it, I do not see reason why we should not be able to vendor it using mach vendor rust. At the beginning when I first wanted to make these patches neqo was changing too fast and we were not good in making releases, so it was hard to keep up with two repos (mozilla-cental and neqo) therefore I pulled it in. We are still not doing well making neqo releases but that is only a question of coordination.

There are no plans in the near future, if ever, to make the mozilla-central copy of the code be the authoritative source. The plan was to handle it as nss code, a separate branch that is pulled in from time to time. Probably the rust version of this is to use mach vendor rust or are there advantages of using update.sh (I am new to rust :), my apologies for dummy questions)?

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6ff3db0a421
Part 0 - Make necko recognize Http3. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/c28174eaccbe
Part 1 - Make Dashboard knows http3.r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/58ca75a25253
Part 2 - Add NS_ERROR_NET_HTTP3_PROTOCOL_ERROR error. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/21b721e37d39
Part 3 - Add Http3 to nsHttpConnectionInfo. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/f09b9a4ba633
Part 4 - Add AltSvc support for Http3. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/6b6b75fbec7f
Part 5 - Add Http3 prefs. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/6b80553abc74
Part 6 - Add neqo-necko API. r=mayhemer,heycam
https://hg.mozilla.org/integration/autoland/rev/63b7f1ff1714
Part 7 - Add new vendored crates required by neqo and neqo_glue. r=heycam
https://hg.mozilla.org/integration/autoland/rev/496ec0c5a60f
Part 8 - Add Http3Session/Http3Stream. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/97ff4a06c2da
Part 9 - Make Http3 connection and include http3 in nsHttpConnectionMgr. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/c5d8950b4a4b
Part 10 - Implement a posibility to use alt-svc on transaction restart in some cases. r=mayhemer mayhemer
https://hg.mozilla.org/integration/autoland/rev/a5df33ec7393
Part 11 - Missing includes. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/3a458217248d
Part 12 - Add extra-bindgen-flags r=glandium

Comment 27

[Andreea Pavel [:apavel]]

Backed out for failing xpcshell at test_anonymous-coalescing.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=273815436&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=windows%2C7%2Cdebug%2Cxpcshell%2Ctests%2Ctest-windows7-32%2Fdebug-xpcshell-e10s-1%2Cx%28x1%29&revision=3a458217248db4f5a1f9d18480aea606006959aa

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=273808866&repo=autoland&lineNumber=6758

Backout: https://hg.mozilla.org/integration/autoland/rev/1efec06dae2fc6c25b4f0ca1365c01e4b470023f

Comment 28

[Dragana Damjanovic [:dragana]]

(In reply to Andreea Pavel [:apavel] from comment #27)

Backed out for failing xpcshell at test_anonymous-coalescing.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=273815436&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=windows%2C7%2Cdebug%2Cxpcshell%2Ctests%2Ctest-windows7-32%2Fdebug-xpcshell-e10s-1%2Cx%28x1%29&revision=3a458217248db4f5a1f9d18480aea606006959aa

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=273808866&repo=autoland&lineNumber=6758

Backout: https://hg.mozilla.org/integration/autoland/rev/1efec06dae2fc6c25b4f0ca1365c01e4b470023f

Thank you for the report. An sorry for the trouble.
The problem is that the test is racy. I will add one more patch to fix the test.

Comment 29

[Dragana Damjanovic [:dragana]]

Here is a try run with the patch from bug 1592927.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b0108792aa8c6ea15559100943bc0eba2d2da90&selectedJob=273875374

We can land this code as soon as the other patch lands.

Comment 30

[(Away)]

Before relanding please take a look at the large increase in static constructors (bug 1593217) that was seen while this patch was briefly on autoland.

Comment 31

[Honza Bambas (:mayhemer)]

(In reply to :dmajor from comment #30)

Before relanding please take a look at the large increase in static constructors (bug 1593217) that was seen while this patch was briefly on autoland.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a44e34eda1e
Part 0 - Make necko recognize Http3. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/037fb8be69b0
Part 1 - Make Dashboard knows http3.r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/100e9706f5a3
Part 2 - Add NS_ERROR_NET_HTTP3_PROTOCOL_ERROR error. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/b4fdc4c7aa66
Part 3 - Add Http3 to nsHttpConnectionInfo. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/2fd4b5760b2b
Part 4 - Add AltSvc support for Http3. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/325ef0639258
Part 5 - Add Http3 prefs. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/d4a5badae723
Part 6 - Add neqo-necko API. r=mayhemer,heycam
https://hg.mozilla.org/integration/autoland/rev/2801613e5d2d
Part 7 - Add new vendored crates required by neqo and neqo_glue. r=heycam
https://hg.mozilla.org/integration/autoland/rev/0bf045553a40
Part 8 - Add Http3Session/Http3Stream. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/fb1ae3b0bd49
Part 9 - Make Http3 connection and include http3 in nsHttpConnectionMgr. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/dbbba0e989d4
Part 10 - Implement a posibility to use alt-svc on transaction restart in some cases. r=mayhemer mayhemer
https://hg.mozilla.org/integration/autoland/rev/2e0b0af37fe9
Part 11 - Missing includes. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/0f08e0fe8956
Part 12 - Add extra-bindgen-flags r=glandium

Comment 33

[Dragana Damjanovic [:dragana]]

(In reply to :dmajor from comment #30)

Before relanding please take a look at the large increase in static constructors (bug 1593217) that was seen while this patch was briefly on autoland.

I will take a look. The comment #23 i not me, I did not try to reland it.

Comment 34

[(Away)]

FWIW froydnj made a suggestion about the constructors on the Phabricator review.

Comment 35

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7a44e34eda1e
https://hg.mozilla.org/mozilla-central/rev/037fb8be69b0
https://hg.mozilla.org/mozilla-central/rev/100e9706f5a3
https://hg.mozilla.org/mozilla-central/rev/b4fdc4c7aa66
https://hg.mozilla.org/mozilla-central/rev/2fd4b5760b2b
https://hg.mozilla.org/mozilla-central/rev/325ef0639258
https://hg.mozilla.org/mozilla-central/rev/d4a5badae723
https://hg.mozilla.org/mozilla-central/rev/2801613e5d2d
https://hg.mozilla.org/mozilla-central/rev/0bf045553a40
https://hg.mozilla.org/mozilla-central/rev/fb1ae3b0bd49
https://hg.mozilla.org/mozilla-central/rev/dbbba0e989d4
https://hg.mozilla.org/mozilla-central/rev/2e0b0af37fe9
https://hg.mozilla.org/mozilla-central/rev/0f08e0fe8956

Comment 36

[Dragana Damjanovic [:dragana]]

(In reply to Honza Bambas (:mayhemer) from comment #31)

(In reply to :dmajor from comment #30)

Before relanding please take a look at the large increase in static constructors (bug 1593217) that was seen while this patch was briefly on autoland.

This is fixed now. Removing need-info.