Closed Bug 368702 Opened 13 years ago Closed 13 years ago

Effective TLD Service should treat trailing dot properly

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9alpha5

People

(Reporter: gaubugzilla, Assigned: gaubugzilla)

References

Details

Attachments

(1 file, 3 obsolete files)

Testcase:

var tld 
    = Components.classes["@mozilla.org/network/effective-tld-service;1"]
                .getService(Components.interfaces.nsIEffectiveTLDService);

alert(tld.getEffectiveTLDLength("google.com."));

Currently this shows 0 though 4 is the correct value. I should have a patch soon.
Assignee: nobody → trev
Status: NEW → ASSIGNED
Attachment #253346 - Flags: review?(darin.moz)
Attached file mochitest compatible testcase (obsolete) —
Attachment #253348 - Flags: review?(darin.moz)
Should we really count the trailing dot? There's already some confusion about what the length means because an IDN domain would be converted to punycode and the length returned based on the new length. This only affects effectiveTLD's that are not pure ascii, but we do in fact have a couple of those already in our domain list.

Then again if Necko isn't stripping the trailing dot then maybe it does count. I'd like this made explicit in comments in the interface file either way.
If we get a domain with a trailing dot passed we certainly need to count this dot in our result, otherwise you won't be able to take a substring of given length at the end of the domain name and say that it is the effective TLD. And yes, Necko doesn't strip the trailing dot. It actually treats "domain.com" and "domain.com." as different domains (which makes sense since some servers do in fact treat them as different).
Comment on attachment 253348 [details]
mochitest compatible testcase

why not make this an xpcshell testcase, like the stuff in netwerk/test/unit?
Attached patch Unit test (obsolete) — Splinter Review
No good reason not to make an xpcshell testcase but I don't know this test harness and it doesn't seem to be documented. The script to run the tests doesn't work on Windows so I had to do it manually but this seems to work.
Attachment #253348 - Attachment is obsolete: true
Attachment #253990 - Flags: review?(darin.moz)
Attachment #253348 - Flags: review?(darin.moz)
docs are at http://developer.mozilla.org/en/docs/Writing_xpcshell-based_unit_tests

make check is supposed to work on windows, I hope it still does...
Thanks for pointing me to the docs and thanks for telling about "make check" - yes, it works and even makes things quite a lot easier.
Attachment #253346 - Flags: review?(darin.moz) → review?(cbiesinger)
Attachment #253990 - Flags: review?(darin.moz) → review?(cbiesinger)
Flags: wanted1.8.1.x+
Attachment #253346 - Flags: review?(cbiesinger) → review+
Comment on attachment 253990 [details] [diff] [review]
Unit test

I'm somewhat surprised that for a host w/o TLD the length is the string length instead of 0, but ok - this was clearly the case before this patch too.

I'd make Cc and Ci constants
Attachment #253990 - Flags: review?(cbiesinger) → review+
Comment on attachment 253346 [details] [diff] [review]
Make sure to skip trailing dot for TLD checking

The comment will need to be fixed before checking in. "ignore the trailing if one is present" should be "ignore the trailing *dot* if one is present" of course.
Attachment #253346 - Flags: superreview?(dveditz)
Attachment #253990 - Flags: superreview?(dveditz)
Comment on attachment 253346 [details] [diff] [review]
Make sure to skip trailing dot for TLD checking

sr=dveditz
Attachment #253346 - Flags: superreview?(dveditz) → superreview+
Attachment #253990 - Flags: superreview?(dveditz) → superreview+
This is both the fix and the unit test in one patch. The comment is fixed, also Cc and Ci in the unit test are made constants.
Whiteboard: [checkin needed]
mozilla/netwerk/dns/src/nsEffectiveTLDService.cpp  1.2
mozilla/netwerk/test/unit/test_bug368702.js        1.1
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha5
Attachment #253346 - Attachment is obsolete: true
Attachment #253990 - Attachment is obsolete: true
Blocks: abp
You need to log in before you can comment on or make changes to this bug.