[net/dns] Refactoring of job dispatch in HostResolverImpl in preparation for DnsTransactionFactory.

net/base/host_resolver_impl.cc

 // Copyright (c) 2011 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/base/host_resolver_impl.h"
 
 #if defined(OS_WIN)
 #include <Winsock2.h>
 #elif defined(OS_POSIX)
 #include <netdb.h>
 #endif
 
 #include <cmath>
-#include <deque>
+#include <utility>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/debug/debugger.h"
 #include "base/debug/stack_trace.h"
 #include "base/message_loop_proxy.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
 #include "base/threading/worker_pool.h"
 #include "base/time.h"
@@ skipping 94 matching lines @@
 
   return base::CustomHistogram::ArrayToCustomRanges(os_errors,
                                                     arraysize(os_errors));
 }
 
 }  // anonymous namespace
 
 // static
 HostResolver* CreateSystemHostResolver(size_t max_concurrent_resolves,
                                        size_t max_retry_attempts,
                                        NetLog* net_log) {

[mmenke, 2012/01/09 16:57:24]

nit:  Just to reduce code duplication, I'd suggest making this and
CreateNonCachingSystemHostResolver call the same function internally.

   // Maximum of 8 concurrent resolver threads.
   // Some routers (or resolvers) appear to start to provide host-not-found if
   // too many simultaneous resolutions are pending.  This number needs to be
   // further optimized, but 8 is what FF currently does.
   static const size_t kDefaultMaxJobs = 8u;
 
   if (max_concurrent_resolves == HostResolver::kDefaultParallelism)
     max_concurrent_resolves = kDefaultMaxJobs;
 
-  HostResolverImpl* resolver =
-      new HostResolverImpl(NULL, HostCache::CreateDefaultCache(),
-          max_concurrent_resolves, max_retry_attempts, net_log);
+  // TODO(szym): Add experiments with reserved slots for higher priority
+  // requests.
+
+  PrioritizedDispatcher::Limits limits(NUM_PRIORITIES, max_concurrent_resolves);
+
+  HostResolverImpl* resolver = new HostResolverImpl(
+      HostCache::CreateDefaultCache(),
+      limits,
+      HostResolverImpl::ProcTaskParams(NULL, max_retry_attempts),
+      net_log);
 
   return resolver;
 }
