Closed Bug 1324243 Opened 3 years ago Closed 3 years ago

IPV4 urls in IPV6 format should serialize to IPV6 urls

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: manishearth, Assigned: valentin)

References

Details

(Whiteboard: [necko-active])

Attachments

(2 files)

|(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.
Assignee: nobody → valentin.gosu
Whiteboard: [necko-active]
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?
Attachment #8822032 - Flags: feedback?(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?
Attachment #8822032 - Flags: feedback?(manishearth) → feedback+
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.
Attachment #8842347 - Flags: review?(mcmanus)
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.
Attachment #8842347 - Flags: review?(manishearth) → review+
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)
Flags: needinfo?(ted)
Flags: needinfo?(nfroyd)
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.
Flags: needinfo?(ted)
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.
had to back this out for wpt11 bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=81890605&repo=autoland
Flags: needinfo?(valentin.gosu)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/25e9dfe731cc
Backed out changeset 189b914221f4 for wpt-11 bustage
Flags: needinfo?(valentin.gosu)
https://hg.mozilla.org/mozilla-central/rev/5d7591da1a09
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(nfroyd)
Depends on: 1363348
You need to log in before you can comment on or make changes to this bug.