945240 - Change nsStandardURL's internal hostname representation to punycode

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See discussion in bug 942074.

Gavin, how much would this break Firefox/extensions?

Comment 1

[:Gavin Sharp [email: [e-mail]]]

I don't have a good sense offhand. nsLocation is not as often accessed as e.g. docShell.currentURI, for most frontend code, but it is used in a bunch of tests, and addons probably use it more. I would assume most don't care about punycode vs. not, though, so maybe it wouldn't be a big deal?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Addons are my main worry, honestly, followed by our UI.  I can obviously fix whatever tests fail, since I'll know about them!

If you think the UI will be OK, chances are we can just make that change and addons will end up dealing as needed...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Jason, could we add an nsIURI API like GetASCIISpec but that only converts the hostname to ASCII?

Comment 4

[Jorge Villalobos [:jorgev] (he/him)]

It seems unlikely that this will affect many add-ons, and I can't think of a reliable way to check for this in the Add-ons MXR. I think it should be okay to go forward with this change.

Comment 5

[Jason Duell]

bz: sure, you can add a a new field/method to nsIURI.  I don't see why that would be a problem.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Andrew, this seems to be breaking more sites.  Do we have someone who can take this, please?  I'm trying to avoid piling more stuff on my plate...

Note that we'd probably want to us the same punycoding setup in document.URL as well, at least.

Maybe the right answer here really is to store nsStandardURL in punycode and only convert to IDN in things like unescapeURIForUI and whatnot?  That will probably incidentally fix things like bug 1224225 and whatnot.  :(

Comment 7

[Boris Zbarsky [:bzbarsky]]

Andrew, see comment 6.

Comment 8

[Jason Duell]

> Maybe the right answer here really is to store nsStandardURL in punycode and 
> only convert to IDN in things like unescapeURIForUI and whatnot?

Valentin, do you have an opinion here?  Can you take the URI part of this bug? (I'm not sure what's involved on the DOM side).

Comment 9

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

Sure I'll take it. This seems like a major issue for web-compat.

Comment 10

[Boris Zbarsky [:bzbarsky]]

The DOM side mostly depends on what exactly we do on the URI side.

Comment 11

[Andrew Overholt [:overholt]]

OK, once we've got the URI side a bit further along I will find someone to take the DOM side. Please needinfo me when it's time, Valentin.

Comment 12

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

So, I've started looking into this, but I have a question. Why can't we just use GetAsciiSpec and GetAsciiHost ?
From what I can tell, the non-ascii characters in the path, query and hash ( and user/pass) are percent encoded anyway. So the only difference between GetSpec and GetAsciiSpec would be the punycode encoding of the host - am I mistaken about this?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm.  Non-ascii stuff in the path and so forth isn't guaranteed to be %-encoded last I checked.  Certainly the IDL for nsIURI and company claims that it's not guaranteed.  If something _does_ guarantee that now, then yeah, GetAsciiSpec might do the right thing and we should update the documentation.  We'd just need to audit all callers of GetSpec and convert them to GetAsciiSpec as needed, then.

Anyway, looks like we maybe have a "network.standard-url.escape-utf8" preference that controls the escaping behavior.  Defaults to true.

Comment 14

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

Andrew, can we get someone to take this?
From what I can tell no other work needs to be done necko-side.
GetAsciiSpec should work in all cases. The standard-url.escape-utf8 pref is set to be removed in bug 1099194.
It shouldn't be too difficult to get nsContentUtils::GetHostOrIPv6WithBrackets to use GetAsciiHost, and URL.cpp and Link.cpp to use the GetAscii* variants of the nsIURI methods.
We also need to make sure the security checks do the same.

Comment 15

[Andrew Overholt [:overholt]]

baku's been doing some URL stuff recently (IIRC) ...

Comment 16

[Andrea Marchesini [:baku]]

Valentin, does this mean that we can use nsIURI::GetAsciiHost everywhere when we use nsIURI::GetHost?
Or do we want to do it only to what is exposed to content?

Comment 17

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

Location.href and URL.href/hostname etc - should definitely be using GetAsciiSpec/GetAsciiHost.
For internal code, it may vary for each callsite, but IMO we would prefer using the Ascii variants for most things.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Anything that goes to the web needs to be consistent.  If we switch to GetAsciiHost we need to use GetAsciiSpec instead of GetSpec as well, as Valentin says.

I suspect we never want to use GetHost and GetSpec in practice except maybe for logging code or something.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: url.patch

This patch almost green on try... it breaks a IDN test because it wants to have 'domain' with unicode chars, instead having the ascii version of it. I'm fixing it together with other tests when treeherder finishes to run them all.

Mainly I replaced:

GetHost => GetAsciiHost
GetSpec => GetAsciiSpec
GetHostPort => GetAsciiHostPort

everywhere except for logging.

We need an ascii version of nsIURI::specIgnoringRef.

Comment 20

[Boris Zbarsky [:bzbarsky]]

So here's another question... What would break if we just made GetHost/Spec/HostPort aliases for the ASCII versions?  If we only use the other in logging, it might not be that big a deal to just log stuff in punycode....

Comment 21

[Andrea Marchesini [:baku]]

With my patch (that is more or less what you propose but without make those function alisas of their ASCII versions) we unexpectedly pass some WPTs and we break only 1 test: dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html (and this is going to be fixed soon).
But I don't know if we have enough tests for punycode.

Comment 22

[Boris Zbarsky [:bzbarsky]]

valentin, thoughts on comment 20?

Comment 23

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

Theoretically it shouldn't break too many things. Indeed, a lot of WPT tests will actually start passing. The only things I expect would break would be the location bar and dev tools handling of URLs (they prefer showing unicode domain names, instead of punycode).

But I think this could be a good way forward.
1. Forward the GetHost/Spec/HostPort to the ASCII versions.
2. Add GetUnicodeHost/Spec/HostPort to have non-ascii alternatives that we can use
3. Optimize the ASCII methods in nsStandardURL - currently they are a bit slower.

CCing Patrick and Honza in case they have any opinion on this matter.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Location bar doesn't use GetSpec directly anyway.  It uses UnEscapeURIForUI.

So I guess we may still want some accessor (the GetUnicodeSpec thing seems reasonable) that gets the IDN domain name for the few cases that then proceed to call UnEscapeURIForUI.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8770451 [details] [diff] [review]
url.patch

This is almost certainly not sufficient; I don't see any changes to JS .spec users here... :(

Comment 26

[Andrea Marchesini [:baku]]

valentin, are you go do implement what you suggested in comment 23?
If you do the necko part, we can work together on the broken mochitests.

Comment 27

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

Yes, I'll do a try push in a few minutes, and we'll see what breaks.
Thanks for offering to help with that :)

