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

net/dns/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/dns/host_resolver_impl.h"
 
 #include <memory>
 #include <utility>
 
 #include "base/memory/ptr_util.h"
@@ skipping 510 matching lines @@
   }
 
   return true;
 }
 
 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),
         job_(nullptr),
         callback_(callback),
         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);
     // 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());
     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_;
   }
 
   RequestPriority priority() const { return priority_; }
   void set_priority(RequestPriority priority) { priority_ = priority; }
 
   base::TimeTicks request_time() const { return request_time_; }
 
  private:
   const BoundNetLog source_net_log_;
 
   // The request info that started the request.
   const RequestInfo info_;
 
   RequestPriority priority_;
 
   // The resolve job that this request is dependent on.
   Job* job_;
 
   // The user's callback to invoke when the request completes.
   CompletionCallback callback_;
 
   // The address list to save result into.
   AddressList* addresses_;
 
   const base::TimeTicks request_time_;
 
-  DISALLOW_COPY_AND_ASSIGN(Request);
+  DISALLOW_COPY_AND_ASSIGN(RequestImpl);
 };
 
 //------------------------------------------------------------------------------
 
 // Calls HostResolverProc using a worker task runner. Performs retries if
 // necessary.
 //
 // Whenever we try to resolve the host, we post a delayed task to check if host
 // resolution (OnLookupComplete) is completed or not. If the original attempt
 // hasn't completed, then we start another attempt for host resolution. We take
@@ skipping 697 matching lines @@
                                         ERR_ABORTED);
     } else if (is_queued()) {
       // |resolver_| was destroyed without running this Job.
       // TODO(szym): is there any benefit in having this distinction?
       net_log_.AddEvent(NetLog::TYPE_CANCELLED);
       net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_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();
     }
   }
 
   // Add this job to the dispatcher.  If "at_head" is true, adds at the front
   // of the queue.
   void Schedule(bool at_head) {
     DCHECK(!is_queued());
     PrioritizedDispatcher::Handle handle;
     if (!at_head) {
       handle = resolver_->dispatcher_->Add(this, priority());
     } else {
       handle = resolver_->dispatcher_->AddAtHead(this, priority());
     }
     // The dispatcher could have started |this| in the above call to Add, which
     // could have called Schedule again. In that case |handle| will be null,
     // but |handle_| may have been set by the other nested call to Schedule.
     if (!handle.is_null()) {
       DCHECK(handle_.is_null());
       handle_ = handle;
     }
   }
 
-  void AddRequest(std::unique_ptr<Request> req) {
+  void AddRequest(RequestImpl* req) {
     DCHECK_EQ(key_.hostname, req->info().hostname());
 
     req->set_job(this);
     priority_tracker_.Add(req->priority());
 
     req->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(),
                    priority()));
 
     // TODO(szym): Check if this is still needed.
     if (!req->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(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);
     priority_tracker_.Add(req->priority());
     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();
     } else {
       // If we were called from a Request's callback within CompleteRequests,
       // that Request could not have been cancelled, so num_active_requests()
       // could not be 0. Therefore, we are not in CompleteRequests().
       CompleteRequestsWithError(OK /* cancelled */);
     }
   }
 
+  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|.
   void Abort() {
     DCHECK(is_running());
     CompleteRequestsWithError(ERR_NETWORK_CHANGED);
   }
 
   // If DnsTask present, abort it and fall back to ProcTask.
   void AbortDnsTask() {
@@ skipping 62 matching lines @@
       resolver_->dispatcher_->Cancel(handle_);
       handle_.Reset();
     } else if (num_occupied_job_slots_ > 1) {
       resolver_->dispatcher_->OnJobFinished();
       --num_occupied_job_slots_;
     }
     DCHECK_EQ(1u, num_occupied_job_slots_);
   }
 
   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() {
     if (is_queued()) {
       if (priority() != static_cast<RequestPriority>(handle_.priority()))
         priority_change_time_ = base::TimeTicks::Now();
       handle_ = resolver_->dispatcher_->ChangePriority(handle_, priority());
     }
   }
 
@@ skipping 256 matching lines @@
       UMA_HISTOGRAM_BOOLEAN("AsyncDNS.HaveDnsConfig",
                             resolver_->received_dns_config_);
     }
 
     bool did_complete = (entry.error() != ERR_NETWORK_CHANGED) &&
                         (entry.error() != ERR_HOST_RESOLVER_QUEUE_TOO_LARGE);
     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())
+    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());
       if (did_complete) {
         // Record effective total time from creation to completion.
         RecordTotalTime(had_dns_config_, req->info().is_speculative(),
                         base::TimeTicks::Now() - req->request_time());
       }
       req->OnComplete(entry.error(), entry.addresses());
 
@@ skipping 49 matching lines @@
 
   BoundNetLog net_log_;
 
   // Resolves the host using a HostResolverProc.
   scoped_refptr<ProcTask> proc_task_;
 
   // Resolves the host using a DnsTransaction.
   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_;
 };
 
 //-----------------------------------------------------------------------------
 
 HostResolverImpl::ProcTaskParams::ProcTaskParams(
     HostResolverProc* resolver_proc,
     size_t max_retry_attempts)
@@ skipping 34 matching lines @@
 void HostResolverImpl::SetMaxQueuedJobs(size_t value) {
   DCHECK_EQ(0u, dispatcher_->num_queued_jobs());
   DCHECK_GT(value, 0u);
   max_queued_jobs_ = value;
 }
 
 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());
 
   // Check that the caller supplied a valid hostname to resolve.
   std::string labeled_hostname;
   if (!DNSDomainFromDot(info.hostname(), &labeled_hostname))
     return ERR_NAME_NOT_RESOLVED;
 
@@ skipping 36 matching lines @@
         LogFinishRequest(source_net_log, info, rv);
         return rv;
       }
     }
     jobs_.insert(jobit, std::make_pair(key, job));
   } else {
     job = jobit->second;
   }
 
   // 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;
 }
 
 HostResolverImpl::HostResolverImpl(
     const Options& options,
     NetLog* net_log,
     scoped_refptr<base::TaskRunner> worker_task_runner)
     : max_queued_jobs_(0),
       proc_params_(NULL, options.max_retry_attempts),
@@ skipping 110 matching lines @@
     ip_address_ptr = &ip_address;
 
   Key key = GetEffectiveKeyForRequest(info, ip_address_ptr, source_net_log);
 
   int rv = ResolveHelper(key, info, ip_address_ptr, addresses, false, nullptr,
                          source_net_log);
   LogFinishRequest(source_net_log, info, rv);
   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)
   if (enabled && !dns_client_) {
     SetDnsClient(DnsClient::CreateClient(net_log_));
   } else if (!enabled && dns_client_) {
     SetDnsClient(std::unique_ptr<DnsClient>());
   }
 #endif
 }
@@ skipping 421 matching lines @@
     NetworkChangeNotifier::GetDnsConfig(&dns_config);
     dns_client_->SetConfig(dns_config);
     num_dns_failures_ = 0;
     if (dns_client_->GetConfig())
       UMA_HISTOGRAM_BOOLEAN("AsyncDNS.DnsClientEnabled", true);
   }
 
   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