+
+// static
+HostResolver* CreateNonCachingSystemHostResolver(size_t max_concurrent_resolves,

[cbentzel, 2012/01/09 16:05:18]

This should share a helper function with CreateSystemHostResolver - lots of
duplicate logic.

[szym, 2012/01/09 17:06:59]

Yup, just copied-and-pasted to make ConnectionTester work. Will fix.

+                                                 size_t max_retry_attempts,
+                                                 NetLog* net_log) {
+  // Maximum of 8 concurrent resolver threads.
+  // Some routers (or resolvers) appear to start to provide host-not-found if
+  // too many simultaneous resolutions are pending.  This number needs to be
+  // further optimized, but 8 is what FF currently does.
+  static const size_t kDefaultMaxJobs = 8u;
+
+  if (max_concurrent_resolves == HostResolver::kDefaultParallelism)
+    max_concurrent_resolves = kDefaultMaxJobs;
+
+  // TODO(szym): Add experiments with reserved slots for higher priority
+  // requests.
+
+  PrioritizedDispatcher::Limits limits(NUM_PRIORITIES, max_concurrent_resolves);
+
+  HostResolverImpl* resolver = new HostResolverImpl(
+      NULL,
+      limits,
+      HostResolverImpl::ProcTaskParams(NULL, max_retry_attempts),
+      net_log);
+
+  return resolver;
+}
 
 static int ResolveAddrInfo(HostResolverProc* resolver_proc,
                            const std::string& host,
                            AddressFamily address_family,
                            HostResolverFlags host_resolver_flags,
                            AddressList* out,
                            int* os_error) {
   if (resolver_proc) {
     // Use the custom procedure.
     return resolver_proc->Resolve(host, address_family,
@@ skipping 73 matching lines @@
       dict->Set("source_dependency", source_.ToValue());
 
     return dict;
   }
 
  private:
   const HostResolver::RequestInfo info_;
   const NetLog::Source source_;
 };
 
-// Parameters associated with the creation of a HostResolverImpl::Job.
+// Parameters associated with the creation of a HostResolverImpl::Job
+// or a HostResolverImpl::ProcTask.
 class JobCreationParameters : public NetLog::EventParameters {
  public:
-  JobCreationParameters(const std::string& host, const NetLog::Source& source)
+  JobCreationParameters(const std::string& host,
+                        const NetLog::Source& source)
       : host_(host), source_(source) {}
 
   virtual Value* ToValue() const {
     DictionaryValue* dict = new DictionaryValue();
     dict->SetString("host", host_);
     dict->Set("source_dependency", source_.ToValue());
     return dict;
   }
 
  private:
   const std::string host_;
   const NetLog::Source source_;
 };
 
+// Parameters of the HOST_RESOLVER_IMPL_JOB_ATTACH/DETACH event.
+class JobAttachParameters : public NetLog::EventParameters {
+ public:
+  JobAttachParameters(const NetLog::Source& source,
+                      RequestPriority priority)
+      : source_(source), priority_(priority) {}
+
+  virtual Value* ToValue() const {

[mmenke, 2012/01/09 16:57:24]

nit:  OVERRIDE

+    DictionaryValue* dict = new DictionaryValue();
+    dict->Set("source_dependency", source_.ToValue());
+    dict->SetInteger("priority", priority_);
+    return dict;
+  }
+
+ private:
+  const NetLog::Source source_;
+  const RequestPriority priority_;
+};
+
+
 //-----------------------------------------------------------------------------
 
+// Holds the data for a request that could not be complete synchronously.

[mmenke, 2012/01/09 16:57:24]

nit:  "not be completed"

+// It is owned by a Job. Canceled Requests are only marked as canceled rather
+// than removed from the Job's |requests_| list.
 class HostResolverImpl::Request {
  public:
   Request(const BoundNetLog& source_net_log,
           const BoundNetLog& request_net_log,
           const RequestInfo& info,
           const CompletionCallback& callback,
           AddressList* addresses)
       : source_net_log_(source_net_log),
         request_net_log_(request_net_log),
         info_(info),
         job_(NULL),
         callback_(callback),
         addresses_(addresses) {
   }
 
-  // Mark the request as cancelled.
-  void MarkAsCancelled() {
+  // Mark the request as canceled.
+  void MarkAsCanceled() {
     job_ = NULL;
     addresses_ = NULL;
     callback_.Reset();
   }
 
-  bool was_cancelled() const {
+  bool was_canceled() const {
     return callback_.is_null();
   }
 
   void set_job(Job* job) {
-    DCHECK(job != NULL);
+    DCHECK(job);
     // Identify which job the request is waiting on.
     job_ = job;
   }
 
+  // Prepare final AddressList and call completion callback.
   void OnComplete(int error, const AddressList& addrlist) {
     if (error == OK)
-      *addresses_ = CreateAddressListUsingPort(addrlist, port());
+      *addresses_ = CreateAddressListUsingPort(addrlist, info_.port());
     CompletionCallback callback = callback_;
-    MarkAsCancelled();
+    MarkAsCanceled();
     callback.Run(error);
   }
 
-  int port() const {
-    return info_.port();
-  }
-
   Job* job() const {
     return job_;
   }
 
+  // NetLog for the source, passed in HostResolver::Resolve.
   const BoundNetLog& source_net_log() {
     return source_net_log_;
   }
 
+  // NetLog for this request.
   const BoundNetLog& request_net_log() {
     return request_net_log_;
   }
 
   const RequestInfo& info() const {
     return info_;
   }
 
  private:
   BoundNetLog source_net_log_;
   BoundNetLog request_net_log_;
 
   // The request info that started the request.
   RequestInfo info_;
 
-  // The resolve job (running in worker pool) that this request is dependent on.
+  // The resolve job that this request is dependent on.
   Job* job_;
 
   // The user's callback to invoke when the request completes.
   CompletionCallback callback_;
 
   // The address list to save result into.
   AddressList* addresses_;
 
   DISALLOW_COPY_AND_ASSIGN(Request);
 };
 
 //------------------------------------------------------------------------------
 
 // Provide a common macro to simplify code and readability. We must use a
 // macros as the underlying HISTOGRAM macro creates static varibles.
 #define DNS_HISTOGRAM(name, time) UMA_HISTOGRAM_CUSTOM_TIMES(name, time, \
     base::TimeDelta::FromMicroseconds(1), base::TimeDelta::FromHours(1), 100)
 
-// This class represents a request to the worker pool for a "getaddrinfo()"
-// call.
-class HostResolverImpl::Job
-    : public base::RefCountedThreadSafe<HostResolverImpl::Job> {
+// Calls HostResolverProc on the WorkerPool. Performs retries if necessary.
+//
+// Whenever we try to resolve the host, we post a delayed task to check if host
+// resolution (OnLookupComplete) is completed or not. If the original attempt
+// hasn't completed, then we start another attempt for host resolution. We take
+// the results from the first attempt that finishes and ignore the results from
+// all other attempts.
+//
+// TODO(szym): Move to separate source file for testing and mocking.
+//
+class HostResolverImpl::ProcTask
+    : public base::RefCountedThreadSafe<HostResolverImpl::ProcTask> {
  public:
-  Job(int id,
-      HostResolverImpl* resolver,
-      const Key& key,
-      const BoundNetLog& source_net_log,
-      NetLog* net_log)
-     : id_(id),
-       key_(key),
-       resolver_(resolver),
-       origin_loop_(base::MessageLoopProxy::current()),
-       resolver_proc_(resolver->effective_resolver_proc()),
-       unresponsive_delay_(resolver->unresponsive_delay()),
-       attempt_number_(0),
-       completed_attempt_number_(0),
-       completed_attempt_error_(ERR_UNEXPECTED),
-       had_non_speculative_request_(false),
-       net_log_(BoundNetLog::Make(net_log,
-                                  NetLog::SOURCE_HOST_RESOLVER_IMPL_JOB)) {
-    DCHECK(resolver);
-    net_log_.BeginEvent(
-        NetLog::TYPE_HOST_RESOLVER_IMPL_JOB,
-        make_scoped_refptr(
-            new JobCreationParameters(key.hostname, source_net_log.source())));
-  }
+  typedef base::Callback<void(int, int, const AddressList&)> Callback;
 
-  // Attaches a request to this job. The job takes ownership of |req| and will
-  // take care to delete it.
-  void AddRequest(Request* req) {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    req->request_net_log().BeginEvent(
-        NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_ATTACH,
-        make_scoped_refptr(new NetLogSourceParameter(
-            "source_dependency", net_log_.source())));
+  ProcTask(const Key& key,
+          const ProcTaskParams& params,

[cbentzel, 2012/01/09 16:05:18]

Nit: line up arguments

[mmenke, 2012/01/09 16:57:24]

nit:  indent +1

+          const Callback& callback,
+          const BoundNetLog& job_net_log)
+      : key_(key),
+        params_(params),
+        callback_(callback),
+        origin_loop_(base::MessageLoopProxy::current()),
+        attempt_number_(0),
+        completed_attempt_number_(0),
+        completed_attempt_error_(ERR_UNEXPECTED),
+        had_non_speculative_request_(false),
+        net_log_(BoundNetLog::Make(
+            job_net_log.net_log(),
+            NetLog::SOURCE_HOST_RESOLVER_IMPL_PROC_TASK)) {
+    if (!params_.resolver_proc)

[cbentzel, 2012/01/09 16:05:18]

The logic for which proc to use is a bit confusing - partly residual from
before.

if a non-NULL proc is specified, use it.
otherwise, use the default one at the time the ProcTask is created.
otherwise, use SystemHostResolverProc on the worker thread.

It would be a lot clearer if this always needed to be non-NULL, and convert
SystemHostResolverProc over to be a HostResolverProc class. 

I don't think it needs to be addressed in this CL, but might be a good way to
go.

+      params_.resolver_proc = HostResolverProc::GetDefault();
 
-    req->set_job(this);
-    requests_.push_back(req);
+    job_net_log.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_TASK,
+                         new NetLogSourceParameter("source_dependency",
+                                                   net_log_.source()));
 
-    if (!req->info().is_speculative())
-      had_non_speculative_request_ = true;
+    net_log_.BeginEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_TASK,
+                        new JobCreationParameters(key_.hostname,
+                                                  job_net_log.source()));
   }
 
   void Start() {
     DCHECK(origin_loop_->BelongsToCurrentThread());
     StartLookupAttempt();
   }
 
+  // Cancels this ProcTask. It will be orphaned. Any outstanding resolve
+  // attempts running on worker threads will continue running. Only once all the
+  // attempts complete will the final reference to this ProcTask be released.
+  void Cancel() {
+    DCHECK(origin_loop_->BelongsToCurrentThread());
+
+    if (was_canceled())
+      return;
+
+    net_log_.AddEvent(NetLog::TYPE_CANCELLED, NULL);
+
+    callback_.Reset();
+
+    net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_TASK, NULL);
+  }
+
+  void set_had_non_speculative_request() {
+    DCHECK(origin_loop_->BelongsToCurrentThread());
+    had_non_speculative_request_ = true;
+  }
+
+  bool was_canceled() const {
+    DCHECK(origin_loop_->BelongsToCurrentThread());

[cbentzel, 2012/01/09 16:05:18]

This looks incorrect. It will return true even if the resolution completes
successfully and was never explicitly canceled, due to the reset at the end of
OnLookupComplete. This will be bad for statistics gathering on re-issues.

[szym, 2012/01/09 17:06:59]

True. I added callback reset at the end of OnLookupComplete to avoid re-entrancy
issues, but looking at OnProcTaskComplete, I think it will be fine to remove the
reset.

+    return callback_.is_null();
+  }
+
+  bool was_completed() const {
+    DCHECK(origin_loop_->BelongsToCurrentThread());
+    return completed_attempt_number_ > 0;
+  }
+
+ private:
   void StartLookupAttempt() {
     DCHECK(origin_loop_->BelongsToCurrentThread());
     base::TimeTicks start_time = base::TimeTicks::Now();
     ++attempt_number_;
     // Dispatch the lookup attempt to a worker thread.
     if (!base::WorkerPool::PostTask(
             FROM_HERE,
-            base::Bind(&Job::DoLookup, this, start_time, attempt_number_),
+            base::Bind(&ProcTask::DoLookup, this, start_time, attempt_number_),
             true)) {
       NOTREACHED();
 
       // Since we could be running within Resolve() right now, we can't just
       // call OnLookupComplete().  Instead we must wait until Resolve() has
       // returned (IO_PENDING).
       origin_loop_->PostTask(
           FROM_HERE,
-          base::Bind(&Job::OnLookupComplete, this, AddressList(),
+          base::Bind(&ProcTask::OnLookupComplete, this, AddressList(),
                      start_time, attempt_number_, ERR_UNEXPECTED, 0));
       return;
     }
 
     net_log_.AddEvent(
         NetLog::TYPE_HOST_RESOLVER_IMPL_ATTEMPT_STARTED,
         make_scoped_refptr(new NetLogIntegerParameter(
             "attempt_number", attempt_number_)));
 
     // Post a task to check if we get the results within a given time.
     // OnCheckForComplete has the potential for starting a new attempt on a
     // different worker thread if none of our outstanding attempts have
     // completed yet.
-    if (attempt_number_ <= resolver_->max_retry_attempts()) {
+    if (attempt_number_ <= params_.max_retry_attempts) {
       origin_loop_->PostDelayedTask(
           FROM_HERE,
-          base::Bind(&Job::OnCheckForComplete, this),
-          unresponsive_delay_.InMilliseconds());
+          base::Bind(&ProcTask::OnCheckForComplete, this),
+          params_.unresponsive_delay.InMilliseconds());
     }
   }
 
-  // Cancels the current job. The Job will be orphaned. Any outstanding resolve
-  // attempts running on worker threads will continue running. Only once all the
-  // attempts complete will the final reference to this Job be released.
-  void Cancel() {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    net_log_.AddEvent(NetLog::TYPE_CANCELLED, NULL);
-
-    HostResolver* resolver = resolver_;
-    resolver_ = NULL;
-
-    // End here to prevent issues when a Job outlives the HostResolver that
-    // spawned it.
-    net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB, NULL);
-
-    // We will call HostResolverImpl::CancelRequest(Request*) on each one
-    // in order to notify any observers.
-    for (RequestsList::const_iterator it = requests_.begin();
-         it != requests_.end(); ++it) {
-      HostResolverImpl::Request* req = *it;
-      if (!req->was_cancelled())
-        resolver->CancelRequest(req);
-    }
-  }
-
-  bool was_cancelled() const {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    return resolver_ == NULL;
-  }
-
-  bool was_completed() const {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    return completed_attempt_number_ > 0;
-  }
-
-  const Key& key() const {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    return key_;
-  }
-
-  int id() const {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    return id_;
-  }
-
-  const RequestsList& requests() const {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    return requests_;
-  }
-
-  // Returns the first request attached to the job.
-  const Request* initial_request() const {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    DCHECK(!requests_.empty());
-    return requests_[0];
-  }
-
-  // Returns true if |req_info| can be fulfilled by this job.
-  bool CanServiceRequest(const RequestInfo& req_info) const {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    return key_ == resolver_->GetEffectiveKeyForRequest(req_info);
-  }
-
- private:
-  friend class base::RefCountedThreadSafe<HostResolverImpl::Job>;
-
-  ~Job() {
-    // Free the requests attached to this job.
-    STLDeleteElements(&requests_);
-  }
-
   // WARNING: This code runs inside a worker pool. The shutdown code cannot
   // wait for it to finish, so we must be very careful here about using other
   // objects (like MessageLoops, Singletons, etc). During shutdown these objects
   // may no longer exist. Multiple DoLookups() could be running in parallel, so
   // any state inside of |this| must not mutate .
   void DoLookup(const base::TimeTicks& start_time,
                 const uint32 attempt_number) {
     AddressList results;
     int os_error = 0;
     // Running on the worker thread
-    int error = ResolveAddrInfo(resolver_proc_,
+    int error = ResolveAddrInfo(params_.resolver_proc,
                                 key_.hostname,
                                 key_.address_family,
                                 key_.host_resolver_flags,
                                 &results,
                                 &os_error);
 
     origin_loop_->PostTask(
         FROM_HERE,
-        base::Bind(&Job::OnLookupComplete, this, results, start_time,
+        base::Bind(&ProcTask::OnLookupComplete, this, results, start_time,
                    attempt_number, error, os_error));
   }
 
   // Callback to see if DoLookup() has finished or not (runs on origin thread).
   void OnCheckForComplete() {
     DCHECK(origin_loop_->BelongsToCurrentThread());
 
-    if (was_completed() || was_cancelled())
+    if (was_completed() || was_canceled())
       return;
 
-    DCHECK(resolver_);
-    unresponsive_delay_ *= resolver_->retry_factor();
+    params_.unresponsive_delay *= params_.retry_factor;
     StartLookupAttempt();
   }
 
   // Callback for when DoLookup() completes (runs on origin thread).
   void OnLookupComplete(const AddressList& results,
                         const base::TimeTicks& start_time,
                         const uint32 attempt_number,
                         int error,
                         const int os_error) {
     DCHECK(origin_loop_->BelongsToCurrentThread());
     DCHECK(error || results.head());
 
     bool was_retry_attempt = attempt_number > 1;
 
-    if (!was_cancelled()) {
+    if (!was_canceled()) {
       scoped_refptr<NetLog::EventParameters> params;
       if (error != OK) {
         params = new HostResolveFailedParams(attempt_number, error, os_error);
       } else {
         params = new NetLogIntegerParameter("attempt_number", attempt_number_);
       }
       net_log_.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_ATTEMPT_FINISHED,
                         params);
 
       // If host is already resolved, then record data and return.
@@ skipping 20 matching lines @@
     if (error != OK && NetworkChangeNotifier::IsOffline())
       error = ERR_INTERNET_DISCONNECTED;
 
     // We will record data for the first attempt. Don't contaminate with retry
     // attempt's data.
     if (!was_retry_attempt)
       RecordPerformanceHistograms(start_time, error, os_error);
 
     RecordAttemptHistograms(start_time, attempt_number, error, os_error);
 
-    if (was_cancelled())
+    if (was_canceled())
       return;
 
     if (was_retry_attempt) {
       // If retry attempt finishes before 1st attempt, then get stats on how
       // much time is saved by having spawned an extra attempt.
       retry_attempt_finished_time_ = base::TimeTicks::Now();
     }
 
     scoped_refptr<NetLog::EventParameters> params;
     if (error != OK) {
       params = new HostResolveFailedParams(0, error, os_error);
     } else {
       params = new AddressListNetLogParam(results_);
     }
 
-    // End here to prevent issues when a Job outlives the HostResolver that
-    // spawned it.
-    net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB, params);
+    net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_TASK, params);
 
-    DCHECK(!requests_.empty());
-
-     // Use the port number of the first request.
-    if (error == OK)
-      MutableSetPort(requests_[0]->port(), &results_);
-
-    resolver_->OnJobComplete(this, error, os_error, results_);
+    Callback callback = callback_;
+    callback_.Reset();
+    callback.Run(error, os_error, results_);
   }
 
   void RecordPerformanceHistograms(const base::TimeTicks& start_time,
                                    const int error,
                                    const int os_error) const {
     DCHECK(origin_loop_->BelongsToCurrentThread());
     enum Category {  // Used in HISTOGRAM_ENUMERATION.
       RESOLVE_SUCCESS,
       RESOLVE_FAIL,
       RESOLVE_SPECULATIVE_SUCCESS,
@@ skipping 75 matching lines @@
         DNS_HISTOGRAM(base::FieldTrial::MakeName("DNS.ResolveSuccess",
                                                  "DnsParallelism"), duration);
       }
     }
   }
 
   void RecordAttemptHistograms(const base::TimeTicks& start_time,
                                const uint32 attempt_number,
                                const int error,
                                const int os_error) const {
+    DCHECK(origin_loop_->BelongsToCurrentThread());
     bool first_attempt_to_complete =
         completed_attempt_number_ == attempt_number;
     bool is_first_attempt = (attempt_number == 1);
 
     if (first_attempt_to_complete) {
       // If this was first attempt to complete, then record the resolution
       // status of the attempt.
       if (completed_attempt_error_ == OK) {
         UMA_HISTOGRAM_ENUMERATION(
             "DNS.AttemptFirstSuccess", attempt_number, 100);
       } else {
         UMA_HISTOGRAM_ENUMERATION(
             "DNS.AttemptFirstFailure", attempt_number, 100);
       }
     }
 
     if (error == OK)
       UMA_HISTOGRAM_ENUMERATION("DNS.AttemptSuccess", attempt_number, 100);
     else
       UMA_HISTOGRAM_ENUMERATION("DNS.AttemptFailure", attempt_number, 100);
 
     // If first attempt didn't finish before retry attempt, then calculate stats
     // on how much time is saved by having spawned an extra attempt.
-    if (!first_attempt_to_complete && is_first_attempt && !was_cancelled()) {
+    if (!first_attempt_to_complete && is_first_attempt && !was_canceled()) {
       DNS_HISTOGRAM("DNS.AttemptTimeSavedByRetry",
                     base::TimeTicks::Now() - retry_attempt_finished_time_);
     }
 
-    if (was_cancelled() || !first_attempt_to_complete) {
+    if (was_canceled() || !first_attempt_to_complete) {
       // Count those attempts which completed after the job was already canceled
       // OR after the job was already completed by an earlier attempt (so in
       // effect).
       UMA_HISTOGRAM_ENUMERATION("DNS.AttemptDiscarded", attempt_number, 100);
 
-      // Record if job is cancelled.
-      if (was_cancelled())
+      // Record if job is canceled.
+      if (was_canceled())
         UMA_HISTOGRAM_ENUMERATION("DNS.AttemptCancelled", attempt_number, 100);
     }
 
     base::TimeDelta duration = base::TimeTicks::Now() - start_time;
     if (error == OK)
       DNS_HISTOGRAM("DNS.AttemptSuccessDuration", duration);
     else
       DNS_HISTOGRAM("DNS.AttemptFailDuration", duration);
   }
 
-  // Immutable. Can be read from either thread,
-  const int id_;
-
   // Set on the origin thread, read on the worker thread.
   Key key_;
 
-  // Only used on the origin thread (where Resolve was called).
-  HostResolverImpl* resolver_;
-  RequestsList requests_;  // The requests waiting on this job.
+  // Holds an owning reference to the HostResolverProc that we are going to use.
+  // This may not be the current resolver procedure by the time we call
+  // ResolveAddrInfo, but that's OK... we'll use it anyways, and the owning
+  // reference ensures that it remains valid until we are done.
+  ProcTaskParams params_;
+
+  // The listener to the results of this ProcTask.
+  Callback callback_;
 
   // Used to post ourselves onto the origin thread.
   scoped_refptr<base::MessageLoopProxy> origin_loop_;
 
-  // Hold an owning reference to the HostResolverProc that we are going to use.
-  // This may not be the current resolver procedure by the time we call
-  // ResolveAddrInfo, but that's OK... we'll use it anyways, and the owning
-  // reference ensures that it remains valid until we are done.
-  scoped_refptr<HostResolverProc> resolver_proc_;
-
-  // The amount of time after starting a resolution attempt until deciding to
-  // retry.
-  base::TimeDelta unresponsive_delay_;
-
   // Keeps track of the number of attempts we have made so far to resolve the
   // host. Whenever we start an attempt to resolve the host, we increase this
   // number.
   uint32 attempt_number_;
 
   // The index of the attempt which finished first (or 0 if the job is still in
   // progress).
   uint32 completed_attempt_number_;
 
   // The result (a net error code) from the first attempt to complete.
   int completed_attempt_error_;
 
   // The time when retry attempt was finished.
   base::TimeTicks retry_attempt_finished_time_;
 
   // True if a non-speculative request was ever attached to this job
-  // (regardless of whether or not it was later cancelled.
+  // (regardless of whether or not it was later canceled.
   // This boolean is used for histogramming the duration of jobs used to
   // service non-speculative requests.
   bool had_non_speculative_request_;
 
   AddressList results_;
 
   BoundNetLog net_log_;
 
-  DISALLOW_COPY_AND_ASSIGN(Job);
+  DISALLOW_COPY_AND_ASSIGN(ProcTask);
 };
 
 //-----------------------------------------------------------------------------
 
-// This class represents a request to the worker pool for a "probe for IPv6
-// support" call.
+// Represents a request to the worker pool for a "probe for IPv6 support" call.
 class HostResolverImpl::IPv6ProbeJob
     : public base::RefCountedThreadSafe<HostResolverImpl::IPv6ProbeJob> {
  public:
   explicit IPv6ProbeJob(HostResolverImpl* resolver)
       : resolver_(resolver),
         origin_loop_(base::MessageLoopProxy::current()) {
     DCHECK(resolver);
   }
 
   void Start() {
     DCHECK(origin_loop_->BelongsToCurrentThread());
-    if (was_cancelled())
+    if (was_canceled())
       return;
     const bool kIsSlow = true;
     base::WorkerPool::PostTask(
         FROM_HERE, base::Bind(&IPv6ProbeJob::DoProbe, this), kIsSlow);
   }
 
   // Cancels the current job.
   void Cancel() {
     DCHECK(origin_loop_->BelongsToCurrentThread());
-    if (was_cancelled())
+    if (was_canceled())
       return;
     resolver_ = NULL;  // Read/write ONLY on origin thread.
   }
 
  private:
   friend class base::RefCountedThreadSafe<HostResolverImpl::IPv6ProbeJob>;
 
   ~IPv6ProbeJob() {
   }
 
-  bool was_cancelled() const {
+  bool was_canceled() const {
     DCHECK(origin_loop_->BelongsToCurrentThread());
     return !resolver_;
   }
 
   // Run on worker thread.
   void DoProbe() {
     // Do actual testing on this thread, as it takes 40-100ms.
     AddressFamily family = IPv6Supported() ? ADDRESS_FAMILY_UNSPECIFIED
                                            : ADDRESS_FAMILY_IPV4;
 
     origin_loop_->PostTask(
         FROM_HERE,
         base::Bind(&IPv6ProbeJob::OnProbeComplete, this, family));
   }
 
   // Callback for when DoProbe() completes.
   void OnProbeComplete(AddressFamily address_family) {
     DCHECK(origin_loop_->BelongsToCurrentThread());
-    if (was_cancelled())
+    if (was_canceled())
       return;
     resolver_->IPv6ProbeSetDefaultAddressFamily(address_family);
   }
 
   // Used/set only on origin thread.
   HostResolverImpl* resolver_;
 
   // Used to post ourselves onto the origin thread.
   scoped_refptr<base::MessageLoopProxy> origin_loop_;
 
   DISALLOW_COPY_AND_ASSIGN(IPv6ProbeJob);
 };
 
 //-----------------------------------------------------------------------------
 
-// We rely on the priority enum values being sequential having starting at 0,
-// and increasing for lower priorities.
-COMPILE_ASSERT(HIGHEST == 0u &&
-               LOWEST > HIGHEST &&
-               IDLE > LOWEST &&
-               NUM_PRIORITIES > IDLE,
-               priority_indexes_incompatible);
-
-// JobPool contains all the information relating to queued requests, including
-// the limits on how many jobs are allowed to be used for this category of
-// requests.
-class HostResolverImpl::JobPool {
+// Keeps track of the highest priority.
+class PriorityTracker {

[cbentzel, 2012/01/09 16:05:18]

I'm not complete opposed to this - but it seems like a linear scan across the
request priorities would be fine and simpler, since I don't expect that count to
get too large.

[mmenke, 2012/01/09 16:57:24]

nit:  Explicit only needed for single argument constructors.

[mmenke, 2012/01/09 16:57:24]

I also wonder if this class is really necessary.  We only allow 6 HTTP requests
(And thus connect jobs) per domain.  With DNS prefetch, we may end up with more
than 6, so shouldn't really matter.  Suppose this class is small enough that
getting rid of it wouldn't really simplify things much.

  public:
-  JobPool(size_t max_outstanding_jobs, size_t max_pending_requests)
-      : num_outstanding_jobs_(0u) {
-    SetConstraints(max_outstanding_jobs, max_pending_requests);
-  }
-
-  ~JobPool() {
-    // Free the pending requests.
-    for (size_t i = 0; i < arraysize(pending_requests_); ++i)
-      STLDeleteElements(&pending_requests_[i]);
-  }
-
-  // Sets the constraints for this pool. See SetPoolConstraints() for the
-  // specific meaning of these parameters.
-  void SetConstraints(size_t max_outstanding_jobs,
-                      size_t max_pending_requests) {
-    CHECK_NE(max_outstanding_jobs, 0u);
-    max_outstanding_jobs_ = max_outstanding_jobs;
-    max_pending_requests_ = max_pending_requests;
-  }
-
-  // Returns the number of pending requests enqueued to this pool.
-  // A pending request is one waiting to be attached to a job.
-  size_t GetNumPendingRequests() const {
-    size_t total = 0u;
-    for (size_t i = 0u; i < arraysize(pending_requests_); ++i)
-      total += pending_requests_[i].size();
-    return total;
-  }
-
-  bool HasPendingRequests() const {
-    return GetNumPendingRequests() > 0u;
-  }
-
-  // Enqueues a request to this pool. As a result of enqueing this request,
-  // the queue may have reached its maximum size. In this case, a request is
-  // evicted from the queue, and returned. Otherwise returns NULL. The caller
-  // is responsible for freeing the evicted request.
-  Request* InsertPendingRequest(Request* req) {
-    req->request_net_log().BeginEvent(
-        NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_POOL_QUEUE,
-        NULL);
-
-    PendingRequestsQueue& q = pending_requests_[req->info().priority()];
-    q.push_back(req);
-
-    // If the queue is too big, kick out the lowest priority oldest request.
-    if (GetNumPendingRequests() > max_pending_requests_) {
-      // Iterate over the queues from lowest priority to highest priority.
-      for (int i = static_cast<int>(arraysize(pending_requests_)) - 1;
-           i >= 0; --i) {
-        PendingRequestsQueue& q = pending_requests_[i];
-        if (!q.empty()) {
-          Request* req = q.front();
-          q.pop_front();
-          req->request_net_log().AddEvent(
-              NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_POOL_QUEUE_EVICTED, NULL);
-          req->request_net_log().EndEvent(
-              NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_POOL_QUEUE, NULL);
-          return req;
+  explicit PriorityTracker()
+      : highest_priority_(IDLE), total_count_(0) {
+    memset(counts_, 0, sizeof(counts_));
+  }
+
+  RequestPriority highest_priority() const {
+    return highest_priority_;
+  }
+
+  size_t total_count() const {
+    return total_count_;
+  }
+
+  // Returns true if priority changed.
+  bool Add(RequestPriority req_priority) {
+    ++total_count_;
+    ++counts_[req_priority];
+    if (highest_priority_ < req_priority)
+      return false;
+
+    highest_priority_ = req_priority;
+    return true;
+  }
+
+  // Returns true if priority changed.
+  bool Remove(RequestPriority req_priority) {
+    DCHECK_GT(total_count_, 0u);
+    DCHECK_GT(counts_[req_priority], 0u);
+    --total_count_;
+    --counts_[req_priority];
+    if ((counts_[req_priority] == 0u) && (highest_priority_ == req_priority)) {
+      for (size_t i = highest_priority_; i < NUM_PRIORITIES; ++i) {
+        if (counts_[i] > 0) {
+          highest_priority_ = static_cast<RequestPriority>(i);
+          return true;
         }
       }
-    }
-
-    return NULL;
-  }
-
-  // Erases |req| from this container. Caller is responsible for freeing
-  // |req| afterwards.
-  void RemovePendingRequest(Request* req) {
-    PendingRequestsQueue& q = pending_requests_[req->info().priority()];
-    PendingRequestsQueue::iterator it = std::find(q.begin(), q.end(), req);
-    DCHECK(it != q.end());
-    q.erase(it);
-    req->request_net_log().EndEvent(
-        NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_POOL_QUEUE, NULL);
-  }
-
-  // Removes and returns the highest priority pending request.
-  Request* RemoveTopPendingRequest() {
-    DCHECK(HasPendingRequests());
-
-    for (size_t i = 0u; i < arraysize(pending_requests_); ++i) {
-      PendingRequestsQueue& q = pending_requests_[i];
-      if (!q.empty()) {
-        Request* req = q.front();
-        q.pop_front();
-        req->request_net_log().EndEvent(
-            NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_POOL_QUEUE, NULL);
-        return req;
+      DCHECK_EQ(0u, total_count_);
+      // In absence of requests set default.
+      if (highest_priority_ != IDLE) {
+        highest_priority_ = IDLE;
+        return true;
       }
     }
-
-    NOTREACHED();
-    return NULL;
-  }
-
-  // Keeps track of a job that was just added/removed, and belongs to this pool.
-  void AdjustNumOutstandingJobs(int offset) {
-    DCHECK(offset == 1 || (offset == -1 && num_outstanding_jobs_ > 0u));
-    num_outstanding_jobs_ += offset;
-  }
-
-  void ResetNumOutstandingJobs() {
-    num_outstanding_jobs_ = 0;
-  }
-
-  // Returns true if a new job can be created for this pool.
-  bool CanCreateJob() const {
-    return num_outstanding_jobs_ + 1u <= max_outstanding_jobs_;
-  }
-
-  // Removes any pending requests from the queue which are for the
-  // same (hostname / effective address-family) as |job|, and attaches them to
-  // |job|.
-  void MoveRequestsToJob(Job* job) {
-    for (size_t i = 0u; i < arraysize(pending_requests_); ++i) {
-      PendingRequestsQueue& q = pending_requests_[i];
-      PendingRequestsQueue::iterator req_it = q.begin();
-      while (req_it != q.end()) {
-        Request* req = *req_it;
-        if (job->CanServiceRequest(req->info())) {
-          // Job takes ownership of |req|.
-          job->AddRequest(req);
-          req_it = q.erase(req_it);
-        } else {
-          ++req_it;
-        }
+    return false;
+  }
+
+ private:
+  RequestPriority highest_priority_;
+  size_t total_count_;
+  size_t counts_[NUM_PRIORITIES];
+};
+
+//-----------------------------------------------------------------------------
+
+// Aggregates all Requests for the same Key. Dispatched via PriorityDispatch.
+// Spawns ProcTask when started.
+class HostResolverImpl::Job : public PrioritizedDispatcher::Job {
+ public:
+  // Creates new job for |key| where |request_net_log| is bound to the
+  // request that spawned it.
+  Job(HostResolverImpl* resolver,
+      const Key& key,
+      const BoundNetLog& request_net_log)
+      : resolver_(resolver->AsWeakPtr()),
+        key_(key),
+        had_non_speculative_request_(false),
+        net_log_(BoundNetLog::Make(request_net_log.net_log(),
+                                   NetLog::SOURCE_HOST_RESOLVER_IMPL_JOB)),
+        net_error_(ERR_IO_PENDING),
+        os_error_(0) {
+    request_net_log.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_CREATE_JOB, NULL);
+
+    net_log_.BeginEvent(
+        NetLog::TYPE_HOST_RESOLVER_IMPL_JOB,
+        make_scoped_refptr(new JobCreationParameters(
+            key_.hostname, request_net_log.source())));
+  }
+
+  virtual ~Job() {
+    if (net_error_ == ERR_IO_PENDING) {
+      if (is_running()) {
+        proc_task_->Cancel();
+        proc_task_ = NULL;
       }
-    }
+      net_log_.AddEvent(NetLog::TYPE_CANCELLED, NULL);
+      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());
+      }
+      net_error_ = ERR_ABORTED;
+    }
+    net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB,
+                                      net_error_);
+    STLDeleteElements(&requests_);
+  }
+
+  HostResolverImpl* resolver() const {
+    return resolver_;
+  }
+
+  RequestPriority priority() const {
+    return priority_tracker_.highest_priority();
+  }
+
+  // Number of non-canceled requests in |requests_|.
+  size_t num_active_requests() const {
+    return priority_tracker_.total_count();
+  }
+
+  const Key& key() const {
+    return key_;
+  }
+
+  int net_error() const {
+    return net_error_;
+  }
+
+  // Used by HostResolverImpl in |dispatch_|.
+  PrioritizedDispatcher::Handle& handle() {

[cbentzel, 2012/01/09 16:05:18]

non-const ref returns like these are uncommon (and perhaps disallowed). I'd much
rather see a set_handle()

[szym, 2012/01/09 17:06:59]

Currently HostResolverImpl is the only user of |handle_|. I'm considering moving
the logic that uses the handle into the Job.

+    return handle_;
+  }
+
+  // The Job will own |req| and destroy it in ~Job.
+  void AddRequest(Request* req) {
+    DCHECK_EQ(key_.hostname, req->info().hostname());
+
+    req->set_job(this);
+    requests_.push_back(req);
+
+    priority_tracker_.Add(req->info().priority());
+
+    req->request_net_log().AddEvent(
+        NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_ATTACH,
+        make_scoped_refptr(new NetLogSourceParameter(
+            "source_dependency", net_log_.source())));
+
+    net_log_.AddEvent(
+        NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_ATTACH,
+        make_scoped_refptr(new JobAttachParameters(
+            req->request_net_log().source(), priority())));
+
+    // TODO(szym): Check if this is still needed.
+    if (!req->info().is_speculative()) {
+      had_non_speculative_request_ = true;
+      if (proc_task_)
+        proc_task_->set_had_non_speculative_request();
+    }
+  }
+
+  void CancelRequest(Request* req) {
+    DCHECK_EQ(key_.hostname, req->info().hostname());
+    DCHECK(!req->was_canceled());
+    // Don't remove it from |requests_| just mark it canceled.
+    req->MarkAsCanceled();

[cbentzel, 2012/01/09 16:05:18]

What's the rationale for marking as canceled instead of removing the Request
from the list and deleting it? A good amount of the PriorityTracker logic would
get simpler in that case. 

[szym, 2012/01/09 17:06:59]

A Request could be Canceled during the for loop in CompleteRequests, so removing
it from the list here would complicate that loop. This is how
HostResolverImpl::Job behaved before the refactoring.

I don't think changing this behavior would affect the current PriorityTracker.
OTOH if PriorityTracker was replaced with a list scan, then actually removing it
would only save one "if (!req->was_cancelled()) continue;".

[cbentzel, 2012/01/09 20:02:57]

Thanks for the explanation - I agree that it makes the looping operation
simpler. 

+    LogCancelRequest(req->source_net_log(), req->request_net_log(),
+                     req->info());
+    if (priority_tracker_.Remove(req->info().priority())) {
+      net_log_.AddEvent(
+          NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_DETACH,
+          make_scoped_refptr(new JobAttachParameters(
+              req->request_net_log().source(), priority())));
+    }
+  }
+
+  // Called by ProcTask when it completes.
+  void OnProcTaskComplete(int net_error, int os_error,

[cbentzel, 2012/01/09 16:05:18]

Could this be private?

+                         const AddressList& addrlist) {

[cbentzel, 2012/01/09 16:05:18]

Nit: line up columns.

+    DCHECK(is_running());
+    proc_task_ = NULL;
+    net_error_ = net_error;
+    os_error_ = os_error;
+
+    // We are the only consumer of |addrlist|, so we can safely change the port
+    // without copy-on-write. This pays off, when job has only one request.
+    AddressList list = addrlist;
+    if (net_error == OK && !requests_.empty())
+      MutableSetPort(requests_.front()->info().port(), &list);
+    CompleteRequests(list);
+  }
+
+  // Aborts and destroys the job.

[mmenke, 2012/01/09 16:57:24]

nit: Think it's worth mentioning that it completes active requests.

+  void Abort() {
+    // Job should only be aborted if it's running.
+    DCHECK(is_running());
+    proc_task_->Cancel();
+    proc_task_ = NULL;
+    net_error_ = ERR_ABORTED;
+    os_error_ = 0;
+    CompleteRequests(AddressList());
+  }
+
+  bool is_running() const {
+    return proc_task_.get() != NULL;
+  }
+
+  // Called by HostResolverImpl when this job is evicted due to queue overflow.
+  void OnEvicted() {
+    // Must not be running.
+    DCHECK(!is_running());
+    handle_ = PrioritizedDispatcher::Handle();
+
+    net_log_.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_EVICTED, NULL);
+
+    // This signals to CompleteRequests that this job never ran.
+    net_error_ = ERR_HOST_RESOLVER_QUEUE_TOO_LARGE;
+    os_error_ = 0;
+    CompleteRequests(AddressList());
+  }
+
+  // PriorityDispatch::Job interface.
+  virtual void Start() OVERRIDE {
+    DCHECK(!is_running());
+    handle_ = PrioritizedDispatcher::Handle();
+
+    net_log_.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_STARTED, NULL);
+
+    proc_task_ = new ProcTask(
+        key_,
+        resolver_->proc_params_,
+        base::Bind(&Job::OnProcTaskComplete, base::Unretained(this)),

[cbentzel, 2012/01/09 16:05:18]

I wonder if the callback creation and bound method should be on the
HostResolverImpl. It would minimize the Job->Resolver calls, and tend to keep
them Resolver->Job.

[szym, 2012/01/09 17:06:59]

True, but then the innards of Job (the fact that Job runs a ProcTask and maybe
later a DnsTask) are exposed to HostResolverImpl. The current number of
Job->Resolver calls is 2, but could be made 1 by moving calling RemoveJob from
within OnJobFinished. I'm not counting resolver_->proc_params_. If that should
be removed, I'd make it an argument to the constructor.

+        net_log_);
+
+    if (had_non_speculative_request_)
+      proc_task_->set_had_non_speculative_request();
+    // Start() could be called from within Resolve(), hence it must NOT directly
+    // call OnProcTaskComplete, for example, on synchronous failure.
+    proc_task_->Start();
   }
 
  private:
-  typedef std::deque<Request*> PendingRequestsQueue;
-
-  // Maximum number of concurrent jobs allowed to be started for requests
-  // belonging to this pool.
-  size_t max_outstanding_jobs_;
-
-  // The current number of running jobs that were started for requests
-  // belonging to this pool.
-  size_t num_outstanding_jobs_;
-
-  // The maximum number of requests we allow to be waiting on a job,
-  // for this pool.
-  size_t max_pending_requests_;
-
-  // The requests which are waiting to be started for this pool.
-  PendingRequestsQueue pending_requests_[NUM_PRIORITIES];
+  // Completes all Requests. Calls OnJobFinished and deletes self.
+  void CompleteRequests(const AddressList& addrlist) {
+    CHECK(resolver_);
+
+    // 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

[cbentzel, 2012/01/09 16:05:18]

I wonder if we could do this after iterating through the requests? I'd like to
see fewer paths for Job completion than currently exist, and will try to think
some more about how to make that happen.

[szym, 2012/01/09 17:06:59]

I'm not sure we can reduce the number of CompleteRequests calls without changing
the HostResolver API. We need to call Request::OnComplete when:
1) the Request finishes;
2) the Job is Aborted (due to a network change);
3) the Job is Evicted (due to queue overflow).
All of them can be asynchronous and all of them could lead to new Jobs being
dispatched, see HostResolverImplTest.StartWithinCallback.

We could avoid the complexity in CompleteRequests, if we called
MessageLoop::Post(Bind(&Request::OnComplete, Owned(req))). This way all the
re-entrancy issues would go away. What would the price of that be? Performance?

+    // to spawn one. Consequently, the job deletes itself when CompleteRequests
+    // is done.
+    scoped_ptr<Job> self_deleter(this);
+    resolver_->RemoveJob(this);
+    if (net_error_ != ERR_HOST_RESOLVER_QUEUE_TOO_LARGE)
+      resolver_->OnJobFinished(this, addrlist);
+
+    // Complete all of the requests that were attached to the job.
+    for (RequestsList::const_iterator it = requests_.begin();
+        it != requests_.end(); ++it) {

[mmenke, 2012/01/09 16:57:24]

nit:  Fix indent.

+      Request* req = *it;
+
+      if (req->was_canceled())
+        continue;
+
+      DCHECK_EQ(this, req->job());
+      // Update the net log and notify registered observers.
+      LogFinishRequest(req->source_net_log(), req->request_net_log(),
+                       req->info(), net_error_, os_error_);
+
+      req->OnComplete(net_error_, addrlist);
+
+      // Check if the resolver was destroyed as a result of running the
+      // callback. If it was, we could continue, but we choose to bail.

[mmenke, 2012/01/09 16:57:24]

Why do we need to allow callbacks to be able to delete the HostResolverImpl,
anyways?

[szym, 2012/01/09 17:06:59]

Excellent question. See: HostResolverImplTest.DeleteWithinCallback. I haven't
yet figured out from just scanning the code if this is really needed. I think
I'll just break the behavior and see if this could still be an issue.

+      if (!resolver_)
+        return;
+    }
+  }
+
+  // Used to call OnJobFinished and RemoveJob.
+  base::WeakPtr<HostResolverImpl> resolver_;
+
+  Key key_;
+
+  // Tracks the highest priority across |requests_|.
+  PriorityTracker priority_tracker_;
+
+  bool had_non_speculative_request_;
+
+  BoundNetLog net_log_;
+
+  // Store result here in case the job fails fast in Resolve().
+  int net_error_;
+  int os_error_;
+
+  // A ProcTask created and started when this Job is dispatched..
+  scoped_refptr<ProcTask> proc_task_;
+
+  // All Requests waiting for the result of this Job. Some can be canceled.
+  RequestsList requests_;
+
+  // A handle used by HostResolverImpl in |dispatcher_|.
+  PrioritizedDispatcher::Handle handle_;
 };
 
 //-----------------------------------------------------------------------------
 
+HostResolverImpl::ProcTaskParams::ProcTaskParams(
+    HostResolverProc* resolver_proc,
+    size_t max_retry_attempts)
+    : resolver_proc(resolver_proc),
+      max_retry_attempts(max_retry_attempts),
+      unresponsive_delay(base::TimeDelta::FromMilliseconds(6000)),
+      retry_factor(2) {
+}
+
+HostResolverImpl::ProcTaskParams::~ProcTaskParams() {}
+
 HostResolverImpl::HostResolverImpl(
-    HostResolverProc* resolver_proc,
     HostCache* cache,
-    size_t max_jobs,
-    size_t max_retry_attempts,
+    const PrioritizedDispatcher::Limits& job_limits,
+    const ProcTaskParams& proc_params,
     NetLog* net_log)
     : cache_(cache),
-      max_jobs_(max_jobs),
-      max_retry_attempts_(max_retry_attempts),
-      unresponsive_delay_(base::TimeDelta::FromMilliseconds(6000)),
-      retry_factor_(2),
-      next_job_id_(0),
-      resolver_proc_(resolver_proc),
+      dispatcher_(job_limits),
+      max_queued_jobs_(job_limits.total_jobs * 100u),
+      proc_params_(proc_params),
       default_address_family_(ADDRESS_FAMILY_UNSPECIFIED),
       ipv6_probe_monitoring_(false),
       additional_resolver_flags_(0),
       net_log_(net_log) {
-  DCHECK_GT(max_jobs, 0u);
+
+  DCHECK_GE(dispatcher_.num_priorities(), static_cast<size_t>(NUM_PRIORITIES));
 
   // Maximum of 4 retry attempts for host resolution.
   static const size_t kDefaultMaxRetryAttempts = 4u;
 
-  if (max_retry_attempts_ == HostResolver::kDefaultRetryAttempts)
-    max_retry_attempts_ = kDefaultMaxRetryAttempts;
-
-  // It is cumbersome to expose all of the constraints in the constructor,
-  // so we choose some defaults, which users can override later.
-  job_pools_[POOL_NORMAL] = new JobPool(max_jobs, 100u * max_jobs);
+  if (proc_params_.max_retry_attempts == HostResolver::kDefaultRetryAttempts)
+    proc_params_.max_retry_attempts = kDefaultMaxRetryAttempts;
 
 #if defined(OS_WIN)
   EnsureWinsockInit();
 #endif
 #if defined(OS_POSIX) && !defined(OS_MACOSX)
   if (HaveOnlyLoopbackAddresses())
     additional_resolver_flags_ |= HOST_RESOLVER_LOOPBACK_ONLY;
 #endif
   NetworkChangeNotifier::AddIPAddressObserver(this);
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_OPENBSD)
 #if !defined(OS_ANDROID)
   EnsureDnsReloaderInit();
 #endif
   NetworkChangeNotifier::AddDNSObserver(this);
 #endif
 }
 
 HostResolverImpl::~HostResolverImpl() {
   // Cancel the outstanding jobs. Those jobs may contain several attached
-  // requests, which will also be cancelled.
+  // requests, which will also be canceled.

[mmenke, 2012/01/09 16:57:24]

No longer accurate.  We now delete the jobs, which automatically cancel
themselves.

   DiscardIPv6ProbeJob();
 
-  CancelAllJobs();
-
-  // In case we are being deleted during the processing of a callback.
-  if (cur_completing_job_)
-    cur_completing_job_->Cancel();
+  STLDeleteValues(&jobs_);
 
   NetworkChangeNotifier::RemoveIPAddressObserver(this);
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_OPENBSD)
   NetworkChangeNotifier::RemoveDNSObserver(this);
 #endif
-
-  // Delete the job pools.
-  for (size_t i = 0u; i < arraysize(job_pools_); ++i)
-    delete job_pools_[i];
 }
 
