[net] Make HostResolverImpl::OnIPAddressChange faster by removing blocking on HaveOnlyLoopbackAddre…

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>
@@ skipping 218 matching lines @@
  protected:
   virtual ~CallSystemHostResolverProc() {}
 };
 
 AddressList EnsurePortOnAddressList(const AddressList& list, uint16 port) {
   if (list.empty() || list.front().port() == port)
     return list;
   return AddressList::CopyWithPort(list, port);
 }
 
-// Wraps a call to HaveOnlyLoopbackAddresses to be executed on the WorkerPool as
-// it takes 40-100ms and should not block initialization.
-class HaveOnlyLoopbackProbeJob
-    : public base::RefCountedThreadSafe<HaveOnlyLoopbackProbeJob> {
- public:
-  typedef base::Callback<void(bool)> CallbackType;
-  explicit HaveOnlyLoopbackProbeJob(const CallbackType& callback)
-      : result_(false) {
-    const bool kIsSlow = true;
-    base::WorkerPool::PostTaskAndReply(
-        FROM_HERE,
-        base::Bind(&HaveOnlyLoopbackProbeJob::DoProbe, this),
-        base::Bind(&HaveOnlyLoopbackProbeJob::OnProbeComplete, this, callback),
-        kIsSlow);
-  }
-
- private:
-  friend class base::RefCountedThreadSafe<HaveOnlyLoopbackProbeJob>;
-
-  virtual ~HaveOnlyLoopbackProbeJob() {}
-
-  // Runs on worker thread.
-  void DoProbe() {
-    result_ = HaveOnlyLoopbackAddresses();
-  }
-
-  void OnProbeComplete(const CallbackType& callback) {
-    callback.Run(result_);
-  }
-
-  bool result_;
-
-  DISALLOW_COPY_AND_ASSIGN(HaveOnlyLoopbackProbeJob);
-};
-
-
 // Creates NetLog parameters when the resolve failed.
 base::Value* NetLogProcTaskFailedCallback(uint32 attempt_number,
                                           int net_error,
                                           int os_error,
                                           NetLog::LogLevel /* log_level */) {
   DictionaryValue* dict = new DictionaryValue();
   if (attempt_number)
     dict->SetInteger("attempt_number", attempt_number);
 
   dict->SetInteger("net_error", net_error);
@@ skipping 626 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.
+// Wraps a call to TestIPv6Support to be executed on the WorkerPool as it takes
+// 40-100ms.
 class HostResolverImpl::IPv6ProbeJob
     : public base::RefCountedThreadSafe<HostResolverImpl::IPv6ProbeJob> {
  public:
-  IPv6ProbeJob(HostResolverImpl* resolver, NetLog* net_log)
+  IPv6ProbeJob(const base::WeakPtr<HostResolverImpl>& resolver, NetLog* net_log)
       : resolver_(resolver),
-        origin_loop_(base::MessageLoopProxy::current()),
-        net_log_(BoundNetLog::Make(net_log, NetLog::SOURCE_IPV6_PROBE_JOB)) {
+        net_log_(BoundNetLog::Make(net_log, NetLog::SOURCE_IPV6_PROBE_JOB)),
+        result_(false, IPV6_SUPPORT_MAX, OK) {
     DCHECK(resolver);
-  }
-
-  void Start() {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    if (was_canceled())
-      return;
     net_log_.BeginEvent(NetLog::TYPE_IPV6_PROBE_RUNNING);
     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_canceled())
-      return;
-    net_log_.AddEvent(NetLog::TYPE_CANCELLED);
-    resolver_ = NULL;  // Read/write ONLY on origin thread.
+    base::WorkerPool::PostTaskAndReply(
+        FROM_HERE,
+        base::Bind(&IPv6ProbeJob::DoProbe, this),
+        base::Bind(&IPv6ProbeJob::OnProbeComplete, this),
+        kIsSlow);
   }
 
  private:
   friend class base::RefCountedThreadSafe<HostResolverImpl::IPv6ProbeJob>;
 
-  ~IPv6ProbeJob() {
+  virtual ~IPv6ProbeJob() {}
+
+  // Runs on worker thread.
+  void DoProbe() {
+    result_ = TestIPv6Support();
   }
 
-  // Returns true if cancelled or if probe results have already been received
-  // on the origin thread.
-  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.
-    origin_loop_->PostTask(
-        FROM_HERE,
-        base::Bind(&IPv6ProbeJob::OnProbeComplete, this, TestIPv6Support()));
-  }
-
-  // Callback for when DoProbe() completes.
-  void OnProbeComplete(const IPv6SupportResult& support_result) {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    net_log_.EndEvent(
-        NetLog::TYPE_IPV6_PROBE_RUNNING,
-        base::Bind(&IPv6SupportResult::ToNetLogValue,
-                   base::Unretained(&support_result)));
-    if (was_canceled())
+  void OnProbeComplete() {
+    net_log_.EndEvent(NetLog::TYPE_IPV6_PROBE_RUNNING,
+                      base::Bind(&IPv6SupportResult::ToNetLogValue,
+                                 base::Unretained(&result_)));
+    if (!resolver_)
       return;
-
-    // Clear |resolver_| so that no cancel event is logged.
-    HostResolverImpl* resolver = resolver_;
-    resolver_ = NULL;
-
-    resolver->IPv6ProbeSetDefaultAddressFamily(
-        support_result.ipv6_supported ? ADDRESS_FAMILY_UNSPECIFIED
+    resolver_->IPv6ProbeSetDefaultAddressFamily(
+        result_.ipv6_supported ? ADDRESS_FAMILY_UNSPECIFIED
                                       : ADDRESS_FAMILY_IPV4);
   }
 
   // Used/set only on origin thread.
-  HostResolverImpl* resolver_;
-
-  // Used to post ourselves onto the origin thread.
-  scoped_refptr<base::MessageLoopProxy> origin_loop_;
+  base::WeakPtr<HostResolverImpl> resolver_;
 
   BoundNetLog net_log_;
 
+  IPv6SupportResult result_;
+
   DISALLOW_COPY_AND_ASSIGN(IPv6ProbeJob);
 };
 
+// Wraps a call to HaveOnlyLoopbackAddresses to be executed on the WorkerPool as
+// it takes 40-100ms and should not block initialization.
+class HostResolverImpl::LoopbackProbeJob
+    : public base::RefCountedThreadSafe<HostResolverImpl::LoopbackProbeJob> {

[eroman, 2012/11/03 00:56:55]

you may consider not making this refcounted anymore. Instead the
PostTaskAndReply() could Own() this in the reply callback.

[szym, 2012/11/05 23:08:04]

I forgot about this option. Thanks!

+ public:
+  explicit LoopbackProbeJob(const base::WeakPtr<HostResolverImpl>& resolver)
+      : resolver_(resolver),
+        result_(false) {
+    DCHECK(resolver);
+    const bool kIsSlow = true;
+    base::WorkerPool::PostTaskAndReply(
+        FROM_HERE,
+        base::Bind(&LoopbackProbeJob::DoProbe, this),
+        base::Bind(&LoopbackProbeJob::OnProbeComplete, this),
+        kIsSlow);
+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<HostResolverImpl::LoopbackProbeJob>;
+
+  virtual ~LoopbackProbeJob() {}
+
+  // Runs on worker thread.
+  void DoProbe() {
+    result_ = HaveOnlyLoopbackAddresses();
+  }
+
+  void OnProbeComplete() {
+    if (!resolver_)
+      return;
+    resolver_->SetHaveOnlyLoopbackAddresses(result_);
+  }
+
+  // Used/set only on origin thread.
+  base::WeakPtr<HostResolverImpl> resolver_;
+
+  bool result_;
+
+  DISALLOW_COPY_AND_ASSIGN(LoopbackProbeJob);
+};
+
 //-----------------------------------------------------------------------------
 
 // Resolves the hostname using DnsTransaction.
 // TODO(szym): This could be moved to separate source file as well.
 class HostResolverImpl::DnsTask : public base::SupportsWeakPtr<DnsTask> {
  public:
   typedef base::Callback<void(int net_error,
                               const AddressList& addr_list,
                               base::TimeDelta ttl)> Callback;
 
@@ skipping 160 matching lines @@
   DISALLOW_COPY_AND_ASSIGN(DnsTask);
 };
 
 //-----------------------------------------------------------------------------
 
 // Aggregates all Requests for the same Key. Dispatched via PriorityDispatch.
 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,
+  Job(const base::WeakPtr<HostResolverImpl>& resolver,
       const Key& key,
       RequestPriority priority,
       const BoundNetLog& request_net_log)
-      : resolver_(resolver->weak_ptr_factory_.GetWeakPtr()),
+      : resolver_(resolver),
         key_(key),
         priority_tracker_(priority),
         had_non_speculative_request_(false),
         had_dns_config_(false),
         dns_task_error_(OK),
         creation_time_(base::TimeTicks::Now()),
         priority_change_time_(creation_time_),
         net_log_(BoundNetLog::Make(request_net_log.net_log(),
                                    NetLog::SOURCE_HOST_RESOLVER_IMPL_JOB)) {
     request_net_log.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_CREATE_JOB);
@@ skipping 455 matching lines @@
     const PrioritizedDispatcher::Limits& job_limits,
     const ProcTaskParams& proc_params,
     scoped_ptr<DnsClient> dns_client,
     NetLog* net_log)
     : cache_(cache.Pass()),
       dispatcher_(job_limits),
       max_queued_jobs_(job_limits.total_jobs * 100u),
       proc_params_(proc_params),
       default_address_family_(ADDRESS_FAMILY_UNSPECIFIED),
       weak_ptr_factory_(this),
+      probe_weak_ptr_factory_(this),
       dns_client_(dns_client.Pass()),
       received_dns_config_(false),
       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)
-  new HaveOnlyLoopbackProbeJob(
-      base::Bind(&HostResolverImpl::SetHaveOnlyLoopbackAddresses,
-                 weak_ptr_factory_.GetWeakPtr()));
+  new LoopbackProbeJob(weak_ptr_factory_.GetWeakPtr());
 #endif
   NetworkChangeNotifier::AddIPAddressObserver(this);
   NetworkChangeNotifier::AddDNSObserver(this);
   if (!HaveDnsConfig())
     OnDNSChanged();
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_OPENBSD) && \
     !defined(OS_ANDROID)
   EnsureDnsReloaderInit();
 #endif
 }
 
 HostResolverImpl::~HostResolverImpl() {
-  DiscardIPv6ProbeJob();
-
   // This will also cancel all outstanding requests.
   STLDeleteValues(&jobs_);
 
   NetworkChangeNotifier::RemoveIPAddressObserver(this);
   NetworkChangeNotifier::RemoveDNSObserver(this);
 }
 
 void HostResolverImpl::SetMaxQueuedJobs(size_t value) {
   DCHECK_EQ(0u, dispatcher_.num_queued_jobs());
   DCHECK_GT(value, 0u);
@@ skipping 64 matching lines @@
         category = ipv4 ? AF_WASTE_CACHE_IPV4 : AF_WASTE_CACHE_UNSPEC;
       } else if (found_other_family_job) {
         category = ipv4 ? AF_WASTE_JOB_IPV4 : AF_WASTE_JOB_UNSPEC;
       } else {
         category = ipv4 ? AF_WASTE_NONE_IPV4 : AF_WASTE_NONE_UNSPEC;
       }
       UMA_HISTOGRAM_ENUMERATION("DNS.ResolveUnspecWaste", category,
                                 AF_WASTE_MAX);
     }
 
