[net/dns] Resolve AF_UNSPEC on dual-stacked systems. Sort addresses according to RFC3484.

net/base/host_resolver_impl.cc

Index: net/base/host_resolver_impl.cc
diff --git a/net/base/host_resolver_impl.cc b/net/base/host_resolver_impl.cc
index a66f950e5fa8d9d6c477ebc49e1aaac329df6f6d..a2adee958bc11b6b68a12a2227ba77b32bb473ff 100644
--- a/net/base/host_resolver_impl.cc
+++ b/net/base/host_resolver_impl.cc
@@ -39,6 +39,7 @@
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
 #include "net/base/net_util.h"
+#include "net/dns/address_sorter.h"
 #include "net/dns/dns_client.h"
 #include "net/dns/dns_config_service.h"
 #include "net/dns/dns_protocol.h"
@@ -1053,26 +1054,27 @@ class HostResolverImpl::DnsTask {
                               const AddressList& addr_list,
                               base::TimeDelta ttl)> Callback;
 
-  DnsTask(DnsTransactionFactory* factory,
+  DnsTask(DnsClient* client,
           const Key& key,
           const Callback& callback,
           const BoundNetLog& job_net_log)
-      : callback_(callback), net_log_(job_net_log) {
-    DCHECK(factory);
+      : client_(client),
+        family_(key.address_family),
+        callback_(callback),
+        net_log_(job_net_log) {
+    DCHECK(client);
     DCHECK(!callback.is_null());
 
-    // For now we treat ADDRESS_FAMILY_UNSPEC as if it was IPV4.
-    uint16 qtype = (key.address_family == ADDRESS_FAMILY_IPV6)
-                   ? dns_protocol::kTypeAAAA
-                   : dns_protocol::kTypeA;
-    // TODO(szym): Implement "happy eyeballs".
-    transaction_ = factory->CreateTransaction(
+    uint16 qtype = (family_ == ADDRESS_FAMILY_IPV4) ?
+                   dns_protocol::kTypeA :
+                   dns_protocol::kTypeAAAA;  // If unspecified, do IPv6 first.
+    transaction_ = client_->GetTransactionFactory()->CreateTransaction(
         key.hostname,
         qtype,
         base::Bind(&DnsTask::OnTransactionComplete, base::Unretained(this),
-                   base::TimeTicks::Now()),
+                                                    true /* first_query */,
+                                                    base::TimeTicks::Now()),
         net_log_);
-    DCHECK(transaction_.get());
   }
 
   int Start() {
@@ -1080,46 +1082,117 @@ class HostResolverImpl::DnsTask {
     return transaction_->Start();
   }
 
-  void OnTransactionComplete(const base::TimeTicks& start_time,
+ private:
+  void OnTransactionComplete(bool first_query,
+                             const base::TimeTicks& start_time,
                              DnsTransaction* transaction,
                              int net_error,
                              const DnsResponse* response) {
     DCHECK(transaction);
     // Run |callback_| last since the owning Job will then delete this DnsTask.
-    DnsResponse::Result result = DnsResponse::DNS_SUCCESS;
-    if (net_error == OK) {
-      CHECK(response);
-      DNS_HISTOGRAM("AsyncDNS.TransactionSuccess",
+    if (net_error != OK) {
+      DNS_HISTOGRAM("AsyncDNS.TransactionFailure",
                     base::TimeTicks::Now() - start_time);
-      AddressList addr_list;
-      base::TimeDelta ttl;
-      result = response->ParseToAddressList(&addr_list, &ttl);
-      UMA_HISTOGRAM_ENUMERATION("AsyncDNS.ParseToAddressList",
-                                result,
-                                DnsResponse::DNS_PARSE_RESULT_MAX);
-      if (result == DnsResponse::DNS_SUCCESS) {
-        net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_DNS_TASK,
-                          new AddressListNetLogParam(addr_list));
-        callback_.Run(net_error, addr_list, ttl);
+      OnFailure(net_error, DnsResponse::DNS_SUCCESS);

[mmenke, 2012/06/07 18:32:34]

These DNS_SUCCESSes everywhere caused me a fair bit of confusion, until I
remembered they meant there was no error parsing the data we received.  I think
this is either worth a comment in OnFailure that this is specifically a code for
parsing the result, if received, or "DnsResponse::DNS_SUCCESS" should be renamed
to something less confusing and more accurate, like maybe "DNS_NO_PARSE_ERROR"
or "DNS_PARSER_OK".

No need to do anything in this CL, though feel free to do so if you want.

+      return;
+    }
+
+    CHECK(response);
+    DNS_HISTOGRAM("AsyncDNS.TransactionSuccess",
+                  base::TimeTicks::Now() - start_time);
+    AddressList addr_list;
+    base::TimeDelta ttl;
+    DnsResponse::Result result = response->ParseToAddressList(&addr_list, &ttl);
+    UMA_HISTOGRAM_ENUMERATION("AsyncDNS.ParseToAddressList",
+                              result,
+                              DnsResponse::DNS_PARSE_RESULT_MAX);
+    if (result != DnsResponse::DNS_SUCCESS) {
+      // Fail even if the other query succeeds.
+      OnFailure(ERR_DNS_MALFORMED_RESPONSE, result);
+      return;
+    }
+
+    bool needs_sort = false;
+    if (first_query) {
+      if (family_ == ADDRESS_FAMILY_UNSPECIFIED) {

[mmenke, 2012/06/07 18:32:34]

nit:  You might want to swap these two if's, so the ADDRESS_FAMILY_UNSPECIFIED
behavior is together, rather than having that little intervening
ADDRESS_FAMILY_IPV6 case.

+        first_addr_list_ = addr_list;
+        first_ttl_ = ttl;
+        // Use fully-qualified domain name to avoid search.
+        transaction_ = client_->GetTransactionFactory()->CreateTransaction(
+            response->GetDottedName() + ".",
+            dns_protocol::kTypeA,
+            base::Bind(&DnsTask::OnTransactionComplete, base::Unretained(this),
+                                                        false /* first_query */,
+                                                        base::TimeTicks::Now()),
+            net_log_);
+        net_error = transaction_->Start();
+        if (net_error != ERR_IO_PENDING)
+          OnFailure(net_error, DnsResponse::DNS_SUCCESS);
         return;
+      } else if (family_ == ADDRESS_FAMILY_IPV6) {
+        needs_sort = true;
       }
-      net_error = ERR_DNS_MALFORMED_RESPONSE;
     } else {
-      DNS_HISTOGRAM("AsyncDNS.TransactionFailure",
+      if (!first_addr_list_.empty()) {
+        ttl = std::min(ttl, first_ttl_);
+        addr_list.insert(addr_list.begin(), first_addr_list_.begin(),
+                                            first_addr_list_.end());
+        needs_sort = true;
+      }
+    }
+
+    if (addr_list.empty()) {
+      // TODO(szym): Don't fallback to ProcTask in this case.
+      OnFailure(ERR_NAME_NOT_RESOLVED, DnsResponse::DNS_NO_ADDRESSES);
+      return;
+    }
+
+    if (needs_sort) {
+      base::TimeTicks start_time = base::TimeTicks::Now();
+      if (!client_->GetAddressSorter()->Sort(&addr_list)) {
+        DNS_HISTOGRAM("AsyncDNS.SortFailure",
+                      base::TimeTicks::Now() - start_time);
+        OnFailure(ERR_DNS_SORT_ERROR, DnsResponse::DNS_SUCCESS);
+        return;
+      }
+      DNS_HISTOGRAM("AsyncDNS.SortSuccess",
                     base::TimeTicks::Now() - start_time);
     }
+
+    // AddressSorter prunes unusable destinations.
+    if (addr_list.empty()) {
+      OnFailure(ERR_NAME_NOT_RESOLVED, DnsResponse::DNS_SUCCESS);
+      return;
+    }
+
+    OnSuccess(addr_list, ttl);
+  }
+
+  void OnFailure(int net_error, DnsResponse::Result result) {
+    DCHECK_NE(OK, net_error);
     net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_DNS_TASK,
                       new DnsTaskFailedParams(net_error, result));
     callback_.Run(net_error, AddressList(), base::TimeDelta());
   }
 
- private:
+  void OnSuccess(const AddressList& addr_list, base::TimeDelta ttl) {
+    net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_DNS_TASK,
+                      new AddressListNetLogParam(addr_list));
+    callback_.Run(OK, addr_list, ttl);
+  }
+
+  DnsClient* client_;
+  AddressFamily family_;
   // The listener to the results of this DnsTask.
   Callback callback_;
-
   const BoundNetLog net_log_;
 
   scoped_ptr<DnsTransaction> transaction_;
+
+  // TODO(szym): wrap this up as HostResolver::Result, add canonical name.
+  // Results from the first transaction. Used only if |family_| is unspecified.
+  AddressList first_addr_list_;
+  base::TimeDelta first_ttl_;
 };
 
 //-----------------------------------------------------------------------------
@@ -1213,7 +1286,7 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job {
   }
 
   // Marks |req| as cancelled. If it was the last active Request, also finishes
-  // this Job marking it either as aborted or cancelled, and deletes it.
+  // this Job marking it as cancelled, and deletes it.
   void CancelRequest(Request* req) {
     DCHECK_EQ(key_.hostname, req->info().hostname());
     DCHECK(!req->was_canceled());
@@ -1349,7 +1422,7 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job {
   void StartDnsTask() {
     DCHECK(resolver_->HaveDnsConfig());
     dns_task_.reset(new DnsTask(
-        resolver_->dns_client_->GetTransactionFactory(),
+        resolver_->dns_client_.get(),
         key_,
         base::Bind(&Job::OnDnsTaskComplete, base::Unretained(this)),
         net_log_));
@@ -1381,6 +1454,7 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job {
     }
 
     UmaAsyncDnsResolveStatus(RESOLVE_STATUS_DNS_SUCCESS);
+
     CompleteRequests(net_error, addr_list, ttl);
   }