[net/dns] Perform A/AAAA queries for AF_UNSPEC resolutions in parallel.

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 948 matching lines @@
 
   DISALLOW_COPY_AND_ASSIGN(LoopbackProbeJob);
 };
 
 //-----------------------------------------------------------------------------
 
 // Resolves the hostname using DnsTransaction.
 // TODO(szym): This could be moved to separate source file as well.
 class HostResolverImpl::DnsTask : public base::SupportsWeakPtr<DnsTask> {
  public:
-  typedef base::Callback<void(int net_error,
-                              const AddressList& addr_list,
-                              base::TimeDelta ttl)> Callback;
+  class Delegate {
+   public:
+    virtual void OnDnsTaskComplete(base::TimeTicks start_time,
+                                   int net_error,
+                                   const AddressList& addr_list,
+                                   base::TimeDelta ttl) = 0;
+
+    // Called when the first of two jobs succeeds.  If the first completed
+    // transaction fails, this is not called.  Also not called when the DnsTask
+    // only needs to run one transaction.
+    virtual void OnFirstDnsTransactionComplete() = 0;
+
+   protected:
+    Delegate() {}
+    virtual ~Delegate() {}
+  };
 
   DnsTask(DnsClient* client,
           const Key& key,
-          const Callback& callback,
+          Delegate* delegate,
           const BoundNetLog& job_net_log)
       : client_(client),
-        family_(key.address_family),
-        callback_(callback),
-        net_log_(job_net_log) {
+        key_(key),
+        delegate_(delegate),
+        net_log_(job_net_log),
+        num_completed_transactions_(0),
+        task_start_time_(base::TimeTicks::Now()) {
     DCHECK(client);
-    DCHECK(!callback.is_null());
+    DCHECK(delegate_);
+  }
 
-    // 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,
+  bool needs_two_transactions() const {
+    return key_.address_family == ADDRESS_FAMILY_UNSPECIFIED;
+  }
+
+  void StartFirstTransaction() {
+    DCHECK_EQ(0u, num_completed_transactions_);
+    net_log_.BeginEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_DNS_TASK);
+    if (key_.address_family == ADDRESS_FAMILY_IPV6) {
+      StartAAAA();
+    } else {
+      StartA();
+    }
+  }
+
+  void StartSecondTransaction() {
+    DCHECK(needs_two_transactions());
+    StartAAAA();
+  }
+
+ private:
+  void StartA() {
+    DCHECK(!transaction_a_);
+    DCHECK_NE(ADDRESS_FAMILY_IPV6, key_.address_family);
+    transaction_a_ = CreateTransaction(ADDRESS_FAMILY_IPV4);
+    transaction_a_->Start();
+  }
+
+  void StartAAAA() {
+    DCHECK(!transaction_aaaa_);
+    DCHECK_NE(ADDRESS_FAMILY_IPV4, key_.address_family);
+    transaction_aaaa_ = CreateTransaction(ADDRESS_FAMILY_IPV6);
+    transaction_aaaa_->Start();
+  }
+
+  scoped_ptr<DnsTransaction> CreateTransaction(AddressFamily family) {
+    DCHECK_NE(ADDRESS_FAMILY_UNSPECIFIED, family);
+    return client_->GetTransactionFactory()->CreateTransaction(
+        key_.hostname,
+        family == ADDRESS_FAMILY_IPV6 ? dns_protocol::kTypeAAAA :
+                                        dns_protocol::kTypeA,
         base::Bind(&DnsTask::OnTransactionComplete, base::Unretained(this),
-                   true /* first_query */, base::TimeTicks::Now()),
+                   base::TimeTicks::Now()),
         net_log_);
   }
 
