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 dc280656c05c9498456d0520caa1b8d3326f42bd..db2c81e4e5da3549e864491f148206cc6cd5751b 100644
--- a/net/dns/host_resolver_impl.cc
+++ b/net/dns/host_resolver_impl.cc
@@ -528,13 +528,13 @@ 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)
       : source_net_log_(source_net_log),
         info_(info),
         priority_(priority),
@@ -543,16 +543,9 @@ class HostResolverImpl::Request {
         addresses_(addresses),
         request_time_(base::TimeTicks::Now()) {}
 
-  // Mark the request as canceled.
-  void MarkAsCanceled() {
-    job_ = nullptr;
-    addresses_ = nullptr;
-    callback_.Reset();
-  }
+  ~RequestImpl() override;
 
-  bool was_canceled() const {
-    return callback_.is_null();
-  }
+  void ChangeRequestPriority(RequestPriority priority) override;
 
   void set_job(Job* job) {
     DCHECK(job);
@@ -562,22 +555,29 @@ class HostResolverImpl::Request {
 
   // Prepare final AddressList and call completion callback.
   void OnComplete(int error, const AddressList& addr_list) {
-    DCHECK(!was_canceled());
     if (error == OK)
       *addresses_ = EnsurePortOnAddressList(addr_list, info_.port());
     CompletionCallback callback = callback_;
-    MarkAsCanceled();
+    MarkAsCompleted();
     callback.Run(error);
   }
 
+  void MarkAsCompleted() {
+    RemoveJob();
+    addresses_ = nullptr;
+    callback_.Reset();
+  }
+
+  bool MarkedAsCompleted() { return callback_.is_null(); }
+
   Job* job() const {
     return job_;
   }
 
+  void RemoveJob();
+
   // NetLog for the source, passed in HostResolver::Resolve.
-  const BoundNetLog& source_net_log() {
-    return source_net_log_;
-  }
+  const BoundNetLog& source_net_log() const { return source_net_log_; }
 
   const RequestInfo& info() const {
     return info_;
@@ -607,7 +607,7 @@ class HostResolverImpl::Request {
 
   const base::TimeTicks request_time_;
 
-  DISALLOW_COPY_AND_ASSIGN(Request);
+  DISALLOW_COPY_AND_ASSIGN(RequestImpl);
 };
 
 //------------------------------------------------------------------------------
@@ -1325,11 +1325,12 @@ 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())
+    for (RequestImpl* req : requests_) {
+      if (!req || req->MarkedAsCompleted())
         continue;
       DCHECK_EQ(this, req->job());
       LogCancelRequest(req->source_net_log(), req->info());
+      req->MarkAsCompleted();
     }
   }
 
@@ -1352,7 +1353,7 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
     }
   }
 
-  void AddRequest(std::unique_ptr<Request> req) {
+  void AddRequest(RequestImpl* req) {
     DCHECK_EQ(key_.hostname, req->info().hostname());
 
     req->set_job(this);
@@ -1375,14 +1376,13 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
         proc_task_->set_had_non_speculative_request();
     }
 
-    requests_.push_back(std::move(req));
+    requests_.push_back(req);
 
     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);
@@ -1390,21 +1390,17 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
     UpdatePriority();
   }
 
-  // Marks |req| as cancelled. If it was the last active Request, also finishes
+  // Remove |req|. If it was the last active Request, also finishes
   // this Job, marking it as cancelled, and deletes it.
-  void CancelRequest(Request* req) {
+  void CancelRequest(RequestImpl* req) {
     DCHECK_EQ(key_.hostname, req->info().hostname());
-    DCHECK(!req->was_canceled());
 
-    // Don't remove it from |requests_| just mark it canceled.
-    req->MarkAsCanceled();
+    RemoveRequest(req);
     LogCancelRequest(req->source_net_log(), req->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()));
+                                 req->source_net_log().source(), priority()));
 
     if (num_active_requests() > 0) {
       UpdatePriority();
@@ -1416,6 +1412,16 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
     }
   }
 
