Bug 882516 - Use AF_UNSPEC cached entries for AF_INET and AF_INET6 DNS requests, if possible. r=sworkman

netwerk/dns/DNS.cpp

@@ -171,6 +171,11 @@ NetAddrElement::NetAddrElement(const PRNetAddr *prNetAddr)
   PRNetAddrToNetAddr(prNetAddr, &mAddress);
 }
 
+NetAddrElement::NetAddrElement(const NetAddrElement& netAddr)
+{
+  mAddress = netAddr.mAddress;
+}
+
 NetAddrElement::~NetAddrElement()
 {
 }
@@ -178,18 +183,9 @@ NetAddrElement::~NetAddrElement()
 AddrInfo::AddrInfo(const char *host, const PRAddrInfo *prAddrInfo,
                    bool disableIPv4, const char *cname)
 {
-  size_t hostlen = strlen(host);
-  mHostName = static_cast<char*>(moz_xmalloc(hostlen + 1));
-  memcpy(mHostName, host, hostlen + 1);
-  if (cname) {
-      size_t cnameLen = strlen(cname);
-      mCanonicalName = static_cast<char*>(moz_xmalloc(cnameLen + 1));
-      memcpy(mCanonicalName, cname, cnameLen + 1);
-  }
-  else {
-      mCanonicalName = nullptr;
-  }
+  MOZ_ASSERT(prAddrInfo, "Cannot construct AddrInfo with a null prAddrInfo pointer!");
 
+  Init(host, cname);
   PRNetAddr tmpAddr;
   void *iter = nullptr;
   do {
@@ -201,6 +197,11 @@ AddrInfo::AddrInfo(const char *host, const PRAddrInfo *prAddrInfo,
   } while (iter);
 }
 
+AddrInfo::AddrInfo(const char *host, const char *cname)
+{
+  Init(host, cname);
+}
+
 AddrInfo::~AddrInfo()
 {
   NetAddrElement *addrElement;
@@ -211,5 +212,31 @@ AddrInfo::~AddrInfo()
   moz_free(mCanonicalName);
 }
 
+void
+AddrInfo::Init(const char *host, const char *cname)
+{
+  MOZ_ASSERT(host, "Cannot initialize AddrInfo with a null host pointer!");
+
+  size_t hostlen = strlen(host);
+  mHostName = static_cast<char*>(moz_xmalloc(hostlen + 1));
+  memcpy(mHostName, host, hostlen + 1);
+  if (cname) {
+    size_t cnameLen = strlen(cname);
+    mCanonicalName = static_cast<char*>(moz_xmalloc(cnameLen + 1));
+    memcpy(mCanonicalName, cname, cnameLen + 1);
+  }
+  else {
+    mCanonicalName = nullptr;
+  }
+}
+
+void
+AddrInfo::AddAddress(NetAddrElement *address)
+{
+  MOZ_ASSERT(address, "Cannot add the address to an uninitialized list");
+
+  mAddresses.insertBack(address);
+}
+
 } // namespace dns
 } // namespace mozilla

netwerk/dns/DNS.h

