[net] HostResolverImpl + DnsTransaction + DnsConfigService = Asynchronous DNS ready for experiments.

net/base/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/base/host_resolver_impl.h"
 
 #if defined(OS_WIN)
 #include <Winsock2.h>
 #elif defined(OS_POSIX)
 #include <netdb.h>
 #endif
 
 #include <cmath>
 #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/rand_util.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
 #include "base/threading/worker_pool.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
+#include "net/base/address_family.h"
 #include "net/base/address_list.h"
 #include "net/base/address_list_net_log_param.h"
 #include "net/base/dns_reloader.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/host_resolver_proc.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
 #include "net/base/net_util.h"
+#include "net/dns/dns_config_service.h"
+#include "net/dns/dns_protocol.h"
+#include "net/dns/dns_response.h"
+#include "net/dns/dns_session.h"
+#include "net/dns/dns_transaction.h"
+#include "net/socket/client_socket_factory.h"
 
 #if defined(OS_WIN)
 #include "net/base/winsock_init.h"
 #endif
 
 namespace net {
 
 namespace {
 
 // Limit the size of hostnames that will be resolved to combat issues in
 // some platform's resolvers.
 const size_t kMaxHostLength = 4096;
 
 // Default TTL for successful resolutions with ProcTask.
 const unsigned kCacheEntryTTLSeconds = 60;
 
+// Default TTL for unsuccessful resolutions with ProcTask.
+const unsigned kNegativeCacheEntryTTLSeconds = 0;
+
 // Maximum of 8 concurrent resolver threads (excluding retries).
 // 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 kDefaultMaxProcTasks = 8u;
 
 // Helper to mutate the linked list contained by AddressList to the given
 // port. Note that in general this is dangerous since the AddressList's
 // data might be shared (and you should use AddressList::SetPort).
 //
@@ skipping 81 matching lines @@
                       int* os_error) OVERRIDE {
     return SystemHostResolverProc(hostname,
                                   address_family,
                                   host_resolver_flags,
                                   addrlist,
                                   os_error);
   }
 };
 
 // Extra parameters to attach to the NetLog when the resolve failed.
