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

net/base/host_resolver_impl.cc

Index: net/base/host_resolver_impl.cc
diff --git a/net/base/host_resolver_impl.cc b/net/base/host_resolver_impl.cc
index 89d657b1828755d732d51f5507cbc7edd285e6b9..224a0e9b54ad273e579d2d5bcae1f7c4f5b29c20 100644
--- a/net/base/host_resolver_impl.cc
+++ b/net/base/host_resolver_impl.cc
@@ -236,42 +236,6 @@ AddressList EnsurePortOnAddressList(const AddressList& list, uint16 port) {
   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,
@@ -918,87 +882,95 @@ class HostResolverImpl::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_;
+  void OnProbeComplete() {
+    net_log_.EndEvent(NetLog::TYPE_IPV6_PROBE_RUNNING,
+                      base::Bind(&IPv6SupportResult::ToNetLogValue,
+                                 base::Unretained(&result_)));
+    if (!resolver_)
+      return;
+    resolver_->IPv6ProbeSetDefaultAddressFamily(
+        result_.ipv6_supported ? ADDRESS_FAMILY_UNSPECIFIED
+                                      : ADDRESS_FAMILY_IPV4);
   }
 
-  // Run on worker thread.
-  void DoProbe() {
-    // Do actual testing on this thread, as it takes 40-100ms.
-    origin_loop_->PostTask(
+  // Used/set only on origin thread.
+  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(&IPv6ProbeJob::OnProbeComplete, this, TestIPv6Support()));
+        base::Bind(&LoopbackProbeJob::DoProbe, this),
+        base::Bind(&LoopbackProbeJob::OnProbeComplete, this),
+        kIsSlow);
   }
 
-  // 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())
-      return;
+ private:
+  friend class base::RefCountedThreadSafe<HostResolverImpl::LoopbackProbeJob>;
 
-    // Clear |resolver_| so that no cancel event is logged.
-    HostResolverImpl* resolver = resolver_;
-    resolver_ = NULL;
+  virtual ~LoopbackProbeJob() {}
 
-    resolver->IPv6ProbeSetDefaultAddressFamily(
-        support_result.ipv6_supported ? ADDRESS_FAMILY_UNSPECIFIED
-                                      : ADDRESS_FAMILY_IPV4);
+  // Runs on worker thread.
+  void DoProbe() {
+    result_ = HaveOnlyLoopbackAddresses();
   }
 
-  // Used/set only on origin thread.
-  HostResolverImpl* resolver_;
+  void OnProbeComplete() {
+    if (!resolver_)
+      return;
+    resolver_->SetHaveOnlyLoopbackAddresses(result_);
+  }
 
-  // Used to post ourselves onto the origin thread.
-  scoped_refptr<base::MessageLoopProxy> origin_loop_;
+  // Used/set only on origin thread.
+  base::WeakPtr<HostResolverImpl> resolver_;
 
-  BoundNetLog net_log_;
+  bool result_;
 
-  DISALLOW_COPY_AND_ASSIGN(IPv6ProbeJob);
+  DISALLOW_COPY_AND_ASSIGN(LoopbackProbeJob);
 };
 
 //-----------------------------------------------------------------------------
@@ -1181,11 +1153,11 @@ 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),
@@ -1661,6 +1633,7 @@ HostResolverImpl::HostResolverImpl(
       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),
@@ -1679,9 +1652,7 @@ HostResolverImpl::HostResolverImpl(
   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);
@@ -1694,8 +1665,6 @@ HostResolverImpl::HostResolverImpl(
 }
 
 HostResolverImpl::~HostResolverImpl() {
-  DiscardIPv6ProbeJob();
-
   // This will also cancel all outstanding requests.
   STLDeleteValues(&jobs_);
 
@@ -1780,8 +1749,8 @@ int HostResolverImpl::Resolve(const RequestInfo& info,
                                 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.
@@ -1871,9 +1840,9 @@ void HostResolverImpl::CancelRequest(RequestHandle req_handle) {
 
 void HostResolverImpl::SetDefaultAddressFamily(AddressFamily address_family) {
   DCHECK(CalledOnValidThread());
-  ipv6_probe_monitoring_ = false;
-  DiscardIPv6ProbeJob();
   default_address_family_ = address_family;
+  ipv6_probe_monitoring_ = false;
+  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 {
@@ -1884,7 +1853,7 @@ void HostResolverImpl::ProbeIPv6Support() {
   DCHECK(CalledOnValidThread());
   DCHECK(!ipv6_probe_monitoring_);
   ipv6_probe_monitoring_ = true;
-  OnIPAddressChanged();  // Give initial setup call.
+  OnIPAddressChanged();
 }
 
 HostCache* HostResolverImpl::GetHostCache() {
@@ -2002,13 +1971,6 @@ void HostResolverImpl::RemoveJob(Job* 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 ||
@@ -2019,8 +1981,6 @@ void HostResolverImpl::IPv6ProbeSetDefaultAddressFamily(
                 "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) {
@@ -2092,16 +2052,14 @@ void HostResolverImpl::TryServingAllJobsFromHosts() {
 }
 
 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().