@@ -114,6 +114,7 @@ union NetAddr {
 class NetAddrElement : public LinkedListElement<NetAddrElement> {
 public:
   NetAddrElement(const PRNetAddr *prNetAddr);
+  NetAddrElement(const NetAddrElement& netAddr);
   ~NetAddrElement();
 
   NetAddr mAddress;
@@ -121,13 +122,23 @@ public:
 
 class AddrInfo {
 public:
+  // Creates an AddrInfo object. It calls the AddrInfo(const char*, const char*)
+  // to initialize the host and the cname.
   AddrInfo(const char *host, const PRAddrInfo *prAddrInfo, bool disableIPv4,
            const char *cname);
+
+  // Creates a basic AddrInfo object (initialize only the host and the cname).
+  AddrInfo(const char *host, const char *cname);
   ~AddrInfo();
 
+  void AddAddress(NetAddrElement *address);
+
   char *mHostName;
   char *mCanonicalName;
   LinkedList<NetAddrElement> mAddresses;
+
+private:
+  void Init(const char *host, const char *cname);
 };
 
 // Copies the contents of a PRNetAddr to a NetAddr.

netwerk/dns/nsHostResolver.cpp

@@ -564,22 +564,9 @@ nsHostResolver::ResolveHost(const char            *host,
                 Telemetry::Accumulate(Telemetry::DNS_LOOKUP_METHOD2, METHOD_HIT);
 
                 // For entries that are in the grace period with a failed connect,
-                // or all cached negative entries, use the cache but start a new lookup in
-                // the background
-                if ((((TimeStamp::NowLoRes() > he->rec->expiration) &&
-                      he->rec->mBlacklistedItems.Length()) ||
-                     he->rec->negative) && !he->rec->resolving) {
-                    LOG(("Using %s cache entry for host [%s] but starting async renewal.",
-                         he->rec->negative ? "negative" :"positive", host));
-                    IssueLookup(he->rec);
-
-                    if (!he->rec->negative) {
-                        // negative entries are constantly being refreshed, only
-                        // track positive grace period induced renewals
-                        Telemetry::Accumulate(Telemetry::DNS_LOOKUP_METHOD2,
-                                              METHOD_RENEWAL);
-                    }
-                }
+                // or all cached negative entries, use the cache but start a new
+                // lookup in the background
+                ConditionallyRefreshRecord(he->rec, host);
                 
                 if (he->rec->negative) {
                     Telemetry::Accumulate(Telemetry::DNS_LOOKUP_METHOD2,
@@ -619,22 +606,90 @@ nsHostResolver::ResolveHost(const char            *host,
                 rv = NS_ERROR_OFFLINE;
             }
 
-            // otherwise, hit the resolver...
-            else {
+            // If this is an IPV4 or IPV6 specific request, check if there is
+            // an AF_UNSPEC entry we can use. Otherwise, hit the resolver...
+            else if (!he->rec->resolving) {
+                if (!(flags & RES_BYPASS_CACHE) &&
+                    ((af == PR_AF_INET) || (af == PR_AF_INET6))) {
+                    // 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));
+                    NS_ASSERTION(PL_DHASH_ENTRY_IS_FREE(unspecHe) ||
+                                 (PL_DHASH_ENTRY_IS_BUSY(unspecHe) &&
+                                  unspecHe->rec),
+                                "Valid host entries should contain a record");
+                    if (PL_DHASH_ENTRY_IS_BUSY(unspecHe) &&
+                        unspecHe->rec &&
+                        unspecHe->rec->HasUsableResult(flags) &&
+                        TimeStamp::NowLoRes() <= (he->rec->expiration +
+                            TimeDuration::FromSeconds(mGracePeriod * 60))) {
+                        LOG(("Specific DNS request (%s) for an unspecified "
+                             "cached record",
+                            (af == PR_AF_INET) ? "AF_INET" : "AF_INET6"));
+
+                        // 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();
+                        he->rec->addr_info = nullptr;
+                        while (addrIter) {
+                            if ((af == addrIter->mAddress.inet.family) &&
+                                 !unspecHe->rec->Blacklisted(&addrIter->mAddress)) {
+                                if (!he->rec->addr_info) {
+                                    he->rec->addr_info = new AddrInfo(
+                                        unspecHe->rec->addr_info->mHostName,
+                                        unspecHe->rec->addr_info->mCanonicalName);
+                                }
+                                he->rec->addr_info->AddAddress(
+                                    new NetAddrElement(*addrIter));
+                            }
+                            addrIter = addrIter->getNext();
+                        }
+                        if (he->rec->HasUsableResult(flags)) {
+                            result = he->rec;
+                            Telemetry::Accumulate(Telemetry::DNS_LOOKUP_METHOD2,
+                                                  METHOD_HIT);
+                            ConditionallyRefreshRecord(he->rec, host);
+                        }
+                        // For AF_INET6, a new lookup means another AF_UNSPEC
+                        // lookup. We have already iterated through the
+                        // AF_UNSPEC addresses, so we mark this record as
+                        // negative.
+                        else if (af == PR_AF_INET6) {
+                            result = he->rec;
+                            he->rec->negative = true;
+                            status = NS_ERROR_UNKNOWN_HOST;
+                            Telemetry::Accumulate(Telemetry::DNS_LOOKUP_METHOD2,
+                                                  METHOD_NEGATIVE_HIT);
+                        }
+                    }
+                }
+                // If no valid address was found in the cache or this is an
+                // AF_UNSPEC request, then start a new lookup.
+                if (!result) {
+                    LOG(("No valid address was found in the cache for the "
+                         "requested IP family"));
                     // Add callback to the list of pending callbacks.
                     PR_APPEND_LINK(callback, &he->rec->callbacks);
-
-                if (!he->rec->resolving) {
                     he->rec->flags = flags;
-                    rv = IssueLookup(he->rec);
+                    IssueLookup(he->rec);
                     Telemetry::Accumulate(Telemetry::DNS_LOOKUP_METHOD2,
                                           METHOD_NETWORK_FIRST);
-                    if (NS_FAILED(rv))
+                    if (NS_FAILED(rv)) {
                         PR_REMOVE_AND_INIT_LINK(callback);
-                    else
-                        LOG(("DNS lookup for host [%s] blocking pending 'getaddrinfo' query.", host));
                     }
-                else if (he->rec->onQueue) {
+                    else {
+                        LOG(("DNS lookup for host [%s] blocking pending "
+                             "'getaddrinfo' query.", host));
+                    }
+                }
+            }
+            else {
+                // The record is being resolved. Append our callback.
+                PR_APPEND_LINK(callback, &he->rec->callbacks);
+                if (he->rec->onQueue) {
                     Telemetry::Accumulate(Telemetry::DNS_LOOKUP_METHOD2,
                                           METHOD_NETWORK_SHARED);
 
@@ -642,12 +697,14 @@ nsHostResolver::ResolveHost(const char            *host,
                     // lower priority than the request is being made at.
                     // In that case we should upgrade to the higher queue.
 
-                    if (IsHighPriority(flags) && !IsHighPriority(he->rec->flags)) {
+                    if (IsHighPriority(flags) &&
+                        !IsHighPriority(he->rec->flags)) {
                         // Move from (low|med) to high.
                         MoveQueue(he->rec, mHighQ);
                         he->rec->flags = flags;
                         ConditionallyCreateThread(he->rec);
-                    } else if (IsMediumPriority(flags) && IsLowPriority(he->rec->flags)) {
+                    } else if (IsMediumPriority(flags) &&
+                               IsLowPriority(he->rec->flags)) {
                         // Move from low to med.
                         MoveQueue(he->rec, mMediumQ);
                         he->rec->flags = flags;
@@ -770,6 +827,26 @@ nsHostResolver::IssueLookup(nsHostRecord *rec)
     return rv;
 }
 
+nsresult
+nsHostResolver::ConditionallyRefreshRecord(nsHostRecord *rec, const char *host)
+{
+    if ((((TimeStamp::NowLoRes() > rec->expiration) &&
+        rec->mBlacklistedItems.Length()) ||
+        rec->negative) && !rec->resolving) {
+        LOG(("Using %s cache entry for host [%s] but starting async renewal.",
+            rec->negative ? "negative" :"positive", host));
+        IssueLookup(rec);
+
+        if (!rec->negative) {
+            // negative entries are constantly being refreshed, only
+            // track positive grace period induced renewals
+            Telemetry::Accumulate(Telemetry::DNS_LOOKUP_METHOD2,
+                METHOD_RENEWAL);
+        }
+    }
+    return NS_OK;
+}
+
 void
 nsHostResolver::DeQueue(PRCList &aQ, nsHostRecord **aResult)
 {

netwerk/dns/nsHostResolver.h

@@ -245,6 +245,12 @@ private:
     void     ClearPendingQueue(PRCList *aPendingQueue);
     nsresult ConditionallyCreateThread(nsHostRecord *rec);
 
+    /**
+     * Starts a new lookup in the background for entries that are in the grace
+     * period with a failed connect or all cached entries are negative.
+     */
+    nsresult ConditionallyRefreshRecord(nsHostRecord *rec, const char *host);
+    
     static void  MoveQueue(nsHostRecord *aRec, PRCList &aDestQ);
     
     static void ThreadFunc(void *);