-class HostResolveFailedParams : public NetLog::EventParameters {
+class ProcTaskFailedParams : public NetLog::EventParameters {
  public:
-  HostResolveFailedParams(uint32 attempt_number,
-                          int net_error,
-                          int os_error)
+  ProcTaskFailedParams(uint32 attempt_number, int net_error, int os_error)
       : attempt_number_(attempt_number),
         net_error_(net_error),
         os_error_(os_error) {
   }
 
   virtual Value* ToValue() const OVERRIDE {
     DictionaryValue* dict = new DictionaryValue();
     if (attempt_number_)
       dict->SetInteger("attempt_number", attempt_number_);
 
@@ skipping 21 matching lines @@
 
     return dict;
   }
 
  private:
   const uint32 attempt_number_;
   const int net_error_;
   const int os_error_;
 };
 
+// Extra parameters to attach to the NetLog when the DnsTask failed.
+class DnsTaskFailedParams : public NetLog::EventParameters {
+ public:
+  DnsTaskFailedParams(int net_error, int dns_error)
+      : net_error_(net_error), dns_error_(dns_error) {
+  }
+
+  virtual Value* ToValue() const OVERRIDE {
+    DictionaryValue* dict = new DictionaryValue();
+    dict->SetInteger("net_error", net_error_);
+    if (dns_error_)
+      dict->SetInteger("dns_error", dns_error_);
+    return dict;
+  }
+
+ private:
+  const int net_error_;
+  const int dns_error_;
+};
+
 // Parameters representing the information in a RequestInfo object, along with
 // the associated NetLog::Source.
 class RequestInfoParameters : public NetLog::EventParameters {
  public:
   RequestInfoParameters(const HostResolver::RequestInfo& info,
                         const NetLog::Source& source)
       : info_(info), source_(source) {}
 
   virtual Value* ToValue() const OVERRIDE {
     DictionaryValue* dict = new DictionaryValue();
     dict->SetString("host", info_.host_port_pair().ToString());
     dict->SetInteger("address_family",
                      static_cast<int>(info_.address_family()));
     dict->SetBoolean("allow_cached_response", info_.allow_cached_response());
     dict->SetBoolean("is_speculative", info_.is_speculative());
     dict->SetInteger("priority", info_.priority());
 
     if (source_.is_valid())
       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
-// or a HostResolverImpl::ProcTask.
+// Parameters associated with the creation of a HostResolverImpl::Job.
 class JobCreationParameters : public NetLog::EventParameters {
  public:
   JobCreationParameters(const std::string& host,
                         const NetLog::Source& source)
       : host_(host), source_(source) {}
 
   virtual Value* ToValue() const OVERRIDE {
     DictionaryValue* dict = new DictionaryValue();
     dict->SetString("host", host_);
     dict->Set("source_dependency", source_.ToValue());
@@ skipping 43 matching lines @@
 }
 
 // Logs when a request has just completed (before its callback is run).
 void LogFinishRequest(const BoundNetLog& source_net_log,
                       const BoundNetLog& request_net_log,
                       const HostResolver::RequestInfo& info,
                       int net_error,
                       int os_error) {
   scoped_refptr<NetLog::EventParameters> params;
   if (net_error != OK) {
-    params = new HostResolveFailedParams(0, net_error, os_error);
+    params = new ProcTaskFailedParams(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);
 }
 
 // Logs when a request has been cancelled.
 void LogCancelRequest(const BoundNetLog& source_net_log,
                       const BoundNetLog& request_net_log,
                       const HostResolverImpl::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);
 }
 
 //-----------------------------------------------------------------------------
 
 // Keeps track of the highest priority.
 class PriorityTracker {
  public:
-  PriorityTracker()
-      : highest_priority_(IDLE), total_count_(0) {
+  explicit PriorityTracker(RequestPriority priority)
+      : highest_priority_(priority), total_count_(0) {
     memset(counts_, 0, sizeof(counts_));
   }
 
   RequestPriority highest_priority() const {
     return highest_priority_;
   }
 
   size_t total_count() const {
     return total_count_;
   }
@@ skipping 22 matching lines @@
   }
 
  private:
   RequestPriority highest_priority_;
   size_t total_count_;
   size_t counts_[NUM_PRIORITIES];
 };
 
 //-----------------------------------------------------------------------------
 
+// Convenience wrapper for PrioritizedDispatcher::Handle so that we don't forget
+// to update it when cancelling or changing the priority.

[mmenke, 2012/02/15 19:54:16]

nit: canceling.

[szym, 2012/02/15 20:28:05]

I think the consensus was that it's okay to use non-US English in comments :P

+// TODO(szym): consider making PrioritizedDispatcher::Handle into this.
+class DispatcherHandle {

[cbentzel, 2012/02/15 19:48:35]

Is this needed as much anymore, since you're restricted to only one dispatcher
in the current implementation? I'm not totally opposed to keeping it, but it
feels a bit extraneous.

[szym, 2012/02/15 20:28:05]

I prefer this over having to remember to set handle_ to Handle(). I'll probably
just move it to net/base/prioritized_dispatcher.h. I'm actually thinking about
replacing PrioritizedDispatcher::Handle with this, although storing the pointer
to |dispatcher_| with each handle is extraneous in case there's only one
dispatcher.

+ public:
+  DispatcherHandle() : dispatcher_(NULL) {}
+  DispatcherHandle(PrioritizedDispatcher* dispatcher,
+                   const PrioritizedDispatcher::Handle& handle)
+      : dispatcher_(dispatcher), handle_(handle) {
+    if (handle_.is_null())
+      dispatcher_ = NULL;
+  }
+
+  bool is_null() const {
+    return dispatcher_ == NULL;
+  }
+
+  void Reset() {
+    dispatcher_ = NULL;
+    handle_ = PrioritizedDispatcher::Handle();
+  }
+
+  void ChangePriority(RequestPriority priority) {
+    DCHECK(!is_null());
+    handle_ = dispatcher_->ChangePriority(handle_, priority);
+  }
+
+  void Cancel() {
+    DCHECK(!is_null());
+    dispatcher_->Cancel(handle_);
+    Reset();
+  }
+
+ private:
+  PrioritizedDispatcher* dispatcher_;
+  PrioritizedDispatcher::Handle handle_;
+};
+
+//-----------------------------------------------------------------------------
+
 HostResolver* CreateHostResolver(size_t max_concurrent_resolves,
                                  size_t max_retry_attempts,
-                                 bool use_cache,
+                                 HostCache* cache,
+                                 scoped_ptr<DnsConfigService> config_service,
                                  NetLog* net_log) {
   if (max_concurrent_resolves == HostResolver::kDefaultParallelism)
     max_concurrent_resolves = kDefaultMaxProcTasks;
 
   // TODO(szym): Add experiments with reserved slots for higher priority
   // requests.
 
   PrioritizedDispatcher::Limits limits(NUM_PRIORITIES, max_concurrent_resolves);
 
   HostResolverImpl* resolver = new HostResolverImpl(
-      use_cache ? HostCache::CreateDefaultCache() : NULL,
+      cache,
       limits,
       HostResolverImpl::ProcTaskParams(NULL, max_retry_attempts),
+      config_service.Pass(),
       net_log);
 
   return resolver;
 }
 
 }  // anonymous namespace
 
 //-----------------------------------------------------------------------------
 
 HostResolver* CreateSystemHostResolver(size_t max_concurrent_resolves,
                                        size_t max_retry_attempts,
                                        NetLog* net_log) {
   return CreateHostResolver(max_concurrent_resolves,
                             max_retry_attempts,
-                            true /* use_cache */,
+                            HostCache::CreateDefaultCache(),
+                            scoped_ptr<DnsConfigService>(NULL),
                             net_log);
 }
 
 HostResolver* CreateNonCachingSystemHostResolver(size_t max_concurrent_resolves,
                                                  size_t max_retry_attempts,
                                                  NetLog* net_log) {
   return CreateHostResolver(max_concurrent_resolves,
                             max_retry_attempts,
-                            false /* use_cache */,
+                            NULL,
+                            scoped_ptr<DnsConfigService>(NULL),
                             net_log);
 }
 
+HostResolver* CreateAsyncHostResolver(size_t max_concurrent_resolves,
+                                      size_t max_retry_attempts,
+                                      NetLog* net_log) {
+  scoped_ptr<DnsConfigService> config_service =
+      DnsConfigService::CreateSystemService();
+  config_service->Watch();

[cbentzel, 2012/02/15 19:48:35]

Should Watch be called by HostResolverImpl, so it can attach itself as an
observer first?

[szym, 2012/02/15 20:28:05]

I'll move it although it doesn't matter: DnsConfigService::AddObserver will call
the callback if config is already available. Also, the plan is to replace
DnsConfigService::Observer with NetworkChangeNotifier.

+  return CreateHostResolver(max_concurrent_resolves,
+                            max_retry_attempts,
+                            HostCache::CreateDefaultCache(),
+                            config_service.Pass(),
+                            net_log);
+}
+
 //-----------------------------------------------------------------------------
 
 // Holds the data for a request that could not be completed synchronously.
 // 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,
@@ skipping 83 matching lines @@
 // 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:
-  typedef base::Callback<void(int, int, const AddressList&)> Callback;
+  typedef base::Callback<void(int net_error,
+                              int os_error,
+                              const AddressList& addr_list)> Callback;
 
   ProcTask(const Key& key,
            const ProcTaskParams& params,
            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)) {
+        net_log_(job_net_log) {
     if (!params_.resolver_proc)
       params_.resolver_proc = HostResolverProc::GetDefault();
     // If default is unset, use the system proc.
     if (!params_.resolver_proc)
       params_.resolver_proc = new CallSystemHostResolverProc();
 
-    job_net_log.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_CREATE_PROC_TASK,
-                         new NetLogSourceParameter("source_dependency",
-                                                   net_log_.source()));
-
-    net_log_.BeginEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_TASK,
-                        new JobCreationParameters(key_.hostname,
-                                                  job_net_log.source()));
+    net_log_.BeginEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_TASK, NULL);
   }
 
   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;
   }
 
