[net] Ensure aborted HostResolverImpl::Jobs release slots in the dispatcher.

Also re-introduces a missing life check in case a request callback deletes the HostResolver.

R=eroman@chromium.org,cbentzel@chromium.org
BUG=115399
TEST=./net_unittest --gtest_filter=HostResolverImplTest.CanceledRequestsReleaseJobSlots


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

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

net/base/host_resolver_impl.cc

@@ -1076,8 +1076,6 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job {
         NetLog::TYPE_HOST_RESOLVER_IMPL_JOB,
         make_scoped_refptr(new JobCreationParameters(
             key_.hostname, request_net_log.source())));
-
-    handle_ = resolver_->dispatcher_.Add(this, priority);
   }
 
   virtual ~Job() {
@@ -1091,7 +1089,7 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job {
       net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB,
                                         ERR_ABORTED);
     } else if (is_queued()) {
-      // This Job was cancelled without running.
+      // |resolver_| was destroyed without running this Job.
       // TODO(szym): is there's any benefit in having this distinction?
       net_log_.AddEvent(NetLog::TYPE_CANCELLED, NULL);
       net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB, NULL);
@@ -1111,6 +1109,10 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job {
     STLDeleteElements(&requests_);
   }
 
+  void Schedule() {
+    handle_ = resolver_->dispatcher_.Add(this, priority());
+  }
+
   RequestPriority priority() const {
     return priority_tracker_.highest_priority();
   }
@@ -1176,6 +1178,8 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job {
       } else {
         resolver_->dispatcher_.Cancel(handle_);
         handle_.Reset();
+        net_log_.AddEvent(NetLog::TYPE_CANCELLED, NULL);
+        net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB, NULL);
       }
     }
   }