-    // Create new Job.
-    job = new Job(this, key, info.priority(), request_net_log);
+    job = new Job(weak_ptr_factory_.GetWeakPtr(), key, info.priority(),
+                  request_net_log);
     job->Schedule();
 
     // Check for queue overflow.
     if (dispatcher_.num_queued_jobs() > max_queued_jobs_) {
       Job* evicted = static_cast<Job*>(dispatcher_.EvictOldestLowest());
       DCHECK(evicted);
       evicted->OnEvicted();  // Deletes |evicted|.
       if (evicted == job) {
         rv = ERR_HOST_RESOLVER_QUEUE_TOO_LARGE;
         LogFinishRequest(source_net_log, request_net_log, info, rv);
@@ skipping 69 matching lines @@
   DCHECK(CalledOnValidThread());
   Request* req = reinterpret_cast<Request*>(req_handle);
   DCHECK(req);
   Job* job = req->job();
   DCHECK(job);
   job->CancelRequest(req);
 }
 
 void HostResolverImpl::SetDefaultAddressFamily(AddressFamily address_family) {
   DCHECK(CalledOnValidThread());
+  default_address_family_ = address_family;
   ipv6_probe_monitoring_ = false;
-  DiscardIPv6ProbeJob();
-  default_address_family_ = address_family;
+  OnIPAddressChanged();

[eroman, 2012/11/03 00:56:55]

This has the sideffect of cancelling all current requests, is that desired?

[szym, 2012/11/05 23:08:04]

I've realized that when |ipv6_probe_monitoring_ == false| we can just ignore the
output of the current probe job. I removed the trigger from here and added a
check to IPv6ProbeSetDefaultAddressFamily.

 }
 
 AddressFamily HostResolverImpl::GetDefaultAddressFamily() const {
   return default_address_family_;
 }
 
 void HostResolverImpl::ProbeIPv6Support() {
   DCHECK(CalledOnValidThread());
   DCHECK(!ipv6_probe_monitoring_);
   ipv6_probe_monitoring_ = true;
-  OnIPAddressChanged();  // Give initial setup call.
+  OnIPAddressChanged();
 }
 
 HostCache* HostResolverImpl::GetHostCache() {
   return cache_.get();
 }
 
 base::Value* HostResolverImpl::GetDnsConfigAsValue() const {
   // Check if async DNS is disabled.
   if (!dns_client_.get())
     return NULL;
@@ skipping 97 matching lines @@
     cache_->Set(key, entry, base::TimeTicks::Now(), ttl);
 }
 
 void HostResolverImpl::RemoveJob(Job* job) {
   DCHECK(job);
   JobMap::iterator it = jobs_.find(job->key());
   if (it != jobs_.end() && it->second == job)
     jobs_.erase(it);
 }
 
-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();
 }
 
 void HostResolverImpl::SetHaveOnlyLoopbackAddresses(bool result) {
   if (result) {
     additional_resolver_flags_ |= HOST_RESOLVER_LOOPBACK_ONLY;
   } else {
     additional_resolver_flags_ &= ~HOST_RESOLVER_LOOPBACK_ONLY;
   }
 }
 
