882516 - DNS: Redundant DNS query for second connection to host

Status: RESOLVED FIXED

(Core :: Networking: DNS, defect)

Description

[Steve Workman [:sworkman] (INACTIVE)]

There is a redundant DNS query being made for the second connection to a particular host. In nsHttpConnectionMgr::nsConnectionEntry, once the first TCP connection to a host is made, subsequent connections are created preferring the address family of the first connection. The first connection is made using AF_UNSPEC in the DNS query, with the second being AF_INET or AF_INET6. This results in the DNS cache being ignored for the second connection, since address family is part of the hash key. So, even though a DNS entry is present with the desired IP information, it is ignored because the entry has a different address family. Thus nsHostResolver making a second redundant DNS query.

This bug was discovered by analyzing network waterfall diagrams created by webpagetest.org.

Comment 1

[Steve Workman [:sworkman] (INACTIVE)]

Steps to reproduce:
Note, I have been unable to reproduce this on my mac laptop, but it is consistently reproducible on Win7 and Ubuntu VMs.

1. Start a capture using wireshark. Filter output to DNS queries for www.google.com.
2. Start Firefox with NSPR_LOG_MODULES=nsHostResolver:5.
3. Load up http://www.google.com/search?q=barack+obama.

You should observe two calls to getaddrinfo in the nsHostResolver log output, and two DNS queries in the wireshark capture (referring to www.google.com).

Comment 2

[Honza Bambas (:mayhemer)]

Side note: I've first discovered this immediately using the event tracer ;) I know, next time I'l find a bug like this I'll file it immediately...

Comment 3

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 1.0 Check for AF_UNSPEC entries in DNS cache for AF_INET/6 resolution requests

Uploaded a base patch for Adrian to start working on this.

Adrian, the code should look something like this: For AF_INET and AF_INET6 hostname resolution requests, check if there is an AF_UNSPEC entry in the DNS cache. If there is and it's not expired, create a new entry by copying addresses of the requested address family from the AF_UNSPEC entry. Then, return this DNS record in the lookup result.
You'll also need to consider things like blacklisted addresses and whether to send a refresh request or not.

Comment 4

[Adrian Lungu]

Attachment: proposal for bugfix

