368702 - Effective TLD Service should treat trailing dot properly

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Wladimir Palant]

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.

Comment 1

[Wladimir Palant]

Attachment: Make sure to skip trailing dot for TLD checking

Comment 2

[Wladimir Palant]

Attachment: mochitest compatible testcase

Comment 3

[Daniel Veditz [:dveditz]]

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.

Comment 4

[Wladimir Palant]

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 5

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

Comment on attachment 253348 [details]
mochitest compatible testcase

why not make this an xpcshell testcase, like the stuff in netwerk/test/unit?

Comment 6

[Wladimir Palant]

Attachment: Unit test

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.

Comment 7

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

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

Comment 8

[Wladimir Palant]

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.

Comment 9

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

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

Comment 10

[Wladimir Palant]

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.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 253346 [details] [diff] [review]
Make sure to skip trailing dot for TLD checking

sr=dveditz

Comment 12

[Wladimir Palant]

Attachment: Patch to be checked in

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.

Comment 13

[Nickolay_Ponomarev]

mozilla/netwerk/dns/src/nsEffectiveTLDService.cpp  1.2
mozilla/netwerk/test/unit/test_bug368702.js        1.1