@@ -1487,6 +1491,7 @@ int HostResolverImpl::Resolve(const RequestInfo& info,
   if (jobit == jobs_.end()) {
     // Create new Job.
     job = new Job(this, key, request_net_log, info.priority());
+    job->Schedule();
 
     // Check for queue overflow.
     if (dispatcher_.num_queued_jobs() > max_queued_jobs_) {
@@ -1572,8 +1577,10 @@ void HostResolverImpl::CancelRequest(RequestHandle req_handle) {
     // that Request could not have been cancelled, so job->num_active_requests()
     // could not be 0. Therefore, we are not in Job::CompleteRequests().
     RemoveJob(job);
-    if (job->is_running())
+    if (job->is_running()) {
+      dispatcher_.OnJobFinished();
       job->Abort();
+    }
     delete job;
   }
 }
@@ -1709,7 +1716,6 @@ HostResolverImpl::Key HostResolverImpl::GetEffectiveKeyForRequest(
 }
 
 void HostResolverImpl::AbortAllInProgressJobs() {
-  base::WeakPtr<HostResolverImpl> self = AsWeakPtr();
   // In Abort, a Request callback could spawn new Jobs with matching keys, so
   // first collect and remove all running jobs from |jobs_|.
   std::vector<Job*> jobs_to_abort;
@@ -1724,10 +1730,16 @@ void HostResolverImpl::AbortAllInProgressJobs() {
     }
   }
 
+  // Check if no dispatcher slots leaked out.
+  DCHECK_EQ(dispatcher_.num_running_jobs(), jobs_to_abort.size());
+
+  // Life check to bail once |this| is deleted.
+  base::WeakPtr<HostResolverImpl> self = AsWeakPtr();
+
   // Then Abort them and dispatch new Jobs.
-  for (size_t i = 0; i < jobs_to_abort.size(); ++i) {
-    jobs_to_abort[i]->Abort();
+  for (size_t i = 0; self && i < jobs_to_abort.size(); ++i) {
     dispatcher_.OnJobFinished();
+    jobs_to_abort[i]->Abort();
   }
   STLDeleteElements(&jobs_to_abort);
 }

net/base/host_resolver_impl_unittest.cc

@@ -15,6 +15,7 @@
 #include "base/stringprintf.h"
 #include "base/synchronization/condition_variable.h"
 #include "base/synchronization/lock.h"
+#include "base/test/test_timeouts.h"
 #include "base/time.h"
 #include "net/base/address_list.h"
 #include "net/base/completion_callback.h"
@@ -90,6 +91,45 @@ HostResolver::RequestInfo CreateResolverRequestForAddressFamily(
   return info;
 }
 
+// Using WaitingHostResolverProc you can simulate very long lookups.
+class WaitingHostResolverProc : public HostResolverProc {
+ public:
+  explicit WaitingHostResolverProc(HostResolverProc* previous)
+      : HostResolverProc(previous),
+        is_waiting_(false, false),
+        is_signaled_(false, false) {}
+
+  // Waits until a call to |Resolve| is blocked. It is recommended to always
+  // |Wait| before |Signal|, and required if issuing a series of two or more
+  // calls to |Signal|, because |WaitableEvent| does not count the number of
+  // signals.
+  void Wait() {
+    is_waiting_.Wait();
+  }
+
+  // Signals a waiting call to |Resolve|.
+  void Signal() {
+    is_signaled_.Signal();
+  }
+
+  // HostResolverProc methods:
+  virtual int Resolve(const std::string& host,
+                      AddressFamily address_family,
+                      HostResolverFlags host_resolver_flags,
+                      AddressList* addrlist,
+                      int* os_error) OVERRIDE {
+    is_waiting_.Signal();
+    is_signaled_.Wait();
+    return ResolveUsingPrevious(host, address_family, host_resolver_flags,
+                                addrlist, os_error);
+  }
+
+ private:
+  virtual ~WaitingHostResolverProc() {}
+  base::WaitableEvent is_waiting_;
+  base::WaitableEvent is_signaled_;
+};
+
 // A variant of WaitingHostResolverProc that pushes each host mapped into a
 // list.
 // (and uses a manual-reset event rather than auto-reset).
@@ -143,6 +183,72 @@ class CapturingHostResolverProc : public HostResolverProc {
   base::WaitableEvent event_;
 };
 
+// A variant of WaitingHostResolverProc which waits for a specific number of
+// requests.
+class CountingHostResolverProc : public HostResolverProc {
+ public:
+  explicit CountingHostResolverProc(HostResolverProc* previous)
+      : HostResolverProc(previous),
+        num_requests_waiting_(0),
+        num_slots_available_(0),
+        requests_waiting_(&lock_),
+        slots_available_(&lock_) {}
+
+  // Waits until |count| calls to |Resolve| are blocked. Returns false when
+  // timed out.
+  bool WaitFor(unsigned count) {
+    base::AutoLock lock(lock_);
+    base::Time start_time = base::Time::Now();
+    while (num_requests_waiting_ < count) {
+      requests_waiting_.TimedWait(TestTimeouts::action_timeout());
+      if (base::Time::Now() > start_time + TestTimeouts::action_timeout())
+        return false;
+    }
+    return true;
+  }
+
+  // Signals |count| waiting calls to |Resolve|. First come first served.
+  void SignalMultiple(unsigned count) {
+    base::AutoLock lock(lock_);
+    num_slots_available_ += count;
+    slots_available_.Broadcast();
+  }
+
+  // Signals all waiting calls to |Resolve|. Beware of races.
+  void SignalAll() {
+    base::AutoLock lock(lock_);
+    num_slots_available_ += num_requests_waiting_;
+    slots_available_.Broadcast();
+  }
+
+  // HostResolverProc methods:
+  virtual int Resolve(const std::string& host,
+                      AddressFamily address_family,
+                      HostResolverFlags host_resolver_flags,
+                      AddressList* addrlist,
+                      int* os_error) OVERRIDE {
+    {
+      base::AutoLock lock(lock_);
+      ++num_requests_waiting_;
+      requests_waiting_.Broadcast();
+      while (!num_slots_available_)
+        slots_available_.Wait();
+      --num_slots_available_;
+      --num_requests_waiting_;
+    }
+    return ResolveUsingPrevious(host, address_family, host_resolver_flags,
+                                addrlist, os_error);
+  }
+
+ private:
+  virtual ~CountingHostResolverProc() {}
+  unsigned num_requests_waiting_;
+  unsigned num_slots_available_;
+  base::Lock lock_;
+  base::ConditionVariable requests_waiting_;
+  base::ConditionVariable slots_available_;
+};
+
 // This resolver function creates an IPv4 address, whose numeral value
 // describes a hash of the requested hostname, and the value of the requested
 // address_family.
@@ -467,52 +573,6 @@ TEST_F(HostResolverImplTest, FailedAsynchronousLookup) {
   EXPECT_EQ(ERR_DNS_CACHE_MISS, err);
 }
 
-// Using WaitingHostResolverProc you can simulate very long lookups.
-class WaitingHostResolverProc : public HostResolverProc {
- public:
-  explicit WaitingHostResolverProc(HostResolverProc* previous)
-      : HostResolverProc(previous),
-        is_waiting_(false, false),
-        is_signaled_(false, false) {}
-
-  // If |manual_reset| is true, once Signalled, it will let all Resolve calls
-  // proceed.
-  WaitingHostResolverProc(HostResolverProc* previous, bool manual_reset)
-      : HostResolverProc(previous),
-        is_waiting_(false, false),
-        is_signaled_(manual_reset, false) {}
-
-  // Waits until a call to |Resolve| is blocked. It is recommended to always
-  // |Wait| before |Signal|, and required if issuing a series of two or more
-  // calls to |Signal|, because |WaitableEvent| does not count the number of
-  // signals.
-  void Wait() {
-    is_waiting_.Wait();
-  }
-
-  // Signals a waiting call to |Resolve|.
-  void Signal() {
-    is_signaled_.Signal();
-  }
-
-  // HostResolverProc methods:
-  virtual int Resolve(const std::string& host,
-                      AddressFamily address_family,
-                      HostResolverFlags host_resolver_flags,
-                      AddressList* addrlist,
-                      int* os_error) OVERRIDE {
-    is_waiting_.Signal();
-    is_signaled_.Wait();
-    return ResolveUsingPrevious(host, address_family, host_resolver_flags,
-                                addrlist, os_error);
-  }
-
- private:
-  virtual ~WaitingHostResolverProc() {}
-  base::WaitableEvent is_waiting_;
-  base::WaitableEvent is_signaled_;
-};
-
 TEST_F(HostResolverImplTest, AbortedAsynchronousLookup) {
   scoped_refptr<WaitingHostResolverProc> resolver_proc(
       new WaitingHostResolverProc(NULL));
@@ -559,8 +619,8 @@ TEST_F(HostResolverImplTest, AbortedAsynchronousLookup) {
       NetLog::TYPE_HOST_RESOLVER_IMPL_JOB,
       NetLog::PHASE_BEGIN);
   pos = ExpectLogContainsSomewhereAfter(net_log_entries, pos + 1,
-        NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_TASK,
-        NetLog::PHASE_BEGIN);
+      NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_TASK,
+      NetLog::PHASE_BEGIN);
 
   // The Request needs to be cancelled. (The Job is "aborted".)
   // Don't care about order in which Request, Job and ProcTask end.
@@ -808,6 +868,60 @@ TEST_F(HostResolverImplTest, CancelMultipleRequests) {
   MessageLoop::current()->Run();
 }
 
+// Helper class used by HostResolverImplTest.CanceledRequestsReleaseJobSlots.
+class CountingDelegate : public ResolveRequest::Delegate {
+ public:
+  CountingDelegate() : num_completions_(0) {}
+
+  virtual void OnCompleted(ResolveRequest* resolve) OVERRIDE {
+    ++num_completions_;
+    MessageLoop::current()->Quit();
+  }
+
+  unsigned num_completions() const { return num_completions_; }
+
+ private:
+  unsigned num_completions_;
+};
+
+TEST_F(HostResolverImplTest, CanceledRequestsReleaseJobSlots) {
+  scoped_refptr<CountingHostResolverProc> resolver_proc(
+      new CountingHostResolverProc(NULL));
+
+  scoped_ptr<HostResolver> host_resolver(
+      CreateHostResolverImpl(resolver_proc));
+
+  CountingDelegate delegate;
+  std::vector<ResolveRequest*> requests;
+
+  // Fill up the dispatcher and queue.
+  for (unsigned i = 0; i < kMaxJobs + 1; ++i) {
+    std::string hostname = "a_";
+    hostname[1] = 'a' + i;
+    requests.push_back(new ResolveRequest(host_resolver.get(), hostname, 80,
+                                          &delegate));
+    requests.push_back(new ResolveRequest(host_resolver.get(), hostname, 81,
+                                          &delegate));
+  }
+
+  EXPECT_TRUE(resolver_proc->WaitFor(kMaxJobs));
+
+  // Cancel all but last two.
+  for (unsigned i = 0; i < requests.size() - 2; ++i) {
+    requests[i]->Cancel();
+  }
+
+  EXPECT_TRUE(resolver_proc->WaitFor(kMaxJobs + 1));
+  EXPECT_EQ(0u, delegate.num_completions());
+
+  resolver_proc->SignalAll();
+
+  while (delegate.num_completions() < 2)
+    MessageLoop::current()->Run();
+
+  MessageLoop::current()->AssertIdle();
+}
+
 // Helper class used by HostResolverImplTest.CancelWithinCallback.
 class CancelWithinCallbackVerifier : public ResolveRequest::Delegate {
  public:
@@ -1185,9 +1299,8 @@ class StartWithinAbortedCallbackVerifier : public ResolveRequest::Delegate {
 // Tests that a new Request made from the callback of a previously aborted one
 // will not be aborted.
 TEST_F(HostResolverImplTest, AbortOnlyExistingRequestsOnIPAddressChange) {
-  // Setting |manual_reset| to true so that Signal unblocks all calls.
-  scoped_refptr<WaitingHostResolverProc> resolver_proc(
-      new WaitingHostResolverProc(CreateCatchAllHostResolverProc(), true));
+  scoped_refptr<CountingHostResolverProc> resolver_proc(
+      new CountingHostResolverProc(CreateCatchAllHostResolverProc()));
   scoped_ptr<HostResolver> host_resolver(CreateHostResolverImpl(resolver_proc));
 
   StartWithinAbortedCallbackVerifier verifier1("zzz");
@@ -1198,8 +1311,8 @@ TEST_F(HostResolverImplTest, AbortOnlyExistingRequestsOnIPAddressChange) {
   ResolveRequest req2(host_resolver.get(), "eee", 80, &verifier2);
   ResolveRequest req3(host_resolver.get(), "ccc", 90, &verifier3);
   // The jobs start immediately.
-  // Wait until at least one is blocked.
-  resolver_proc->Wait();
+  // Wait until all are blocked;
+  resolver_proc->WaitFor(3u);
   // Trigger an IP address change.
   NetworkChangeNotifier::NotifyObserversOfIPAddressChangeForTests();
   // This should abort all running jobs.
@@ -1208,7 +1321,7 @@ TEST_F(HostResolverImplTest, AbortOnlyExistingRequestsOnIPAddressChange) {
   EXPECT_EQ(ERR_ABORTED, req2.result());
   EXPECT_EQ(ERR_ABORTED, req3.result());
   // Unblock all calls to proc.
-  resolver_proc->Signal();
+  resolver_proc->SignalMultiple(6u);
   // Run until the re-started requests finish.
   EXPECT_EQ(OK, verifier1.WaitUntilDone());
   EXPECT_EQ(OK, verifier2.WaitUntilDone());