Change HostResolver::Resolve() to take an std::unique_ptr<Request>* rather than a RequestHandle*

net/dns/host_resolver_impl.cc

Index: net/dns/host_resolver_impl.cc
diff --git a/net/dns/host_resolver_impl.cc b/net/dns/host_resolver_impl.cc
index 2345dad8c741a17ab5ca385cf92380be2f90ef8c..527eaad1f2e0686d7a840b4a59e26d4f1a9767dd 100644
--- a/net/dns/host_resolver_impl.cc
+++ b/net/dns/host_resolver_impl.cc
@@ -18,6 +18,7 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
+#include "base/callback_helpers.h"
 #include "base/compiler_specific.h"
 #include "base/debug/debugger.h"
 #include "base/debug/leak_annotations.h"
@@ -525,46 +526,41 @@ const unsigned HostResolverImpl::kMaximumDnsFailures = 16;
 // Holds the data for a request that could not be completed synchronously.
 // It is owned by a Job. Canceled Requests are only marked as canceled rather
 // than removed from the Job's |requests_| list.
-class HostResolverImpl::Request {
+class HostResolverImpl::RequestImpl : public HostResolver::Request {
  public:
-  Request(const BoundNetLog& source_net_log,
-          const RequestInfo& info,
-          RequestPriority priority,
-          const CompletionCallback& callback,
-          AddressList* addresses)
+  RequestImpl(const BoundNetLog& source_net_log,
+              const RequestInfo& info,
+              RequestPriority priority,
+              const CompletionCallback& callback,
+              AddressList* addresses,
+              Job* job)
       : source_net_log_(source_net_log),
         info_(info),
         priority_(priority),
-        job_(nullptr),
+        job_(job),
         callback_(callback),
         addresses_(addresses),
         request_time_(base::TimeTicks::Now()) {}
 
-  // Mark the request as canceled.
-  void MarkAsCanceled() {
+  ~RequestImpl() override;
+
+  void ChangeRequestPriority(RequestPriority priority) override;
+
+  void OnJobCancelled(Job* job) {
+    DCHECK_EQ(job_, job);
     job_ = nullptr;
     addresses_ = nullptr;
     callback_.Reset();
   }
 
-  bool was_canceled() const {
-    return callback_.is_null();
-  }
-
-  void set_job(Job* job) {
-    DCHECK(job);
-    // Identify which job the request is waiting on.
-    job_ = job;
-  }
-
   // Prepare final AddressList and call completion callback.
-  void OnComplete(int error, const AddressList& addr_list) {
-    DCHECK(!was_canceled());
+  void OnJobCompleted(Job* job, int error, const AddressList& addr_list) {
+    DCHECK_EQ(job_, job);
     if (error == OK)
       *addresses_ = EnsurePortOnAddressList(addr_list, info_.port());
-    CompletionCallback callback = callback_;
-    MarkAsCanceled();
-    callback.Run(error);
+    job_ = nullptr;
+    addresses_ = nullptr;
+    base::ResetAndReturn(&callback_).Run(error);
   }
 
   Job* job() const {
@@ -604,7 +600,7 @@ class HostResolverImpl::Request {
 
   const base::TimeTicks request_time_;
 
-  DISALLOW_COPY_AND_ASSIGN(Request);
+  DISALLOW_COPY_AND_ASSIGN(RequestImpl);
 };
 
 //------------------------------------------------------------------------------
@@ -1321,12 +1317,14 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
     }
     // else CompleteRequests logged EndEvent.
 
-    // Log any remaining Requests as cancelled.
-    for (const std::unique_ptr<Request>& req : requests_) {
-      if (req->was_canceled())
-        continue;
-      DCHECK_EQ(this, req->job());
-      LogCancelRequest(req->source_net_log(), req->info());
+    if (!requests_.empty()) {
+      // Log any remaining Requests as cancelled.
+      for (RequestImpl* req : requests_) {
+        DCHECK_EQ(this, req->job());
+        req->OnJobCancelled(this);
+        LogCancelRequest(req->source_net_log(), req->info());
+      }
+      requests_.clear();
     }
   }
 
@@ -1349,37 +1347,34 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
     }
   }
 
-  void AddRequest(std::unique_ptr<Request> req) {
-    DCHECK_EQ(key_.hostname, req->info().hostname());
+  void AddRequest(RequestImpl* request) {
+    DCHECK_EQ(key_.hostname, request->info().hostname());
 
-    req->set_job(this);
-    priority_tracker_.Add(req->priority());
+    priority_tracker_.Add(request->priority());
 
-    req->source_net_log().AddEvent(
+    request->source_net_log().AddEvent(
         NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_ATTACH,
         net_log_.source().ToEventParametersCallback());
 
     net_log_.AddEvent(
         NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_REQUEST_ATTACH,
-        base::Bind(&NetLogJobAttachCallback,
-                   req->source_net_log().source(),
+        base::Bind(&NetLogJobAttachCallback, request->source_net_log().source(),
                    priority()));
 
     // TODO(szym): Check if this is still needed.
-    if (!req->info().is_speculative()) {
+    if (!request->info().is_speculative()) {
       had_non_speculative_request_ = true;
       if (proc_task_.get())
         proc_task_->set_had_non_speculative_request();
     }
 
-    requests_.push_back(std::move(req));
+    requests_.push_back(request);
 
     UpdatePriority();
   }
 
-  void ChangeRequestPriority(Request* req, RequestPriority priority) {
+  void ChangeRequestPriority(RequestImpl* req, RequestPriority priority) {
     DCHECK_EQ(key_.hostname, req->info().hostname());
-    DCHECK(!req->was_canceled());
 
     priority_tracker_.Remove(req->priority());
     req->set_priority(priority);
@@ -1387,24 +1382,23 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
     UpdatePriority();
   }
 
-  // Marks |req| as cancelled. If it was the last active Request, also finishes
-  // this Job, marking it as cancelled, and deletes it.
-  void CancelRequest(Request* req) {
-    DCHECK_EQ(key_.hostname, req->info().hostname());
-    DCHECK(!req->was_canceled());
+  // Detach cancelled request. If it was the last active Request, also finishes
+  // this Job.
+  void CancelRequest(RequestImpl* request) {
+    DCHECK_EQ(key_.hostname, request->info().hostname());
+    DCHECK(!requests_.empty());
 
-    // Don't remove it from |requests_| just mark it canceled.
-    req->MarkAsCanceled();
-    LogCancelRequest(req->source_net_log(), req->info());
+    LogCancelRequest(request->source_net_log(), request->info());
 
-    priority_tracker_.Remove(req->priority());
-    net_log_.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_REQUEST_DETACH,
-                      base::Bind(&NetLogJobAttachCallback,
-                                 req->source_net_log().source(),
-                                 priority()));
+    priority_tracker_.Remove(request->priority());
+    net_log_.AddEvent(
+        NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_REQUEST_DETACH,
+        base::Bind(&NetLogJobAttachCallback, request->source_net_log().source(),
+                   priority()));
 
     if (num_active_requests() > 0) {
       UpdatePriority();
+      RemoveRequest(request);
     } else {
       // If we were called from a Request's callback within CompleteRequests,
       // that Request could not have been cancelled, so num_active_requests()
@@ -1413,6 +1407,12 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
     }
   }
 
+  void RemoveRequest(RequestImpl* request) {
+    auto it = std::find(requests_.begin(), requests_.end(), request);
+    DCHECK(it != requests_.end());
+    requests_.erase(it);
+  }
+
   // Called from AbortAllInProgressJobs. Completes all requests and destroys
   // the job. This currently assumes the abort is due to a network change.
   // TODO This should not delete |this|.
@@ -1773,11 +1773,11 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
     if (did_complete)
       resolver_->CacheResult(key_, entry, ttl);
 
-    // Complete all of the requests that were attached to the job.
-    for (const std::unique_ptr<Request>& req : requests_) {
-      if (req->was_canceled())
-        continue;
-
+    // Complete all of the requests that were attached to the job and
+    // detach them.
+    while (!requests_.empty()) {
+      RequestImpl* req = requests_.front();
+      requests_.pop_front();
       DCHECK_EQ(this, req->job());
       // Update the net log and notify registered observers.
       LogFinishRequest(req->source_net_log(), req->info(), entry.error());
@@ -1786,7 +1786,7 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
         RecordTotalTime(had_dns_config_, req->info().is_speculative(),
                         base::TimeTicks::Now() - req->request_time());
       }
-      req->OnComplete(entry.error(), entry.addresses());
+      req->OnJobCompleted(this, entry.error(), entry.addresses());
 
       // Check if the resolver was destroyed as a result of running the
       // callback. If it was, we could continue, but we choose to bail.
@@ -1847,7 +1847,7 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
   std::unique_ptr<DnsTask> dns_task_;
 
   // All Requests waiting for the result of this Job. Some can be canceled.
-  std::vector<std::unique_ptr<Request>> requests_;
+  std::deque<RequestImpl*> requests_;
 
   // A handle used in |HostResolverImpl::dispatcher_|.
   PrioritizedDispatcher::Handle handle_;
@@ -1902,11 +1902,12 @@ int HostResolverImpl::Resolve(const RequestInfo& info,
                               RequestPriority priority,
                               AddressList* addresses,
                               const CompletionCallback& callback,
-                              RequestHandle* out_req,
+                              std::unique_ptr<Request>* out_req,
                               const BoundNetLog& source_net_log) {
   DCHECK(addresses);
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(false, callback.is_null());
+  DCHECK(out_req);
 
   // Check that the caller supplied a valid hostname to resolve.
   std::string labeled_hostname;
@@ -1959,12 +1960,11 @@ int HostResolverImpl::Resolve(const RequestInfo& info,
   }
 
   // Can't complete synchronously. Create and attach request.
-  std::unique_ptr<Request> req(
-      new Request(source_net_log, info, priority, callback, addresses));
-  if (out_req)
-    *out_req = reinterpret_cast<RequestHandle>(req.get());
+  std::unique_ptr<RequestImpl> req(new RequestImpl(
+      source_net_log, info, priority, callback, addresses, job));
+  job->AddRequest(req.get());
+  *out_req = std::move(req);
 
-  job->AddRequest(std::move(req));
   // Completion happens during Job::CompleteRequests().
   return ERR_IO_PENDING;
 }
@@ -2095,25 +2095,6 @@ int HostResolverImpl::ResolveFromCache(const RequestInfo& info,
   return rv;
 }
 
-void HostResolverImpl::ChangeRequestPriority(RequestHandle req_handle,
-                                             RequestPriority priority) {
-  DCHECK(CalledOnValidThread());
-  Request* req = reinterpret_cast<Request*>(req_handle);
-  DCHECK(req);
-  Job* job = req->job();
-  DCHECK(job);
-  job->ChangeRequestPriority(req, priority);
-}
-
-void HostResolverImpl::CancelRequest(RequestHandle req_handle) {
-  DCHECK(CalledOnValidThread());
-  Request* req = reinterpret_cast<Request*>(req_handle);
-  DCHECK(req);
-  Job* job = req->job();
-  DCHECK(job);
-  job->CancelRequest(req);
-}
-
 void HostResolverImpl::SetDnsClientEnabled(bool enabled) {
   DCHECK(CalledOnValidThread());
 #if defined(ENABLE_BUILT_IN_DNS)
@@ -2555,4 +2536,14 @@ void HostResolverImpl::SetDnsClient(std::unique_ptr<DnsClient> dns_client) {
   AbortDnsTasks();
 }
 
+HostResolverImpl::RequestImpl::~RequestImpl() {
+  if (job_)
+    job_->CancelRequest(this);
+}
+
+void HostResolverImpl::RequestImpl::ChangeRequestPriority(
+    RequestPriority priority) {
+  job_->ChangeRequestPriority(this, priority);
+}
+
 }  // namespace net