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

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[[e-mail]]

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.

Comment 1

[[e-mail]]

Attachment: 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?

Comment 2

[[e-mail]]

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.

Comment 3

[[e-mail]]

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

Comment 4

[[e-mail]]

er, s/Ascii//, of course!

Comment 5

[[e-mail]]

this would be useful in bug 367799.

Comment 6

[[e-mail]]

Comment on attachment 287065 [details] [diff] [review]
patch v1

meant to ask for r+sr here ;)

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 8

[[e-mail]]

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.

Comment 9

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

(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.

Comment 10

[[e-mail]]

Attachment: 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.

Comment 11

[[e-mail]]

Attachment: patch as really checked in

now without crash on startup!

Comment 12

[[e-mail]]

added test for this method to mozilla/netwerk/test/unit/test_idnservice.js rev 1.2.