expose nsIURI::GetHost() "display normalization" behavior on nsIIDNService

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: dwitte@gmail.com, Assigned: dwitte@gmail.com)

Tracking

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

10 years ago
when constructing an nsIURI via nsStandardURL, normalization is performed on the hostname (see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/base/src/nsStandardURL.cpp&rev=1.101&root=/cvsroot#393). the result will be either UTF8 or ACE, depending on the network.IDN_show_punycode pref, the IDN whitelist, whether the UTF8 hostname contains blacklisted chars, etc. - a so-called "display IDN" encoding. when using nsIURI::GetHost(), this is the representation that will be returned. the implementation of this normalization is buried deep in the guts of nsStandardURL, making it hard to duplicate by consumers. in addition, the outcome of this encoding can change with time, e.g. by the user changing that pref or by TLD's being added to/removed from the IDN whitelist. thus, internal code uses a more consistent representation (such as nsIURI::GetAsciiHost()) when storing this data cross-session (e.g. cookies).

having this normalization method exposed on an interface, e.g. as nsIIDNService::ConvertToDisplayIDN(), would allow consumers to convert these strings back when they want to display them. for instance, in cookieviewer, it'd be nice for consistency to have the displayed host match what you'd see in the urlbar. currently it'll always be ASCII/ACE. the same should go for permissions and probably passwords.

i have a patch that moves the relevant nsStandardURL code onto nsIDNService, including the network.IDN_show_punycode pref and the IDN whitelist.
(Assignee)

Updated

10 years ago
Assignee: nobody → dwitte
(Assignee)

Comment 1

10 years ago
Created attachment 287065 [details] [diff] [review]
patch v1

first-cut patch to move code from nsStandardURL to nsIDNService, and add an nsIIDNService::ConvertToDisplayIDN() method.

biesi, do you have any comments on this idea?
Attachment #287065 - Flags: review?(cbiesinger)
(Assignee)

Comment 2

10 years ago
aside: there's a call to net_ToLowerCase at
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/netwerk/base/src/nsStandardURL.cpp&rev=1.101&root=/cvsroot#1486
which may no longer be necessary, since it comes after the normalization/lowercasing.
(Assignee)

Comment 3

10 years ago
Mardak, if this lands you might want to use it in bug 223895 to display the eTLD output in a manner consistent with GetAsciiHost().
(Assignee)

Comment 4

10 years ago
er, s/Ascii//, of course!
(Assignee)

Updated

10 years ago
Blocks: 367799
(Assignee)

Comment 5

10 years ago
this would be useful in bug 367799.
(Assignee)

Comment 6

10 years ago
Comment on attachment 287065 [details] [diff] [review]
patch v1

meant to ask for r+sr here ;)
Attachment #287065 - Flags: superreview?(cbiesinger)
Comment on attachment 287065 [details] [diff] [review]
patch v1

+      *_isASCII = NS_FAILED(rv);

NS_FAILED isn't actually a boolean, it's either 0 or 0x80000000, so you should make this NS_FAILED(rv) != 0 or something

also, remove the empty line after this one

+    }
+
+  } else {

and the one here

I don't really like the DisplayIDN name but I can't think of anything better, so r+sr=biesi
Attachment #287065 - Flags: superreview?(cbiesinger)
Attachment #287065 - Flags: superreview+
Attachment #287065 - Flags: review?(cbiesinger)
Attachment #287065 - Flags: review+
(Assignee)

Comment 8

10 years ago
Comment on attachment 287065 [details] [diff] [review]
patch v1

thanks! will fix up nits on checkin.

requesting approval1.9, patch is just shuffling code around into a more accessible place, low risk.
Attachment #287065 - Flags: approval1.9?

Updated

10 years ago
Attachment #287065 - Flags: approval1.9? → approval1.9+
(In reply to comment #7)
> (From update of attachment 287065 [details] [diff] [review])
> +      *_isASCII = NS_FAILED(rv);
> 
> NS_FAILED isn't actually a boolean, it's either 0 or 0x80000000, so you should
> make this NS_FAILED(rv) != 0 or something

NS_FAILED does return a boolean - it uses NS_UNLIKELY.
(Assignee)

Comment 10

10 years ago
Created attachment 291366 [details] [diff] [review]
patch as checked in

checked in, with one minor change; shifted the lowercasing in the ASCII case above where we check the whitelist, so hosts like "ella.ES" will still be whitelisted.
Attachment #287065 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

10 years ago
Created attachment 291388 [details] [diff] [review]
patch as really checked in

now without crash on startup!
Attachment #291366 - Attachment is obsolete: true
(Assignee)

Comment 12

10 years ago
added test for this method to mozilla/netwerk/test/unit/test_idnservice.js rev 1.2.
Flags: in-testsuite+

Updated

10 years ago
Depends on: 427957
You need to log in before you can comment on or make changes to this bug.