Closed Bug 1581637 Opened 4 months ago Closed 3 months ago

Add Http3 support

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: dragana, Assigned: dragana)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [necko-triaged])

Attachments

(13 files, 3 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

We will add http3 support behind a pref.

Assignee: nobody → dd.mozilla
Blocks: QUIC
Priority: -- → P2
Whiteboard: [necko-triaged]
Attached file Add Http3 prefs. r=mayhemer (obsolete) —
Attachment #9093142 - Attachment is obsolete: true
Attachment #9094286 - Attachment description: Part 2 - Make Dashboard knows http3.r=mayhemer → Part 1 - Make Dashboard knows http3.r=mayhemer
Attached file Part 6 - Import neqo. (obsolete) —
Attachment #9095463 - Attachment description: Import neqo. → Part 6 - Import neqo.
Attachment #9095466 - Attachment description: Add neqo-necko API. → Part 7- Add neqo-necko API.
Attachment #9095467 - Attachment description: Add new vendored crates required by neqo and neqo_glue. → Part 8 - Add new vendored crates required by neqo and neqo_glue.
Attached file neqo-crypto/build.rs changes. (obsolete) —
Attachment #9095469 - Attachment description: Add extra-bindgen-flags → Part 13 - Add extra-bindgen-flags

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

Attachment #9095468 - Attachment is obsolete: true

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.

Flags: needinfo?(dd.mozilla)

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

Flags: needinfo?(dd.mozilla)

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.

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)?

Flags: needinfo?(dd.mozilla)

(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!

Flags: needinfo?(dd.mozilla)

(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)?

Attachment #9095463 - Attachment is obsolete: true
Duplicate of this bug: 1584584
Attachment #9095466 - Attachment description: Part 7- Add neqo-necko API. → Part 6 - Add neqo-necko API.
Attachment #9095467 - Attachment description: Part 8 - Add new vendored crates required by neqo and neqo_glue. → Part 7 - Add new vendored crates required by neqo and neqo_glue.
Attachment #9094296 - Attachment description: Part 9 - Add Http3Session/Http3Stream. r=mayhemer → Part 8 - Add Http3Session/Http3Stream. r=mayhemer
Attachment #9094297 - Attachment description: Part 10 - Make Http3 connection and include http3 in nsHttpConnectionMgr. r=mayhemer → Part 9 - Make Http3 connection and include http3 in nsHttpConnectionMgr. r=mayhemer
Attachment #9094298 - Attachment description: Part 11 - Implement a posibility to use alt-svc on transaction restart in some cases. r= mayhemer → Part 10 - Implement a posibility to use alt-svc on transaction restart in some cases. r= mayhemer
Attachment #9094299 - Attachment description: Part 12 - Missing includes. r=mayhemer → Part 11 - Missing includes. r=mayhemer
Attachment #9095469 - Attachment description: Part 13 - Add extra-bindgen-flags → Part 12 - Add extra-bindgen-flags
Pushed by rmaries@mozilla.com:
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
Depends on: 1592927

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.

Regressions: 1593217

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.

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

Flags: needinfo?(dd.mozilla)
Pushed by csabou@mozilla.com:
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

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

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

Depends on: 1593446
Regressions: 1593394
Depends on: 1593614
Depends on: 1593810
Depends on: 1593950

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

Flags: needinfo?(dd.mozilla)
Regressions: 1594342
Regressions: 1594171
Depends on: 1594398
Depends on: 1595337
Depends on: 1595449
No longer depends on: 1595449
Depends on: 1595779
No longer depends on: 1595337
Regressions: 1595337
You need to log in before you can comment on or make changes to this bug.