-void HostResolverImpl::SetPoolConstraints(JobPoolIndex pool_index,
-                                          size_t max_outstanding_jobs,
-                                          size_t max_pending_requests) {
-  DCHECK(CalledOnValidThread());
-  CHECK_GE(pool_index, 0);
-  CHECK_LT(pool_index, POOL_COUNT);
-  CHECK(jobs_.empty()) << "Can only set constraints during setup";
-  JobPool* pool = job_pools_[pool_index];
-  pool->SetConstraints(max_outstanding_jobs, max_pending_requests);
+void HostResolverImpl::SetMaxQueuedJobs(size_t value) {
+  DCHECK_EQ(0u, dispatcher_.num_queued_jobs());
+  DCHECK_GT(value, 0u);
+  max_queued_jobs_ = value;
 }
 
 int HostResolverImpl::Resolve(const RequestInfo& info,
                               AddressList* addresses,
                               const CompletionCallback& callback,
                               RequestHandle* out_req,
                               const BoundNetLog& source_net_log) {
   DCHECK(addresses);
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(false, callback.is_null());
 
   // Make a log item for the request.
   BoundNetLog request_net_log = BoundNetLog::Make(net_log_,
       NetLog::SOURCE_HOST_RESOLVER_IMPL_REQUEST);
 
-  // Update the net log and notify registered observers.
-  OnStartRequest(source_net_log, request_net_log, info);
+  LogStartRequest(source_net_log, request_net_log, info);
 
   // Build a key that identifies the request in the cache and in the
   // outstanding jobs map.
   Key key = GetEffectiveKeyForRequest(info);
 
   int rv = ResolveHelper(key, info, addresses, request_net_log);
   if (rv != ERR_DNS_CACHE_MISS) {
-    OnFinishRequest(source_net_log, request_net_log, info,
-                    rv,
-                    0  /* os_error (unknown since from cache) */);
+    LogFinishRequest(source_net_log, request_net_log, info, rv,
+                     0  /* os_error (unknown since from cache) */);
     return rv;
   }
 
-  // Create a handle for this request, and pass it back to the user if they
-  // asked for it (out_req != NULL).
-  Request* req = new Request(source_net_log, request_net_log, info,
-                             callback, addresses);
+  // 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()) {
+    // Create new Job.
+    job = new Job(this, key, request_net_log);
+    job->handle() = dispatcher_.Add(job, info.priority());
+
+    // Check for queue overflow.
+    if (dispatcher_.num_queued_jobs() > max_queued_jobs_) {

[cbentzel, 2012/01/09 16:05:18]

Do you want to check if job->handle().is_null() here? Potentially want to evict
only if we added something to the queue. 

[szym, 2012/01/09 17:06:59]

We could check that but that would be redundant. The queue should never have >
max_queued_jobs_, and if it does that means we have added. I'm okay with a
DCHECK for sanity.

+      Job* evicted = static_cast<Job*>(dispatcher_.EvictOldestLowest());

[cbentzel, 2012/01/09 16:05:18]

Why do you need to do the static_cast here? If you kept as base class, would the
evicted == job compare against the base job class?

[szym, 2012/01/09 17:06:59]

I only need to cast to call evicted->OnEvicted() which is not a part of the
PrioritizedDispatcher::Job interface.

[cbentzel, 2012/01/09 20:02:57]

Yup, I didn't see that. Thanks.

+      DCHECK(evicted);
+      if (evicted == job) {
+        delete job;
+        rv = ERR_HOST_RESOLVER_QUEUE_TOO_LARGE;
+        LogFinishRequest(source_net_log, request_net_log, info, rv, 0);
+        return rv;
+      }
+      evicted->OnEvicted();  // Deletes |evicted|.
+    }
+
+    jobs_.insert(jobit, std::make_pair(key, job));
+  } else {
+    job = jobit->second;
+  }
+
+  // Can't complete synchronously. Create and attach request.
+  Request* req = new Request(source_net_log, request_net_log, info, callback,
+                             addresses);
+  job->AddRequest(req);
+  if (!job->handle().is_null())
+    job->handle() = dispatcher_.ChangePriority(job->handle(), job->priority());
   if (out_req)
     *out_req = reinterpret_cast<RequestHandle>(req);
 
-  // Next we need to attach our request to a "job". This job is responsible for
-  // calling "getaddrinfo(hostname)" on a worker thread.
-  scoped_refptr<Job> job;
-
-  // If there is already an outstanding job to resolve |key|, use
-  // it. This prevents starting concurrent resolves for the same hostname.
-  job = FindOutstandingJob(key);
-  if (job) {
-    job->AddRequest(req);
-  } else {
-    JobPool* pool = GetPoolForRequest(req);
-    if (CanCreateJobForPool(*pool)) {
-      CreateAndStartJob(req);
-    } else {
-      return EnqueueRequest(pool, req);
-    }
-  }
-
-  // Completion happens during OnJobComplete(Job*).
+  DCHECK_EQ(ERR_IO_PENDING, job->net_error());
+  // Completion happens during Job::CompleteRequests().
   return ERR_IO_PENDING;
 }
 
 int HostResolverImpl::ResolveHelper(const Key& key,
                                     const RequestInfo& info,
                                     AddressList* addresses,
                                     const BoundNetLog& request_net_log) {
   // The result of |getaddrinfo| for empty hosts is inconsistent across systems.
   // On Windows it gives the default interface's address, whereas on Linux it
   // gives an error. We will make it fail on all platforms for consistency.
@@ skipping 12 matching lines @@
                                        AddressList* addresses,
                                        const BoundNetLog& source_net_log) {
   DCHECK(CalledOnValidThread());
   DCHECK(addresses);
 
   // Make a log item for the request.
   BoundNetLog request_net_log = BoundNetLog::Make(net_log_,
       NetLog::SOURCE_HOST_RESOLVER_IMPL_REQUEST);
 
   // Update the net log and notify registered observers.
-  OnStartRequest(source_net_log, request_net_log, info);
+  LogStartRequest(source_net_log, request_net_log, info);
 
-  // Build a key that identifies the request in the cache and in the

[cbentzel, 2012/01/09 16:05:18]

Why did you get rid of this comment? You didn't in ::Resolve(

-  // outstanding jobs map.
   Key key = GetEffectiveKeyForRequest(info);
 
   int rv = ResolveHelper(key, info, addresses, request_net_log);
-  OnFinishRequest(source_net_log, request_net_log, info,
-                  rv,
-                  0  /* os_error (unknown since from cache) */);
+  LogFinishRequest(source_net_log, request_net_log, info, rv,
+                   0  /* os_error (unknown since from cache) */);
   return rv;
 }
 