Comment 28

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

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

Comment 29

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

Attachment: bug945240-use-ascii.patch

WIP patch. Needs comments, cleaning up

Comment 30

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

Attachment: bug945240-fix-tests.patch

:baku, can you help with also fixing:
test_setting_document.domain_idn.html
test_postMessage_idn.xhtml
test_postMessage_origin.xhtml
browser_identity_UI.js

Thanks

Comment 31

[Andrea Marchesini [:baku]]

Valentin, I have some sec-critical bugs to fix before helping with this. How urgent is this?

Comment 32

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

Not very urgent. I'm almost done with the main patch, and I don't think we can land it without fixing the display of URIs in the location bar as well. Some of these tests I think we'll disable anyway, since they're no longer applicable.

Comment 33

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=039b8945aa38

Comment 34

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

Attachment: Make nsIURI.host & variants return ASCII strings

* nsStandardURL::GetHost/GetHostPort/GetSpec contain an punycode encoded hostname.
* Added nsIURI::GetDisplayHost/GetDisplayHostPort/GetDisplaySpec which have unicode hostnames, depending on the hostname, character blacklist and the network.IDN_show_punycode pref
* remove mHostEncoding since it's not needed anymore (the hostname is always ASCII encoded)
* Add mCheckedIfHostA to know when GetDisplayHost can return the regular host, or when we need to use the cached mDisplayHost

MozReview-Commit-ID: 4qV9Ynhr2Jl

Comment 35

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

Attachment: Make nsIURI.host & variants return ASCII strings

Rebased on m-c

Comment 36

[Andrea Marchesini [:baku]]

Attachment: test2.patch

Comment 37

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

Comment on attachment 8799363 [details] [diff] [review]
test2.patch

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

