Change nsStandardURL's internal hostname representation to punycode

RESOLVED FIXED in Firefox 56

Status

()

Core
Networking
RESOLVED FIXED
4 years ago
18 days ago

People

(Reporter: bz, Assigned: valentin)

Tracking

(Blocks: 4 bugs, {addon-compat, dev-doc-complete})

unspecified
mozilla56
x86
Mac OS X
addon-compat, dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [necko-next] btpp-active)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 10 obsolete attachments)

59 bytes, text/x-review-board-request
mcmanus
: review+
Details | Review
59 bytes, text/x-review-board-request
mcmanus
: review+
Details | Review
59 bytes, text/x-review-board-request
mcmanus
: review+
Details | Review
59 bytes, text/x-review-board-request
mcmanus
: review+
Details | Review
59 bytes, text/x-review-board-request
mcmanus
: review+
Details | Review
See discussion in bug 942074.

Gavin, how much would this break Firefox/extensions?
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?
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...
Keywords: addon-compat
Jason, could we add an nsIURI API like GetASCIISpec but that only converts the hostname to ASCII?
Flags: needinfo?(jduell.mcbugs)
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.
bz: sure, you can add a a new field/method to nsIURI.  I don't see why that would be a problem.
Flags: needinfo?(jduell.mcbugs)
Blocks: 1273168
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.  :(
Flags: needinfo?(jduell.mcbugs)
Andrew, see comment 6.
Flags: needinfo?(overholt)
> 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).
Flags: needinfo?(jduell.mcbugs) → needinfo?(valentin.gosu)
(Assignee)

Comment 9

a year ago
Sure I'll take it. This seems like a major issue for web-compat.
Assignee: nobody → valentin.gosu
Flags: needinfo?(valentin.gosu)
Whiteboard: [necko-active]
The DOM side mostly depends on what exactly we do on the URI side.
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.
Flags: needinfo?(overholt)
Whiteboard: [necko-active] → [necko-active] btpp-active
(Assignee)

Comment 12

a year ago
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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 14

a year ago
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.
Assignee: valentin.gosu → nobody
Flags: needinfo?(overholt)
baku's been doing some URL stuff recently (IIRC) ...
Flags: needinfo?(overholt) → needinfo?(amarchesini)
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?
Flags: needinfo?(amarchesini) → needinfo?(valentin.gosu)
(Assignee)

Comment 17

a year ago
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.
Flags: needinfo?(valentin.gosu)
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.
Created attachment 8770451 [details] [diff] [review]
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.
Assignee: nobody → amarchesini
Attachment #8770451 - Flags: review?(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....
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.
valentin, thoughts on comment 20?
Flags: needinfo?(valentin.gosu)
(Assignee)

Comment 23

