1324243 - IPV4 urls in IPV6 format should serialize to IPV6 urls

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Manish Goregaokar [:manishearth]]

|(new URL("http://[::192.9.5.5]")).href| should be "http://[::c009:505]/", not "http://[::192.9.5.5]"

See https://url.spec.whatwg.org/#concept-ipv6-serializer

Chrome agrees with the spec here.

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Attachment: bug1324243-ipv6-normalize.patch

The current parser does not IPv6 normalization at all. I think we can use the rust-url ipv6 parser for this. Manish, what do you think about this approach? Also, do we have a guide for changing the third_party crates in the tree, and upstreaming the changes, or do we have to make the changes upstream and wait for the crates to be published?

Comment 2

[Manish Goregaokar [:manishearth]]

Comment on attachment 8822032 [details] [diff] [review]
bug1324243-ipv6-normalize.patch

Review of attachment 8822032 [details] [diff] [review]:
-----------------------------------------------------------------

This is an ... interesting approach. The reason I filed this bug was to make Gecko spec-complete independent of rust-url, so that there's no noise in the mismatches. This would mean making gecko handle ipv6 here in C++ code, or at least that was my intent.

This approach uses the existing code from rust-url in the C++ Gecko code that operates on nsStandardURL (not RustUrl). That seems a bit strange to me. It does solve our problem -- we will no longer get mismatches due to ipv6.

It does make some things more complicated. It means that Gecko will have different semantics in non-Rust and Rust mode. It also means that we will need to unconditionally to compile in rust-url, but most of it won't get used (which is okay).

I personally like this approach, though.

If this can land it would be the first rust-url code unconditionally running on nightly. Exciting!

> or do we have to make the changes upstream and wait for the crates to be published?

yes. There are checksums in the lockfile so it's not easy to edit in-tree, and you shouldn't be doing that either. For rapid prototyping you can use [replace] in Cargo.toml

::: netwerk/base/nsStandardURL.cpp
@@ +2029,5 @@
>      if (NS_SUCCEEDED(rv)) {
>        hostBuf = ipString;
>      }
>  
> +    if (hostBuf.Length() > 0 && hostBuf.First() == '[' && hostBuf.Last() == ']') {

Shouldn't the previous block of code be in the else block of this if?

Comment 3

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a51f83332bd

Comment 4

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1725a94fbe47

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1324243 - Normalize IPv6

Review commit: https://reviewboard.mozilla.org/r/116102/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116102/

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 8842347 [details]
Bug 1324243 - Normalize IPv6

https://reviewboard.mozilla.org/r/116102/#review117910

I'm content to let :manishearth do the whole review here.

Comment 7

[Manish Goregaokar [:manishearth]]

Comment on attachment 8842347 [details]
Bug 1324243 - Normalize IPv6

https://reviewboard.mozilla.org/r/116102/#review118020

Looks fine to me.

As I've mentioned before, we should beware that this introduces a hard dependency on rust in our url parsing algorithm, which has not been the case so far (everything can be preffed off).

I guess we're okay with that? There may be distro issues.

Also, this means we're shipping more vendored code. Which we haven't actually trust-verified.

Comment 8

[Manish Goregaokar [:manishearth]]

Thoughts on landing this? See comment 7, this introduces a hard dep on rust, and we don't have trust checking (though rust-url's dependencies are mostly maintained by Servo and Rust anyway)

Comment 9

[(not currently active) Ted Mielczarek]

Regardless of hard dependency or not we're already shipping this code, so I don't think it makes a difference. I would like to have one or more people do an audit of what we've vendored at some point, but I wouldn't block something like this on it.

Comment 10

[Valentin Gosu [:valentin] (he/him)]

We already have a hard dependency on Rust, and as Ted said, we're already shipping rust-url, so we might as well use it.
I feel pretty confident in using this small part of rust-url, and I don't see many downsides. Removing the dependency is still as easy as backing out the patch.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/189b914221f4
Normalize IPv6 r=manishearth

Comment 12

[Carsten Book [:Tomcat]]

had to back this out for wpt11 bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=81890605&repo=autoland

Comment 13

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/25e9dfe731cc
Backed out changeset 189b914221f4 for wpt-11 bustage

Comment 14

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a00834599e5

Comment 15

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8842347 [details]
Bug 1324243 - Normalize IPv6

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116102/diff/1-2/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d7591da1a09
Normalize IPv6 r=manishearth

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5d7591da1a09