@@ skipping 84 matching lines @@
                         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;
 
     // Ideally the following code would be part of host_resolver_proc.cc,
-    // however it isn't safe to call NetworkChangeNotifier from worker
-    // threads. So we do it here on the IO thread instead.
+    // however it isn't safe to call NetworkChangeNotifier from worker threads.
+    // So we do it here on the IO thread instead.
     if (error != OK && NetworkChangeNotifier::IsOffline())
       error = ERR_INTERNET_DISCONNECTED;
 
-    // If this is the first attempt that is finishing later, then record
-    // data for the first attempt. Won't contaminate with retry attempt's
-    // data.
+    // If this is the first attempt that is finishing later, then record data
+    // for the first attempt. Won'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_canceled())
       return;
 
     scoped_refptr<NetLog::EventParameters> params;
     if (error != OK) {
-      params = new HostResolveFailedParams(attempt_number, error, os_error);
+      params = new ProcTaskFailedParams(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 (was_completed())
       return;
 
     // Copy the results from the first worker thread that resolves the host.
     results_ = results;
     completed_attempt_number_ = attempt_number;
     completed_attempt_error_ = error;
 
     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();
     }
 
     if (error != OK) {
-      params = new HostResolveFailedParams(0, error, os_error);
+      params = new ProcTaskFailedParams(0, error, os_error);
     } else {
       params = new AddressListNetLogParam(results_);
     }
     net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_TASK, params);
 
     callback_.Run(error, os_error, results_);
   }
 
   void RecordPerformanceHistograms(const base::TimeTicks& start_time,
                                    const int error,
@@ skipping 174 matching lines @@
   AddressList results_;
 
   BoundNetLog net_log_;
 
   DISALLOW_COPY_AND_ASSIGN(ProcTask);
 };
 
 //-----------------------------------------------------------------------------
 
 // Represents a request to the worker pool for a "probe for IPv6 support" call.