-  void Start() {
-    net_log_.BeginEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_DNS_TASK);
-    transaction_->Start();
-  }
-
- private:
-  void OnTransactionComplete(bool first_query,
-                             const base::TimeTicks& start_time,
+  void OnTransactionComplete(const base::TimeTicks& start_time,
                              DnsTransaction* transaction,
                              int net_error,
                              const DnsResponse* response) {
     DCHECK(transaction);
     base::TimeDelta duration = base::TimeTicks::Now() - start_time;
-    // Run |callback_| last since the owning Job will then delete this DnsTask.
     if (net_error != OK) {
       DNS_HISTOGRAM("AsyncDNS.TransactionFailure", duration);
       OnFailure(net_error, DnsResponse::DNS_PARSE_OK);
       return;
     }
 
-    CHECK(response);
     DNS_HISTOGRAM("AsyncDNS.TransactionSuccess", duration);
     switch (transaction->GetType()) {
       case dns_protocol::kTypeA:
         DNS_HISTOGRAM("AsyncDNS.TransactionSuccess_A", duration);
         break;
       case dns_protocol::kTypeAAAA:
         DNS_HISTOGRAM("AsyncDNS.TransactionSuccess_AAAA", duration);
         break;
     }
+
     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_);
-        transaction_->Start();
-        return;
-      }
+    ++num_completed_transactions_;
+    if (num_completed_transactions_ == 1) {
+      ttl_ = ttl;
     } else {
-      DCHECK_EQ(ADDRESS_FAMILY_UNSPECIFIED, family_);
-      bool has_ipv6_addresses = !addr_list.empty();
-      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 = (has_ipv6_addresses && addr_list.size() > 1);
+      ttl_ = std::min(ttl_, ttl);
     }
 
-    if (addr_list.empty()) {
-      // TODO(szym): Don't fallback to ProcTask in this case.
-      OnFailure(ERR_NAME_NOT_RESOLVED, DnsResponse::DNS_PARSE_OK);
+    if (transaction->GetType() == dns_protocol::kTypeA) {
+      DCHECK_EQ(transaction_a_.get(), transaction);
+      // Place IPv4 addresses after IPv6.
+      addr_list_.insert(addr_list_.end(), addr_list.begin(), addr_list.end());
+    } else {
+      DCHECK_EQ(transaction_aaaa_.get(), transaction);
+      // Place IPv6 addresses before IPv4.
+      addr_list_.insert(addr_list_.begin(), addr_list.begin(), addr_list.end());
+    }
+
+    if (needs_two_transactions() && num_completed_transactions_ == 1) {

[szym, 2013/08/06 20:35:31]

At this point we could also set key_.hostname = transaction->GetHostname() so
that if the second transaction is still queued, it does not need to perform
suffix search.

[mmenke, 2013/08/19 17:31:11]

Done.  Wonder how difficult it would be to make a unit test for this...

+      delegate_->OnFirstDnsTransactionComplete();
       return;
     }
 
-    if (needs_sort) {
-      // Sort could complete synchronously.
+    // If there are multiple addresses, and at least one is IPv6, need to sort
+    // them.  Note that IPv6 addresses are always put before IPv4 ones, so it's
+    // sufficient to just check the family of the first address.
+    if (addr_list_.size() > 1 &&
+        addr_list_[0].GetFamily() == ADDRESS_FAMILY_IPV6) {
+      // Sort addresses if needed.  Sort could complete synchronously.
       client_->GetAddressSorter()->Sort(
-          addr_list,
+          addr_list_,
           base::Bind(&DnsTask::OnSortComplete,
                      AsWeakPtr(),
-                     base::TimeTicks::Now(),
-                     ttl));
+                     base::TimeTicks::Now()));
     } else {
-      OnSuccess(addr_list, ttl);
+      OnSuccess(addr_list_);
     }
   }
 
   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);
+    OnSuccess(addr_list);
   }
 
   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());
+    delegate_->OnDnsTaskComplete(task_start_time_, net_error, AddressList(),
+                                 base::TimeDelta());
   }
 
-  void OnSuccess(const AddressList& addr_list, base::TimeDelta ttl) {
+  void OnSuccess(const AddressList& addr_list) {
     net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_DNS_TASK,
                       addr_list.CreateNetLogCallback());
-    callback_.Run(OK, addr_list, ttl);
+    delegate_->OnDnsTaskComplete(task_start_time_, OK, addr_list, ttl_);
   }
 
   DnsClient* client_;
-  AddressFamily family_;
+  Key key_;
+
   // The listener to the results of this DnsTask.
-  Callback callback_;
+  Delegate* delegate_;
   const BoundNetLog net_log_;
 
