improve normalization behavior of nsIEffectiveTLDService

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
11 years ago
11 years ago

People

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

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

10.86 KB, patch
Biesinger
: review+
Biesinger
: superreview+
mconnor
: approval1.9+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
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?
(Assignee)

Comment 1

11 years ago
Created attachment 287058 [details] [diff] [review]
patch v1

here's a patch that implements the second option, just to get things rolling.
Attachment #287058 - Flags: superreview?(dveditz)
Attachment #287058 - Flags: review?(dveditz)
(Assignee)

Updated

11 years ago
Blocks: 367799
(Assignee)

Updated

11 years ago
Blocks: 385299
(Assignee)

Updated

11 years ago
Attachment #287058 - Flags: superreview?(dveditz)
Attachment #287058 - Flags: review?(dveditz)
(Assignee)

Comment 2

11 years ago
Created attachment 290945 [details] [diff] [review]
patch v1.1

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)
(Assignee)

Comment 3

11 years ago
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+
(Assignee)

Comment 4

11 years ago
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+
(Assignee)

Comment 6

11 years ago
landed!
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

11 years ago
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.