+//
+// TODO(szym): This could also be replaced with PostTaskAndReply and Callbacks.
 class HostResolverImpl::IPv6ProbeJob
     : public base::RefCountedThreadSafe<HostResolverImpl::IPv6ProbeJob> {
  public:
   explicit IPv6ProbeJob(HostResolverImpl* resolver)
       : resolver_(resolver),
         origin_loop_(base::MessageLoopProxy::current()) {
     DCHECK(resolver);
   }
 
   void Start() {
@@ skipping 47 matching lines @@
   HostResolverImpl* resolver_;
 
   // Used to post ourselves onto the origin thread.
   scoped_refptr<base::MessageLoopProxy> origin_loop_;
 
   DISALLOW_COPY_AND_ASSIGN(IPv6ProbeJob);
 };
 
 //-----------------------------------------------------------------------------
 
+// Resolves the hostname using DnsTransaction.
+// TODO(szym): This could be moved to separate source file as well.
+class HostResolverImpl::DnsTask {
+ public:
+  typedef base::Callback<void(int net_error,
+                              const AddressList& addr_list,
+                              base::TimeDelta ttl)> Callback;
+
+  DnsTask(DnsTransactionFactory* factory,
+          const Key& key,
+          const Callback& callback,
+          const BoundNetLog& job_net_log)
+      : callback_(callback), net_log_(job_net_log) {
+    DCHECK(factory);
+    DCHECK(!callback.is_null());
+
+    // For now we treat ADDRESS_FAMILY_UNSPEC as if it was IPV4.
+    uint16 qtype = (key.address_family == ADDRESS_FAMILY_IPV6)
+                   ? dns_protocol::kTypeAAAA
+                   : dns_protocol::kTypeA;
+    // TODO(szym): Implement "happy eyeballs".
+    transaction_ = factory->CreateTransaction(
+        key.hostname,
+        qtype,
+        base::Bind(&DnsTask::OnTransactionComplete, base::Unretained(this)),
+        net_log_);
+    DCHECK(transaction_.get());
+  }
+
+  int Start() {
+    net_log_.BeginEvent(
+        NetLog::TYPE_HOST_RESOLVER_IMPL_DNS_TASK,
+        new NetLogSourceParameter("source_dependency",
+                                  transaction_->net_log().source()));
+    return transaction_->Start();
+  }
+
+  void OnTransactionComplete(DnsTransaction* transaction,
+                             int net_error,
+                             const DnsResponse* response) {
+    // TODO(szym): Record performance histograms.
+    DnsResponse::Result result = DnsResponse::DNS_SUCCESS;

[mmenke, 2012/02/15 19:54:16]

Fine with this as-is, but does seem a little weird to explicitly log DNS_SUCCESS
when net_error isn't OK.

[szym, 2012/02/15 20:28:05]

It is equivalent to setting os_error = 0 if we don't have an os_error.

+    if (net_error == OK) {
+      AddressList addr_list;
+      base::TimeDelta ttl;
+      result = response->ParseToAddressList(&addr_list, &ttl);
+      if (result == DnsResponse::DNS_SUCCESS) {
+        net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_DNS_TASK,
+                          new AddressListNetLogParam(addr_list));
+        callback_.Run(net_error, addr_list, ttl);

[mmenke, 2012/02/15 19:54:16]

Suggest you add a comment that we've been deleted here.

+        return;
+      }
+      net_error = ERR_DNS_MALFORMED_RESPONSE;
+    }
+    callback_.Run(net_error, AddressList(), base::TimeDelta());
+    net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_DNS_TASK,

[mmenke, 2012/02/15 19:54:16]

Suggest you move this line up, and delete |this| in the callback, just like the
other case.

[szym, 2012/02/15 20:28:05]

Good catch. This is a bug.

+                      new DnsTaskFailedParams(net_error, result));
+  }
+
+ private:
+  // The listener to the results of this DnsTask.
+  Callback callback_;
+
+  const BoundNetLog net_log_;
+
+  scoped_ptr<DnsTransaction> transaction_;
+};
+
+//-----------------------------------------------------------------------------
+
 // 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.