Looks good to me. You might want to run it by a dom peer as well.

Comment 38

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

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

Comment 39

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

Attachment: Fix tests that use nsIURI.host expecting unicode domain name

MozReview-Commit-ID: GKnbpc8GMb2

Comment 40

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

Comment on attachment 8799449 [details] [diff] [review]
Fix tests that use nsIURI.host expecting unicode domain name

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

r? Olli for dom & docshell, Honza for netwerk and PSM.
Honza, is the PSM test still checking for the right thing? nsIURI.asciiHost and nsIURI.host will return the same thing now, the ASCII domain name, and .displayHost/Port/Spec the unicode variant.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8799449 [details] [diff] [review]
Fix tests that use nsIURI.host expecting unicode domain name

>   else
>   {
>     // We're receiving data from the Greek IDN name; since that TLD is
>     // whitelisted for now, the domain we get isn't going to be punycoded.
>-    is(evt.origin, "http://sub1.ÏαÏάδειγμα.δοκιμή", "wrong sender");
>+    is(evt.origin, "http://sub1.xn--hxajbheg2az3al.xn--jxalpdlp", "wrong sender");
Could you fix also the comment.

Comment 42

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

Making a note to look into this.
(00:51:25) MattN: going back to test_disabled_hosts.js… the main thing is that we're doing normalization to be backwards compatible with extensions and other callers
(00:52:32) MattN: and we should preserve that compat.
Theoretically shouldn't be a problem since all URIs will have the same behaviour. Should probably add a pref to revert to the old behaviour just in case we get a regression from this.

Comment 43

[Andrea Marchesini [:baku]]

Valentin,  can you land my patch together with yours?

Comment 44

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

Dao, can we get your help with the URL bar?
The patches change nsIURI.host .hostPort and .spec to return the ASCII version.
The URL bar should use .displayHost .displayHostPort and .displaySpec instead.

Comment 45

[Boris Zbarsky [:bzbarsky]]

The URL bar is using nsTextToSubURI::UnEscapeURIForUI, no?  Similar for other things that do UI display of URIs.

I don't see your patches fixing that function; they should.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Oh, and I did say that in comment 6...

Comment 47

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

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

Comment 48

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

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #45)
> The URL bar is using nsTextToSubURI::UnEscapeURIForUI, no?  Similar for
> other things that do UI display of URIs.
> I don't see your patches fixing that function; they should.

AFAICT UnEscapeURIForUI doesn't know anything about punycode vs unicode, or even which segment of the URI it's unescaping, it just makes sure no blacklisted characters are present. The important thing is that we call it for strings that come from .displaySpec/Host.
I did a mass replace in browser.js, and that seems to work, but we need to audit all the callsites, and maybe fix the extra UI tests that would be failing.

Comment 49

[Honza Bambas (:mayhemer)]

Comment on attachment 8799449 [details] [diff] [review]
Fix tests that use nsIURI.host expecting unicode domain name

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

::: dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html
@@ +106,5 @@
>    }
>    else
>    {
>      // We're receiving data from the Greek IDN name; since that TLD is
>      // whitelisted for now, the domain we get isn't going to be punycoded.

does this comment need an update?

Comment 50

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63d9ca379eca

Comment 51

[Manish Goregaokar [:manishearth]]

Updates on landing this?

Comment 52

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61ea89acfbe4b1c51b6fd1dd34bbc20214476776

Comment 53

[Boris Zbarsky [:bzbarsky]]

Note also https://github.com/whatwg/html/issues/2568 and whether location should match that...

Comment 54

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4be8cc9c78760c1214c836ee0d3226e389ccee3a

Comment 55

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

Attachment: Bug 945240 - Make nsIURI.host & variants return ASCII strings

* nsStandardURL::GetHost/GetHostPort/GetSpec contain an punycode encoded hostname.
* Added nsIURI::GetDisplayHost/GetDisplayHostPort/GetDisplaySpec which have unicode hostnames, depending on the hostname, character blacklist and the network.IDN_show_punycode pref
* remove mHostEncoding since it's not needed anymore (the hostname is always ASCII encoded)
* Add mCheckedIfHostA to know when GetDisplayHost can return the regular host, or when we need to use the cached mDisplayHost

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

Comment 56

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

Attachment: Bug 945240 - Add unimplemented GetDisplay* methods to RustURL

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

Comment 57

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

Attachment: Bug 945240 - Temporarily set the network.standard-url.punycode-host pref to false

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

Comment 58

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

Attachment: Bug 945240 - Add test checking that URL.origin returns punycode

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

Comment 59

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

Attachment: Bug 945240 - Add unit test for new behaviour

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

Comment 60

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

Attachment: Bug 945240 - Fix web platform tests meta to expect punycode encoding

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

Comment 61

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

Comment on attachment 8794637 [details] [diff] [review]
Make nsIURI.host & variants return ASCII strings

Obsoleting as rebased patch is now in reviewboard.

Comment 62

[Anne (:annevk)]

Comment on attachment 8866761 [details]
Bug 945240 - Fix web platform tests meta to expect punycode encoding

https://reviewboard.mozilla.org/r/138372/#review141560

This looks good. I would have expected more results to change, but I guess IDNA testing is still spotty since it's unclear how the details should work still.

Comment 63

[Patrick McManus [:mcmanus]]

Comment on attachment 8866757 [details]
Bug 945240 - Add unimplemented GetDisplay* methods to RustURL

https://reviewboard.mozilla.org/r/138364/#review141576

Comment 64

[Patrick McManus [:mcmanus]]

Comment on attachment 8866756 [details]
Bug 945240 - Make nsIURI.host & variants return ASCII strings

https://reviewboard.mozilla.org/r/138362/#review141578

lgtm - the tests help :)

