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..fad32b9a0de3ff126b39a2d80e983481fb6328b1 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,42 @@ 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() {
+  // Deletion cancels the outstanding request.

[mmenke, 2016/07/19 19:03:55]

nit:  Comment not needed, since this is part of the HostResolver::Request
contract.

[maksims (do not use this acc), 2016/07/21 07:12:45]

Done.

+  ~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 +601,7 @@ class HostResolverImpl::Request {
 
   const base::TimeTicks request_time_;
 
-  DISALLOW_COPY_AND_ASSIGN(Request);
+  DISALLOW_COPY_AND_ASSIGN(RequestImpl);
 };
 
 //------------------------------------------------------------------------------
@@ -1321,12 +1318,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 +1348,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 +1383,22 @@ 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 DetachRequest(RequestImpl* request) {

[mmenke, 2016/07/19 19:03:55]

I think this can be renamed CancelRequest again?  It's only called on
cancellation, not on success.

[maksims (do not use this acc), 2016/07/21 07:12:45]

Done.

+    DCHECK_EQ(key_.hostname, request->info().hostname());
 
-    // 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);
+    CHECK(it != requests_.end());

[mmenke, 2016/07/19 19:03:55]

nit:  Use DCHECK, to avoid increasing binary size, unless you're particularly
worried about it failing, in which case, land it with with a CHECK, and switch
to to DCHECK fairly promptly..

[maksims (do not use this acc), 2016/07/21 07:12:45]

Done.

+    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,7 +1902,7 @@ 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());
@@ -1959,12 +1959,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);

[mmenke, 2016/07/19 19:03:56]

The old code let out_req by NULL, this code does not.  It looks like some
callers need still to be updated - see
https://cs.chromium.org/chromium/src/chrome/browser/devtools/device/port_forw...,
for instance.

Unfortunately, looks like some consumers of this class are not currently being
tested.  Should do a search for others - to do this, go to
https://cs.chromium.org/chromium/src/net/dns/host_resolver.h?sq=package:chrom...
and click on Resolve, and it will show you all callers at the bottom of the
screen, and you can look through them (Have to jump to each one, individually,
unfortunately).

[maksims (do not use this acc), 2016/07/21 07:12:45]

Done.

 
-  job->AddRequest(std::move(req));
   // Completion happens during Job::CompleteRequests().
   return ERR_IO_PENDING;
 }
@@ -2095,25 +2094,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 +2535,14 @@ void HostResolverImpl::SetDnsClient(std::unique_ptr<DnsClient> dns_client) {
   AbortDnsTasks();
 }
 
+HostResolverImpl::RequestImpl::~RequestImpl() {
+  if (job_)
+    job_->DetachRequest(this);
+}
+
+void HostResolverImpl::RequestImpl::ChangeRequestPriority(
+    RequestPriority priority) {
+  job_->ChangeRequestPriority(this, priority);
+}
+
 }  // namespace net