We will add http3 support behind a pref.

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

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

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.

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

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

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

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

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

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

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.

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.

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

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

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