+  // request that spawned it and |priority| is the initial priority.
   Job(HostResolverImpl* resolver,
       const Key& key,
-      const BoundNetLog& request_net_log)
+      const BoundNetLog& request_net_log,
+      RequestPriority priority)
       : resolver_(resolver->AsWeakPtr()),
         key_(key),
+        priority_tracker_(priority),
         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()) {
+        // |resolver_| was destroyed with this Job still pending.
+        // Clean-up, record in the log, but don't run any callbacks.
         DCHECK_EQ(ERR_IO_PENDING, net_error_);
-        proc_task_->Cancel();
-        proc_task_ = NULL;
+        if (is_proc_running()) {
+          proc_task_->Cancel();
+          proc_task_ = NULL;
+        }
         net_error_ = ERR_ABORTED;
       } else {
+        // The last Request for this Job has been cancelled.
         net_log_.AddEvent(NetLog::TYPE_CANCELLED, NULL);
         net_error_ = OK;  // For NetLog.
       }
 
       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_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 with |dispatcher_|.
-  const PrioritizedDispatcher::Handle& handle() const {
-    return handle_;
+  void AddToDispatcher(PrioritizedDispatcher* dispatcher) {
+    DCHECK(handle_.is_null());
+    handle_ = DispatcherHandle(dispatcher, dispatcher->Add(this, priority()));
   }
 
-  void set_handle(const PrioritizedDispatcher::Handle& handle) {
-    handle_ = handle;
+  bool IsWaitingInDispatch() const {
+    return !handle_.is_null();
   }
 
-  // The Job will own |req| and destroy it in ~Job.
-  void AddRequest(Request* req) {
+  void AddRequest(scoped_ptr<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_REQUEST_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();
     }
+
+    requests_.push_back(req.release());
+
+    if (!handle_.is_null())
+      handle_.ChangePriority(priority());
   }
 
   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();
     LogCancelRequest(req->source_net_log(), req->request_net_log(),
                      req->info());
     priority_tracker_.Remove(req->info().priority());
     net_log_.AddEvent(
         NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_REQUEST_DETACH,
         make_scoped_refptr(new JobAttachParameters(
             req->request_net_log().source(), priority())));
+
+    if (!handle_.is_null()) {
+      if (num_active_requests() > 0) {
+        handle_.ChangePriority(priority());
+      } else {
+        handle_.Cancel();
+      }
+    }
   }
 
   // Aborts and destroys the job, completes all requests as aborted.
   void Abort() {
     // Job should only be aborted if it's running.
     DCHECK(is_running());
-    proc_task_->Cancel();
-    proc_task_ = NULL;
+    if (is_proc_running()) {
+      proc_task_->Cancel();
+      proc_task_ = NULL;
+    }
+    dns_task_.reset();
     net_error_ = ERR_ABORTED;
     os_error_ = 0;
-    CompleteRequests(AddressList());
+    CompleteRequests(AddressList(), base::TimeDelta());
+  }
+
+  bool is_dns_running() const {
+    return dns_task_.get() != NULL;
+  }
+
+  bool is_proc_running() const {
+    return proc_task_.get() != NULL;
   }
 
   bool is_running() const {
-    return proc_task_.get() != NULL;
+    return is_dns_running() || is_proc_running();
   }
 
   // Called by HostResolverImpl when this job is evicted due to queue overflow.
   void OnEvicted() {
     // Must not be running.
     DCHECK(!is_running());
-    handle_ = PrioritizedDispatcher::Handle();
+    handle_.Reset();
 
     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());
+    CompleteRequests(AddressList(), base::TimeDelta());
   }
 
   // PriorityDispatch::Job interface.
   virtual void Start() OVERRIDE {
     DCHECK(!is_running());
-    handle_ = PrioritizedDispatcher::Handle();
+    handle_.Reset();
 
     net_log_.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_STARTED, NULL);
 
