Closed Bug 402013 Opened 17 years ago Closed 17 years ago

improve normalization behavior of nsIEffectiveTLDService

Categories

(Core :: Networking, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dwitte, Assigned: dwitte)

References

()

Details

Attachments

(1 file, 1 obsolete file)

followup from https://bugzilla.mozilla.org/show_bug.cgi?id=368989#c47. nsIEffectiveTLDService normalizes hosts to UTF8, in order to perform comparisons with the eTLD host file. this means that what it returns isn't always compatible with nsIURI::GetHost(), or GetAsciiHost() either, without conversion. this places somewhat of a burden on consumers. it also means eTLD has to re-normalize URI's to get them into UTF8, which is a little ugly. given that the normalization of the host and the eTLD host file must match, there are three possible solutions: 1) make eTLD normalize the host file internally to "display IDN", i.e. compatible with nsIURI::GetHost(). this could be done by exposing the normalization method on a public interface (bug 402008). however, given that this normalization can change depending on a pref (i.e. at runtime!) and the IDN whitelist, this seems like a bad idea. 2) make eTLD use nsIURI::GetAsciiHost(), normalize the host file internally to ASCII/ACE, and state as such in the interface documentation. if consumers want to compare the string they get back from eTLD with their URI, they can then use GetAsciiHost() and things should work. (incidentally, this would make nsIEffectiveTLDService::GetPublicSuffic(nsIURI*) the "fast path", and GetPublicSuffixFromHost(nsACString&) do the extra host normalization.) 3) WONTFIX this bug and stick with UTF8, letting the consumer convert if they need to. option 2) makes the most sense to me. if a consumer wants to convert the resulting host back to display IDN, (maybe for... display!), then fixing bug 402008 will take care of that. biesi, dveditz, what do you think?
Attached patch patch v1 (obsolete) (deleted) — Splinter Review
here's a patch that implements the second option, just to get things rolling.
Attachment #287058 - Flags: superreview?(dveditz)
Attachment #287058 - Flags: review?(dveditz)
Blocks: 367799
Blocks: 385299
Attachment #287058 - Flags: superreview?(dveditz)
Attachment #287058 - Flags: review?(dveditz)
Attached patch patch v1.1 (deleted) — Splinter Review
updated patch, include rv check of GetAsciiHost() since this can fail under ordinary circumstances for certain schemes (thanks Mardak!). dveditz, this is blocking the etld-in-cookies bug from landing, any chance you can get to this soon? (thanks!)
Attachment #287058 - Attachment is obsolete: true
Attachment #290945 - Flags: superreview?(dveditz)
Attachment #290945 - Flags: review?(dveditz)
Comment on attachment 290945 [details] [diff] [review] patch v1.1 switching request to biesi, in the hopes of landing this + the cookie etld patch for b2.
Attachment #290945 - Flags: superreview?(dveditz)
Attachment #290945 - Flags: superreview?(cbiesinger)
Attachment #290945 - Flags: review?(dveditz)
Attachment #290945 - Flags: review?(cbiesinger)
Attachment #290945 - Flags: superreview?(cbiesinger)
Attachment #290945 - Flags: superreview+
Attachment #290945 - Flags: review?(cbiesinger)
Attachment #290945 - Flags: review+
Comment on attachment 290945 [details] [diff] [review] patch v1.1 requesting approval1.9 - need this patch to land blocker bug 385299.
Attachment #290945 - Flags: approval1.9?
Comment on attachment 290945 [details] [diff] [review] patch v1.1 a=mconnor on behalf of drivers for checkin _today_
Attachment #290945 - Flags: approval1.9? → approval1.9+
landed!
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
added unit test to cover new behavior. Checking in test_bug368702.js; /cvsroot/mozilla/netwerk/test/unit/test_bug368702.js,v <-- test_bug368702.js new revision: 1.4; previous revision: 1.3 done
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: