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

net/dns/host_resolver_impl.cc

Index: net/dns/host_resolver_impl.cc
diff --git a/net/dns/host_resolver_impl.cc b/net/dns/host_resolver_impl.cc
index 91475ec827a42cb2ea70a03c61cd2575ffbd13e7..394cdd7142312ce21a52d47489c72846e952b124 100644
--- a/net/dns/host_resolver_impl.cc
+++ b/net/dns/host_resolver_impl.cc
@@ -1954,26 +1954,30 @@ bool HostResolverImpl::ServeFromHosts(const Key& key,
   // 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;
 }