Comment 65

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8866761 [details]
Bug 945240 - Fix web platform tests meta to expect punycode encoding

https://reviewboard.mozilla.org/r/138372/#review141866

You probably need to fix the html/dom/self-origin.sub.html web platform test too.  The only reason it didn't fail for you is that we haven't synced wpt in a while and hence haven't picked up https://github.com/w3c/web-platform-tests/commit/2ea51e5825a5d237f49e36f205bee17f897d66fa (which you may want to just include).

Comment 66

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8866758 [details]
Bug 945240 - Temporarily set the network.standard-url.punycode-host pref to false

https://reviewboard.mozilla.org/r/138366/#review141982

Comment 67

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8866759 [details]
Bug 945240 - Add test checking that URL.origin returns punycode

https://reviewboard.mozilla.org/r/138368/#review141984

I should not review this.

Comment 68

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8866760 [details]
Bug 945240 - Add unit test for new behaviour

https://reviewboard.mozilla.org/r/138370/#review141986

Comment 69

[:Gijs (he/him)]

I won't be able to properly (mostly: non-sleepily) review the effect this stuff has on browser/ until Monday. One thing I can think of offhand: do we need to update createExposableURI, and/or what happens with these patches when using reader mode (particularly, we use documentURI (which I guess uses documentURIObject.spec!) in https://dxr.mozilla.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm#478 to display a domain name in the content). I expect there may be other places...

If there's urgency, looks like Matt might have more context, or Dão or Marco might be quicker.

Comment 70

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