It crashes when it adds a new address (specHe->rec->addr_info->mAddresses.insertBack(addrIter);). I have to recompile firefox with 'no optimization' flag so I can see what is happening (some variables are not visible, probably because of the compiler's optimizations). I will check this tomorrow morning.

Comment 5

[Masatoshi Kimura [:emk]]

I think we should remove address family from the hash key, always use AF_UNSPEC, and filter the result on our own if the caller asks AF_INET or AF_INET6.

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Masatoshi Kimura [:emk] from comment #5)
> I think we should remove address family from the hash key, always use
> AF_UNSPEC, and filter the result on our own if the caller asks AF_INET or
> AF_INET6.

I did some investigating on this one: if I 'dig www.facebook.com any', no IPv6 addresses are returned, but if I 'dig www.facebook.com aaaa' an IPv6 addresses is returned. So, if a client of nsIDNSService asks for a AF_INET6 and internally that's converted to an AF_UNSPEC, we would return no addresses, when facebook does have IPv6 addresses to offer. So, I don't think we can do it this way.

For the method proposed in the patch, we should just issue the lookup if no IPv4/IPv6 addresses are found in the AF_UNSPEC entry.

Comment 7

[Masatoshi Kimura [:emk]]

(In reply to Steve Workman [:sworkman] from comment #6)
> I did some investigating on this one: if I 'dig www.facebook.com any', no
> IPv6 addresses are returned, but if I 'dig www.facebook.com aaaa' an IPv6
> addresses is returned. So, if a client of nsIDNSService asks for a AF_INET6
> and internally that's converted to an AF_UNSPEC, we would return no
> addresses, when facebook does have IPv6 addresses to offer. So, I don't
> think we can do it this way.

Facebook uses a blacklist to prevent returning IPv6 addresses to some broken networks.
I can get an IPv6 address with AF_UNSPEC for www.facebook.com because I have a decent IPv6 network:

  >nslookup www.facebook.com
  Server:  cdns01.kddi.ne.jp
  Address:  2001:268:fd07:4::1

  Non-authoritative answer:
  Name:    star.c10r.facebook.com
  Addresses:  2a03:2880:10:1f07:face:b00c:0:1
            31.13.82.17
  Aliases:  www.facebook.com

We shouldn't disable this workaround. Otherwise some users will have a delay when connecting to Facebook. If some users can't get IPv6 addresses even if they have a decent IPv6 connection, they should contact to Facebook, not us.

Furthermore, we *already* converts AF_INET6 to AF_UNSPEC because NSPR doesn't support PR_AF_INET6 (see bug 825501).

Comment 8

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Masatoshi Kimura [:emk] from comment #7)
> Facebook uses a blacklist to prevent returning IPv6 addresses to some broken
> networks.

Ah, I see. It's interesting that their blacklist doesn't prevent IPv6 addresses being returned for explicit AF_INET6 requests from the same network.

> I can get an IPv6 address with AF_UNSPEC for www.facebook.com because I have
> a decent IPv6 network:
> 
>   >nslookup www.facebook.com
>   Server:  cdns01.kddi.ne.jp
>   Address:  2001:268:fd07:4::1
> 
>   Non-authoritative answer:
>   Name:    star.c10r.facebook.com
>   Addresses:  2a03:2880:10:1f07:face:b00c:0:1
>             31.13.82.17
>   Aliases:  www.facebook.com
> 
> We shouldn't disable this workaround. Otherwise some users will have a delay
> when connecting to Facebook. 

Agreed, we shouldn't disable their workaround, or try to introduce new logic to force IPv6. But if they are responding to AF_INET6 requests with AF_INET6 responses … well, that could be a bug on their part, or it could be that they allow IPV6 if it's specifically asked for.

> If some users can't get IPv6 addresses even if
> they have a decent IPv6 connection, they should contact to Facebook, not us.

Indeed, but if some extension can't get IPv6 addresses because it explicitly requests AF_INET6 and we don't return any such addresses because of NSPR's limitations (which you mentioned next), then it's our problem. I'm not sure how many extensions out there would be requesting AF_INET6 using nsIDNSService (I imagine not many) but my point is that we have some responsibility here too.

> Furthermore, we *already* converts AF_INET6 to AF_UNSPEC because NSPR
> doesn't support PR_AF_INET6 (see bug 825501).

Right. I had forgotten about the lack of NSPR support for AF_INET6.

So, I think that you're suggesting a general cleanup in the DNS code, adding more consistency in how we make requests. If I'm understanding you right, then I think there are two approaches to achieve more purity in our DNS code, but these are bigger than the scope of this bug:

1 - Rely on AF_UNSPEC for all requests.
2 - Fix NSPR or change resolvers so that INET6 requests are actually sent out.

The first approach would fix this bug because it removes 'af' from the hash key. But I'm concerned that forcing all DNS requests to UNSPEC could cause problems, especially for extensions which explicitly request certain address families AND for DNS resolvers like Facebook's which WILL return an IPV6 address if you explicitly request it. I don't know the numbers for either extensions or resolvers in these cases, so this concern could be practically baseless. And, as you pointed out, we already do this for AF_INET6, so it's probably fine. But still, the change is big and affects ALL DNS requests.

The second approach does not fix the redundant DNS request, but AF_INET and AF_INET6 in an API call to nsIDNSService would be translated to AF_INET and AF_INET6 in DNS requests on the wire. There is already work in Bug 773648 to integrate the unbound DNS Resolver into Firefox.

Really, I'm not sold completely either way. I just want to remove this redundant DNS request. So, for now, I'd prefer to keep working on the current approach and keep the scope of this fix small.

Comment 9

[Masatoshi Kimura [:emk]]

I don't think we should consider nsIDNSService::RESOLVE_DISABLE_IPV6 and nsIDNSService::RESOLVE_DISABLE_IPV4 correspond to AF_INET and AF_INET6, respectively. Otherwise it will generate three redundant DNS requests (one for AF_UNSPEC, another for AF_INET, the other for AF_INET6) at worst.
If we use AF_UNSPEC cache to respond explicit AF_INET6 request, the result will be inconsistent depending on whether the AF_UNSPEC result is already in the cache or not. Because some host (e.g. Facebook) will respond differently between both address families. (IIRC it is an intended behavior.)
If we consider nsIDNSService only filter the result and have no ability to request AF_INET and AF_INET6 explicitly, all the above problems will be solved.

Comment 10

[Adrian Lungu]

Attachment: Redundat DNS query fix

Comment 11

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 775883 [details] [diff] [review]
Redundat DNS query fix

Review of attachment 775883 [details] [diff] [review]:
-----------------------------------------------------------------

Good job, Adrian. r- but this is going in the right direction for the current approach.

::: netwerk/dns/DNS.cpp
@@ +186,5 @@
> +  Init(host, cname);
> +  PRNetAddr tmpAddr;
> +    void *iter = nullptr;
> +    do {
> +      iter = PR_EnumerateAddrInfo(iter, prAddrInfo, 0, &tmpAddr);

I know this is existing code, but can you add a null ptr assertion for prAddrInfo here please? It looks like the only place AddrInfo is created using this constructor is in nsHostResolver::ThreadFunc. And in that function, the PRAddrInfo pointer should not be null. So, let's add a little protection for future changes.

MOZ_ASSERT(prAddrInfo, "Cannot construct AddrInfo with a null prAddrInfo pointer!");

@@ +216,1 @@
>    size_t hostlen = strlen(host);

Can you add a null pointer assertion for host here, please:

MOZ_ASSERT(host, "Cannot initialize AddrInfo with a null host pointer!");

::: netwerk/dns/DNS.h
@@ +123,5 @@
>  class AddrInfo {
>  public:
>    AddrInfo(const char *host, const PRAddrInfo *prAddrInfo, bool disableIPv4,
>             const char *cname);
> +  AddrInfo(const AddrInfo& ai);

I think you can change this to:

AddrInfo(const char *host, const char *cname);

and just pass in mHostName and mCanonicalName rather than an AddrInfo object, since both member vars are public already.

Then, rather than defining a new Init function, move the contents of Init to this constructor, and call it in the other AddrInfo constructor. I'd also put this prototype first. Add a couple of comments to explain that one constructor creates a base AddrInfo, while the other copies addresses from PRNetAddr.

This will look cleaner, and it will be easier for someone who is just reading the code to see the main difference between the two constructors, i.e. what is being used to create the AddrInfo.

::: netwerk/dns/nsHostResolver.cpp
@@ +49,5 @@
>                  // Add callback to the list of pending callbacks.
>                  PR_APPEND_LINK(callback, &he->rec->callbacks);
>  
>                  if (!he->rec->resolving) {
> +

Please remove this empty line.

@@ +57,5 @@
> +                        // First, search for an entry with AF_UNSPEC
> +                        const nsHostKey unspecKey = { host, flags, PR_AF_UNSPEC };
> +                        nsHostDBEnt *unspecHe = static_cast<nsHostDBEnt *>
> +                            (PL_DHashTableOperate(&mDB, &unspecKey, PL_DHASH_LOOKUP));
> +                        if (unspecHe) {

I think we need to add more to this condition to match what is earlier in the function:

if (unspecHe &&
    !(flags & RES_BYPASS_CACHE) &&
    unspecHe->rec->HasResult() &&
    TimeStamp::NowLoRes() <= (he->rec->expiration +
        TimeDuration::FromSeconds(mGracePeriod * 60)))

This will skip copying if a cache bypass was requested, and if the existing entry has already expired.

@@ +59,5 @@
> +                        nsHostDBEnt *unspecHe = static_cast<nsHostDBEnt *>
> +                            (PL_DHashTableOperate(&mDB, &unspecKey, PL_DHASH_LOOKUP));
> +                        if (unspecHe) {
> +                            NS_ASSERTION(unspecHe->rec, "Host entry should contain a record");
> +                            LOG(("Specific dns (request INET = %d , INET6 = %d) for an unspecified cached record",

80 chars max per line, please.

@@ +64,5 @@
> +                                (af == PR_AF_INET), (af == PR_AF_INET6)));
> +
> +                            // Create a new record in the cache for the new address family.
> +                            nsHostDBEnt *specHe = static_cast<nsHostDBEnt *>
> +                                (PL_DHashTableOperate(&mDB, &key, PL_DHASH_ADD));

Actually, this is done earlier in the function. 'he' should point to a new entry already.

@@ +66,5 @@
> +                            // Create a new record in the cache for the new address family.
> +                            nsHostDBEnt *specHe = static_cast<nsHostDBEnt *>
> +                                (PL_DHashTableOperate(&mDB, &key, PL_DHASH_ADD));
> +
> +                            // Search for any valid address in the cache

Change to "... in the AF_UNSPEC entry in the cache."

@@ +78,5 @@
> +                                }
> +                                // The unspecified (any) host entry may have both INET
> +                                // and INET6 addresses, so they must be filtered.
> +                                // Also, some of this addresses may be blacklisted.
> +                                // The blacklisted addresses will also be ignore.

You can remove this comment. I think it's already said in the comment before the 'do' statement.

@@ +82,5 @@
> +                                // The blacklisted addresses will also be ignore.
> +                                if (addrIter && !unspecHe->rec->Blacklisted(&addrIter->mAddress) &&
> +                                      ((af == PR_AF_INET && addrIter->mAddress.inet.ip) ||
> +                                       (af == PR_AF_INET6 &&
> +                                           (addrIter->mAddress.inet6.ip.u64[0] | addrIter->mAddress.inet6.ip.u64[1])))) {

You should be able to use mAddress.inet.family here. mAddress.inet and mAddress.inet6 are shared memory in a union, so you can't use inet.ip or inet6.ip to differentiate between address families. The family variable seems to be common in the union.

@@ +83,5 @@
> +                                if (addrIter && !unspecHe->rec->Blacklisted(&addrIter->mAddress) &&
> +                                      ((af == PR_AF_INET && addrIter->mAddress.inet.ip) ||
> +                                       (af == PR_AF_INET6 &&
> +                                           (addrIter->mAddress.inet6.ip.u64[0] | addrIter->mAddress.inet6.ip.u64[1])))) {
> +                                    specHe->rec->addr_info = new AddrInfo(*unspecHe->rec->addr_info);

addr_info should be created once per he->rec. Then add addrIter in each iteration. I suggest you create it when the first approriate address is found. That way, if no addresses are found, addr_info will still be null.

@@ +84,5 @@
> +                                      ((af == PR_AF_INET && addrIter->mAddress.inet.ip) ||
> +                                       (af == PR_AF_INET6 &&
> +                                           (addrIter->mAddress.inet6.ip.u64[0] | addrIter->mAddress.inet6.ip.u64[1])))) {
> +                                    specHe->rec->addr_info = new AddrInfo(*unspecHe->rec->addr_info);
> +                                    specHe->rec->addr_info->mAddresses.insertBack(new NetAddrElement(*addrIter));

I suggest you create an AddAddress function in AddrInfo:

AddAddress(NetAddrElement *address);

It's good to keep the implementation of the list hidden, and the code will be easier to read.

@@ +86,5 @@
> +                                           (addrIter->mAddress.inet6.ip.u64[0] | addrIter->mAddress.inet6.ip.u64[1])))) {
> +                                    specHe->rec->addr_info = new AddrInfo(*unspecHe->rec->addr_info);
> +                                    specHe->rec->addr_info->mAddresses.insertBack(new NetAddrElement(*addrIter));
> +                                }
> +                            } while (addrIter);

You may also want to keep count of the number of addresses copied. If it's zero, I think you should issue a lookup.
Note: for AF_INET6, this just means another UNSPEC lookup, so it might be worthwhile skipping them in this specific case. I suggest you issue a lookup here only if no IPv4 addresses were found and copied. Add a comment to explain why so that this can be changed in the future.

After copying, you also need to consider issuing a lookup for entries in the grace period. Look at the code which follows the comment "For entries that are in the grace period..."

@@ +92,5 @@
> +                                // Save the first valid address.
> +                                specHe->rec->addr = new NetAddr();
> +                                memcpy(specHe->rec->addr, specHe->rec->addr_info->mAddresses.getFirst(), sizeof(NetAddr));
> +                                result = specHe->rec;
> +                            }

I think this if branch is wrong. addr is used if the URL was an IP address literal. You want to consider it, but before the do.. while loop.

If addr is non-null, copy it and move on.
Else, check addr_info and copy the addresses belonging to its linked list.
Else, no entries available to copy.

@@ +94,5 @@
> +                                memcpy(specHe->rec->addr, specHe->rec->addr_info->mAddresses.getFirst(), sizeof(NetAddr));
> +                                result = specHe->rec;
> +                            }
> +                            else {
> +                                LOG(("No valid address was found in the cache for the requested IP family"));

Remove this else, and put the LOG with if (!result) below.

@@ +108,5 @@
> +                                              METHOD_NETWORK_FIRST);
> +                        if (NS_FAILED(rv))
> +                            PR_REMOVE_AND_INIT_LINK(callback);
> +                        else
> +                            LOG(("DNS lookup for host [%s] blocking pending 'getaddrinfo' query.", host));

Please use braces, even for one-line if/else statements.

Comment 12

[Adrian Lungu]

Attachment: Redundat DNS query fix

There are still a couple of unit tests that are failing. I'll keep looking for the issue.

Comment 13

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 781064 [details] [diff] [review]
Redundat DNS query fix

Review of attachment 781064 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good. Almost there. f+

Once you get the tests passing, let's go for a full review again.

::: netwerk/dns/DNS.cpp
@@ +223,5 @@
>    moz_free(mCanonicalName);
>  }
>  
> +nsresult
> +AddrInfo::AddAddress(NetAddrElement *address)

This can probably return void, since LinkedList::insertBack is void.

@@ +225,5 @@
>  
> +nsresult
> +AddrInfo::AddAddress(NetAddrElement *address)
> +{
> +  MOZ_ASSERT(&mAddresses, "Cannot add the address to an uninitialized list");

Check address for null too.

::: netwerk/dns/DNS.h
@@ +123,5 @@
>  class AddrInfo {
>  public:
> +  // Creates an AddrInfo object.
> +  // It calls the AddrInfo(const char*, const char*)
> +  // to initialize the host and the cname.

Good to keep lines under 80 chars, but also good to try to maximize the space that is available :) This sentence probably doesn't need to break so early.

::: netwerk/dns/nsHostResolver.cpp
@@ +542,5 @@
>                  LOG(("Using cached record for host [%s].\n", host));
>                  // put reference to host record on stack...
>                  result = he->rec;
>                  Telemetry::Accumulate(Telemetry::DNS_LOOKUP_METHOD2, METHOD_HIT);
> +                ConditionallyIssueLookup(he->rec, host);

Let's rename this to ConditionallyRefreshRecord or something like that. This will clearly differentiate it from IssueLookup, and states the purpose of the function better.

@@ +588,5 @@
>                  // Add callback to the list of pending callbacks.
>                  PR_APPEND_LINK(callback, &he->rec->callbacks);
>  
>                  if (!he->rec->resolving) {
> +                    bool unspeHeAvaialble = false;;

unspecHeAvailable

@@ +599,5 @@
> +                        nsHostDBEnt *unspecHe = static_cast<nsHostDBEnt *>
> +                            (PL_DHashTableOperate(&mDB, &unspecKey, PL_DHASH_LOOKUP));
> +                        NS_ASSERTION(unspecHe->rec, "Host entry should contain a record");
> +                        if (unspecHe &&
> +                            !(flags & RES_BYPASS_CACHE) &&

We can probably move this up a level:

if (!(flags & RES_BYPASS_CACHE) &&
    (af == PR_AF_INET) || (af == PR_AF_INET6)) {

...because if the request is to bypass the cache, then we don't need to query it at all.

@@ +600,5 @@
> +                            (PL_DHashTableOperate(&mDB, &unspecKey, PL_DHASH_LOOKUP));
> +                        NS_ASSERTION(unspecHe->rec, "Host entry should contain a record");
> +                        if (unspecHe &&
> +                            !(flags & RES_BYPASS_CACHE) &&
> +                            unspecHe->rec->HasResult() &&

Note, Pat is working on a patch to change this to HasUsableResult. You'll probably notice it when you update your repo at some point.

@@ +605,5 @@
> +                            TimeStamp::NowLoRes() <= (he->rec->expiration +
> +                                TimeDuration::FromSeconds(mGracePeriod * 60))) {
> +                            LOG(("Specific DNS request (INET = %d , INET6 = %d) "
> +                                "for an unspecified cached record",
> +                                (af == PR_AF_INET), (af == PR_AF_INET6)));

Minor change:

LOG(("Specific DNS request (%s) "
     "for an unspecified cached record",
     (af == PR_AF_INET) ? "AF_INET" : "AF_INET6"));

@@ +611,5 @@
> +
> +                            // First, check if the host name is an IP address literal.
> +                            if (unspecHe->rec->addr) {
> +                                he->rec->addr = new NetAddr();
> +                                memcpy(he->rec->addr,

So, this is correct for what I asked, but I looked again, and an IP address literal would have been processed already in the function. So, we don't want to do this - basically, ignore rec->addr.

@@ +624,5 @@
> +                                he->rec->addr_info = nullptr;
> +                                do {
> +                                    if (!addrIter) {
> +                                        addrIter = unspecHe->rec->addr_info->mAddresses.getFirst();
> +                                    }

We can probably remove some of the indentation here by putting getFirst outside the while loop:

addrIter = unspecHe->rec->addr_info->mAddresses.getFirst();
while (addrIter) {
    ...
    addrIter = addrIter->getNext();
}

@@ +639,5 @@
> +                                    }
> +                                } while (addrIter);
> +                                if (he->rec && he->rec->HasResult()) {
> +                                    result = he->rec;
> +                                    ConditionallyIssueLookup(he->rec, host);

In this conditional function, we need to know if the record is negative, i.e. did a DNS request return no addresses. If we didn't find any INET6 addresses, we need to set the record to negative now, set the result to the record, and set status to NS_ERROR_UNKNOWN_HOST. We should also tell Telemetry that this was a negative cache hit.

Then, later we shouldn't need to check for (af != PR_AF_INET6 || !unspecHeAvailable), and we might not need unspecHeAvailable at all.

Note: Check for other possible Telemetry records we should add here.

@@ +648,5 @@
> +
> +                    // If no valid address was found in the cache, then start a new lookup.
> +                    // For AF_INET6, a new lookup means another AF_UNSPEC lookup.
> +                    // We have already iterated through the AF_UNSPEC addresses,
> +                    // so we can skip this step for AF_INET6.

If we change empty INET6 results to negative cache entries, we don't need this comment here, or the subsequent check for (af != PR_AF_INET6 || !unspeHeAvaialble).

@@ +664,5 @@
> +                        }
> +                    }
> +                    if (!result) {
> +                        status = NS_ERROR_UNKNOWN_HOST;
> +                    }

I think this is wrong. The status should only be NS_ERROR_UNKNOWN_HOST for empty INET6 records.

::: netwerk/dns/nsHostResolver.h
@@ +243,5 @@
>      void     OnLookupComplete(nsHostRecord *, nsresult, mozilla::net::AddrInfo *);
>      void     DeQueue(PRCList &aQ, nsHostRecord **aResult);
>      void     ClearPendingQueue(PRCList *aPendingQueue);
>      nsresult ConditionallyCreateThread(nsHostRecord *rec);
> +    nsresult ConditionallyIssueLookup(nsHostRecord *rec, const char *host);

Add a short comment here to explain the conditions in which a lookup will be issued.

Comment 14

[Adrian Lungu]

Attachment: Redundant DNS query fix

Comment 15

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 781967 [details] [diff] [review]
Redundant DNS query fix

Review of attachment 781967 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there. One more cleanup iteration :)

::: netwerk/dns/DNS.cpp
@@ +180,5 @@
>  {
>  }
>  
>  AddrInfo::AddrInfo(const char *host, const PRAddrInfo *prAddrInfo,
> +                   bool disableIPv4, const char *cname) : AddrInfo (host, cname)

Please put ": AddrInfo(host, cname) on a new line"; also remove the space between AddrInfo and the open parenthesis.

::: netwerk/dns/nsHostResolver.cpp
@@ +620,5 @@
>              }
>  
>              // otherwise, hit the resolver...
>              else {
> +                if (!he->rec->resolving) {

Looks like it could be ok to make this an "else if" followed by a single "else" - this means you get the opportunity to reduce the indent by one step. Phew!

@@ +625,5 @@
> +                    // If this is an IPV4 or IPV6 specific request, check if
> +                    // there is an AF_UNSPEC entry we can use.
> +                    if (!(flags & RES_BYPASS_CACHE) &&
> +                        ((af == PR_AF_INET) || (af == PR_AF_INET6))) {
> +                        TimeStamp startTimeExtraSearch = TimeStamp::Now();

Looks like this isn't used now, right? Please remove it.

@@ +642,4 @@
>  
> +                            // Search for any valid address in the AF_UNSPEC entry in the cache.
> +                            // (not blacklisted and from the right family).
> +                            NetAddrElement *addrIter = unspecHe->rec->addr_info->mAddresses.getFirst();

This is running onto the next line. If it goes past 80 chars, add a new line and indent in to one step past the previous indent.

Please check 80 chars max on all lines :)

@@ +658,5 @@
> +                            }
> +                            if (he->rec->HasUsableResult(flags)) {
> +                                result = he->rec;
> +                                Telemetry::Accumulate(Telemetry::DNS_LOOKUP_METHOD2, METHOD_HIT);
> +                                ConditionallyRefreshRecord(he->rec, host);

I see you use this here, but you removed the call from earlier in the ResolveHost - any reason for this? Is the code no longer the same?

@@ +674,5 @@
> +                        }
> +                    }
> +                    // If no valid address was found in the cache, then start a new lookup.
> +                    if (!result) {
> +                        LOG(("No valid address was found in the cache for the requested IP family"));

Over 80 chars long: please split manually.

Comment 16

[Adrian Lungu]

Attachment: Redundant DNS query fix

Comment 17

[Adrian Lungu]

Attachment: Redundant DNS query fix

Added the bug number in the patch's description.

Comment 18

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 786580 [details] [diff] [review]
Redundant DNS query fix

Review of attachment 786580 [details] [diff] [review]:
-----------------------------------------------------------------

Good work! r=me with a couple of little changes: comments and indentation.

Once you're done, please push to try to verify all is ok - post the link to the results in a comment.

When that's done, I'll land the patch for you.

::: netwerk/dns/nsHostResolver.cpp
@@ +605,5 @@
>              else if (flags & RES_OFFLINE) {
>                  rv = NS_ERROR_OFFLINE;
>              }
>  
>              // otherwise, hit the resolver...

Let's update this comment. Pull the next comment up and merge them:

"If this is an IPV4 or IPv6 specific request, check if there is an AF_UNSPEC entry we can use. Otherwise, hit the resolver ..."

Remember, 80 chars max please :)

@@ +662,5 @@
> +                            Telemetry::Accumulate(Telemetry::DNS_LOOKUP_METHOD2,
> +                                                  METHOD_NEGATIVE_HIT);
> +                            }
> +                        }
> +                    }

Ooops. Looks like these closing braces need de-indented to match the 'else if'. Please update the subsequent rows too.

@@ +664,5 @@
> +                            }
> +                        }
> +                    }
> +                    // If no valid address was found in the cache, then start
> +                    // a new lookup.

"If no valid address was found or this is an AF_UNSPEC request, ..."

Comment 19

[Adrian Lungu]

Attachment: bug-882516-fix.patch

I have also pushed the patch on the try server, so we should have the results by tomorrow.
https://tbpl.mozilla.org/?tree=Try&rev=2d7fffff7940

Comment 20

[Adrian Lungu]

Attachment: bug-882516-fix.patch

Unit test failures fixed. Try server build:
https://tbpl.mozilla.org/?tree=Try&rev=f2d18987841b

Comment 21

[Steve Workman [:sworkman] (INACTIVE)]

Awesome - patch looks good to me.

Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ab1c5795b4f

(Note: Since I'm not a Necko peer, I should for record-keeping that the review was delegated to me by jduell).

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9ab1c5795b4f