Closed Bug 1324243 Opened 7 years ago Closed 7 years ago

IPV4 urls in IPV6 format should serialize to IPV6 urls


(Core :: Networking, defect)

Not set



Tracking Status
firefox55 --- fixed


(Reporter: manishearth, Assigned: valentin)



(Whiteboard: [necko-active])


(2 files)

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


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]

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

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

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
Flags: needinfo?(valentin.gosu)
Backout by
Backed out changeset 189b914221f4 for wpt-11 bustage
Flags: needinfo?(valentin.gosu)
Closed: 7 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.