[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 9ec0a379e197f59bebc3e506545fc55a973e4f6c..aafe57e277206d342deec391c90bd41fd0356f64 100644
--- a/net/base/host_resolver_impl.cc
+++ b/net/base/host_resolver_impl.cc
@@ -38,6 +38,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"
@@ -609,7 +610,7 @@ class HostResolverImpl::ProcTask
   void Cancel() {
     DCHECK(origin_loop_->BelongsToCurrentThread());
 
-    if (was_canceled())
+    if (was_canceled() || was_completed())
       return;
 
     callback_.Reset();
@@ -1042,32 +1043,33 @@ class HostResolverImpl::IPv6ProbeJob
 
 // Resolves the hostname using DnsTransaction.
 // TODO(szym): This could be moved to separate source file as well.
-class HostResolverImpl::DnsTask {
+class HostResolverImpl::DnsTask : public base::SupportsWeakPtr<DnsTask> {
  public:
   typedef base::Callback<void(int net_error,
                               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(
+    // If unspecified, do IPv4 first, because suffix search will be faster.
+    uint16 qtype = (family_ == ADDRESS_FAMILY_IPV6) ?
+                   dns_protocol::kTypeAAAA :
+                   dns_protocol::kTypeA;
+    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() {
@@ -1075,47 +1077,135 @@ 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,
-                          addr_list.CreateNetLogCallback());
-        callback_.Run(net_error, addr_list, ttl);
+      OnFailure(net_error, DnsResponse::DNS_PARSE_OK);
+      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_PARSE_OK) {
+      // Fail even if the other query succeeds.
+      OnFailure(ERR_DNS_MALFORMED_RESPONSE, result);
+      return;
+    }
+
+    bool needs_sort = false;
+    if (first_query) {
+      DCHECK(client_->GetConfig()) <<
+          "Transaction should have been aborted when config changed!";
+      if (family_ == ADDRESS_FAMILY_IPV6) {
+        needs_sort = (addr_list.size() > 1);
+      } else if (family_ == ADDRESS_FAMILY_UNSPECIFIED) {
+        first_addr_list_ = addr_list;
+        first_ttl_ = ttl;
+        // Use fully-qualified domain name to avoid search.
+        transaction_ = client_->GetTransactionFactory()->CreateTransaction(
+            response->GetDottedName() + ".",
+            dns_protocol::kTypeAAAA,
+            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_PARSE_OK);
         return;
       }
-      net_error = ERR_DNS_MALFORMED_RESPONSE;
     } else {
-      DNS_HISTOGRAM("AsyncDNS.TransactionFailure",
+      DCHECK_EQ(ADDRESS_FAMILY_UNSPECIFIED, family_);
+      if (!first_addr_list_.empty()) {
+        ttl = std::min(ttl, first_ttl_);
+        // Place IPv4 addresses after IPv6.
+        addr_list.insert(addr_list.end(), first_addr_list_.begin(),
+                                          first_addr_list_.end());
+      }
+      needs_sort = (addr_list.size() > 1);

[mmenke1, 2012/08/13 20:45:10]

So what do you think of not sorting when we don't have any IPv6 addresses?

We'd need something like:

needs_sort = (addr_list.size() > 1 && addr_list.size() !=
first_addr_list_.size());

Believe you said that the sorting on windows could take up to 10 milliseconds,
throw in the latency of passing messages between threads, and seems like it may
be worth the slightly added complexity.

[szym, 2012/08/13 20:56:23]

I think it's worth it. I added an explicit bool for readability.

[mmenke1, 2012/08/13 21:16:51]

Think the bool is much clearer.  I'll give this a final skim, and probably sign
off on it tomorrow.

+    }
+
+    if (addr_list.empty()) {
+      // TODO(szym): Don't fallback to ProcTask in this case.
+      OnFailure(ERR_NAME_NOT_RESOLVED, DnsResponse::DNS_PARSE_OK);
+      return;
+    }
+
+    if (needs_sort) {
+      // Sort could complete synchronously.
+      client_->GetAddressSorter()->Sort(
+          addr_list,
+          base::Bind(&DnsTask::OnSortComplete, AsWeakPtr(),
+                     base::TimeTicks::Now(),
+                     ttl));
+    } else {
+      OnSuccess(addr_list, ttl);
+    }
+  }
+
+  void OnSortComplete(base::TimeTicks start_time,
+                      base::TimeDelta ttl,
+                      bool success,
+                      const AddressList& addr_list) {
+    if (!success) {
+      DNS_HISTOGRAM("AsyncDNS.SortFailure",
                     base::TimeTicks::Now() - start_time);
+      OnFailure(ERR_DNS_SORT_ERROR, DnsResponse::DNS_PARSE_OK);
+      return;
+    }
+
+    DNS_HISTOGRAM("AsyncDNS.SortSuccess",
+                  base::TimeTicks::Now() - start_time);
+
+    // AddressSorter prunes unusable destinations.
+    if (addr_list.empty()) {
+      LOG(WARNING) << "Address list empty after RFC3484 sort";
+      OnFailure(ERR_NAME_NOT_RESOLVED, DnsResponse::DNS_PARSE_OK);
+      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,
         base::Bind(&NetLogDnsTaskFailedCallback, 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,
+                      addr_list.CreateNetLogCallback());
+    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_;
+
+  // Results from the first transaction. Used only if |family_| is unspecified.
+  AddressList first_addr_list_;
+  base::TimeDelta first_ttl_;
 };
 
 //-----------------------------------------------------------------------------
@@ -1214,7 +1304,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());
@@ -1381,7 +1471,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_));
@@ -1415,6 +1505,7 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job {
     }
 
     UmaAsyncDnsResolveStatus(RESOLVE_STATUS_DNS_SUCCESS);
+
     CompleteRequests(net_error, addr_list, ttl);
   }