Change ProxyResolver::GetProxyForURL() to take a scoped_ptr<Request>* rather than a RequestHandle*

net/proxy/multi_threaded_proxy_resolver.cc

Index: net/proxy/multi_threaded_proxy_resolver.cc
diff --git a/net/proxy/multi_threaded_proxy_resolver.cc b/net/proxy/multi_threaded_proxy_resolver.cc
index 7f03bbfbba3859c2066198e16bbefde40e741bcf..6ca9695845644d7820d14b982eb38df7b3e2c262 100644
--- a/net/proxy/multi_threaded_proxy_resolver.cc
+++ b/net/proxy/multi_threaded_proxy_resolver.cc
@@ -116,13 +116,19 @@ class MultiThreadedProxyResolver : public ProxyResolver,
   int GetProxyForURL(const GURL& url,
                      ProxyInfo* results,
                      const CompletionCallback& callback,
-                     RequestHandle* request,
+                     scoped_ptr<Request>* request,
                      const BoundNetLog& net_log) override;
-  void CancelRequest(RequestHandle request) override;
-  LoadState GetLoadState(RequestHandle request) const override;
+
+  void RemovePendingJob(Job* job) {
+    PendingJobsQueue::iterator it =
+        std::find(pending_jobs_.begin(), pending_jobs_.end(), job);
+    DCHECK(it != pending_jobs_.end());
+    pending_jobs_.erase(it);
+  }
 
  private:
   class GetProxyForURLJob;
+  class RequestImpl;
   // FIFO queue of pending jobs waiting to be started.
   // TODO(eroman): Make this priority queue.
   typedef std::deque<scoped_refptr<Job>> PendingJobsQueue;
@@ -230,6 +236,34 @@ class Job : public base::RefCountedThreadSafe<Job> {
   bool was_cancelled_;
 };
 
+class MultiThreadedProxyResolver::RequestImpl : public ProxyResolver::Request {
+ public:
+  RequestImpl(Job* job, MultiThreadedProxyResolver* resolver)
+      : job_(job), resolver_(resolver) {}
+
+  ~RequestImpl() override {
+    DCHECK(resolver_->CalledOnValidThread());
+
+    if (job_->executor()) {

[eroman, 2015/11/24 01:20:59]

This doesn't seem correct. If the job was already completed, then job_ may have
already been freed.

Try running this with ASAN, will probably reveal a use-after-free.

(Note that the model changed. Before CancelRequest() was guaranteed to only be
called for oustanding requests, but generally destructors are called always,
even if the request does not need to be cancelled. So code needs to be
structured to so cancellation is only done when necessary).

+      // If the job was already submitted to the executor, just mark it
+      // as cancelled so the user callback isn't run on completion.
+      job_->Cancel();
+    } else {
+      // Otherwise the job is just sitting in a queue.
+      resolver_->RemovePendingJob(job_);
+    }
+  }
+
+  LoadState GetLoadState() override {
+    DCHECK(resolver_->CalledOnValidThread());
+    return LOAD_STATE_RESOLVING_PROXY_FOR_URL;
+  }
+
+ private:
+  Job* job_;
+  MultiThreadedProxyResolver* resolver_;
+};
+
 // CreateResolverJob -----------------------------------------------------------
 
 // Runs on the worker thread to call ProxyResolverFactory::CreateProxyResolver.
@@ -441,8 +475,11 @@ MultiThreadedProxyResolver::~MultiThreadedProxyResolver() {
 }
 
 int MultiThreadedProxyResolver::GetProxyForURL(
-    const GURL& url, ProxyInfo* results, const CompletionCallback& callback,
-    RequestHandle* request, const BoundNetLog& net_log) {
+    const GURL& url,
+    ProxyInfo* results,
+    const CompletionCallback& callback,
+    scoped_ptr<Request>* request,
+    const BoundNetLog& net_log) {
   DCHECK(CalledOnValidThread());
   DCHECK(!callback.is_null());
 
@@ -452,7 +489,7 @@ int MultiThreadedProxyResolver::GetProxyForURL(
   // Completion will be notified through |callback|, unless the caller cancels
   // the request using |request|.
   if (request)
-    *request = reinterpret_cast<RequestHandle>(job.get());
+    request->reset(new RequestImpl(job.get(), this));
 
   // If there is an executor that is ready to run this request, submit it!
   Executor* executor = FindIdleExecutor();
@@ -475,31 +512,6 @@ int MultiThreadedProxyResolver::GetProxyForURL(
   return ERR_IO_PENDING;
 }
 
-void MultiThreadedProxyResolver::CancelRequest(RequestHandle req) {
-  DCHECK(CalledOnValidThread());
-  DCHECK(req);
-
-  Job* job = reinterpret_cast<Job*>(req);
-  DCHECK_EQ(Job::TYPE_GET_PROXY_FOR_URL, job->type());
-
-  if (job->executor()) {
-    // If the job was already submitted to the executor, just mark it
-    // as cancelled so the user callback isn't run on completion.
-    job->Cancel();
-  } else {
-    // Otherwise the job is just sitting in a queue.
-    PendingJobsQueue::iterator it =
-        std::find(pending_jobs_.begin(), pending_jobs_.end(), job);
-    DCHECK(it != pending_jobs_.end());
-    pending_jobs_.erase(it);
-  }
-}
-
-LoadState MultiThreadedProxyResolver::GetLoadState(RequestHandle req) const {
-  DCHECK(CalledOnValidThread());
-  DCHECK(req);
-  return LOAD_STATE_RESOLVING_PROXY_FOR_URL;
-}
 
 Executor* MultiThreadedProxyResolver::FindIdleExecutor() {
   DCHECK(CalledOnValidThread());