Closed Bug 1324193 Opened 3 years ago Closed 3 years ago

Bump rust-url to 1.2.4

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

Attachments

(1 file)

Brings in some crash fixes and correctness updates. Any further mismatches *should* be due to things needing changes in nsStandardURL, not rust-url.
Ah, forgot to turn off the fallback mode.
Now the failures are:

IDNA issues all over the place (bug 945240)

> TEST-UNEXPECTED-FAIL | dom/tests/mochitest/whatwg/test_postMessage_idn.xhtml | wrong origin -- IDN issue, perhaps? - got "http://sub1.xn--lt-uia.example.org:8000", expected "http://sub1.ält.example.org:8000"

IPv4-in-ipv6 being printed as hex (https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b09455e1f766fc9f215057f0a1012c3119892ca&selectedJob=32931324)

> TEST-UNEXPECTED-FAIL | dom/url/tests/test_url.html | IPv6 hostname - got "[::c009:505]", expected "[::192.9.5.5]"
> TEST-UNEXPECTED-FAIL | dom/url/tests/test_url.html | undefined assertion name - got "http://[::c009:505]/", expected "http://[::192.9.5.5]/"

c: drive stuff -- this looks like a gecko bug? (https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b09455e1f766fc9f215057f0a1012c3119892ca&selectedJob=32931291)

> TEST-UNEXPECTED-FAIL | layout/style/test/test_bug379440.html | Serialize unloadable URLs using their specified value - got "url(\"file:///tmp/foo\"), url(\"file:///c:/\"), url(\"http://example.com/\"), crosshair", expected "url(\"file:///tmp/foo\"), url(\"file:///c|/\"), url(\"http://example.com/\"), crosshair"

Tons of UNEXPECTED-PASSes on WPT https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b09455e1f766fc9f215057f0a1012c3119892ca&selectedJob=32931347

> TEST-UNEXPECTED-PASS | /url/a-element-xhtml.xhtml | Parsing: <http://foo.com:b@d/> against <http://example.org/foo/bar> - expected FAIL

ipv6 double colon (https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b09455e1f766fc9f215057f0a1012c3119892ca&selectedJob=32931412)

> TEST-UNEXPECTED-FAIL | docshell/test/unit/test_nsDefaultURIFixup_info.js | do_single_test_run - [do_single_test_run : 584] "http://[fe80:cd00::cde:1257:0:211e:729c]/" == "http://[fe80:cd00:0:cde:1257:0:211e:729c]/" 

Empty ftp (https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b09455e1f766fc9f215057f0a1012c3119892ca&selectedJob=32931563)

> TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_URIs.js | do_check_property - [do_check_property : 319] "" == "ftp://"

Differing percent encoding (https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b09455e1f766fc9f215057f0a1012c3119892ca&selectedJob=32931563)

> TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_bug376844.js | run_test - [run_test : 19] "http://example.com/?'" == "http://example.com/?%27"
> TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_standardurl.js | test_filterWhitespace - [test_filterWhitespace : 314] "http://test.com/pa%0D%0A%09th?query#hash" == "http://test.com/pa%0D%0A%09th?qu%0D%0A%09ery#hash"


Something else? (https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b09455e1f766fc9f215057f0a1012c3119892ca&selectedJob=32931602)

> TEST-UNEXPECTED-FAIL | security/manager/ssl/tests/unit/test_sts_fqdn.js | run_test - [run_test : 46] "" == ".."


Not sure which of these are rust-url bugs.
> IDNA issues all over the place

bug 945240

> IPv4-in-ipv6 being printed as hex

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

Chrome agrees with the spec

bug 1324243

> c: drive stuff 

bug 1324251

Chrome, Firefox, Safari agree here.

Tests disagree: https://github.com/w3c/web-platform-tests/blob/be5820ecd36650bff4728208629553931fd42ec2/url/urltestdata.json#L1337

These are webkit's own tests (https://trac.webkit.org/browser/trunk/LayoutTests/fast/url/file.html), so they seem to disagree with themselves? Webkit trunk might pass these though.

https://url.spec.whatwg.org/#windows-drive-letter

> Tons of UNEXPECTED-PASSes on WPT

yay. Not sure if I should file bugs for fixing these in nsStandardURL? It seems to be a whole bunch of bugs.

> ipv6 double colon

Not sure how fixups will interact with rust-url but this specific bug is due to https://github.com/servo/rust-url/issues/253


> Empty ftp

bug 1324244

> Differing percent encoding

discussion at https://github.com/whatwg/url/issues/172 

gecko bugs: bug 1324246, bug 1324247

(consistent in firefox and chrome, does not match spec or webkit.)

> Something else? 

https://github.com/servo/rust-url/issues/255
Perfheder reports 4-8% increase on most of the tests I ran. To be expected.

(I wanted to run perfherder to get an idea of the baseline perf impact when we start attempting to reduce it)
Comment on attachment 8819524 [details]
Bug 1324193 - Bump rust-url to 1.2.4;

https://reviewboard.mozilla.org/r/99264/#review99616

Just a few minor questions that we can address upstream if needed.
Go ahead and land this.

::: third_party/rust/url/src/lib.rs:499
(Diff revision 1)
>      /// URLs that do *not* are either path-only like `unix:/run/foo.socket`
>      /// or cannot-be-a-base like `data:text/plain,Stuff`.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```

Not sure if this is required, but in the previous files you used \`\`\`rust

::: third_party/rust/url/tests/urltestdata.json:2164
(Diff revision 1)
>      "hash": ""
>    },
>    {
>      "input": "http://www.google.com/foo?bar=baz# »",
>      "base": "about:blank",
> -    "href": "http://www.google.com/foo?bar=baz# »",
> +    "href": "http://www.google.com/foo?bar=baz# %C2%BB",

I was under the impression that spaces should also be percent encoded. No?
Attachment #8819524 - Flags: review?(valentin.gosu) → review+
> Not sure if this is required, but in the previous files you used \`\`\`rust

Not required

> I was under the impression that spaces should also be percent encoded. No?

Not in hashes.

(That file is synced with the wpt testset, and that change comes from wpt)
https://hg.mozilla.org/mozilla-central/rev/389bc37ce175
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.