Relands r58007.

Fixes the problem with the flaky unittest that happened due to message loop posting race conditions.
BUG=53386
TEST=HostResolverImplTest.AbortOnIPAddressChanged,HostResolverImplTest.OnlyAbortExistingRequestsOnIPAddressChange

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

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

4 files changed, 152 insertions, 25 deletions

diff --git a/chrome/browser/net/passive_log_collector.cc b/chrome/browser/net/passive_log_collector.cc
index 3dd8ccd..c62c3321 100644
--- a/chrome/browser/net/passive_log_collector.cc
+++ b/chrome/browser/net/passive_log_collector.cc
@@ -251,6 +251,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
@@ -534,4 +536,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..78b2087 100644
--- a/net/base/host_resolver_impl_unittest.cc
+++ b/net/base/host_resolver_impl_unittest.cc
@@ -255,7 +255,7 @@ class HostResolverImplTest : public testing::Test {
 };
 
 TEST_F(HostResolverImplTest, SynchronousLookup) {
-  AddressList adrlist;
+  AddressList addrlist;
   const int kPortnum = 80;
 
   scoped_refptr<RuleBasedHostResolverProc> resolver_proc =
@@ -267,7 +267,7 @@ TEST_F(HostResolverImplTest, SynchronousLookup) {
 
   HostResolver::RequestInfo info("just.testing", kPortnum);
   CapturingBoundNetLog log(CapturingNetLog::kUnbounded);
-  int err = host_resolver->Resolve(info, &adrlist, NULL, NULL, log.bound());
+  int err = host_resolver->Resolve(info, &addrlist, NULL, NULL, log.bound());
   EXPECT_EQ(OK, err);
 
   EXPECT_EQ(2u, log.entries().size());
@@ -276,7 +276,7 @@ TEST_F(HostResolverImplTest, SynchronousLookup) {
   EXPECT_TRUE(LogContainsEndEvent(
       log.entries(), 1, NetLog::TYPE_HOST_RESOLVER_IMPL));
 
-  const struct addrinfo* ainfo = adrlist.head();
+  const struct addrinfo* ainfo = addrlist.head();
   EXPECT_EQ(static_cast<addrinfo*>(NULL), ainfo->ai_next);
   EXPECT_EQ(sizeof(struct sockaddr_in), ainfo->ai_addrlen);
 
@@ -287,7 +287,7 @@ TEST_F(HostResolverImplTest, SynchronousLookup) {
 }
 
 TEST_F(HostResolverImplTest, AsynchronousLookup) {
-  AddressList adrlist;
+  AddressList addrlist;
   const int kPortnum = 80;
 
   scoped_refptr<RuleBasedHostResolverProc> resolver_proc =
@@ -299,7 +299,7 @@ TEST_F(HostResolverImplTest, AsynchronousLookup) {
 
   HostResolver::RequestInfo info("just.testing", kPortnum);
   CapturingBoundNetLog log(CapturingNetLog::kUnbounded);
-  int err = host_resolver->Resolve(info, &adrlist, &callback_, NULL,
+  int err = host_resolver->Resolve(info, &addrlist, &callback_, NULL,
                                    log.bound());
   EXPECT_EQ(ERR_IO_PENDING, err);
 
@@ -316,7 +316,7 @@ TEST_F(HostResolverImplTest, AsynchronousLookup) {
   EXPECT_TRUE(LogContainsEndEvent(
       log.entries(), 1, NetLog::TYPE_HOST_RESOLVER_IMPL));
 
-  const struct addrinfo* ainfo = adrlist.head();
+  const struct addrinfo* ainfo = addrlist.head();
   EXPECT_EQ(static_cast<addrinfo*>(NULL), ainfo->ai_next);
   EXPECT_EQ(sizeof(struct sockaddr_in), ainfo->ai_addrlen);
 
@@ -338,11 +338,11 @@ TEST_F(HostResolverImplTest, CanceledAsynchronousLookup) {
                              CreateDefaultCache(),
                              kMaxJobs,
                              &net_log));
-    AddressList adrlist;
+    AddressList addrlist;
     const int kPortnum = 80;
 
     HostResolver::RequestInfo info("just.testing", kPortnum);
-    int err = host_resolver->Resolve(info, &adrlist, &callback_, NULL,
+    int err = host_resolver->Resolve(info, &addrlist, &callback_, NULL,
                                      log.bound());
     EXPECT_EQ(ERR_IO_PENDING, err);
 
@@ -395,13 +395,13 @@ TEST_F(HostResolverImplTest, NumericIPv4Address) {
 
   scoped_refptr<HostResolver> host_resolver(
       CreateHostResolverImpl(resolver_proc));
-  AddressList adrlist;
+  AddressList addrlist;
   const int kPortnum = 5555;
   HostResolver::RequestInfo info("127.1.2.3", kPortnum);
-  int err = host_resolver->Resolve(info, &adrlist, NULL, NULL, BoundNetLog());
+  int err = host_resolver->Resolve(info, &addrlist, NULL, NULL, BoundNetLog());
   EXPECT_EQ(OK, err);
 
-  const struct addrinfo* ainfo = adrlist.head();
+  const struct addrinfo* ainfo = addrlist.head();
   EXPECT_EQ(static_cast<addrinfo*>(NULL), ainfo->ai_next);
   EXPECT_EQ(sizeof(struct sockaddr_in), ainfo->ai_addrlen);
 
@@ -420,13 +420,13 @@ TEST_F(HostResolverImplTest, NumericIPv6Address) {
   // the caller should have removed them.
   scoped_refptr<HostResolver> host_resolver(
       CreateHostResolverImpl(resolver_proc));
-  AddressList adrlist;
+  AddressList addrlist;
   const int kPortnum = 5555;
   HostResolver::RequestInfo info("2001:db8::1", kPortnum);
-  int err = host_resolver->Resolve(info, &adrlist, NULL, NULL, BoundNetLog());
+  int err = host_resolver->Resolve(info, &addrlist, NULL, NULL, BoundNetLog());
   EXPECT_EQ(OK, err);
 
-  const struct addrinfo* ainfo = adrlist.head();
+  const struct addrinfo* ainfo = addrlist.head();
   EXPECT_EQ(static_cast<addrinfo*>(NULL), ainfo->ai_next);
   EXPECT_EQ(sizeof(struct sockaddr_in6), ainfo->ai_addrlen);
 
@@ -450,10 +450,10 @@ TEST_F(HostResolverImplTest, EmptyHost) {
 
   scoped_refptr<HostResolver> host_resolver(
       CreateHostResolverImpl(resolver_proc));
-  AddressList adrlist;
+  AddressList addrlist;
   const int kPortnum = 5555;
   HostResolver::RequestInfo info("", kPortnum);
-  int err = host_resolver->Resolve(info, &adrlist, NULL, NULL, BoundNetLog());
+  int err = host_resolver->Resolve(info, &addrlist, NULL, NULL, BoundNetLog());
   EXPECT_EQ(ERR_NAME_NOT_RESOLVED, err);
 }
 
@@ -464,11 +464,11 @@ TEST_F(HostResolverImplTest, LongHost) {
 
   scoped_refptr<HostResolver> host_resolver(
       CreateHostResolverImpl(resolver_proc));
-  AddressList adrlist;
+  AddressList addrlist;
   const int kPortnum = 5555;
   std::string hostname(4097, 'a');
   HostResolver::RequestInfo info(hostname, kPortnum);
-  int err = host_resolver->Resolve(info, &adrlist, NULL, NULL, BoundNetLog());
+  int err = host_resolver->Resolve(info, &addrlist, NULL, NULL, BoundNetLog());
   EXPECT_EQ(ERR_NAME_NOT_RESOLVED, err);
 }
 
@@ -1103,6 +1103,90 @@ 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<WaitingHostResolverProc> resolver_proc =
+      new WaitingHostResolverProc(NULL);
+  HostCache* cache = CreateDefaultCache();
+  scoped_refptr<HostResolver> host_resolver(
+      new HostResolverImpl(resolver_proc, cache, 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.
+  resolver_proc->Signal();
+
+  EXPECT_EQ(ERR_ABORTED, callback.WaitForResult());
+  EXPECT_EQ(0u, cache->size());
+}
+
+class ResolveWithinCallback : public CallbackRunner< Tuple1<int> > {
+ public:
+  ResolveWithinCallback(
+      const scoped_refptr<MockHostResolver>& host_resolver,
+      const HostResolver::RequestInfo& info)
+      : host_resolver_(host_resolver),
+        info_(info) {
+    DCHECK(host_resolver.get());
+  }
+
+  virtual void RunWithParams(const Tuple1<int>& params) {
+    // Ditch the WaitingHostResolverProc so that the subsequent request
+    // succeeds.
+    host_resolver_->Reset(NULL);
+    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<MockHostResolver> host_resolver_;
+  const HostResolver::RequestInfo info_;
+  AddressList addrlist_;
+  TestCompletionCallback callback_;
+  TestCompletionCallback nested_callback_;
+};
+
+TEST_F(HostResolverImplTest, OnlyAbortExistingRequestsOnIPAddressChange) {
+  scoped_refptr<WaitingHostResolverProc> resolver_proc =
+      new WaitingHostResolverProc(NULL);
+  scoped_refptr<MockHostResolver> host_resolver(new MockHostResolver());
+  host_resolver->Reset(resolver_proc);
+
+  // 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());
+  resolver_proc->Signal();
+  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) {