Closed Bug 1468503 Opened Last year Closed Last year

Implement nsIEffectiveTLDService.hasRootDomain

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file, 1 obsolete file)

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.
Attached patch sec.patch (obsolete) — Splinter Review
I'm not particularly happy about the use of Find() but I haven't found a better solution.
Assignee: nobody → amarchesini
Attachment #8985179 - Flags: review?(bugs)
Comment on attachment 8985179 [details] [diff] [review]
sec.patch

># HG changeset patch
># User Andrea Marchesini <amarchesini@mozilla.com>
># 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.
Attachment #8985179 - Flags: review?(bugs) → review-
Attached patch sec.patchSplinter Review
Better comments.
Attachment #8985179 - Attachment is obsolete: true
Attachment #8985205 - Flags: review?(bugs)
Comment on attachment 8985205 [details] [diff] [review]
sec.patch

># HG changeset patch
># User Andrea Marchesini <amarchesini@mozilla.com>
># 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.
Attachment #8985205 - Flags: review?(bugs) → review+
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=
Whiteboard: [necko-triaged]
> 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.
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca68fa3212d3
Implement nsIEffectiveTLDService.hasRootDomain, r=smaug
(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.
Blocks: 1468592
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/ca68fa3212d3
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Duplicate of this bug: 1276357
You need to log in before you can comment on or make changes to this bug.