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

net/proxy/mojo_proxy_resolver_impl_unittest.cc

 // Copyright 2015 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/mojo_proxy_resolver_impl.h"
 
 #include <string>
 #include <utility>
 #include <vector>
 
@@ skipping 84 matching lines @@
     interfaces::HostResolverRequestInfoPtr request_info,
     interfaces::HostResolverRequestClientPtr client) {
 }
 
 void TestRequestClient::OnConnectionError() {
   event_waiter_.NotifyEvent(CONNECTION_ERROR);
 }
 
 class MockProxyResolverV8Tracing : public ProxyResolverV8Tracing {
  public:
-  struct Request {
+  struct Job {
     GURL url;
     ProxyInfo* results;
     CompletionCallback callback;
     bool cancelled = false;
   };
+
+  class RequestImpl : public ProxyResolver::Request {
+   public:
+    RequestImpl(Job* job, MockProxyResolverV8Tracing* resolver)
+        : job_(job), resolver_(resolver) {}
+
+    ~RequestImpl() override {
+      job_->cancelled = true;

[eroman, 2016/02/24 19:20:29]

This does change the meaning from before:

Previously cancelled was only set when the request was actually cancelled,
whereas now it is set every time the RequestImpl is destroyed.

This also means that the "cancel_callback_" is run always, even when the request
was not truly cancelled.

To address this, I suggest the following changes:

  (1) Rename Job::callback --> Job::callback_, and make it private (to avoid
code calling it directly).
  (2) Instead of test code calling job->callback.Run(XXX), replace those with
job->Complete(XXX), where the new method is resets the callback after running
it:

   void Job::Complete(int result) {
      DCHECK(!callback_.is_null())
      callback_.Run(result);
      callback_.Reset();
    }

This allows distinguishing between requests that were completed vs those that
did not:

   bool Job::WasCompleted() {
     return callback_.is_null();
   }

Now this code in ~RequestImpl() can distinguish between an actual cancellation,
versus destruction of an already completed request:

   if (job_->WasCompleted())
     return;
   
   job_->cancelled = true;
   if (!resolver_->cancel_callback_.is_null()) {
      ...
   }

+      if (!resolver_->cancel_callback_.is_null()) {
+        resolver_->cancel_callback_.Run();
+        resolver_->cancel_callback_.Reset();
+      }
+    }
+
+    LoadState GetLoadState() override {
+      return LOAD_STATE_RESOLVING_PROXY_FOR_URL;
+    }
+
+   private:
+    Job* job_;
+    MockProxyResolverV8Tracing* resolver_;
+  };
+
   MockProxyResolverV8Tracing() {}
 
   // ProxyResolverV8Tracing overrides.
   void GetProxyForURL(const GURL& url,
                       ProxyInfo* results,
                       const CompletionCallback& callback,
-                      ProxyResolver::RequestHandle* request,
+                      scoped_ptr<ProxyResolver::Request>* request,
                       scoped_ptr<Bindings> bindings) override;
-  void CancelRequest(ProxyResolver::RequestHandle request_handle) override;
-  LoadState GetLoadState(ProxyResolver::RequestHandle request) const override;
 
   // Wait until the mock resolver has received a CancelRequest call.

[eroman, 2016/02/24 19:20:29]

This comment is no longer up to date.

   void WaitForCancel();
 
-  const std::vector<Request>& pending_requests() { return pending_requests_; }
+  const std::vector<scoped_ptr<Job>>& pending_jobs() { return pending_jobs_; }
 
  private:
   base::Closure cancel_callback_;
-  std::vector<Request> pending_requests_;
+  std::vector<scoped_ptr<Job>> pending_jobs_;
 };
 
 void MockProxyResolverV8Tracing::GetProxyForURL(
     const GURL& url,
     ProxyInfo* results,
     const CompletionCallback& callback,
-    ProxyResolver::RequestHandle* request,
+    scoped_ptr<ProxyResolver::Request>* request,
     scoped_ptr<Bindings> bindings) {
-  pending_requests_.push_back(Request());
-  auto& pending_request = pending_requests_.back();
-  pending_request.url = url;
-  pending_request.results = results;
-  pending_request.callback = callback;
-  *request =
-      reinterpret_cast<ProxyResolver::RequestHandle>(pending_requests_.size());
+  pending_jobs_.push_back(make_scoped_ptr(new Job()));
+  auto pending_job = pending_jobs_.back().get();
+  pending_job->url = url;
+  pending_job->results = results;
+  pending_job->callback = callback;
+  request->reset(new RequestImpl(pending_job, this));
 }
 
-void MockProxyResolverV8Tracing::CancelRequest(
-    ProxyResolver::RequestHandle request_handle) {
-  size_t id = reinterpret_cast<size_t>(request_handle) - 1;
-  pending_requests_[id].cancelled = true;
-  if (!cancel_callback_.is_null()) {
-    cancel_callback_.Run();
-    cancel_callback_.Reset();
-  }
-}
-
-LoadState MockProxyResolverV8Tracing::GetLoadState(
-    ProxyResolver::RequestHandle request) const {
-  return LOAD_STATE_RESOLVING_PROXY_FOR_URL;
-}
 
 void MockProxyResolverV8Tracing::WaitForCancel() {
-  while (std::find_if(pending_requests_.begin(), pending_requests_.end(),
-                      [](const Request& request) {
-                        return request.cancelled;
-                      }) != pending_requests_.end()) {
+  while (std::find_if(pending_jobs_.begin(), pending_jobs_.end(),
+                      [](const scoped_ptr<Job>& job) {
+                        return job->cancelled;
+                      }) != pending_jobs_.end()) {
     base::RunLoop run_loop;
     cancel_callback_ = run_loop.QuitClosure();
     run_loop.Run();
   }
 }
 
 }  // namespace
 
 class MojoProxyResolverImplTest : public testing::Test {
  protected:
   void SetUp() override {
     scoped_ptr<MockProxyResolverV8Tracing> mock_resolver(
         new MockProxyResolverV8Tracing);
     mock_proxy_resolver_ = mock_resolver.get();
     resolver_impl_.reset(new MojoProxyResolverImpl(std::move(mock_resolver)));
     resolver_ = resolver_impl_.get();
   }
 
   MockProxyResolverV8Tracing* mock_proxy_resolver_;
 
   scoped_ptr<MojoProxyResolverImpl> resolver_impl_;
   interfaces::ProxyResolver* resolver_;
 };
 
 TEST_F(MojoProxyResolverImplTest, GetProxyForUrl) {
   interfaces::ProxyResolverRequestClientPtr client_ptr;
   TestRequestClient client(mojo::GetProxy(&client_ptr));
 
   resolver_->GetProxyForUrl("http://example.com", std::move(client_ptr));
-  ASSERT_EQ(1u, mock_proxy_resolver_->pending_requests().size());
-  const MockProxyResolverV8Tracing::Request& request =
-      mock_proxy_resolver_->pending_requests()[0];
-  EXPECT_EQ(GURL("http://example.com"), request.url);
+  ASSERT_EQ(1u, mock_proxy_resolver_->pending_jobs().size());
+  const MockProxyResolverV8Tracing::Job* job =
+      mock_proxy_resolver_->pending_jobs()[0].get();
+  EXPECT_EQ(GURL("http://example.com"), job->url);
 
-  request.results->UsePacString(
+  job->results->UsePacString(
       "PROXY proxy.example.com:1; "
       "SOCKS4 socks4.example.com:2; "
       "SOCKS5 socks5.example.com:3; "
       "HTTPS https.example.com:4; "
       "QUIC quic.example.com:65000; "
       "DIRECT");
-  request.callback.Run(OK);
+  job->callback.Run(OK);
   client.WaitForResult();
 
   EXPECT_EQ(OK, client.error());
   std::vector<ProxyServer> servers =
       client.results().To<std::vector<ProxyServer>>();
   ASSERT_EQ(6u, servers.size());
   EXPECT_EQ(ProxyServer::SCHEME_HTTP, servers[0].scheme());
   EXPECT_EQ("proxy.example.com", servers[0].host_port_pair().host());
   EXPECT_EQ(1, servers[0].host_port_pair().port());
 
@@ skipping 14 matching lines @@
   EXPECT_EQ(65000, servers[4].host_port_pair().port());
 
   EXPECT_EQ(ProxyServer::SCHEME_DIRECT, servers[5].scheme());
 }
 
 TEST_F(MojoProxyResolverImplTest, GetProxyForUrlFailure) {
   interfaces::ProxyResolverRequestClientPtr client_ptr;
   TestRequestClient client(mojo::GetProxy(&client_ptr));
 
   resolver_->GetProxyForUrl("http://example.com", std::move(client_ptr));
-  ASSERT_EQ(1u, mock_proxy_resolver_->pending_requests().size());
-  const MockProxyResolverV8Tracing::Request& request =
-      mock_proxy_resolver_->pending_requests()[0];
-  EXPECT_EQ(GURL("http://example.com"), request.url);
-  request.callback.Run(ERR_FAILED);
+  ASSERT_EQ(1u, mock_proxy_resolver_->pending_jobs().size());
+  const MockProxyResolverV8Tracing::Job* job =
+      mock_proxy_resolver_->pending_jobs()[0].get();
+  EXPECT_EQ(GURL("http://example.com"), job->url);
+  job->callback.Run(ERR_FAILED);
   client.WaitForResult();
 
   EXPECT_EQ(ERR_FAILED, client.error());
   std::vector<ProxyServer> proxy_servers =
       client.results().To<std::vector<ProxyServer>>();
   EXPECT_TRUE(proxy_servers.empty());
 }
 
 TEST_F(MojoProxyResolverImplTest, GetProxyForUrlMultiple) {
   interfaces::ProxyResolverRequestClientPtr client_ptr1;
   TestRequestClient client1(mojo::GetProxy(&client_ptr1));
   interfaces::ProxyResolverRequestClientPtr client_ptr2;
   TestRequestClient client2(mojo::GetProxy(&client_ptr2));
 
   resolver_->GetProxyForUrl("http://example.com", std::move(client_ptr1));
   resolver_->GetProxyForUrl("https://example.com", std::move(client_ptr2));
-  ASSERT_EQ(2u, mock_proxy_resolver_->pending_requests().size());
-  const MockProxyResolverV8Tracing::Request& request1 =
-      mock_proxy_resolver_->pending_requests()[0];
-  EXPECT_EQ(GURL("http://example.com"), request1.url);
-  const MockProxyResolverV8Tracing::Request& request2 =
-      mock_proxy_resolver_->pending_requests()[1];
-  EXPECT_EQ(GURL("https://example.com"), request2.url);
-  request1.results->UsePacString("HTTPS proxy.example.com:12345");
-  request1.callback.Run(OK);
-  request2.results->UsePacString("SOCKS5 another-proxy.example.com:6789");
-  request2.callback.Run(OK);
+  ASSERT_EQ(2u, mock_proxy_resolver_->pending_jobs().size());
+  const MockProxyResolverV8Tracing::Job* job1 =
+      mock_proxy_resolver_->pending_jobs()[0].get();
+  EXPECT_EQ(GURL("http://example.com"), job1->url);
+  const MockProxyResolverV8Tracing::Job* job2 =
+      mock_proxy_resolver_->pending_jobs()[1].get();
+  EXPECT_EQ(GURL("https://example.com"), job2->url);
+  job1->results->UsePacString("HTTPS proxy.example.com:12345");
+  job1->callback.Run(OK);
+  job2->results->UsePacString("SOCKS5 another-proxy.example.com:6789");
+  job2->callback.Run(OK);
   client1.WaitForResult();
   client2.WaitForResult();
 
   EXPECT_EQ(OK, client1.error());
   std::vector<ProxyServer> proxy_servers1 =
       client1.results().To<std::vector<ProxyServer>>();
   ASSERT_EQ(1u, proxy_servers1.size());
   ProxyServer& server1 = proxy_servers1[0];
   EXPECT_EQ(ProxyServer::SCHEME_HTTPS, server1.scheme());
   EXPECT_EQ("proxy.example.com", server1.host_port_pair().host());
   EXPECT_EQ(12345, server1.host_port_pair().port());
 
   EXPECT_EQ(OK, client2.error());
   std::vector<ProxyServer> proxy_servers2 =
       client2.results().To<std::vector<ProxyServer>>();
   ASSERT_EQ(1u, proxy_servers1.size());
   ProxyServer& server2 = proxy_servers2[0];
   EXPECT_EQ(ProxyServer::SCHEME_SOCKS5, server2.scheme());
   EXPECT_EQ("another-proxy.example.com", server2.host_port_pair().host());
   EXPECT_EQ(6789, server2.host_port_pair().port());
 }
 
 TEST_F(MojoProxyResolverImplTest, DestroyClient) {
   interfaces::ProxyResolverRequestClientPtr client_ptr;
   scoped_ptr<TestRequestClient> client(
       new TestRequestClient(mojo::GetProxy(&client_ptr)));
 
   resolver_->GetProxyForUrl("http://example.com", std::move(client_ptr));
-  ASSERT_EQ(1u, mock_proxy_resolver_->pending_requests().size());
-  const MockProxyResolverV8Tracing::Request& request =
-      mock_proxy_resolver_->pending_requests()[0];
-  EXPECT_EQ(GURL("http://example.com"), request.url);
-  request.results->UsePacString("PROXY proxy.example.com:8080");
+  ASSERT_EQ(1u, mock_proxy_resolver_->pending_jobs().size());
+  const MockProxyResolverV8Tracing::Job* job =
+      mock_proxy_resolver_->pending_jobs()[0].get();
+  EXPECT_EQ(GURL("http://example.com"), job->url);
+  job->results->UsePacString("PROXY proxy.example.com:8080");
   client.reset();
   mock_proxy_resolver_->WaitForCancel();
 }
 
 TEST_F(MojoProxyResolverImplTest, DestroyService) {
   interfaces::ProxyResolverRequestClientPtr client_ptr;
   TestRequestClient client(mojo::GetProxy(&client_ptr));
 
   resolver_->GetProxyForUrl("http://example.com", std::move(client_ptr));
-  ASSERT_EQ(1u, mock_proxy_resolver_->pending_requests().size());
+  ASSERT_EQ(1u, mock_proxy_resolver_->pending_jobs().size());
   resolver_impl_.reset();
   client.event_waiter().WaitForEvent(TestRequestClient::CONNECTION_ERROR);
 }
 
 }  // namespace net