a year ago
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.
Flags: needinfo?(valentin.gosu)
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 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... :(
Attachment #8770451 - Flags: review?(bzbarsky) → review-
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.
(Assignee)

Comment 27

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

Comment 28

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2549e090f3f
(Assignee)

Comment 29

a year ago
Created attachment 8770883 [details] [diff] [review]
bug945240-use-ascii.patch

WIP patch. Needs comments, cleaning up
(Assignee)

Updated

a year ago
Assignee: amarchesini → valentin.gosu
Status: NEW → ASSIGNED
(Assignee)

Comment 30

a year ago
Created attachment 8770884 [details] [diff] [review]
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
Valentin, I have some sec-critical bugs to fix before helping with this. How urgent is this?
(Assignee)

Comment 32

a year ago
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.
(Assignee)

Comment 33

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=039b8945aa38
(Assignee)

Comment 34

a year ago
Created attachment 8772848 [details] [diff] [review]
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
Attachment #8772848 - Flags: review?(mcmanus)
(Assignee)

Updated

a year ago
Attachment #8770883 - Attachment is obsolete: true
(Assignee)

Comment 35

11 months ago
Created attachment 8794637 [details] [diff] [review]
Make nsIURI.host & variants return ASCII strings

Rebased on m-c
(Assignee)

Updated

11 months ago
Attachment #8772848 - Attachment is obsolete: true
Attachment #8772848 - Flags: review?(mcmanus)
(Assignee)

Updated

11 months ago
Whiteboard: [necko-active] btpp-active → [necko-next] btpp-active
Created attachment 8799363 [details] [diff] [review]
test2.patch
Attachment #8799363 - Flags: review?(valentin.gosu)
(Assignee)

Comment 37

10 months ago
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.
Attachment #8799363 - Flags: review?(valentin.gosu) → review+
(Assignee)

Comment 38

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c668735d5657
(Assignee)

Updated

10 months ago
Attachment #8794637 - Flags: review?(mcmanus)
(Assignee)

Comment 39

10 months ago
Created attachment 8799449 [details] [diff] [review]
Fix tests that use nsIURI.host expecting unicode domain name

MozReview-Commit-ID: GKnbpc8GMb2
(Assignee)

Updated

10 months ago
Attachment #8770884 - Attachment is obsolete: true
(Assignee)

Comment 40

10 months ago
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.
Attachment #8799449 - Flags: review?(honzab.moz)
Attachment #8799449 - Flags: review?(bugs)
(Assignee)

Updated

10 months ago
Attachment #8770451 - Attachment is obsolete: true
Attachment #8799363 - Flags: review?(ehsan)
Keywords: dev-doc-needed

Comment 41

10 months ago
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.
Attachment #8799449 - Flags: review?(bugs) → review+
(Assignee)

Comment 42

10 months ago
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.
Flags: needinfo?(valentin.gosu)
Attachment #8799363 - Flags: review?(ehsan) → review+
Attachment #8794637 - Flags: review?(mcmanus) → review+
Valentin,  can you land my patch together with yours?
(Assignee)

Comment 44

10 months ago
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.
Flags: needinfo?(valentin.gosu) → needinfo?(dao+bmo)
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.
Oh, and I did say that in comment 6...
(Assignee)

Comment 47

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a40f15deee97
(Assignee)

Comment 48

10 months ago
(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 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?
Attachment #8799449 - Flags: review?(honzab.moz) → review+
(Assignee)

Updated

9 months ago
Blocks: 1318426
(Assignee)

Comment 50

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63d9ca379eca
Updates on landing this?
(Assignee)

Updated

3 months ago
Blocks: 414090
(Assignee)

Comment 52

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=61ea89acfbe4b1c51b6fd1dd34bbc20214476776
Note also https://github.com/whatwg/html/issues/2568 and whether location should match that...
(Assignee)

Comment 54

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4be8cc9c78760c1214c836ee0d3226e389ccee3a
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8866757 - Flags: review?(mcmanus)
(Assignee)

Comment 61

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

Obsoleting as rebased patch is now in reviewboard.
Attachment #8794637 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Attachment #8799363 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Attachment #8799449 - Attachment is obsolete: true
Flags: needinfo?(dao+bmo)

Comment 62

3 months ago
mozreview-review
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.
Attachment #8866761 - Flags: review?(annevk) → review+

Comment 63

3 months ago
mozreview-review
Comment on attachment 8866757 [details]
Bug 945240 - Add unimplemented GetDisplay* methods to RustURL

https://reviewboard.mozilla.org/r/138364/#review141576
Attachment #8866757 - Flags: review?(mcmanus) → review+

Comment 64

3 months ago
mozreview-review
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 :)
Attachment #8866756 - Flags: review?(mcmanus) → review+
(Reporter)

Comment 65

3 months ago
mozreview-review
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

3 months ago
mozreview-review
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
Attachment #8866758 - Flags: review?(bugs) → review+

Comment 67

3 months ago
mozreview-review
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.
Attachment #8866759 - Flags: review?(bugs)

Comment 68

3 months ago
mozreview-review
Comment on attachment 8866760 [details]
Bug 945240 - Add unit test for new behaviour

https://reviewboard.mozilla.org/r/138370/#review141986
Attachment #8866760 - Flags: review?(bugs) → review+
(Assignee)

Updated

3 months ago
Attachment #8866759 - Flags: review?(gijskruitbosch+bugs)

Comment 69

3 months ago
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.
(Assignee)

Comment 70

3 months ago
(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

3 months ago
mozreview-review
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. :-(
Attachment #8866759 - Flags: review?(gijskruitbosch+bugs) → review-
(Assignee)

Comment 72

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

Comment 73

3 months ago
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.
> 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

3 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 84

3 months ago
(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

3 months ago
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?
Flags: needinfo?(valentin.gosu)

Comment 86

3 months ago
(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).
(Assignee)

Comment 87

3 months ago
(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

3 months ago
mozreview-review
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?
Attachment #8866759 - Flags: review?(bugs) → review+
(Assignee)

Comment 89

3 months ago
(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.
Flags: needinfo?(valentin.gosu)

Comment 90

3 months ago
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

3 months ago
(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

3 months ago
mozreview-review
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`.
Attachment #8868616 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 93

3 months ago
(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

3 months ago
(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
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

3 months ago
mozreview-review
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
Attachment #8868617 - Flags: review?(mcmanus) → review+
Blocks: 1360285

Updated

2 months ago
Blocks: 1361135

Comment 97

2 months ago
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.
Flags: needinfo?(valentin.gosu)

Comment 98

2 months ago
(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.
(Assignee)

Comment 99

2 months ago
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)
(Assignee)

Comment 100

a month ago
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.
Flags: needinfo?(valentin.gosu)
(Assignee)

Comment 101

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43a6eda6e54cb8329e991fc0a5f7108a5408536b
(Assignee)

Comment 102

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=25689bac6bad65a2d2478e0e16aeb7ed5569a416
(Assignee)

Comment 103

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=972c701b117d9394e00dc88adccf136734b7d5f9
(Assignee)

Comment 104

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bc9ca82227a6bc9b273855590138cd2686ca5dc
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8866761 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8866759 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8868616 - Attachment is obsolete: true

Comment 110

a month ago
mozreview-review
Comment on attachment 8866760 [details]
Bug 945240 - Add unit test for new behaviour

https://reviewboard.mozilla.org/r/138370/#review161694
Attachment #8866760 - Flags: review?(mcmanus) → review+

Comment 111

a month ago
mozreview-review
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
Attachment #8866758 - Flags: review?(mcmanus) → review+
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

a month ago
Pushed by valentin.gosu@gmail.com:
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
(Assignee)

Comment 114

a month ago
(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.
Component: DOM → Networking
Summary: Consider making Location.href punycode non-ASCII hostnames → Change nsStandardURL's internal hostname representation to punycode

Comment 115

a month ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
The various dependencies of this bug should probably move to the new bug...
(Assignee)

Updated

a month ago
Blocks: 1380617
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.
Keywords: dev-doc-needed → dev-doc-complete

Updated

19 days ago
Depends on: 1385883

Updated

18 days ago
Depends on: 1386195

Updated

18 days ago
Depends on: 1386213
You need to log in before you can comment on or make changes to this bug.