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

net/proxy/multi_threaded_proxy_resolver.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/proxy/multi_threaded_proxy_resolver.h"
 
 #include <deque>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 98 matching lines @@
       size_t max_num_threads,
       const scoped_refptr<ProxyResolverScriptData>& script_data,
       scoped_refptr<Executor> executor);
 
   ~MultiThreadedProxyResolver() override;
 
   // ProxyResolver implementation:
   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;
   typedef std::vector<scoped_refptr<Executor>> ExecutorList;
 
   // Returns an idle worker thread which is ready to receive GetProxyForURL()
   // requests. If all threads are occupied, returns NULL.
   Executor* FindIdleExecutor();
 
   // Creates a new worker thread, and appends it to |executors_|.
@@ skipping 87 matching lines @@
 
   virtual ~Job() {}
 
  private:
   const Type type_;
   CompletionCallback callback_;
   Executor* executor_;
   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.
 class CreateResolverJob : public Job {
  public:
   CreateResolverJob(const scoped_refptr<ProxyResolverScriptData>& script_data,
                     ProxyResolverFactory* factory)
       : Job(TYPE_CREATE_RESOLVER, CompletionCallback()),
         script_data_(script_data),
         factory_(factory) {}
@@ skipping 191 matching lines @@
   DCHECK(CalledOnValidThread());
   // We will cancel all outstanding requests.
   pending_jobs_.clear();
 
   for (auto& executor : executors_) {
     executor->Destroy();
   }
 }
 
 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());
 
   scoped_refptr<GetProxyForURLJob> job(
       new GetProxyForURLJob(url, results, callback, net_log));
 
   // 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();
   if (executor) {
     DCHECK_EQ(0u, pending_jobs_.size());
     executor->StartJob(job.get());
     return ERR_IO_PENDING;
   }
 
   // Otherwise queue this request. (We will schedule it to a thread once one
   // becomes available).
   job->WaitingForThread();
   pending_jobs_.push_back(job);
 
   // If we haven't already reached the thread limit, provision a new thread to
   // drain the requests more quickly.
   if (executors_.size() < max_num_threads_)
     AddNewExecutor();
 
   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());
   for (ExecutorList::iterator it = executors_.begin();
        it != executors_.end(); ++it) {
     Executor* executor = it->get();
     if (!executor->outstanding_job())
       return executor;
   }
   return NULL;
@@ skipping 110 matching lines @@
   return ERR_IO_PENDING;
 }
 
 void MultiThreadedProxyResolverFactory::RemoveJob(
     MultiThreadedProxyResolverFactory::Job* job) {
   size_t erased = jobs_.erase(job);
   DCHECK_EQ(1u, erased);
 }
 
 }  // namespace net