+    if (resolver_->dns_transaction_factory_.get()) {
+      StartDnsTask();
+    } else {
+      StartProcTask();
+    }
+  }
+
+ private:
+  // TODO(szym): Since DnsTransaction does not consume threads, we can increase
+  // the limits on |dispatcher_|. But in order to keep the number of WorkerPool
+  // threads low, we will need to use an "inner" PrioritizedDispatcher with
+  // tighter limits.
+  void StartProcTask() {
     proc_task_ = new ProcTask(
         key_,
         resolver_->proc_params_,
         base::Bind(&Job::OnProcTaskComplete, base::Unretained(this)),
         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:
   // Called by ProcTask when it completes.
   void OnProcTaskComplete(int net_error, int os_error,
-                          const AddressList& addrlist) {
-    DCHECK(is_running());
+                          const AddressList& addr_list) {
+    DCHECK(is_proc_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);
+    base::TimeDelta ttl = base::TimeDelta::FromSeconds(
+        kNegativeCacheEntryTTLSeconds);
+    if (net_error == OK)
+      ttl = base::TimeDelta::FromSeconds(kCacheEntryTTLSeconds);
+    CompleteRequests(addr_list, ttl);
+  }
+
+  void StartDnsTask() {
+    dns_task_.reset(new DnsTask(
+        resolver_->dns_transaction_factory_.get(),
+        key_,
+        base::Bind(&Job::OnDnsTaskComplete, base::Unretained(this)),
+        net_log_));
+
+    int rv = dns_task_->Start();
+    if (rv != ERR_IO_PENDING) {

[mmenke, 2012/02/15 19:54:16]

Maybe reset |dns_task| here, and maybe DCHECK it's NULL when ProcTask starts?

+      DCHECK_NE(OK, rv);
+      StartProcTask();
+    }
+  }
+
+  // Called by DnsTask when it completes.
+  void OnDnsTaskComplete(int net_error,
+                         const AddressList& addr_list,
+                         base::TimeDelta ttl) {
+    DCHECK(is_dns_running());
+    dns_task_.reset();
+
+    if (net_error != OK) {
+      // TODO(szym): Some net errors indicate lack of connectivity. Starting
+      // ProcTask in that case is a waste of time.
+      StartProcTask();
+      return;
+    }
+
+    net_error_ = OK;

[cbentzel, 2012/02/15 19:48:35]

Are net_error_ and os_error_ needed as members, or could they be passed in as
arguments to CompleteRequests?

There's one little bit of funkiness where ERR_IO_PENDING is used both to
indicate that the Job hasn't been dispatched yet, and that it is actively doing
a DnsTask/ProcTask. Perhaps that could be better served with an explicit state.

If you'd like to table that to another cleanup CL, that would also be fine -
this one already has a lot of stuff.

[szym, 2012/02/15 20:28:05]

Agreed that the state indication is all over place. I'll clean it up.

+    os_error_ = 0;
+    CompleteRequests(addr_list, ttl);
   }
 
   // Completes all Requests. Calls OnJobFinished and deletes self.
-  void CompleteRequests(const AddressList& addrlist) {
+  void CompleteRequests(const AddressList& addr_list, base::TimeDelta ttl) {
     CHECK(resolver_);
 
+    // We are the only consumer of |addr_list|, so we can safely change the port
+    // without copy-on-write. This pays off, when job has only one request.
+    AddressList list = addr_list;
+    if (net_error_ == OK && !requests_.empty())
+      MutableSetPort(requests_.front()->info().port(), &list);
+
     // 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_->OnJobFinished(this, addrlist);
+    resolver_->OnJobFinished(this, addr_list, ttl);
 
     // Complete all of the requests that were attached to the job.
     for (RequestsList::const_iterator it = requests_.begin();
          it != requests_.end(); ++it) {
       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);
+      req->OnComplete(net_error_, addr_list);
 
       // Check if the resolver was destroyed as a result of running the
       // callback. If it was, we could continue, but we choose to bail.
       if (!resolver_)
         return;
     }
   }
 
-  // Used to call OnJobFinished and RemoveJob.
+  // Used to call OnJobFinished.
   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..
+  // Resolves the host using a HostResolverProc.
   scoped_refptr<ProcTask> proc_task_;
 
+  // Resolves the host using a DnsTransaction.
+  scoped_ptr<DnsTask> dns_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_;
+  DispatcherHandle 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(
     HostCache* cache,
     const PrioritizedDispatcher::Limits& job_limits,
     const ProcTaskParams& proc_params,
+    scoped_ptr<DnsConfigService> dns_config_service,
     NetLog* net_log)
     : cache_(cache),
       dispatcher_(job_limits),
       max_queued_jobs_(job_limits.total_jobs * 100u),
       proc_params_(proc_params),
       default_address_family_(ADDRESS_FAMILY_UNSPECIFIED),
+      dns_config_service_(dns_config_service.Pass()),
       ipv6_probe_monitoring_(false),
       additional_resolver_flags_(0),
       net_log_(net_log) {
 
   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 (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
+
+  if (dns_config_service_.get())

[cbentzel, 2012/02/15 19:48:35]

Maybe want a note here that |this| is never removed as an observer, since it
destroys dns_config_service_ in the destructor. Or perhaps this is obvious.

+    dns_config_service_->AddObserver(this);
 }
 
 HostResolverImpl::~HostResolverImpl() {
   DiscardIPv6ProbeJob();
 
   // This will also cancel all outstanding requests.
   STLDeleteValues(&jobs_);
 
   NetworkChangeNotifier::RemoveIPAddressObserver(this);
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_OPENBSD)
@@ skipping 33 matching lines @@
     return rv;
   }
 
   // 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->set_handle(dispatcher_.Add(job, info.priority()));
+    job = new Job(this, key, request_net_log, info.priority());
+    job->AddToDispatcher(&dispatcher_);
 
     // Check for queue overflow.
     if (dispatcher_.num_queued_jobs() > max_queued_jobs_) {
       Job* evicted = static_cast<Job*>(dispatcher_.EvictOldestLowest());
       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->set_handle(dispatcher_.ChangePriority(job->handle(), job->priority()));
+  scoped_ptr<Request> req(new Request(source_net_log,
+                                      request_net_log,
+                                      info,
+                                      callback,
+                                      addresses));
   if (out_req)
-    *out_req = reinterpret_cast<RequestHandle>(req);
+    *out_req = reinterpret_cast<RequestHandle>(req.get());
+
+  job->AddRequest(req.Pass());
 
   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) {
@@ skipping 35 matching lines @@
 void HostResolverImpl::CancelRequest(RequestHandle req_handle) {
   DCHECK(CalledOnValidThread());
   Request* req = reinterpret_cast<Request*>(req_handle);
   DCHECK(req);
 
   Job* job = req->job();
   DCHECK(job);
 
   job->CancelRequest(req);
 
-  if (!job->handle().is_null()) {
-    // Still in queue.
-    if (job->num_active_requests()) {
-      job->set_handle(dispatcher_.ChangePriority(job->handle(),
-                                                 job->priority()));
+  if (job->num_active_requests() == 0) {
+    if (job->is_running()) {
+      // 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.

[mmenke, 2012/02/15 19:54:16]

I find this comment confusing.

Maybe something like:

If this were called from Request's callback in  Job::CompleteRequests(), that
Request would not have been cancelled, so job->num_active_requests() would not
be 0.  Therefore, we weren't called from Job::CompleteRequests().

+      job->Abort();
     } else {
-      dispatcher_.Cancel(job->handle());
       RemoveJob(job);
       delete job;
     }
-  } else {
-    // 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();
-    }
   }
 }
 
 void HostResolverImpl::SetDefaultAddressFamily(AddressFamily address_family) {
   DCHECK(CalledOnValidThread());
   ipv6_probe_monitoring_ = false;
   DiscardIPv6ProbeJob();
   default_address_family_ = address_family;
 }
 
@@ skipping 54 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::OnJobFinished(Job* job, const AddressList& addrlist) {
+void HostResolverImpl::OnJobFinished(Job* job,
+                                     const AddressList& addrlist,

[mmenke, 2012/02/15 19:54:16]

At some point (Not necessarily in this CL), you might want to switch all of
these to addr_list, for consistency.

+                                     base::TimeDelta ttl) {
   DCHECK(job);
-  DCHECK(job->handle().is_null());
+  DCHECK(!job->IsWaitingInDispatch());
   RemoveJob(job);
   if (job->net_error() == ERR_HOST_RESOLVER_QUEUE_TOO_LARGE)
     return;
 
   // Signal dispatcher that a slot has opened.
   dispatcher_.OnJobFinished();
   if (job->net_error() == ERR_ABORTED)
     return;
   // Write result to the cache.
   if (cache_.get()) {
-    base::TimeDelta ttl = base::TimeDelta::FromSeconds(0);
-    if (job->net_error() == OK)
-      ttl = base::TimeDelta::FromSeconds(kCacheEntryTTLSeconds);
     cache_->Set(job->key(), job->net_error(), addrlist,
                 base::TimeTicks::Now(), ttl);
   }
 }
 
 void HostResolverImpl::RemoveJob(Job* job) {
   DCHECK(job);
   jobs_.erase(job->key());
 }
 
@@ skipping 28 matching lines @@
     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);
 }
 
 void HostResolverImpl::AbortAllInProgressJobs() {
   base::WeakPtr<HostResolverImpl> self = AsWeakPtr();
   // Scan |jobs_| for running jobs and abort them.
   for (JobMap::iterator it = jobs_.begin(); it != jobs_.end(); ) {

[cbentzel, 2012/02/15 19:48:35]

BUG: I don't think this works correctly. The problem is that it will potentially
activate start a new Job running that doesn't needed to be aborted - for
example, it might have the new DnsTransactionFactory after OnDnsChanged. I think
you want to accumulate the jobs to abort in one loop in a different data
structure, and then abort them.

[szym, 2012/02/15 20:28:05]

Right. That's a bug.
HostResolverImplTest.OnlyAbortExistingRequestsOnIPAddressChange should have
caught that. I'll investigate then fix.

     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;
     } else {
       // Keep it in |dispatch_|.
-      DCHECK(!job->handle().is_null());
+      DCHECK(job->IsWaitingInDispatch());
     }
   }
 }
 
 void HostResolverImpl::OnIPAddressChanged() {
   if (cache_.get())
     cache_->clear();
   if (ipv6_probe_monitoring_) {
     DiscardIPv6ProbeJob();
     ipv6_probe_job_ = new IPv6ProbeJob(this);
@@ skipping 16 matching lines @@
   // as NSCD's cache should be dropped automatically by the OS when
   // 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().
 }
 
+void HostResolverImpl::OnConfigChanged(const DnsConfig& dns_config) {
+  // We want a new factory in place, before we Abort running Jobs, so that the
+  // newly started jobs use the new factory.
+  bool had_factory = (dns_transaction_factory_.get() != NULL);
+  if (dns_config.IsValid()) {
+    dns_transaction_factory_ = DnsTransactionFactory::CreateFactory(
+        new DnsSession(dns_config,
+                       ClientSocketFactory::GetDefaultFactory(),
+                       base::Bind(&base::RandInt),
+                       net_log_));
+  } else {
+    dns_transaction_factory_.reset();
+  }
+  if (had_factory) {

[cbentzel, 2012/02/15 19:48:35]

Nit: braces not needed.

Also - would it be easier to just call OnDNSChanged each time? It seems like it
would be a no-op if there wasn't a factory before, so would simplify the logic a
tiny bit.

[szym, 2012/02/15 20:28:05]

OnDNSChanged aborts all in-flight Jobs. Calling it after we got the DnsConfig
for the first time might hamper startup. Also, if we have a bad DnsConfigService
that always returns invalid DnsConfig, that would make us dump all the Jobs on
the floor every now and then.

+    OnDNSChanged();
+  }
+}
+
 }  // namespace net