-  scoped_ptr<DnsTransaction> transaction_;
+  scoped_ptr<DnsTransaction> transaction_a_;
+  scoped_ptr<DnsTransaction> transaction_aaaa_;
 
-  // Results from the first transaction. Used only if |family_| is unspecified.
-  AddressList first_addr_list_;
-  base::TimeDelta first_ttl_;
+  // Number of transactions that have started, but not yet completed.

[szym, 2013/08/06 20:35:31]

The name of the variable is in direct conflict with the comment above it. I
don't think this really needs a comment.

[mmenke, 2013/08/19 17:31:11]

Done.

+  unsigned num_completed_transactions_;
+
+  // These are updated as each transaction completes.
+  base::TimeDelta ttl_;
+  // IPv6 addresses must appear first in the list.
+  AddressList addr_list_;
+
+  base::TimeTicks task_start_time_;
 
   DISALLOW_COPY_AND_ASSIGN(DnsTask);
 };
 
 //-----------------------------------------------------------------------------
 
 // Aggregates all Requests for the same Key. Dispatched via PriorityDispatch.
-class HostResolverImpl::Job : public PrioritizedDispatcher::Job {
+class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
+                              public HostResolverImpl::DnsTask::Delegate {
  public:
   // Creates new job for |key| where |request_net_log| is bound to the
   // request that spawned it.
   Job(const base::WeakPtr<HostResolverImpl>& resolver,
       const Key& key,
       RequestPriority priority,
       const BoundNetLog& request_net_log)
       : resolver_(resolver),
         key_(key),
         priority_tracker_(priority),
         had_non_speculative_request_(false),
         had_dns_config_(false),
+        num_occupied_job_slots_(0),
         dns_task_error_(OK),
         creation_time_(base::TimeTicks::Now()),
         priority_change_time_(creation_time_),
         net_log_(BoundNetLog::Make(request_net_log.net_log(),
                                    NetLog::SOURCE_HOST_RESOLVER_IMPL_JOB)) {
     request_net_log.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_CREATE_JOB);
 
     net_log_.BeginEvent(
         NetLog::TYPE_HOST_RESOLVER_IMPL_JOB,
         base::Bind(&NetLogJobCreationCallback,
                    request_net_log.source(),
                    &key_.hostname));
   }
 
   virtual ~Job() {
     if (is_running()) {
       // |resolver_| was destroyed with this Job still in flight.
       // Clean-up, record in the log, but don't run any callbacks.
       if (is_proc_running()) {
         proc_task_->Cancel();
         proc_task_ = NULL;
       }
       // Clean up now for nice NetLog.
-      dns_task_.reset(NULL);
+      KillDnsTask();

[szym, 2013/08/06 20:35:31]

I suggest leave it as is. Job is destroyed while running only if the whole
resolver is destroyed as well, so all we want to do is log.

[mmenke, 2013/08/19 17:31:11]

I'm not comfortable relying on that being true - it's the kind of thing that's
hard to test for.  Could very easily see that change, and then permanently
losing dispatcher slots as a result.

[szym, 2013/08/19 18:47:21]

I understand the sentiment. My major concern is any extra work during
destruction:
~HostResolverImpl
STLDeleteValues
~Job
Job::KillDnsTask
PrioritizedDispatcher::OnJobComplete
Job::Start
// spawns new WorkerPool tasks or sends new UDP packets

Would it make sense to add PrioritizedDispatcher::Flush() or ::Clear() which
means "remove all Jobs from all queues"?

[mmenke, 2013/08/19 19:31:33]

I don't think performance is a huge concern, since it happens at most 8 times
(We only cancel/complete the second request)....  However, I agree that the
re-entrancy you're pointing out is a greater concern than my worry about
regressions.

I don't think adding either of the new functions you suggest to the
PrioritizedDispatcher really answers my concern, which is after some future
refactor, we end up here when not deleting a Job, so permanently lose a job
slot.  We could add some sort of Pause() or PrepareToDie() function that stops
it from starting new jobs, but I think either would be more effort than it's
worth.

I've gone with your original suggestion, with a comment about it.  Open to other
ideas.

[szym, 2013/08/19 19:46:45]

I wasn't clear. I am suggesting going with your code, but in:
~HostResolverImpl(){
  dispatcher_.Flush();  // Empty the queues so that no further jobs get started.
  STLDeleteValues(jobs_);  // It's ok to KillDnsTask here, because OnJobComplete
is almost a no-op.
}

[mmenke, 2013/08/19 19:57:40]

That causes problems in the other case...  When ReduceToOneJobSlot() calls
resolver_->dispatcher_.Cancel(handle_), then handle won't have been queued, and
we'd get a DCHECK in PriorityQueue::Erase.  We could remove those DCHECKs,
though I rather like them.

[szym, 2013/08/19 20:00:41]

Well, then how about PrioritizedDispatcher::Disable() which tells it to stop
dispatching jobs? The simplest way to implement this is to reduce the number of
allowed job slots to 0.

[mmenke, 2013/08/19 20:41:12]

Done.  That was what I meant by my PrepareToDie() suggestion above.  More
complicated than I like, but it works, and it's not too bad.

       net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB,
                                         ERR_ABORTED);
     } else if (is_queued()) {
       // |resolver_| was destroyed without running this Job.
       // TODO(szym): is there any benefit in having this distinction?
       net_log_.AddEvent(NetLog::TYPE_CANCELLED);
       net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB);
     }
     // else CompleteRequests logged EndEvent.
 
     // Log any remaining Requests as cancelled.
     for (RequestsList::const_iterator it = requests_.begin();
          it != requests_.end(); ++it) {
       Request* req = *it;
       if (req->was_canceled())
         continue;
       DCHECK_EQ(this, req->job());
       LogCancelRequest(req->source_net_log(), req->request_net_log(),
                        req->info());
     }
   }
 
