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

net/proxy/proxy_resolver_factory_mojo.cc

Index: net/proxy/proxy_resolver_factory_mojo.cc
diff --git a/net/proxy/proxy_resolver_factory_mojo.cc b/net/proxy/proxy_resolver_factory_mojo.cc
index 2070cf9bef250d0927712c09e91ab47505ccbf26..ba876d8a83e1811c2423ee4971449aebc44636c8 100644
--- a/net/proxy/proxy_resolver_factory_mojo.cc
+++ b/net/proxy/proxy_resolver_factory_mojo.cc
@@ -118,13 +118,14 @@ class ProxyResolverMojo : public ProxyResolver {
   int GetProxyForURL(const GURL& url,
                      ProxyInfo* results,
                      const net::CompletionCallback& callback,
-                     RequestHandle* request,
+                     scoped_ptr<Request>* request,
                      const BoundNetLog& net_log) override;
-  void CancelRequest(RequestHandle request) override;
-  LoadState GetLoadState(RequestHandle request) const override;
 
  private:
   class Job;
+  class RequestImpl;
+
+  base::ThreadChecker thread_checker_;
 
   // Mojo error handler.
   void OnConnectionError();
@@ -142,13 +143,24 @@ class ProxyResolverMojo : public ProxyResolver {
 
   std::set<Job*> pending_jobs_;
 
-  base::ThreadChecker thread_checker_;
 
   scoped_ptr<base::ScopedClosureRunner> on_delete_callback_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(ProxyResolverMojo);
 };
 
+class ProxyResolverMojo::RequestImpl : public ProxyResolver::Request {
+ public:
+  RequestImpl(scoped_ptr<Job> job);

[eroman, 2016/02/24 03:08:06]

explicit

+
+  ~RequestImpl() override;
+
+  LoadState GetLoadState() override;
+
+ private:
+  scoped_ptr<Job> job_;
+};
+
 class ProxyResolverMojo::Job
     : public ClientMixin<interfaces::ProxyResolverRequestClient> {
  public:
@@ -159,13 +171,15 @@ class ProxyResolverMojo::Job
       const BoundNetLog& net_log);
   ~Job() override;
 
-  // Cancels the job and prevents the callback from being run.
-  void Cancel();
-
+  bool IsCallback();
+  void PacScriptTerminated();
   // Returns the LoadState of this job.
   LoadState GetLoadState();
 
+  ProxyResolverMojo* GetResolver();

[eroman, 2016/02/24 03:08:06]

resolver()

+
  private:
+  friend class base::RefCounted<Job>;
   // Mojo error handler.
   void OnConnectionError();
 
@@ -183,6 +197,21 @@ class ProxyResolverMojo::Job
   mojo::Binding<interfaces::ProxyResolverRequestClient> binding_;
 };
 
+ProxyResolverMojo::RequestImpl::RequestImpl(scoped_ptr<Job> job)
+    : job_(std::move(job)) {}
+
+ProxyResolverMojo::RequestImpl::~RequestImpl() {
+  ProxyResolverMojo* resolver = job_->GetResolver();
+  if (resolver) {
+    resolver->RemoveJob(job_.get());
+  }
+}
+
+LoadState ProxyResolverMojo::RequestImpl::GetLoadState() {
+  CHECK_EQ(1u, job_->GetResolver()->pending_jobs_.count(job_.get()));
+  return job_->GetLoadState();
+}
+
 ProxyResolverMojo::Job::Job(ProxyResolverMojo* resolver,
                             const GURL& url,
                             ProxyInfo* results,
@@ -204,16 +233,18 @@ ProxyResolverMojo::Job::Job(ProxyResolverMojo* resolver,
       &ProxyResolverMojo::Job::OnConnectionError, base::Unretained(this)));
 }
 