(In reply to :Gijs from comment #69)
> I won't be able to properly (mostly: non-sleepily) review the effect this
> stuff has on browser/ until Monday. One thing I can think of offhand: do we
> need to update createExposableURI, and/or what happens with these patches
> when using reader mode (particularly, we use documentURI (which I guess uses

It didn't seem to hit trigger any tests, but it's definitely possible.
Anyway, we can easily fix it by replacing .spec/host with .displaySpec/displayHost where needed.

> If there's urgency, looks like Matt might have more context, or Dão or Marco
> might be quicker.
I don't think there's any urgency, except for bitrot. Feel free to CC them on the patch if they can any add anything to the conversation.

Comment 71

[:Gijs (he/him)]

Comment on attachment 8866759 [details]
Bug 945240 - Add test checking that URL.origin returns punycode

https://reviewboard.mozilla.org/r/138368/#review142584

We should also update:

https://dxr.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46/browser/base/content/tabbrowser.xml#1440-1447

https://dxr.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46/browser/base/content/tabbrowser.xml#7668

https://dxr.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46/browser/base/content/browser-places.js#415

everything in https://dxr.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46/browser/base/content/urlbarBindings.xml#804 (did you test copy/paste ? Does this pass try with mochitest-bc ? That would be surprising...)

https://dxr.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46/browser/base/content/utilityOverlay.js#928

This also produces punycode in the bookmarks popup and other bookmarks UI, so presumably those need updating too. If you need more details about what to do there, I would suggest asking ::mak - he should probably also review that patch separately.


and maybe:

https://dxr.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46/browser/base/content/browser.js#6584

https://dxr.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46/browser/base/content/browser.js#l7635 (splitting out the 'real' href and the displayed one)

https://dxr.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46/browser/base/content/browser.js#8027-8028,8032-8033

https://dxr.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46/browser/base/content/urlbarBindings.xml#211

https://dxr.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46/browser/base/content/urlbarBindings.xml#1291-1293 (also splitting out href vs. what is displayed, at a minimum)

https://dxr.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46/browser/components/downloads/content/allDownloadsViewOverlay.js#1369

https://dxr.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46/browser/components/downloads/content/downloads.js#1069


and then here we need to check what IE/Edge do:

https://dxr.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46/browser/components/migration/IEProfileMigrator.js#68
because of:
https://dxr.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46/browser/components/migration/MSMigrationUtils.jsm#674


and that's what I found when looking only in non-test browser/ paths. I'm fairly sure there's more lurking, but I have a lot of other reviews to get to and this is already so much that I think this shouldn't land as-is. :-(

Comment 72

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f59e7a80dd6af29c424a376d2c0802166f1cc78

Comment 73

[Anne (:annevk)]

It seems you did not change all origin getters for this. E.g., WPT html/dom/self-origin.sub.html was not affected: https://github.com/w3c/web-platform-tests/issues/5955.

I'm not sure if we want to change all of them in this bug, but we should change all of them so a follow-up might be warranted depending on how you want to tackle things.

Comment 74

[Boris Zbarsky [:bzbarsky]]

> E.g., WPT html/dom/self-origin.sub.html was not affected

It's affected.  The patches here were just not tested against a version of wpt that would detect the difference: see comment 65.

As in, if these patches land in their current form we will start failing html/dom/self-origin.sub.html as it currently exists upstream, but not as it currently exists in our tree, because the latter doesn't test what it thinks it's testing.

Comment 75

[Anne (:annevk)]

Great! Hopefully me changing it upstream is sufficient then, but if you want to take that change here that's also fine with me.

Comment 76

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

Attachment: Bug 945240 - Change firefox code to use uri.displaySpec when unicode URLs are wanted

* Make URL bar show unicode URL by making losslessDecodeURI call uri.displaySpec
* Make sure URL bar copying returns unicode variant

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

Comment 77

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

Attachment: Bug 945240 - Add a pref to be able to restore previous behaviour where nsIURI.host/.spec returned unicode instead of punycode

This is to deal with possible bugs or web-compat issues that may arrise.

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

Comment 78

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

Comment on attachment 8866756 [details]
Bug 945240 - Make nsIURI.host & variants return ASCII strings

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

Comment 79

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

Comment on attachment 8866757 [details]
Bug 945240 - Add unimplemented GetDisplay* methods to RustURL

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

Comment 80

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

Comment on attachment 8866758 [details]
Bug 945240 - Temporarily set the network.standard-url.punycode-host pref to false

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

Comment 81

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

Comment on attachment 8866760 [details]
Bug 945240 - Add unit test for new behaviour

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

Comment 82

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

Comment on attachment 8866761 [details]
Bug 945240 - Fix web platform tests meta to expect punycode encoding

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

Comment 83

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

Comment on attachment 8866759 [details]
Bug 945240 - Add test checking that URL.origin returns punycode

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

Comment 84

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

(In reply to :Gijs from comment #71)
> (did you test copy/paste ? Does this pass try with mochitest-bc ?
> That would be surprising...)

We don't have great test coverage for unicode URLs. I added one.

> https://dxr.mozilla.org/mozilla-central/rev/
> e66dedabe582ba7b394aee4f89ed70fe389b3c46/browser/base/content/utilityOverlay.
> js#928
> 
> This also produces punycode in the bookmarks popup and other bookmarks UI,
> so presumably those need updating too. If you need more details about what
> to do there, I would suggest asking ::mak - he should probably also review
> that patch separately.

Indeed, there are a few other UI issues I'm missing. I'll submit a separate patch for this.

> https://dxr.mozilla.org/mozilla-central/rev/
> e66dedabe582ba7b394aee4f89ed70fe389b3c46/browser/base/content/urlbarBindings.
> xml#1291-1293 (also splitting out href vs. what is displayed, at a minimum)

Not exactly sure what you meant by slplitting out - I changed the instances to use displaySpec.

> content/allDownloadsViewOverlay.js#1369
> content/downloads.js#1069

I didn't change these two since they deal with file URIs.

> 
> and then here we need to check what IE/Edge do:
> 
> IEProfileMigrator.js#68
> MSMigrationUtils.jsm#674

I don't know the code too well, but it seems to me that it doesn't matter if the URLs are unicode or punycode in this case.

Comment 85

[:Gijs (he/him)]

So, I originally assumed, based on the first patch and the bug summary, that this affected nsIURI.spec and location.href, but not location.hostname / nsIURI.host. However, the first patch sure looks like this also affects nsIURI.host. In which case, this is going to affect a lot more frontend code. :-(

Am I just misreading things? What about principal origins and nsIURI.prePath and such?

Because the browser frontend is generally what displays stuff, I expect there'll be a long tail here, from the preferences / permissions / page info style stuff to notification bars ("we searched for föo, did you mean to go to http://föo ?"), the identity popup, and all kinds of other browser UI...


Would it be possible to split the frontend patches out more and find appropriate reviewers?

Comment 86

[:Gijs (he/him)]

(In reply to Valentin Gosu [:valentin] from comment #84)
> > https://dxr.mozilla.org/mozilla-central/rev/
> > e66dedabe582ba7b394aee4f89ed70fe389b3c46/browser/base/content/urlbarBindings.
> > xml#1291-1293 (also splitting out href vs. what is displayed, at a minimum)
> 
> Not exactly sure what you meant by slplitting out - I changed the instances
> to use displaySpec.

I meant that we're generating HTML, and so I kind of assumed that maybe you want to generate:

<a href="punycode-href">pretty-href</a>

But if we're confident the unicode href works x-platform then we can just use displaySpec throughout.

> > and then here we need to check what IE/Edge do:
> > 
> > IEProfileMigrator.js#68
> > MSMigrationUtils.jsm#674
> 
> I don't know the code too well, but it seems to me that it doesn't matter if
> the URLs are unicode or punycode in this case.

Basically we're doing string compares with stuff IE/Edge have plugged in the registry to see if the user has typed a given URL in the IE/Edge location bar (vs. just visited it). The string compares will obviously fail if one is punycode and the other is unicode. I don't know what IE/Edge do. Status quo would be switching to displaySpec (though a followup wouldn't hurt).

Comment 87

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

(In reply to :Gijs from comment #85)
> Am I just misreading things? What about principal origins and nsIURI.prePath
> and such?

Principals are not affected, as they already use GetAsciiHost and GetAsciiSpec.

All nsIURI methods that contain the hostname - including prePath - are affected.
If needed, I can add a .displayPrePath as well.

> Would it be possible to split the frontend patches out more and find
> appropriate reviewers?

Yes, that's probably necessary. But I could use some help finding the places in the UI that are affected by this change.

Comment 88

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8866759 [details]
Bug 945240 - Add test checking that URL.origin returns punycode

https://reviewboard.mozilla.org/r/138368/#review143534

Do we have similar wpt tests?

Comment 89

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

(In reply to Olli Pettay [:smaug] from comment #88)
> Comment on attachment 8866759 [details]
> Bug 945240 - Add test checking that URL.origin returns punycode
> 
> https://reviewboard.mozilla.org/r/138368/#review143534
> 
> Do we have similar wpt tests?

There are a few, and Anne is working on adding more.

Comment 90

[Anne (:annevk)]

See https://github.com/whatwg/html/pull/2689#issuecomment-302074202 for a list of WPT pull requests that add tests for the various cases affected. Reviews appreciated.

Comment 91

[:Gijs (he/him)]

(In reply to Valentin Gosu [:valentin] from comment #87)
> (In reply to :Gijs from comment #85)
> > Would it be possible to split the frontend patches out more and find
> > appropriate reviewers?
> 
> Yes, that's probably necessary. But I could use some help finding the places
> in the UI that are affected by this change.

What I did for the list in comment #71 was to use DXR and look for '.spec', excluding things with path:test , and then auditing manually. This is painful and takes a long time, but I don't really have a better idea. You'd have to do the same for .host and .prePath and any others ('.host' already finds '.hostPort', and '.spec' finds '.specIgnoringRef' so not sure there are any).

In a general sense, places that are affected is going to be "anything that displays a URL or hostname" and there's quite a lot of those places...

To start with, I expect we'd want to update the identity panel, permissions doorhangers, the popup blocker code, the "did you mean to go to 'foo'?" notification bar, the various dialogs in about:preferences that display cookies/indexeddb/localstorage/appcache/whatever data by host, probably the page info dialog, and then bookmarks/history/synced tabs. Devtools probably will also want changes. But all that's just off the top of my head, and I expect there's more that I've forgotten about.

Comment 92

[:Gijs (he/him)]

Comment on attachment 8868616 [details]
Bug 945240 - Change firefox code to use uri.displaySpec when unicode URLs are wanted

https://reviewboard.mozilla.org/r/140202/#review143526

r=me in the sense that these changes themselves look OK, but I think a lot more stuff will want changing.

::: browser/base/content/test/urlbar/browser_urlbarCopying.js:140
(Diff revision 1)
>    },
>    {
>      copyVal: "<example.com/\xe9>\xe9",
>      copyExpected: "http://example.com/\xe9"
>    },
> +  { // Note: it seems BrowserTestUtils.loadURI fails for unicode domains

Can you elaborate on what 'fails' means here? I know that there's URI spec checking in some places in BrowserTestUtils - perhaps some of those need updating?

::: browser/base/content/test/urlbar/browser_urlbarCopying.js:227
(Diff revision 1)
>    if (testCase.setup) {
>      testCase.setup();
>    }
>  
>    if (testCase.loadURL) {
> +    info(`Loading : ${testCase.loadURL}\n`);

Shouldn't need an extra newline in `info`.

Comment 93

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

(In reply to :Gijs from comment #92)
> > +  { // Note: it seems BrowserTestUtils.loadURI fails for unicode domains
> 
> Can you elaborate on what 'fails' means here? I know that there's URI spec
> checking in some places in BrowserTestUtils - perhaps some of those need
> updating?

I didn't look to much into it. The issue is definitely in BrowserTestUtils.
A few times, when I tried to load a unicode URL, it actually loaded a different one. I think some UTF8->UTF16 transformations are failing.

Comment 94

[:Gijs (he/him)]

(In reply to Valentin Gosu [:valentin] from comment #93)
> (In reply to :Gijs from comment #92)
> > > +  { // Note: it seems BrowserTestUtils.loadURI fails for unicode domains
> > 
> > Can you elaborate on what 'fails' means here? I know that there's URI spec
> > checking in some places in BrowserTestUtils - perhaps some of those need
> > updating?
> 
> I didn't look to much into it. The issue is definitely in BrowserTestUtils.
> A few times, when I tried to load a unicode URL, it actually loaded a
> different one. I think some UTF8->UTF16 transformations are failing.

This is confusing - BTU is a simple JSM that calls "normal" browser/docshell/webnavigation APIs. I don't think it does any character encoding/transformations itself: https://dxr.mozilla.org/mozilla-central/rev/85e5d15c31691c89b82d6068c26260416493071f/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#478

Comment 95

[Boris Zbarsky [:bzbarsky]]

Speaking of devtools, what do we plan to have Error.stack and stacks in devtools showing?  That is, things that come from the JS script "filename"?  I assume we're changing its behavior as well?

Comment 96

[Patrick McManus [:mcmanus]]

Comment on attachment 8868617 [details]
Bug 945240 - Add a pref to be able to restore previous behaviour where nsIURI.host/.spec returned unicode instead of punycode

https://reviewboard.mozilla.org/r/140204/#review144640

Comment 97

[:shell escalante]

Hi Valentin,

I looks like all the reviews are in.  Is there something blocking landing the fixes?  This resolution would go a long way towards uBlock Origyn being upgraded to a WebExtension.

Comment 98

[:Gijs (he/him)]

(In reply to :shell escalante from comment #97)
> Hi Valentin,
> 
> I looks like all the reviews are in.  Is there something blocking landing
> the fixes?  This resolution would go a long way towards uBlock Origyn being
> upgraded to a WebExtension.

See comment #92 / comment 85.

Comment 99

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

I could land it as it is, but there is still a lot of code in the Firefox frontend that hasn't been fixed. I'm considering doing a mass-replace of .host/.spec/etc to .displayHost/.displaySpec/etc (and need to add .displayPrePath)

Comment 100

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

Ok, I've tried doing the mass replace thing, but it may introduce further regressions if for example we key something in C++ code using the asciiSpec, and the JS code is using displaySpec to search for it. Also, the patches tend to bitrot quite quickly.

My new approach is to land these changes preffed off, then a few days later land the pref change along with the tests, and handle each regression individually (either by opening bugs or by fixing them myself). For each one of these, I think we should also add unit tests, as IDNA is severely under-tested in our code-base.

Comment 101

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43a6eda6e54cb8329e991fc0a5f7108a5408536b

Comment 102

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=25689bac6bad65a2d2478e0e16aeb7ed5569a416

Comment 103

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=972c701b117d9394e00dc88adccf136734b7d5f9

Comment 104

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bc9ca82227a6bc9b273855590138cd2686ca5dc

Comment 105

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

Comment on attachment 8866756 [details]
Bug 945240 - Make nsIURI.host & variants return ASCII strings

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

Comment 106

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

Comment on attachment 8866757 [details]
Bug 945240 - Add unimplemented GetDisplay* methods to RustURL

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

Comment 107

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

Comment on attachment 8868617 [details]
Bug 945240 - Add a pref to be able to restore previous behaviour where nsIURI.host/.spec returned unicode instead of punycode

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

Comment 108

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

Comment on attachment 8866758 [details]
Bug 945240 - Temporarily set the network.standard-url.punycode-host pref to false

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

Comment 109

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

Comment on attachment 8866760 [details]
Bug 945240 - Add unit test for new behaviour

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

Comment 110

[Patrick McManus [:mcmanus]]

Comment on attachment 8866760 [details]
Bug 945240 - Add unit test for new behaviour

https://reviewboard.mozilla.org/r/138370/#review161694

Comment 111

[Patrick McManus [:mcmanus]]

Comment on attachment 8866758 [details]
Bug 945240 - Temporarily set the network.standard-url.punycode-host pref to false

https://reviewboard.mozilla.org/r/138366/#review161698

Comment 112

[Patrick McManus [:mcmanus]]

I'm not sure I'm on board with the "turn it on and fix things that are reported.. plan" but landing it off is certainly ok.

Comment 113

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/35d7c15f5df2
Make nsIURI.host & variants return ASCII strings r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/abd19b1b5859
Add unimplemented GetDisplay* methods to RustURL r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/905c1230c0a9
Add a pref to be able to restore previous behaviour where nsIURI.host/.spec returned unicode instead of punycode r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/2dca6b785721
Temporarily set the network.standard-url.punycode-host pref to false r=mcmanus,smaug
https://hg.mozilla.org/integration/autoland/rev/82e7b5e70e23
Add unit test for new behaviour r=mcmanus,smaug

Comment 114

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

(In reply to Patrick McManus [:mcmanus] from comment #112)
> I'm not sure I'm on board with the "turn it on and fix things that are
> reported.. plan" but landing it off is certainly ok.

Thanks for the review!
Changing the title of this bug to better reflect what landed.
Will open new bug for the extra work to expose this changes to outside code.

Comment 115

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

https://hg.mozilla.org/mozilla-central/rev/35d7c15f5df2
https://hg.mozilla.org/mozilla-central/rev/abd19b1b5859
https://hg.mozilla.org/mozilla-central/rev/905c1230c0a9
https://hg.mozilla.org/mozilla-central/rev/2dca6b785721
https://hg.mozilla.org/mozilla-central/rev/82e7b5e70e23

Comment 116

[Boris Zbarsky [:bzbarsky]]

The various dependencies of this bug should probably move to the new bug...

Comment 117

[Chris Mills [:cmills]]

I've had a go at writing a note about this in the Fx56 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/56#Other

I wasn't sure where else to mention this - it's a bug fix really, and there are so many functions that use URLs where this could be mentioned...

Let me know if you think anything else is desperately needed.