Abort host resolution requests with ERR_ABORTED on ip address change.

BUG=53386

Review URL: http://codereview.chromium.org/3277002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@58007 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 127 insertions, 7 deletions

diff --git a/chrome/browser/net/passive_log_collector.cc b/chrome/browser/net/passive_log_collector.cc
index adc6500..3181ad9 100644
--- a/chrome/browser/net/passive_log_collector.cc
+++ b/chrome/browser/net/passive_log_collector.cc
@@ -250,6 +250,8 @@ void PassiveLogCollector::SourceTracker::AddToDeletionQueue(
   DCHECK_GE(sources_.find(source_id)->second.reference_count, 0);
   DCHECK_LE(deletion_queue_.size(), max_graveyard_size_);
 
+  DCHECK(std::find(deletion_queue_.begin(), deletion_queue_.end(),
+                   source_id) == deletion_queue_.end());
   deletion_queue_.push_back(source_id);
 
   // After the deletion queue has reached its maximum size, start
@@ -533,4 +535,3 @@ PassiveLogCollector::DNSJobTracker::DoAddEntry(const Entry& entry,
     return ACTION_NONE;
   }
 }
-
diff --git a/net/base/host_resolver_impl.cc b/net/base/host_resolver_impl.cc
index f4f7bbd..b93ae31 100644
--- a/net/base/host_resolver_impl.cc
+++ b/net/base/host_resolver_impl.cc
@@ -258,7 +258,9 @@ class HostResolverImpl::Request {
   void OnComplete(int error, const AddressList& addrlist) {
     if (error == OK)
       addresses_->SetFrom(addrlist, port());
-    callback_->Run(error);
+    CompletionCallback* callback = callback_;
+    MarkAsCancelled();
+    callback->Run(error);
   }
 
   int port() const {
@@ -840,8 +842,7 @@ HostResolverImpl::~HostResolverImpl() {
   // requests, which will also be cancelled.
   DiscardIPv6ProbeJob();
 
-  for (JobMap::iterator it = jobs_.begin(); it != jobs_.end(); ++it)
-    it->second->Cancel();
+  CancelAllJobs();
 
   // In case we are being deleted during the processing of a callback.
   if (cur_completing_job_)
@@ -1035,9 +1036,7 @@ void HostResolverImpl::Shutdown() {
   DCHECK(CalledOnValidThread());
 
   // Cancel the outstanding jobs.
-  for (JobMap::iterator it = jobs_.begin(); it != jobs_.end(); ++it)
-    it->second->Cancel();
-  jobs_.clear();
+  CancelAllJobs();
   DiscardIPv6ProbeJob();
 
   shutdown_ = true;
@@ -1090,6 +1089,18 @@ void HostResolverImpl::OnJobComplete(Job* job,
   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_);
@@ -1199,6 +1210,7 @@ void HostResolverImpl::OnIPAddressChanged() {
     ipv6_probe_job_ = new IPv6ProbeJob(this);
     ipv6_probe_job_->Start();
   }
+  AbortAllJobs();
 #if defined(OS_LINUX)
   if (HaveOnlyLoopbackAddresses()) {
     additional_resolver_flags_ |= HOST_RESOLVER_LOOPBACK_ONLY;
@@ -1317,4 +1329,20 @@ int HostResolverImpl::EnqueueRequest(JobPool* pool, Request* req) {
   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::AbortAllJobs() {
+  JobMap jobs;
+  jobs.swap(jobs_);
+  for (JobMap::iterator it = jobs.begin(); it != jobs.end(); ++it) {
+    AbortJob(it->second);
+    it->second->Cancel();
+  }
+}
+
 }  // namespace net
diff --git a/net/base/host_resolver_impl.h b/net/base/host_resolver_impl.h
index a06a478..58d8598 100644
--- a/net/base/host_resolver_impl.h
+++ b/net/base/host_resolver_impl.h
@@ -156,6 +156,14 @@ class HostResolverImpl : public HostResolver,
   void OnJobComplete(Job* job, int net_error, int os_error,
                      const AddressList& addrlist);
 
+  // Aborts |job|.  Same as OnJobComplete() except does not remove |job|
+  // from |jobs_| and does not cache the result (ERR_ABORTED).
+  void AbortJob(Job* job);
+
+  // Used by both OnJobComplete() and AbortJob();
+  void OnJobCompleteInternal(Job* job, int net_error, int os_error,
+                             const AddressList& addrlist);
+
   // Called when a request has just been started.
   void OnStartRequest(const BoundNetLog& source_net_log,
                       const BoundNetLog& request_net_log,
@@ -211,6 +219,12 @@ class HostResolverImpl : public HostResolver,
   // Adds a pending request |req| to |pool|.
   int EnqueueRequest(JobPool* pool, Request* req);
 
+  // Cancels all jobs.
+  void CancelAllJobs();
+
+  // Aborts all jobs.
+  void AbortAllJobs();
+
   // Cache of host resolution results.
   scoped_ptr<HostCache> cache_;
 
diff --git a/net/base/host_resolver_impl_unittest.cc b/net/base/host_resolver_impl_unittest.cc
index a78d1c0..8cd1dd5 100644
--- a/net/base/host_resolver_impl_unittest.cc
+++ b/net/base/host_resolver_impl_unittest.cc
@@ -1103,6 +1103,83 @@ TEST_F(HostResolverImplTest, FlushCacheOnIPAddressChange) {
   EXPECT_EQ(OK, callback.WaitForResult());
 }
 
+// Test that IP address changes send ERR_ABORTED to pending requests.
+TEST_F(HostResolverImplTest, AbortOnIPAddressChanged) {
+  scoped_refptr<HostResolver> host_resolver(
+      new HostResolverImpl(NULL, CreateDefaultCache(), kMaxJobs, NULL));
+
+  // Resolve "host1".
+  HostResolver::RequestInfo info("host1", 70);
+  TestCompletionCallback callback;
+  AddressList addrlist;
+  int rv = host_resolver->Resolve(info, &addrlist, &callback, NULL,
+                                  BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  // Triggering an IP address change.
+  NetworkChangeNotifier::NotifyObserversOfIPAddressChangeForTests();
+  MessageLoop::current()->RunAllPending();  // Notification happens async.
+
+  EXPECT_EQ(ERR_ABORTED, callback.WaitForResult());
+
+  // Resolve "host1" again, should not be cached, so should be async.
+  rv = host_resolver->Resolve(info, &addrlist, &callback, NULL, BoundNetLog());
+  ASSERT_EQ(ERR_IO_PENDING, rv);
+}
+
+class ResolveWithinCallback : public CallbackRunner< Tuple1<int> > {
+ public:
+  ResolveWithinCallback(
+      const scoped_refptr<HostResolver>& host_resolver,
+      const HostResolver::RequestInfo& info)
+      : host_resolver_(host_resolver),
+        info_(info) {
+    DCHECK(host_resolver.get());
+  }
+
+  virtual void RunWithParams(const Tuple1<int>& params) {
+    callback_.RunWithParams(params);
+    EXPECT_EQ(ERR_IO_PENDING,
+              host_resolver_->Resolve(info_, &addrlist_, &nested_callback_,
+                                      NULL, BoundNetLog()));
+  }
+
+  int WaitForResult() {
+    return callback_.WaitForResult();
+  }
+
+  int WaitForNestedResult() {
+    return nested_callback_.WaitForResult();
+  }
+
+ private:
+  const scoped_refptr<HostResolver> host_resolver_;
+  const HostResolver::RequestInfo info_;
+  AddressList addrlist_;
+  TestCompletionCallback callback_;
+  TestCompletionCallback nested_callback_;
+};
+
+TEST_F(HostResolverImplTest, OnlyAbortExistingRequestsOnIPAddressChange) {
+  scoped_refptr<HostResolver> host_resolver(
+      new HostResolverImpl(NULL, CreateDefaultCache(), kMaxJobs, NULL));
+
+  // Resolve "host1".
+  HostResolver::RequestInfo info("host1", 70);
+  ResolveWithinCallback callback(host_resolver, info);
+  AddressList addrlist;
+  int rv = host_resolver->Resolve(info, &addrlist, &callback, NULL,
+                                  BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  // Triggering an IP address change.
+  NetworkChangeNotifier::NotifyObserversOfIPAddressChangeForTests();
+  MessageLoop::current()->RunAllPending();  // Notification happens async.
+
+  EXPECT_EQ(ERR_ABORTED, callback.WaitForResult());
+  EXPECT_EQ(OK, callback.WaitForNestedResult());
+}
+
 // Tests that when the maximum threads is set to 1, requests are dequeued
 // in order of priority.
 TEST_F(HostResolverImplTest, HigherPriorityRequestsStartedFirst) {