-ProxyResolverMojo::Job::~Job() {
+void ProxyResolverMojo::Job::PacScriptTerminated() {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (!callback_.is_null())
     callback_.Run(ERR_PAC_SCRIPT_TERMINATED);
+  callback_.Reset();
+  resolver_ = nullptr;
 }
 
-void ProxyResolverMojo::Job::Cancel() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(!callback_.is_null());
-  callback_.Reset();
+ProxyResolverMojo::Job::~Job() {}
+
+bool ProxyResolverMojo::Job::IsCallback() {

[eroman, 2016/02/24 03:08:06]

Is this used anywhere? I believe you can delete.

Otherwise as far as naming, HasCallback() or WasCancelled() would be better.

+  return !callback_.is_null();
 }
 
 LoadState ProxyResolverMojo::Job::GetLoadState() {
@@ -221,10 +252,15 @@ LoadState ProxyResolverMojo::Job::GetLoadState() {
                                    : LOAD_STATE_RESOLVING_PROXY_FOR_URL;
 }
 
+ProxyResolverMojo* ProxyResolverMojo::Job::GetResolver() {
+  return resolver_;
+};
+
 void ProxyResolverMojo::Job::OnConnectionError() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DVLOG(1) << "ProxyResolverMojo::Job::OnConnectionError";
-  resolver_->RemoveJob(this);
+  if (resolver_)

[eroman, 2016/02/24 03:08:06]

Your refactor here looks correct (good job reading through this file!)

But on a side-note, I find the (existing) code quite confusing!

resolver_->RemoveJob() may call back into |this| to invoke
PacScriptTerminated(), which may then invoke hte callback. Even more confusing
is ReportResult() below, which calls RemoveJob() but must first call
callback_.Reset() otherwise it will dispatch to the callback.

I think we can improve this by eliminating the re-entrancy.

For instance introducing a Job::CompleteRequest(int) method:

void Job::CompleteRequest(int result) {
  if (resolver_)
    resolver_->RemoveJob(this);
  resolver_ = nullptr;

  if (!callback_.is_null())
    callback_.Run(result);
  callback_.Reset();
}

And changing RemoveJob() to not do any re-entrancy into |job|:

ProxyResolverMojo::RemoveJob(Job* job) {
  DCHECK(....);
  pending_jobs_.erase(job);
}

This means all the places that would otherwise call resolver_->RemoveJob(this),
possibly doing a null check or re-setting callback_, can now just call
CompleteRequest() with the expected result.

+    resolver_->RemoveJob(this);

[eroman, 2016/02/24 03:08:06]

Based on suggestion above, this would become:

   CompleteRequest(ERR_PAC_SCRIPT_TERMINATED);

 }
 
 void ProxyResolverMojo::Job::ReportResult(
@@ -275,42 +311,29 @@ void ProxyResolverMojo::OnConnectionError() {
 
 void ProxyResolverMojo::RemoveJob(Job* job) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  size_t num_erased = pending_jobs_.erase(job);
-  DCHECK(num_erased);
-  delete job;
+  pending_jobs_.erase(job);
+  job->PacScriptTerminated();
 }
 
 int ProxyResolverMojo::GetProxyForURL(const GURL& url,
                                       ProxyInfo* results,
                                       const CompletionCallback& callback,
-                                      RequestHandle* request,
+                                      scoped_ptr<Request>* request,
                                       const BoundNetLog& net_log) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (!mojo_proxy_resolver_ptr_)
     return ERR_PAC_SCRIPT_TERMINATED;
 
-  Job* job = new Job(this, url, results, callback, net_log);
-  bool inserted = pending_jobs_.insert(job).second;
+  scoped_ptr<Job> job(new Job(this, url, results, callback, net_log));
+  bool inserted = pending_jobs_.insert(job.get()).second;
   DCHECK(inserted);
-  *request = job;
+  request->reset(new RequestImpl(std::move(job)));
 
   return ERR_IO_PENDING;
 }
 
-void ProxyResolverMojo::CancelRequest(RequestHandle request) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  Job* job = static_cast<Job*>(request);
-  DCHECK(job);
-  job->Cancel();
-  RemoveJob(job);
-}
 
-LoadState ProxyResolverMojo::GetLoadState(RequestHandle request) const {
-  Job* job = static_cast<Job*>(request);
-  CHECK_EQ(1u, pending_jobs_.count(job));
-  return job->GetLoadState();
-}
 
 }  // namespace