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

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 1059 matching lines @@
         had_non_speculative_request_(false),
         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, NULL);
 
     net_log_.BeginEvent(
         NetLog::TYPE_HOST_RESOLVER_IMPL_JOB,
         make_scoped_refptr(new JobCreationParameters(
             key_.hostname, request_net_log.source())));
 
     handle_ = resolver_->dispatcher_.Add(this, priority);

[cbentzel, 2012/03/02 19:45:27]

One concern is that this could call Job::Dispatch immediately, which does
virtual dispatch. Nothing derives from this class so I believe it is safe, but
not sure. 

   }
 
   virtual ~Job() {
     if (is_running()) {
       // |resolver_| was destroyed with this Job still in flight.
       // Clean-up, record in the log, but don't run any callbacks.
       if (is_proc_running()) {
         proc_task_->Cancel();
         proc_task_ = NULL;
       }
@@ skipping 474 matching lines @@
   Job* job = req->job();
   DCHECK(job);
 
   job->CancelRequest(req);
 
   if (job->num_active_requests() == 0) {
     // If we were called from a Requests' callback within Job::CompleteRequests,
     // 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();

[cbentzel, 2012/03/02 19:05:02]

Does the ordering here matter? It's different in the AbortAllJobs case. Perhaps
we should move it into a helper function or make it part of abort?

[szym, 2012/03/02 19:09:30]

I feel that the preferred order is Abort then dispatch. Although arguably since
Jobs cannot complete synchronously it might make more sense to dispatch then
Abort.

Most importantly, Abort could delete |this| and |dispatcher_|. In
AbortAllInProgressJobs we use a self-life check which we are forced to anyway
(since we are in a loop, and it is not safe to Abort a Job once we know we are
deleted, due to dependencies such as NetLog).

I am okay changing it to dispatch then Abort which would let us move the
self-check to the for loop condition.

       job->Abort();
+    }
     delete job;
   }
 }
 
 void HostResolverImpl::SetDefaultAddressFamily(AddressFamily address_family) {
   DCHECK(CalledOnValidThread());
   ipv6_probe_monitoring_ = false;
   DiscardIPv6ProbeJob();
   default_address_family_ = address_family;
 }
@@ skipping 130 matching lines @@
     Job* job = it->second;
     if (job->is_running()) {
       jobs_to_abort.push_back(job);
       jobs_.erase(it++);
     } else {
       DCHECK(job->is_queued());
       ++it;
     }
   }
 
+  // Check if no dispatcher slots leaked out.
+  DCHECK_EQ(dispatcher_.num_running_jobs(), jobs_to_abort.size());
+
   // Then Abort them and dispatch new Jobs.
   for (size_t i = 0; i < jobs_to_abort.size(); ++i) {
     jobs_to_abort[i]->Abort();
+    if (!self)
+      break;  // |this| got destroyed in a request callback.
     dispatcher_.OnJobFinished();
   }
   STLDeleteElements(&jobs_to_abort);
 }
 
 void HostResolverImpl::OnIPAddressChanged() {
   if (cache_.get())
     cache_->clear();
   if (ipv6_probe_monitoring_) {
     DiscardIPv6ProbeJob();
@@ skipping 37 matching lines @@
   } else {
     dns_transaction_factory_.reset();
   }
   // Don't Abort running Jobs unless they were running on DnsTransaction.
   // TODO(szym): This will change once http://crbug.com/114827 is fixed.
   if (had_factory)
     OnDNSChanged(NetworkChangeNotifier::CHANGE_DNS_SETTINGS);
 }
 
 }  // namespace net