[net/dns] When serving AF_UNSPEC addresses from DnsHosts, serve one from each family.

net/dns/host_resolver_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/dns/host_resolver_impl.h"
 
 #if defined(OS_WIN)
 #include <Winsock2.h>
 #elif defined(OS_POSIX)
 #include <netdb.h>
@@ skipping 1936 matching lines @@
 bool HostResolverImpl::ServeFromHosts(const Key& key,
                                       const RequestInfo& info,
                                       AddressList* addresses) {
   DCHECK(addresses);
   if (!HaveDnsConfig())
     return false;
 
   // HOSTS lookups are case-insensitive.
   std::string hostname = StringToLowerASCII(key.hostname);
 
-  // If |address_family| is ADDRESS_FAMILY_UNSPECIFIED other implementations
-  // (glibc and c-ares) return the first matching line. We have more
-  // flexibility, but lose implicit ordering.
-  // TODO(szym) http://crbug.com/117850
   const DnsHosts& hosts = dns_client_->GetConfig()->hosts;
-  DnsHosts::const_iterator it = hosts.find(
-      DnsHostsKey(hostname,
-                  key.address_family == ADDRESS_FAMILY_UNSPECIFIED ?
-                      ADDRESS_FAMILY_IPV4 : key.address_family));
 
-  if (it == hosts.end()) {
-    if (key.address_family != ADDRESS_FAMILY_UNSPECIFIED)
-      return false;
+  if (key.address_family == ADDRESS_FAMILY_UNSPECIFIED) {
+    // If |address_family| is ADDRESS_FAMILY_UNSPECIFIED other implementations
+    // (glibc and c-ares) return the first matching line. We have more
+    // flexibility, but lose implicit ordering.
+    // We prefer IPv6, "happy eyeballs" will fallback to IPv4 if needed.

[mmenke, 2013/03/27 19:58:02]

nit:  "fall back" (fallback is a noun, not a verb)

[mmenke, 2013/03/27 19:58:02]

nit:  Also, that comma should be a semi-colon, or there should be a conjunction.

+    addresses->clear();
+    DnsHosts::const_iterator it = hosts.find(
+        DnsHostsKey(hostname, ADDRESS_FAMILY_IPV6));
+    if (it != hosts.end())
+      addresses->push_back(IPEndPoint(it->second, info.port()));
+    it = hosts.find(DnsHostsKey(hostname, ADDRESS_FAMILY_IPV4));
+    if (it != hosts.end())
+      addresses->push_back(IPEndPoint(it->second, info.port()));
+    return !addresses->empty();
+  } else {
+    DnsHosts::const_iterator it = hosts.find(
+        DnsHostsKey(hostname, key.address_family));

[mmenke, 2013/03/27 19:58:02]

optional:  This may be a little prettier with both cases merged:

addresses->clear();
if (key.address_family == IPV4 || key.address_family == UNSPEC) {
  DoStuff
}

etc.

 
-    it = hosts.find(DnsHostsKey(hostname, ADDRESS_FAMILY_IPV6));
     if (it == hosts.end())
       return false;
+    *addresses = AddressList::CreateFromIPAddress(it->second, info.port());

[mmenke, 2013/03/27 19:58:02]

I think it's a mistake to clear addresses on failure in one path, but not the
other.  This can result in code that always assumes on behavior or the other if
people fail to test the non-UNSPEC case, or only the UNSPEC case.

   }
-
-  *addresses = AddressList::CreateFromIPAddress(it->second, info.port());
   return true;
 }
 
 void HostResolverImpl::CacheResult(const Key& key,
                                    const HostCache::Entry& entry,
                                    base::TimeDelta ttl) {
   if (cache_.get())
     cache_->Set(key, entry, base::TimeTicks::Now(), ttl);
 }
 
@@ skipping 184 matching lines @@
   }
   DnsConfig dns_config;
   NetworkChangeNotifier::GetDnsConfig(&dns_config);
   dns_client_->SetConfig(dns_config);
   num_dns_failures_ = 0;
   if (dns_config.IsValid())
     UMA_HISTOGRAM_BOOLEAN("AsyncDNS.DnsClientEnabled", true);
 }
 
 }  // namespace net