@@ skipping 51 matching lines @@
 
   for (JobMap::iterator it = jobs_.begin(); self && it != jobs_.end(); ) {
     Job* job = it->second;
     ++it;
     // This could remove |job| from |jobs_|, but iterator will remain valid.
     job->ServeFromHosts();
   }
 }
 
 void HostResolverImpl::OnIPAddressChanged() {
+  // Abandon all ProbeJobs.
+  probe_weak_ptr_factory_.InvalidateWeakPtrs();
   if (cache_.get())
     cache_->clear();
-  if (ipv6_probe_monitoring_) {
-    DiscardIPv6ProbeJob();
-    ipv6_probe_job_ = new IPv6ProbeJob(this, net_log_);
-    ipv6_probe_job_->Start();
-  }
+  if (ipv6_probe_monitoring_)
+    new IPv6ProbeJob(probe_weak_ptr_factory_.GetWeakPtr(), net_log_);
 #if defined(OS_POSIX) && !defined(OS_MACOSX)
-  // TODO(szym): Use HaveOnlyLoopbackProbeJob. http://crbug.com/157933
-  SetHaveOnlyLoopbackAddresses(HaveOnlyLoopbackAddresses());
+  new LoopbackProbeJob(probe_weak_ptr_factory_.GetWeakPtr());
 #endif
   AbortAllInProgressJobs();
   // |this| may be deleted inside AbortAllInProgressJobs().
 }
 
 void HostResolverImpl::OnDNSChanged() {
   DnsConfig dns_config;
   NetworkChangeNotifier::GetDnsConfig(&dns_config);
   if (net_log_) {
     net_log_->AddGlobalEntry(
@@ skipping 26 matching lines @@
   // |this| may be deleted inside AbortAllInProgressJobs().
   if (self)
     TryServingAllJobsFromHosts();
 }
 
 bool HostResolverImpl::HaveDnsConfig() const {
   return (dns_client_.get() != NULL) && (dns_client_->GetConfig() != NULL);
 }
 
 }  // namespace net