+  void RemoveRequest(RequestImpl* req) {
+    size_t req_position =
+        std::find(requests_.begin(), requests_.end(), req) - requests_.begin();
+    if (req_position >= requests_.size())
+      return;  // no request found, might have already been removed

[mmenke, 2016/07/12 18:50:23]

Does this case ever happen?  Doesn't seem like it should, if we always use
request_.MarkAsCompleted() to remove them.

Then can replace all this with a DCHECK_NE(std::find(requests_.begin(),
requests_.end(), req), requests_.end());

[maksims (do not use this acc), 2016/07/13 11:40:26]

right, I just wanted to be sure.

+
+    priority_tracker_.Remove(req->priority());
+    requests_[req_position] = nullptr;

[mmenke, 2016/07/12 18:50:23]

Growing requests_ eternally over the lifetime of the job seems not great.  Can
we just just remove it from the list?

Need to make sure we only walk through requests, removing them, using something
like:

while (!requests_.empty()) {
  requests_.begin()->DoThingThatRemovesRequest();
}

but don't think it will be too complicated.

[maksims (do not use this acc), 2016/07/13 11:40:27]

The problem is that the request might have already been removed while we are
going through requests in Job::CompleteRequests(). Then "error: attempt to
compare a past-the-end iterator to a singular iterator" occurs. 

I guess that was the reason why requests were stored and just marked
completed/canceled instead of being removed.

[mmenke, 2016/07/14 18:25:31]

Right, the problem is CancelRequest calls this method - which seems unnecessary,
it could just as easily use MarkAsCompleted, which should ensure this is never
called twice for the same job, right?

[maksims (do not use this acc), 2016/07/19 15:00:26]

It should be better now considering changed the request's implementation.

+  }
+
   // 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|.
@@ -1498,9 +1504,14 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
   }
 
   AddressList MakeAddressListForRequest(const AddressList& list) const {
-    if (requests_.empty())
-      return list;
-    return AddressList::CopyWithPort(list, requests_.front()->info().port());
+    if (requests_.empty()) {

[mmenke, 2016/07/12 18:50:23]

We walk through everything in requests_ if requests_ has nothing in it?

[maksims (do not use this acc), 2016/07/13 11:40:27]

no,no,no. my fault. sorry

[maksims (do not use this acc), 2016/07/19 15:00:26]

Reverted as long as new implementation allows old way of doing things here.

+      for (const RequestImpl* req : requests_) {
+        if (!req)
+          continue;
+        return AddressList::CopyWithPort(list, req->info().port());
+      }
+    }
+    return list;
   }
 
   void UpdatePriority() {
@@ -1777,10 +1788,9 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
       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())
+    for (RequestImpl* req : requests_) {
+      if (!req || req->MarkedAsCompleted())
         continue;
-
       DCHECK_EQ(this, req->job());
       // Update the net log and notify registered observers.
       LogFinishRequest(req->source_net_log(), req->info(), entry.error());
@@ -1850,7 +1860,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::vector<RequestImpl*> requests_;
 
   // A handle used in |HostResolverImpl::dispatcher_|.
   PrioritizedDispatcher::Handle handle_;
@@ -1905,7 +1915,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());
@@ -1962,12 +1972,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->AddRequest(req.get());
+  *out_req = std::move(req);
 
-  job->AddRequest(std::move(req));
   // Completion happens during Job::CompleteRequests().
   return ERR_IO_PENDING;
 }
@@ -2098,25 +2107,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)
@@ -2558,4 +2548,19 @@ 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);
+}
+
+void HostResolverImpl::RequestImpl::RemoveJob() {
+  job_->RemoveRequest(this);
+  job_ = nullptr;
+}

[mmenke, 2016/07/12 18:50:24]

These methods should all be inlined, since all the other RequestImpl methods
are.

[maksims (do not use this acc), 2016/07/13 11:40:27]

Nope, otherwise 
../../net/dns/host_resolver_impl.cc:545:11: error: member access into incomplete
type 'net::HostResolverImpl::Job'

../../net/dns/host_resolver_impl.h:172:9: note: forward declaration of
'net::HostResolverImpl::Job'
  class Job;

+
 }  // namespace net