-  // Add this job to the dispatcher.
-  void Schedule() {
-    handle_ = resolver_->dispatcher_.Add(this, priority());
+  // Add this job to the dispatcher.  If "at_head" is true, adds at the front
+  // of the queue.
+  void Schedule(bool at_head) {
+    DCHECK(!is_queued());
+    PrioritizedDispatcher::Handle handle;
+    if (!at_head) {
+      handle = resolver_->dispatcher_.Add(this, priority());
+    } else {
+      handle = resolver_->dispatcher_.AddAtHead(this, priority());
+    }
+    // This test is needed for the case that the dispatcher starts |this|
+    // synchronously, and then |this| adds itself again to the dispatcher,
+    // and the second attempt does not complete synchronously.

[szym, 2013/08/06 20:35:31]

"attempt" is ambiguous. Suggest:
"The dispatcher could have started |this| in Add, which could have called
Schedule again. In that case |handle| will be null, but |handle_| will already
have been set by the other Schedule."

[mmenke, 2013/08/19 17:31:11]

Done (With some slight rewording).

+    if (!handle.is_null()) {
+      DCHECK(handle_.is_null());
+      handle_ = handle;
+    }
   }
 
   void AddRequest(scoped_ptr<Request> req) {
     DCHECK_EQ(key_.hostname, req->info().hostname());
 
     req->set_job(this);
     priority_tracker_.Add(req->info().priority());
 
     req->request_net_log().AddEvent(
         NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_ATTACH,
@@ skipping 48 matching lines @@
   // Called from AbortAllInProgressJobs. Completes all requests and destroys
   // the job. This currently assumes the abort is due to a network change.
   void Abort() {
     DCHECK(is_running());
     CompleteRequestsWithError(ERR_NETWORK_CHANGED);
   }
 
   // If DnsTask present, abort it and fall back to ProcTask.
   void AbortDnsTask() {
     if (dns_task_) {
-      dns_task_.reset();
+      KillDnsTask();
       dns_task_error_ = OK;
       StartProcTask();
     }
   }
 
   // Called by HostResolverImpl when this job is evicted due to queue overflow.
   // Completes all requests and destroys the job.
   void OnEvicted() {
     DCHECK(!is_running());
     DCHECK(is_queued());
@@ skipping 28 matching lines @@
 
   bool is_queued() const {
     return !handle_.is_null();
   }
 
   bool is_running() const {
     return is_dns_running() || is_proc_running();
   }
 
  private:
+  void KillDnsTask() {
+    if (dns_task_) {
+      FinishOrCancelSecondDispatcherJob();
+      dns_task_.reset();
+    }
+  }
+
+  // If this task is occupying two slots in the PrioritizedDispatcher, cancels
+  // or finishes the second Job, depending on if its started or not.  Otherwise,
+  // does nothing.
+  void FinishOrCancelSecondDispatcherJob() {

[szym, 2013/08/06 20:35:31]

I've spent a lot of time trying to make it more obvious what this function does.
The key problem seems to be that it is supposed to solve three separate issues,
but the code ends up the same in both cases.

Case 1: The Job ends while running, so clean it up.
 Subcase 1: occupying two job slots. Signal OnJobFinished for the extra slot.
 Subcase 2: queued for another job slot. Cancel the queue handle.
OnJobFinished for the remaining slot is called in CompleteRequests.

Case 2: Work in the first job slot has finished. Leverage it for the other job
slot.
 Subcase 1: other job slot already running. Signal OnJobFinished for the
completed slot.
 Subcase 2: other job still in queue. Cancel its queue handle and take over the
job slot we already have.

Case 3: DnsTask failed. Fallback to ProcTask.
 Similar subcases to case 1, except that ProcTask takes over the remaining slot.


How about:
// Reduce the number of job slots occupied and queued in the dispatcher
// to one. If there is work still queued for the second job slot, let it
// take over this one.
ReduceToOneJobSlot() {
  DCHECK(num_occupied_job_slots_ >= 1);
  if (is_queued()) {
    dispatcher_.Cancel(handle_);
    handle_.Reset();
  } else if (num_occupied_job_slots_ > 1) {
    dispatcher_.OnJobFinished();
    --num_occupied_job_slots_;
  }
  DCHECK(num_occupied_job_slots_ == 1);
}

[mmenke, 2013/08/19 17:31:11]

Done, though I changed the comment (What "let" means in the second sentence
seems rather ambiguous), and kept the DCHECK_*s instead of DCHECK.

+    DCHECK_GE(num_occupied_job_slots_, 1u);
+    if (num_occupied_job_slots_ > 1) {
+      DCHECK(dns_task_->needs_two_transactions());
+      resolver_->dispatcher_.OnJobFinished();
+      --num_occupied_job_slots_;
+    } else if (is_queued()) {
+      resolver_->dispatcher_.Cancel(handle_);
+      handle_.Reset();
+    }
+    DCHECK_EQ(1u, num_occupied_job_slots_);
+  }
+
   void UpdatePriority() {
     if (is_queued()) {
       if (priority() != static_cast<RequestPriority>(handle_.priority()))
         priority_change_time_ = base::TimeTicks::Now();
       handle_ = resolver_->dispatcher_.ChangePriority(handle_, priority());
     }
   }
 
   AddressList MakeAddressListForRequest(const AddressList& list) const {
     if (requests_.empty())
       return list;
     return AddressList::CopyWithPort(list, requests_.front()->info().port());
   }
 
   // PriorityDispatch::Job:
   virtual void Start() OVERRIDE {
+    DCHECK_LE(num_occupied_job_slots_, 1u);
+
+    handle_.Reset();
+    ++num_occupied_job_slots_;
+
+    if (num_occupied_job_slots_ == 2) {
+      StartSecondDnsTransaction();
+      return;
+    }
+
     DCHECK(!is_running());
-    handle_.Reset();
 
     net_log_.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_STARTED);
 
     had_dns_config_ = resolver_->HaveDnsConfig();
 
     base::TimeTicks now = base::TimeTicks::Now();
     base::TimeDelta queue_time = now - creation_time_;
     base::TimeDelta queue_time_after_change = now - priority_change_time_;
 
     if (had_dns_config_) {
@@ skipping 81 matching lines @@
       ttl = base::TimeDelta::FromSeconds(kCacheEntryTTLSeconds);
 
     // Don't store the |ttl| in cache since it's not obtained from the server.
     CompleteRequests(
         HostCache::Entry(net_error, MakeAddressListForRequest(addr_list)),
         ttl);
   }
 
   void StartDnsTask() {
     DCHECK(resolver_->HaveDnsConfig());
-    base::TimeTicks start_time = base::TimeTicks::Now();
-    dns_task_.reset(new DnsTask(
-        resolver_->dns_client_.get(),
-        key_,
-        base::Bind(&Job::OnDnsTaskComplete, base::Unretained(this), start_time),
-        net_log_));
+    dns_task_.reset(new DnsTask(resolver_->dns_client_.get(), key_, this,
+                                net_log_));
 
-    dns_task_->Start();
+    dns_task_->StartFirstTransaction();
+    // Schedule a second transaciton, if needed.

[szym, 2013/08/06 20:35:31]

nit: transaction

[mmenke, 2013/08/19 17:31:11]

Done.

+    if (dns_task_->needs_two_transactions())
+      Schedule(true);
+  }
+
+  void StartSecondDnsTransaction() {
+    DCHECK(dns_task_->needs_two_transactions());
+    dns_task_->StartSecondTransaction();
   }
 
   // Called if DnsTask fails. It is posted from StartDnsTask, so Job may be
   // deleted before this callback. In this case dns_task is deleted as well,
   // so we use it as indicator whether Job is still valid.
   void OnDnsTaskFailure(const base::WeakPtr<DnsTask>& dns_task,
                         base::TimeDelta duration,
                         int net_error) {
     DNS_HISTOGRAM("AsyncDNS.ResolveFail", duration);
 
     if (dns_task == NULL)
       return;
 
     dns_task_error_ = net_error;
 
     // TODO(szym): Run ServeFromHosts now if nsswitch.conf says so.
     // http://crbug.com/117655
 
     // TODO(szym): Some net errors indicate lack of connectivity. Starting
     // ProcTask in that case is a waste of time.
     if (resolver_->fallback_to_proctask_) {
-      dns_task_.reset();
+      KillDnsTask();
       StartProcTask();
     } else {
       UmaAsyncDnsResolveStatus(RESOLVE_STATUS_FAIL);
       CompleteRequestsWithError(net_error);
     }
   }
 
-  // Called by DnsTask when it completes.
-  void OnDnsTaskComplete(base::TimeTicks start_time,
-                         int net_error,
-                         const AddressList& addr_list,
-                         base::TimeDelta ttl) {
+
+  // HostResolverImpl::DnsTask::Delegate implementation:
+
+  virtual void OnDnsTaskComplete(base::TimeTicks start_time,
+                                 int net_error,
+                                 const AddressList& addr_list,
+                                 base::TimeDelta ttl) OVERRIDE {
     DCHECK(is_dns_running());
 
     base::TimeDelta duration = base::TimeTicks::Now() - start_time;
     if (net_error != OK) {
       OnDnsTaskFailure(dns_task_->AsWeakPtr(), duration, net_error);
       return;
     }
     DNS_HISTOGRAM("AsyncDNS.ResolveSuccess", duration);
     // Log DNS lookups based on |address_family|.
     switch(key_.address_family) {
@@ skipping 14 matching lines @@
     resolver_->OnDnsTaskResolve(OK);
 
     base::TimeDelta bounded_ttl =
         std::max(ttl, base::TimeDelta::FromSeconds(kMinimumTTLSeconds));
 
     CompleteRequests(
         HostCache::Entry(net_error, MakeAddressListForRequest(addr_list), ttl),
         bounded_ttl);
   }
 
+  virtual void OnFirstDnsTransactionComplete() OVERRIDE {
+    DCHECK(dns_task_->needs_two_transactions());
+    // If the second job is still queued, start it now.

[szym, 2013/08/06 20:35:31]

"Second job" is a bit confusing. Suggest further explanation:
We already have a job slot at the dispatcher, so reuse it now instead of waiting
in the queue for the second slot.

[mmenke, 2013/08/19 17:31:11]

Done (Comment slightly modified).

+    if (is_queued())
+      dns_task_->StartSecondTransaction();
+    FinishOrCancelSecondDispatcherJob();

[szym, 2013/08/06 20:35:31]

If we already started both jobs, then we are actually finishing the _first_
slot. Renaming the function will help.

We StartSecondTransaction before canceling the second dispatcher job from the
queue. I understand this is because after canceling, we can't check is_queued(),
but it's counter-intuitive.

It'd be more obvious if it was:

DCHECK_EQ(dns_task_->needs_another_transaction(), is_queued());
ReduceToOneJobSlot();
if (dns_task_->needs_another_transaction())
  dns_task_->StartSecondTransaction();

[mmenke, 2013/08/19 17:31:11]

Done.

+  }
+
   // Performs Job's last rites. Completes all Requests. Deletes this.
   void CompleteRequests(const HostCache::Entry& entry,
                         base::TimeDelta ttl) {
     CHECK(resolver_.get());
 
     // This job must be removed from resolver's |jobs_| now to make room for a
     // new job with the same key in case one of the OnComplete callbacks decides
     // to spawn one. Consequently, the job deletes itself when CompleteRequests
     // is done.
     scoped_ptr<Job> self_deleter(this);
 
     resolver_->RemoveJob(this);
 
     if (is_running()) {
-      DCHECK(!is_queued());
       if (is_proc_running()) {
+        DCHECK(!is_queued());
         proc_task_->Cancel();
         proc_task_ = NULL;
       }
-      dns_task_.reset();
+      KillDnsTask();
 
       // Signal dispatcher that a slot has opened.
       resolver_->dispatcher_.OnJobFinished();
     } else if (is_queued()) {
       resolver_->dispatcher_.Cancel(handle_);
       handle_.Reset();
     }
 
     if (num_active_requests() == 0) {
       net_log_.AddEvent(NetLog::TYPE_CANCELLED);
@@ skipping 73 matching lines @@
   Key key_;
 
   // Tracks the highest priority across |requests_|.
   PriorityTracker priority_tracker_;
 
   bool had_non_speculative_request_;
 
   // Distinguishes measurements taken while DnsClient was fully configured.
   bool had_dns_config_;
 
+  // Number of slots occupied by this Job in resolver's PrioritizedDispatcher.
+  unsigned num_occupied_job_slots_;
+
   // Result of DnsTask.
   int dns_task_error_;
 
   const base::TimeTicks creation_time_;
   base::TimeTicks priority_change_time_;
 
   BoundNetLog net_log_;
 
   // Resolves the host using a HostResolverProc.
   scoped_refptr<ProcTask> proc_task_;
@@ skipping 119 matching lines @@
   }
 
   // Next we need to attach our request to a "job". This job is responsible for
   // calling "getaddrinfo(hostname)" on a worker thread.
 
   JobMap::iterator jobit = jobs_.find(key);
   Job* job;
   if (jobit == jobs_.end()) {
     job = new Job(weak_ptr_factory_.GetWeakPtr(), key, info.priority(),
                   request_net_log);
-    job->Schedule();
+    job->Schedule(false);
 
     // Check for queue overflow.
     if (dispatcher_.num_queued_jobs() > max_queued_jobs_) {
       Job* evicted = static_cast<Job*>(dispatcher_.EvictOldestLowest());
       DCHECK(evicted);
       evicted->OnEvicted();  // Deletes |evicted|.
       if (evicted == job) {
         rv = ERR_HOST_RESOLVER_QUEUE_TOO_LARGE;
         LogFinishRequest(source_net_log, request_net_log, info, rv);
         return rv;
@@ skipping 277 matching lines @@
     Job* job = it->second;
     if (job->is_running()) {
       jobs_to_abort.push_back(job);
       jobs_.erase(it++);
     } else {
       DCHECK(job->is_queued());
       ++it;
     }
   }
 
-  // Check if no dispatcher slots leaked out.
-  DCHECK_EQ(dispatcher_.num_running_jobs(), jobs_to_abort.size());
-
   // Life check to bail once |this| is deleted.
   base::WeakPtr<HostResolverImpl> self = weak_ptr_factory_.GetWeakPtr();
 
   // Then Abort them.
   for (size_t i = 0; self.get() && i < jobs_to_abort.size(); ++i) {
     jobs_to_abort[i]->Abort();
     jobs_to_abort[i] = NULL;
   }
 }
 
@@ skipping 111 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