-// See OnJobComplete(Job*) for why it is important not to clean out
-// cancelled requests from Job::requests_.
 void HostResolverImpl::CancelRequest(RequestHandle req_handle) {
   DCHECK(CalledOnValidThread());
   Request* req = reinterpret_cast<Request*>(req_handle);
   DCHECK(req);
 
-  scoped_ptr<Request> request_deleter;  // Frees at end of function.
+  Job* job = req->job();
+  DCHECK(job);
 
-  if (!req->job()) {
-    // If the request was not attached to a job yet, it must have been
-    // enqueued into a pool. Remove it from that pool's queue.
-    // Otherwise if it was attached to a job, the job is responsible for
-    // deleting it.
-    JobPool* pool = GetPoolForRequest(req);
-    pool->RemovePendingRequest(req);
-    request_deleter.reset(req);
+  job->CancelRequest(req);
+
+  if (!job->handle().is_null()) {
+    // Still in queue.
+    if (job->num_active_requests()) {
+      job->handle() = dispatcher_.ChangePriority(job->handle(),
+                                                 job->priority());
+    } else {
+      dispatcher_.Cancel(job->handle());
+      RemoveJob(job);
+      delete job;
+    }
   } else {
-    req->request_net_log().EndEvent(
-        NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_ATTACH, NULL);
+    // Job is running (and could be in CompleteRequests right now).
+    // But to be in Request::OnComplete we would have to have a non-canceled
+    // request. So it is safe to Abort it if it has no more active requests.
+    if (!job->num_active_requests()) {
+      job->Abort();
+    }
   }
-
-  // NULL out the fields of req, to mark it as cancelled.
-  req->MarkAsCancelled();
-  OnCancelRequest(req->source_net_log(), req->request_net_log(), req->info());
 }
 
 void HostResolverImpl::SetDefaultAddressFamily(AddressFamily address_family) {
   DCHECK(CalledOnValidThread());
   ipv6_probe_monitoring_ = false;
   DiscardIPv6ProbeJob();
   default_address_family_ = address_family;
 }
 
 AddressFamily HostResolverImpl::GetDefaultAddressFamily() const {
@@ skipping 18 matching lines @@
   DCHECK(addresses);
   DCHECK(net_error);
   IPAddressNumber ip_number;
   if (!ParseIPLiteralToNumber(key.hostname, &ip_number))
     return false;
 
   DCHECK_EQ(key.host_resolver_flags &
       ~(HOST_RESOLVER_CANONNAME | HOST_RESOLVER_LOOPBACK_ONLY |
         HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6),
             0) << " Unhandled flag";
-  bool ipv6_disabled = default_address_family_ == ADDRESS_FAMILY_IPV4 &&
-    !ipv6_probe_monitoring_;
+  bool ipv6_disabled = (default_address_family_ == ADDRESS_FAMILY_IPV4) &&
+      !ipv6_probe_monitoring_;
   *net_error = OK;
-  if (ip_number.size() == 16 && ipv6_disabled) {
+  if ((ip_number.size() == kIPv6AddressSize) && ipv6_disabled) {
     *net_error = ERR_NAME_NOT_RESOLVED;
   } else {
     *addresses = AddressList::CreateFromIPAddressWithCname(
         ip_number, info.port(),
         (key.host_resolver_flags & HOST_RESOLVER_CANONNAME));
   }
   return true;
 }
 
 bool HostResolverImpl::ServeFromCache(const Key& key,
@@ skipping 11 matching lines @@
   if (!cache_entry)
     return false;
 
   request_net_log.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_CACHE_HIT, NULL);
   *net_error = cache_entry->error;
   if (*net_error == OK)
     *addresses = CreateAddressListUsingPort(cache_entry->addrlist, info.port());
   return true;
 }
 
-void HostResolverImpl::AddOutstandingJob(Job* job) {
-  scoped_refptr<Job>& found_job = jobs_[job->key()];
-  DCHECK(!found_job);
-  found_job = job;
-
-  JobPool* pool = GetPoolForRequest(job->initial_request());
-  pool->AdjustNumOutstandingJobs(1);
+void HostResolverImpl::OnJobFinished(Job* job, const AddressList& addrlist) {
+  DCHECK(job);
+  DCHECK(job->handle().is_null());
+  // Signal dispatcher that a slot has opened.
+  dispatcher_.OnJobFinished();
+  if (job->net_error() == ERR_ABORTED)
+    return;
+  // Write result to the cache.
+  if (cache_.get())
+    cache_->Set(job->key(), job->net_error(), addrlist,
+                base::TimeTicks::Now());
 }
 
-HostResolverImpl::Job* HostResolverImpl::FindOutstandingJob(const Key& key) {
-  JobMap::iterator it = jobs_.find(key);
-  if (it != jobs_.end())
-    return it->second;
-  return NULL;
+void HostResolverImpl::RemoveJob(Job* job) {
+  DCHECK(job);
+  jobs_.erase(job->key());
 }
 
-void HostResolverImpl::RemoveOutstandingJob(Job* job) {
-  JobMap::iterator it = jobs_.find(job->key());
-  DCHECK(it != jobs_.end());
-  DCHECK_EQ(it->second.get(), job);
-  jobs_.erase(it);
-
-  JobPool* pool = GetPoolForRequest(job->initial_request());
-  pool->AdjustNumOutstandingJobs(-1);
-}
-
-void HostResolverImpl::OnJobComplete(Job* job,
-                                     int net_error,
-                                     int os_error,
-                                     const AddressList& addrlist) {
-  RemoveOutstandingJob(job);
-
-  // Write result to the cache.
-  if (cache_.get())
-    cache_->Set(job->key(), net_error, addrlist, base::TimeTicks::Now());
-
-  OnJobCompleteInternal(job, net_error, os_error, addrlist);
-}
-
-void HostResolverImpl::AbortJob(Job* job) {
-  OnJobCompleteInternal(job, ERR_ABORTED, 0 /* no os_error */, AddressList());
-}
-
-void HostResolverImpl::OnJobCompleteInternal(
-    Job* job,
-    int net_error,
-    int os_error,
-    const AddressList& addrlist) {
-  // Make a note that we are executing within OnJobComplete() in case the
-  // HostResolver is deleted by a callback invocation.
-  DCHECK(!cur_completing_job_);
-  cur_completing_job_ = job;
-
-  // Try to start any queued requests now that a job-slot has freed up.
-  ProcessQueuedRequests();
-
-  // Complete all of the requests that were attached to the job.
-  for (RequestsList::const_iterator it = job->requests().begin();
-       it != job->requests().end(); ++it) {
-    Request* req = *it;
-    if (!req->was_cancelled()) {
-      DCHECK_EQ(job, req->job());
-      req->request_net_log().EndEvent(
-          NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_ATTACH, NULL);
-
-      // Update the net log and notify registered observers.
-      OnFinishRequest(req->source_net_log(), req->request_net_log(),
-                      req->info(), net_error, os_error);
-
-      req->OnComplete(net_error, addrlist);
-
-      // Check if the job was cancelled as a result of running the callback.
-      // (Meaning that |this| was deleted).
-      if (job->was_cancelled())
-        return;
-    }
-  }
-
-  cur_completing_job_ = NULL;
-}
-
-void HostResolverImpl::OnStartRequest(const BoundNetLog& source_net_log,
-                                      const BoundNetLog& request_net_log,
-                                      const RequestInfo& info) {
+// static
+void HostResolverImpl::LogStartRequest(const BoundNetLog& source_net_log,
+                                       const BoundNetLog& request_net_log,
+                                       const RequestInfo& info) {
   source_net_log.BeginEvent(
       NetLog::TYPE_HOST_RESOLVER_IMPL,
       make_scoped_refptr(new NetLogSourceParameter(
           "source_dependency", request_net_log.source())));
 
   request_net_log.BeginEvent(
       NetLog::TYPE_HOST_RESOLVER_IMPL_REQUEST,

[mmenke, 2012/01/09 16:57:24]

Random comment:  Wonder if it would be worth considering not logging the
requests.  Suppose we'd lose speculative requests, particularly cache hits if we
did that, so maybe not a great idea.  Wouldn't belong in this CL, regardless.

Suppose could log fake Job entries instead of Request entries instead.  Maybe be
better off just keeping it as it is.

[szym, 2012/01/09 17:06:59]

I was looking into ridding the log from REQUEST entries, but haven't found a
good looking solution for ResolveFromCache. More importantly, I noticed that a
lot of requests lack a valid net_log source, so I think I'd try to fix that as
well.

       make_scoped_refptr(new RequestInfoParameters(
           info, source_net_log.source())));
 }
 
-void HostResolverImpl::OnFinishRequest(const BoundNetLog& source_net_log,
-                                       const BoundNetLog& request_net_log,
-                                       const RequestInfo& info,
-                                       int net_error,
-                                       int os_error) {
-  bool was_resolved = net_error == OK;
-
-  // Log some extra parameters on failure for synchronous requests.
+// static
+void HostResolverImpl::LogFinishRequest(const BoundNetLog& source_net_log,

[mmenke, 2012/01/09 16:57:24]

nit:  Think these can now all just be put in an anonymous namespace, and moved
out of the header.

+                                        const BoundNetLog& request_net_log,
+                                        const RequestInfo& info,
+                                        int net_error,
+                                        int os_error) {
   scoped_refptr<NetLog::EventParameters> params;
-  if (!was_resolved) {
+  if (net_error != OK) {
     params = new HostResolveFailedParams(0, net_error, os_error);
   }
 
   request_net_log.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_REQUEST, params);
   source_net_log.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL, NULL);
 }
 
-void HostResolverImpl::OnCancelRequest(const BoundNetLog& source_net_log,
-                                       const BoundNetLog& request_net_log,
-                                       const RequestInfo& info) {
+// static
+void HostResolverImpl::LogCancelRequest(const BoundNetLog& source_net_log,
+                                        const BoundNetLog& request_net_log,
+                                        const RequestInfo& info) {
   request_net_log.AddEvent(NetLog::TYPE_CANCELLED, NULL);
   request_net_log.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_REQUEST, NULL);
   source_net_log.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL, NULL);
 }
 
 void HostResolverImpl::DiscardIPv6ProbeJob() {
   if (ipv6_probe_job_.get()) {
     ipv6_probe_job_->Cancel();
     ipv6_probe_job_ = NULL;
   }
 }
 
 void HostResolverImpl::IPv6ProbeSetDefaultAddressFamily(
     AddressFamily address_family) {
   DCHECK(address_family == ADDRESS_FAMILY_UNSPECIFIED ||
          address_family == ADDRESS_FAMILY_IPV4);
   if (default_address_family_ != address_family) {
     VLOG(1) << "IPv6Probe forced AddressFamily setting to "
             << ((address_family == ADDRESS_FAMILY_UNSPECIFIED) ?
                 "ADDRESS_FAMILY_UNSPECIFIED" : "ADDRESS_FAMILY_IPV4");
   }
   default_address_family_ = address_family;
   // Drop reference since the job has called us back.
   DiscardIPv6ProbeJob();
 }
 
-bool HostResolverImpl::CanCreateJobForPool(const JobPool& pool) const {
-  DCHECK_LE(jobs_.size(), max_jobs_);
-
-  // We can't create another job if it would exceed the global total.
-  if (jobs_.size() + 1 > max_jobs_)
-    return false;
-
-  // Check whether the pool's constraints are met.
-  return pool.CanCreateJob();
-}
-
-// static
-HostResolverImpl::JobPoolIndex HostResolverImpl::GetJobPoolIndexForRequest(
-    const Request* req) {
-  return POOL_NORMAL;
-}
-
-void HostResolverImpl::ProcessQueuedRequests() {
-  // Find the highest priority request that can be scheduled.
-  Request* top_req = NULL;
-  for (size_t i = 0; i < arraysize(job_pools_); ++i) {
-    JobPool* pool = job_pools_[i];
-    if (pool->HasPendingRequests() && CanCreateJobForPool(*pool)) {
-      top_req = pool->RemoveTopPendingRequest();
-      break;
-    }
-  }
-
-  if (!top_req)
-    return;
-
-  scoped_refptr<Job> job(CreateAndStartJob(top_req));
-
-  // Search for any other pending request which can piggy-back off this job.
-  for (size_t pool_i = 0; pool_i < POOL_COUNT; ++pool_i) {
-    JobPool* pool = job_pools_[pool_i];
-    pool->MoveRequestsToJob(job);
-  }
-}
-
 HostResolverImpl::Key HostResolverImpl::GetEffectiveKeyForRequest(
     const RequestInfo& info) const {
   HostResolverFlags effective_flags =
       info.host_resolver_flags() | additional_resolver_flags_;
   AddressFamily effective_address_family = info.address_family();
   if (effective_address_family == ADDRESS_FAMILY_UNSPECIFIED &&
       default_address_family_ != ADDRESS_FAMILY_UNSPECIFIED) {
     effective_address_family = default_address_family_;
     if (ipv6_probe_monitoring_)
       effective_flags |= HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6;
   }
   return Key(info.hostname(), effective_address_family, effective_flags);
 }
 
-HostResolverImpl::Job* HostResolverImpl::CreateAndStartJob(Request* req) {
-  DCHECK(CanCreateJobForPool(*GetPoolForRequest(req)));
-  Key key = GetEffectiveKeyForRequest(req->info());
-
-  req->request_net_log().AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_CREATE_JOB,
-                                  NULL);
-
-  scoped_refptr<Job> job(new Job(next_job_id_++, this, key,
-                                 req->request_net_log(), net_log_));
-  job->AddRequest(req);
-  AddOutstandingJob(job);
-  job->Start();
-
-  return job.get();
-}
-
-int HostResolverImpl::EnqueueRequest(JobPool* pool, Request* req) {
-  scoped_ptr<Request> req_evicted_from_queue(
-      pool->InsertPendingRequest(req));
-
-  // If the queue has become too large, we need to kick something out.
-  if (req_evicted_from_queue.get()) {
-    Request* r = req_evicted_from_queue.get();
-    int error = ERR_HOST_RESOLVER_QUEUE_TOO_LARGE;
-
-    OnFinishRequest(r->source_net_log(), r->request_net_log(), r->info(), error,
-                    0  /* os_error (not applicable) */);
-
-    if (r == req)
-      return error;
-
-    r->OnComplete(error, AddressList());
-  }
-
-  return ERR_IO_PENDING;
-}
-
-void HostResolverImpl::CancelAllJobs() {
-  JobMap jobs;
-  jobs.swap(jobs_);
-  for (JobMap::iterator it = jobs.begin(); it != jobs.end(); ++it)
-    it->second->Cancel();
-}
-
 void HostResolverImpl::AbortAllInProgressJobs() {
-  for (size_t i = 0; i < arraysize(job_pools_); ++i)
-    job_pools_[i]->ResetNumOutstandingJobs();
-  JobMap jobs;
-  jobs.swap(jobs_);
-  for (JobMap::iterator it = jobs.begin(); it != jobs.end(); ++it) {
-    AbortJob(it->second);
-    it->second->Cancel();
+  base::WeakPtr<HostResolverImpl> self = AsWeakPtr();
+  // Scan |jobs_| for running jobs and abort them.
+  for (JobMap::iterator it = jobs_.begin(); it != jobs_.end(); ) {
+    Job* job = it->second;
+    // Advance the iterator before we might erase it.
+    ++it;
+    if (job->is_running()) {
+      job->Abort();
+      // Check if resolver was deleted in a request callback.
+      if (!self) return;

[mmenke, 2012/01/09 16:57:24]

nit:  Put return on next line.

+    } else {
+      // Keep it in |dispatch_|.
+      DCHECK(!job->handle().is_null());
+    }
   }
 }
 
 void HostResolverImpl::OnIPAddressChanged() {
   if (cache_.get())
     cache_->clear();
   if (ipv6_probe_monitoring_) {
     DiscardIPv6ProbeJob();
     ipv6_probe_job_ = new IPv6ProbeJob(this);
     ipv6_probe_job_->Start();
@@ skipping 16 matching lines @@
   // resolv.conf changes so we don't need to do anything to clear that cache.
   if (cache_.get())
     cache_->clear();
   // Existing jobs will have been sent to the original server so they need to
   // be aborted. TODO(Craig): Should these jobs be restarted?
   AbortAllInProgressJobs();
   // |this| may be deleted inside AbortAllInProgressJobs().
 }
 
 }  // namespace net