1468503 - Implement nsIEffectiveTLDService.hasRootDomain

Status: RESOLVED FIXED

(Core :: Networking: DNS, enhancement, P2)

Description

[Andrea Marchesini [:baku]]

We have several implementations of 'hasRootDomain' in JS. See:
https://searchfox.org/mozilla-central/search?q=hasRootDomain&path=

I just added a new one in nsIClearDataService but the 'right' solution is to have 1 single implementation. Soon, this will be needed in C++ by bug 1468501.

This bug is about moving hasRootDomain into nsIEffectiveTLDService.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: sec.patch

I'm not particularly happy about the use of Find() but I haven't found a better solution.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8985179 [details] [diff] [review]
sec.patch

># HG changeset patch
># User Andrea Marchesini <[e-mail]>
># Parent  75a32b57132f8cba42779555662a057a0416a313
>
>diff --git a/netwerk/dns/nsEffectiveTLDService.cpp b/netwerk/dns/nsEffectiveTLDService.cpp
>--- a/netwerk/dns/nsEffectiveTLDService.cpp
>+++ b/netwerk/dns/nsEffectiveTLDService.cpp
>@@ -341,8 +341,40 @@ bool
> nsEffectiveTLDService::LookupForAdd(const nsACString& aHost, TLDCacheEntry** aEntry)
> {
>   MOZ_ASSERT(NS_IsMainThread());
> 
>   const uint32_t hash = HashString(aHost.BeginReading(), aHost.Length());
>   *aEntry = &mMruTable[hash % kTableSize];
>   return (*aEntry)->mHost == aHost;
> }
>+
>+NS_IMETHODIMP
>+nsEffectiveTLDService::HasRootDomain(const nsACString& aInput,
>+                                     const nsACString& aDomain,
>+                                     bool* aResult)
>+{
>+  if (NS_WARN_IF(!aResult)) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  *aResult = false;
>+
>+  // If the strings are the same, we obviously have a match.
>+  if (aInput == aDomain) {
>+    *aResult = true;
>+    return NS_OK;
>+  }
>+
>+  // If aDomain is not found, we know we do not have it as a root domain.
>+  int32_t index = nsCString(aInput).Find(aDomain.BeginReading());
>+  if (index == kNotFound) {
>+    return NS_OK;
>+  }
>+
>+  // Otherwise, we have aDomain as our root domain iff the index of aDomain is
>+  // aDomain.length subtracted from our length and (since we do not have an
>+  // exact match) the character before the index is a dot or slash.
>+  *aResult = index > 0 &&
>+             (uint32_t)index == aInput.Length() - aDomain.Length() &&
this casting looks odd.

>+             (aInput[index - 1] == '.' || aInput[index -1] == '/');
Missing space before the latter 1

>+
>+    /**
>+     * Returns true if the string passed in is part of the root domain of
>+     * the current string.
I don't understand this comment (which is just moved). What does
'part of the root domain of the current string' mean?

>+  For example, if this is "www.mozilla.org", and
What does 'this' refer to?


>+     * we pass in "mozilla.org", this will return true.  It would return
>+     * false the other way around.
>+     *
>+     * @param aInput The host to be analyzed.
>+     * @param aHost  The host to compare to.
I'm having trouble to understand what aInput is and what is aHost.
And idl talks about aHost, yet the method name is hasRootDomain. And cpp code uses aDomain


r- basically because of documentation issues. It isn't clear from the current js methods what they do, but the new documentation should be clear.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: sec.patch

Better comments.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8985205 [details] [diff] [review]
sec.patch

># HG changeset patch
># User Andrea Marchesini <[e-mail]>
># Parent  75a32b57132f8cba42779555662a057a0416a313
>
>diff --git a/netwerk/dns/nsEffectiveTLDService.cpp b/netwerk/dns/nsEffectiveTLDService.cpp
>--- a/netwerk/dns/nsEffectiveTLDService.cpp
>+++ b/netwerk/dns/nsEffectiveTLDService.cpp
>@@ -341,8 +341,40 @@ bool
> nsEffectiveTLDService::LookupForAdd(const nsACString& aHost, TLDCacheEntry** aEntry)
> {
>   MOZ_ASSERT(NS_IsMainThread());
> 
>   const uint32_t hash = HashString(aHost.BeginReading(), aHost.Length());
>   *aEntry = &mMruTable[hash % kTableSize];
>   return (*aEntry)->mHost == aHost;
> }
>+
>+NS_IMETHODIMP
>+nsEffectiveTLDService::HasRootDomain(const nsACString& aInput,
>+                                     const nsACString& aDomain,
Could you still be consistent with host vs domain. This cpp code uses domain, idl uses host as the latter param.


>+  // If the strings are the same, we obviously have a match.
>+  if (aInput == aDomain) {
>+    *aResult = true;
>+    return NS_OK;
>+  }
>+
>+  // If aDomain is not found, we know we do not have it as a root domain.
>+  int32_t index = nsCString(aInput).Find(aDomain.BeginReading());
Would nsAutoCString work here? nsCString heap allocates if aInput isn't backed by a StringBuffer
Why you need BeginReading()? Find() has a variant which takes const self_type& aString


>+  if (index == kNotFound) {
>+    return NS_OK;
>+  }
>+
>+  // Otherwise, we have aDomain as our root domain iff the index of aDomain is
>+  // aDomain.length subtracted from our length and (since we do not have an
>+  // exact match) the character before the index is a dot or slash.
>+  *aResult = index > 0 &&
>+             (uint32_t)index == aInput.Length() - aDomain.Length() &&
>+             (aInput[index - 1] == '.' || aInput[index - 1] == '/');
So is / for the case when one passes something like http://mozilla.org and domain is mozilla.org?
But that is coming from the old code, so fine, I guess.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

I see a few more instances of hasRootDomain with the same implementation. Maybe we should replace those as well?
https://searchfox.org/mozilla-central/search?q=hasRootDomain&case=false&regexp=false&path=

Comment 6

[Andrea Marchesini [:baku]]

> Would nsAutoCString work here? nsCString heap allocates if aInput isn't
> backed by a StringBuffer

Yes. I can use nsAutoCString but I cannot pass aHost directly.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca68fa3212d3
Implement nsIEffectiveTLDService.hasRootDomain, r=smaug

Comment 8

[Andrea Marchesini [:baku]]

(In reply to Valentin Gosu [:valentin] from comment #5)
> I see a few more instances of hasRootDomain with the same implementation.
> Maybe we should replace those as well?
> https://searchfox.org/mozilla-central/
> search?q=hasRootDomain&case=false&regexp=false&path=

I'm planning to do it in a follow up.

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/ca68fa3212d3