Reland "Change ProxyResolutionService API to use std::unique_ptr<Request> instead of raw pointers."

This is a reland of 2ae89dce

Original change's description:
> Change ProxyResolutionService API to use std::unique_ptr<Request> instead of raw pointers.
> 
> Bug: 479898
> Cq-Include-Trybots: luci.chromium.try:linux_mojo
> Change-Id: I3d1b89fc33ebae27f6ab4007eceffb9eb6134ef6
> Reviewed-on: https://chromium-review.googlesource.com/887598
> Commit-Queue: Matt Menke <mmenke@chromium.org>
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Reviewed-by: Bo <boliu@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Matt Menke <mmenke@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#578436}

Bug: 479898
Change-Id: Id5684b743d27a0edf4d436beb113e37e32a70b89
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1152567
Commit-Queue: Lily Houghton <lilyhoughton@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579208}

chromeos/dbus/services/proxy_resolution_service_provider.cc

@@ -50,6 +50,9 @@ struct ProxyResolutionServiceProvider::Request {
   // this request.
   const scoped_refptr<net::URLRequestContextGetter> context_getter;
 
+  // Handle to ProxyResolutionService's Request
+  std::unique_ptr<net::ProxyResolutionService::Request> request;
+
   // ProxyInfo resolved for |source_url|.
   net::ProxyInfo proxy_info;
 
@@ -160,7 +163,7 @@ void ProxyResolutionServiceProvider::ResolveProxyOnNetworkThread(
           << request_ptr->source_url;
   const int result = proxy_resolution_service->ResolveProxy(
       GURL(request_ptr->source_url), std::string(), &request_ptr->proxy_info,
-      callback, nullptr, nullptr, net::NetLogWithSource());
+      callback, &request_ptr->request, nullptr, net::NetLogWithSource());
   if (result != net::ERR_IO_PENDING) {
     VLOG(1) << "Network proxy resolution completed synchronously.";
     callback.Run(result);

content/browser/renderer_host/pepper/pepper_network_proxy_host.cc

@@ -46,16 +46,7 @@ PepperNetworkProxyHost::PepperNetworkProxyHost(BrowserPpapiHostImpl* host,
                  weak_factory_.GetWeakPtr()));
 }
 
-PepperNetworkProxyHost::~PepperNetworkProxyHost() {
-  while (!pending_requests_.empty()) {
-    // If the proxy_resolution_service_ is NULL, we shouldn't have any outstanding
-    // requests.
-    DCHECK(proxy_resolution_service_);
-    net::ProxyResolutionService::Request* request = pending_requests_.front();
-    proxy_resolution_service_->CancelRequest(request);
-    pending_requests_.pop();
-  }
-}
+PepperNetworkProxyHost::~PepperNetworkProxyHost() = default;
 
 PepperNetworkProxyHost::UIThreadData::UIThreadData() : is_allowed(false) {}
 
@@ -139,7 +130,7 @@ void PepperNetworkProxyHost::TryToSendUnsentRequests() {
     } else {
       // Everything looks valid, so try to resolve the proxy.
       net::ProxyInfo* proxy_info = new net::ProxyInfo;
-      net::ProxyResolutionService::Request* pending_request = nullptr;
+      std::unique_ptr<net::ProxyResolutionService::Request> pending_request;
       base::Callback<void(int)> callback =
           base::Bind(&PepperNetworkProxyHost::OnResolveProxyCompleted,
                      weak_factory_.GetWeakPtr(),
@@ -148,7 +139,7 @@ void PepperNetworkProxyHost::TryToSendUnsentRequests() {
       int result = proxy_resolution_service_->ResolveProxy(
           request.url, std::string(), proxy_info, callback, &pending_request,
           nullptr, net::NetLogWithSource());
-      pending_requests_.push(pending_request);
+      pending_requests_.push(std::move(pending_request));
       // If it was handled synchronously, we must run the callback now;
       // proxy_resolution_service_ won't run it for us in this case.
       if (result != net::ERR_IO_PENDING)

content/browser/renderer_host/pepper/pepper_network_proxy_host.h

@@ -99,7 +99,8 @@ class CONTENT_EXPORT PepperNetworkProxyHost : public ppapi::host::ResourceHost {
 
   // Requests awaiting a response from ProxyResolutionService. We need to store
   // these so that we can cancel them if we get destroyed.
-  base::queue<net::ProxyResolutionService::Request*> pending_requests_;
+  base::queue<std::unique_ptr<net::ProxyResolutionService::Request>>
+      pending_requests_;
 
   base::WeakPtrFactory<PepperNetworkProxyHost> weak_factory_;
 

content/browser/resolve_proxy_msg_helper.cc

@@ -44,44 +44,28 @@ void ResolveProxyMsgHelper::OnResolveProxy(const GURL& url,
     StartPendingRequest();
 }
 
-ResolveProxyMsgHelper::~ResolveProxyMsgHelper() {
-  // Clear all pending requests if the ProxyResolutionService is still alive (if
-  // we have a default request context or override).
-  if (!pending_requests_.empty()) {
-    PendingRequest req = pending_requests_.front();
-    proxy_resolution_service_->CancelRequest(req.request);
-  }
-
-  for (PendingRequestList::iterator it = pending_requests_.begin();
-       it != pending_requests_.end();
-       ++it) {
-    delete it->reply_msg;
-  }
-
-  pending_requests_.clear();
-}
+ResolveProxyMsgHelper::~ResolveProxyMsgHelper() = default;
 
 void ResolveProxyMsgHelper::OnResolveProxyCompleted(int result) {
   CHECK(!pending_requests_.empty());
 
-  const PendingRequest& completed_req = pending_requests_.front();
-  ViewHostMsg_ResolveProxy::WriteReplyParams(
-      completed_req.reply_msg, result == net::OK, proxy_info_.ToPacString());
-  Send(completed_req.reply_msg);
-
   // Clear the current (completed) request.
+  PendingRequest completed_req = std::move(pending_requests_.front());
   pending_requests_.pop_front();
 
+  ViewHostMsg_ResolveProxy::WriteReplyParams(completed_req.reply_msg.get(),
+                                             result == net::OK,
+                                             proxy_info_.ToPacString());
+  Send(completed_req.reply_msg.release());
+
   // Start the next request.
   if (!pending_requests_.empty())
     StartPendingRequest();
 }
 
 void ResolveProxyMsgHelper::StartPendingRequest() {
-  PendingRequest& req = pending_requests_.front();
-
   // Verify the request wasn't started yet.
-  DCHECK(nullptr == req.request);
+  DCHECK(nullptr == pending_requests_.front().request);
 
   if (context_getter_.get()) {
     proxy_resolution_service_ = context_getter_->GetURLRequestContext()->proxy_resolution_service();
@@ -90,14 +74,26 @@ void ResolveProxyMsgHelper::StartPendingRequest() {
 
   // Start the request.
   int result = proxy_resolution_service_->ResolveProxy(
-      req.url, std::string(), &proxy_info_,
+      pending_requests_.front().url, std::string(), &proxy_info_,
       base::BindOnce(&ResolveProxyMsgHelper::OnResolveProxyCompleted,
                      base::Unretained(this)),
-      &req.request, nullptr, net::NetLogWithSource());
+      &pending_requests_.front().request, nullptr, net::NetLogWithSource());
 
   // Completed synchronously.
   if (result != net::ERR_IO_PENDING)
     OnResolveProxyCompleted(result);
 }
 
+ResolveProxyMsgHelper::PendingRequest::PendingRequest(const GURL& url,
+                                                      IPC::Message* reply_msg)
+    : url(url), reply_msg(reply_msg) {}
+
+ResolveProxyMsgHelper::PendingRequest::PendingRequest(
+    PendingRequest&& pending_request) = default;
+
+ResolveProxyMsgHelper::PendingRequest::~PendingRequest() noexcept = default;
+
+ResolveProxyMsgHelper::PendingRequest& ResolveProxyMsgHelper::PendingRequest::
+operator=(PendingRequest&& pending_request) noexcept = default;
+
 }  // namespace content

content/browser/resolve_proxy_msg_helper.h

@@ -58,17 +58,23 @@ class CONTENT_EXPORT ResolveProxyMsgHelper : public BrowserMessageFilter {
   // A PendingRequest is a resolve request that is in progress, or queued.
   struct PendingRequest {
    public:
-    PendingRequest(const GURL& url, IPC::Message* reply_msg)
-        : url(url), reply_msg(reply_msg), request(NULL) {}
+    PendingRequest(const GURL& url, IPC::Message* reply_msg);
+    PendingRequest(PendingRequest&& pending_request) noexcept;
+    ~PendingRequest();
+
+    PendingRequest& operator=(PendingRequest&& pending_request) noexcept;
 
     // The URL of the request.
     GURL url;
 
     // Data to pass back to the delegate on completion (we own it until then).
-    IPC::Message* reply_msg;
+    std::unique_ptr<IPC::Message> reply_msg;
 
     // Handle for cancelling the current request if it has started (else NULL).
-    net::ProxyResolutionService::Request* request;
+    std::unique_ptr<net::ProxyResolutionService::Request> request;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(PendingRequest);
   };
 
   // Info about the current outstanding proxy request.

content/test/proxy_service_mojo_unittest.cc

@@ -158,12 +158,11 @@ class ProxyServiceMojoTest : public testing::Test {
 TEST_F(ProxyServiceMojoTest, Basic) {
   net::ProxyInfo info;
   net::TestCompletionCallback callback;
+  std::unique_ptr<net::ProxyResolutionService::Request> request;
   EXPECT_EQ(net::ERR_IO_PENDING,
             proxy_resolution_service_->ResolveProxy(
-                GURL("http://foo"),
-                std::string(),
-                &info, callback.callback(), nullptr,
-                nullptr, net::NetLogWithSource()));
+                GURL("http://foo"), std::string(), &info, callback.callback(),
+                &request, nullptr, net::NetLogWithSource()));
 
   // PAC file fetcher should have a fetch triggered by the first
   // |ResolveProxy()| request.
@@ -181,11 +180,11 @@ TEST_F(ProxyServiceMojoTest, DnsResolution) {
   net::ProxyInfo info;
   net::TestCompletionCallback callback;
   net::BoundTestNetLog test_net_log;
+  std::unique_ptr<net::ProxyResolutionService::Request> request;
   EXPECT_EQ(net::ERR_IO_PENDING,
             proxy_resolution_service_->ResolveProxy(
-                GURL("http://foo"), std::string(),
-                &info, callback.callback(), nullptr,
-                nullptr, test_net_log.bound()));
+                GURL("http://foo"), std::string(), &info, callback.callback(),
+                &request, nullptr, test_net_log.bound()));
 
   // PAC file fetcher should have a fetch triggered by the first
   // |ResolveProxy()| request.
@@ -214,11 +213,11 @@ TEST_F(ProxyServiceMojoTest, Error) {
   net::ProxyInfo info;
   net::TestCompletionCallback callback;
   net::BoundTestNetLog test_net_log;
+  std::unique_ptr<net::ProxyResolutionService::Request> request;
   EXPECT_EQ(net::ERR_IO_PENDING,
             proxy_resolution_service_->ResolveProxy(
-                GURL("http://foo"), std::string(),
-                &info, callback.callback(), nullptr,
-                nullptr, test_net_log.bound()));
+                GURL("http://foo"), std::string(), &info, callback.callback(),
+                &request, nullptr, test_net_log.bound()));
 
   // PAC file fetcher should have a fetch triggered by the first
   // |ResolveProxy()| request.
@@ -244,11 +243,11 @@ TEST_F(ProxyServiceMojoTest, Error) {
 TEST_F(ProxyServiceMojoTest, ErrorOnInitialization) {
   net::ProxyInfo info;
   net::TestCompletionCallback callback;
+  std::unique_ptr<net::ProxyResolutionService::Request> request;
   EXPECT_EQ(net::ERR_IO_PENDING,
             proxy_resolution_service_->ResolveProxy(
-                GURL("http://foo"), std::string(),
-                &info, callback.callback(), nullptr,
-                nullptr, net::NetLogWithSource()));
+                GURL("http://foo"), std::string(), &info, callback.callback(),
+                &request, nullptr, net::NetLogWithSource()));
 
   // PAC file fetcher should have a fetch triggered by the first
   // |ResolveProxy()| request.

net/http/http_stream_factory_job_controller.cc

@@ -107,7 +107,7 @@ HttpStreamFactory::JobController::~JobController() {
   bound_job_ = nullptr;
   if (proxy_resolve_request_) {
     DCHECK_EQ(STATE_RESOLVE_PROXY_COMPLETE, next_state_);
-    session_->proxy_resolution_service()->CancelRequest(proxy_resolve_request_);
+    proxy_resolve_request_.reset();
   }
   net_log_.EndEvent(NetLogEventType::HTTP_STREAM_JOB_CONTROLLER);
 }
@@ -155,8 +155,7 @@ void HttpStreamFactory::JobController::Preconnect(int num_streams) {
 LoadState HttpStreamFactory::JobController::GetLoadState() const {
   DCHECK(request_);
   if (next_state_ == STATE_RESOLVE_PROXY_COMPLETE)
-    return session_->proxy_resolution_service()->GetLoadState(
-        proxy_resolve_request_);
+    return proxy_resolve_request_->GetLoadState();
   if (bound_job_)
     return bound_job_->GetLoadState();
   if (main_job_)

net/http/http_stream_factory_job_controller.h

@@ -370,7 +370,7 @@ class HttpStreamFactory::JobController
   bool can_start_alternative_proxy_job_;
 
   State next_state_;
-  ProxyResolutionService::Request* proxy_resolve_request_;
+  std::unique_ptr<ProxyResolutionService::Request> proxy_resolve_request_;
   const HttpRequestInfo request_info_;
   ProxyInfo proxy_info_;
   const SSLConfig server_ssl_config_;

net/proxy_resolution/proxy_resolution_service.h

@@ -23,9 +23,11 @@
 #include "net/base/net_export.h"
 #include "net/base/network_change_notifier.h"
 #include "net/base/proxy_server.h"
+#include "net/log/net_log_with_source.h"
 #include "net/proxy_resolution/proxy_config_service.h"
 #include "net/proxy_resolution/proxy_config_with_annotation.h"
 #include "net/proxy_resolution/proxy_info.h"
+#include "net/proxy_resolution/proxy_resolver.h"
 #include "url/gurl.h"
 
 class GURL;
@@ -39,9 +41,7 @@ namespace net {
 
 class DhcpPacFileFetcher;
 class NetLog;
-class NetLogWithSource;
 class ProxyDelegate;
-class ProxyResolver;
 class ProxyResolverFactory;
 class PacFileData;
 class PacFileFetcher;
@@ -123,7 +123,17 @@ class NET_EXPORT ProxyResolutionService
   ~ProxyResolutionService() override;
 
   // Used to track proxy resolution requests that complete asynchronously.
-  class Request;
+  class Request {
+   public:
+    virtual ~Request() = default;
+    virtual LoadState GetLoadState() const = 0;
+
+   protected:
+    Request() = default;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(Request);
+  };
 
   // Determines the appropriate proxy for |url| for a |method| request and
   // stores the result in |results|. If |method| is empty, the caller can expect
@@ -135,10 +145,9 @@ class NET_EXPORT ProxyResolutionService
   // ResolveProxy.
   //
   // The caller is responsible for ensuring that |results| and |callback|
-  // remain valid until the callback is run or until |request| is cancelled
-  // via CancelRequest.  |request| is only valid while the completion
-  // callback is still pending. NULL can be passed for |request| if
-  // the caller will not need to cancel the request.
+  // remain valid until the callback is run or until |request| is cancelled,
+  // which occurs when the unique pointer to it is deleted (by leaving scope or
+  // otherwise).  |request| must not be NULL.
   //
   // We use the three possible proxy access types in the following order,
   // doing fallback if one doesn't work.  See "pac_script_decider.h"
@@ -152,7 +161,7 @@ class NET_EXPORT ProxyResolutionService
                    const std::string& method,
                    ProxyInfo* results,
                    CompletionOnceCallback callback,
-                   Request** request,
+                   std::unique_ptr<Request>* request,
                    ProxyDelegate* proxy_delegate,
                    const NetLogWithSource& net_log);
 
@@ -185,12 +194,6 @@ class NET_EXPORT ProxyResolutionService
   void ReportSuccess(const ProxyInfo& proxy_info,
                      ProxyDelegate* proxy_delegate);
 
-  // Call this method with a non-null |request| to cancel the PAC request.
-  void CancelRequest(Request* request);
-
-  // Returns the LoadState for this |request| which must be non-NULL.
-  LoadState GetLoadState(const Request* request) const;
-
   // Sets the PacFileFetcher and DhcpPacFileFetcher dependencies. This
   // is needed if the ProxyResolver is of type ProxyResolverWithoutFetch.
   void SetPacFileFetchers(
@@ -312,11 +315,11 @@ class NET_EXPORT ProxyResolutionService
                            UpdateConfigAfterFailedAutodetect);
   FRIEND_TEST_ALL_PREFIXES(ProxyResolutionServiceTest,
                            UpdateConfigFromPACToDirect);
-  friend class Request;
   class InitProxyResolver;
   class PacFileDeciderPoller;
+  class RequestImpl;
 
-  typedef std::set<scoped_refptr<Request>> PendingRequests;
+  typedef std::set<RequestImpl*> PendingRequests;
 
   enum State {
     STATE_NONE,
@@ -355,7 +358,7 @@ class NET_EXPORT ProxyResolutionService
                          const std::string& method,
                          ProxyInfo* results,
                          CompletionOnceCallback callback,
-                         Request** request,
+                         std::unique_ptr<Request>* request,
                          ProxyDelegate* proxy_delegate,
                          const NetLogWithSource& net_log);
 
@@ -368,10 +371,10 @@ class NET_EXPORT ProxyResolutionService
   void SetReady();
 
   // Returns true if |pending_requests_| contains |req|.
-  bool ContainsPendingRequest(Request* req);
+  bool ContainsPendingRequest(RequestImpl* req);
 
   // Removes |req| from the list of pending requests.
-  void RemovePendingRequest(Request* req);
+  void RemovePendingRequest(RequestImpl* req);
 
   // Called when proxy resolution has completed (either synchronously or
   // asynchronously). Handles logging the result, and cleaning out
@@ -472,6 +475,10 @@ class NET_EXPORT ProxyResolutionService
 
   THREAD_CHECKER(thread_checker_);
 
+  // Flag used by |SetReady()| to check if |this| has been deleted by a
+  // synchronous callback.
+  base::WeakPtrFactory<ProxyResolutionService> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ProxyResolutionService);
 };
 

net/url_request/url_request_ftp_job.cc

@@ -46,7 +46,6 @@ URLRequestFtpJob::URLRequestFtpJob(
       priority_(DEFAULT_PRIORITY),
       proxy_resolution_service_(
           request_->context()->proxy_resolution_service()),
-      proxy_resolve_request_(NULL),
       http_response_info_(NULL),
       read_in_progress_(false),
       ftp_transaction_factory_(ftp_transaction_factory),
@@ -129,8 +128,7 @@ void URLRequestFtpJob::Start() {
 
 void URLRequestFtpJob::Kill() {
   if (proxy_resolve_request_) {
-    proxy_resolution_service_->CancelRequest(proxy_resolve_request_);
-    proxy_resolve_request_ = nullptr;
+    proxy_resolve_request_.reset();
   }
   if (ftp_transaction_)
     ftp_transaction_.reset();
@@ -282,7 +280,7 @@ void URLRequestFtpJob::RestartTransactionWithAuth() {
 
 LoadState URLRequestFtpJob::GetLoadState() const {
   if (proxy_resolve_request_)
-    return proxy_resolution_service_->GetLoadState(proxy_resolve_request_);
+    return proxy_resolve_request_->GetLoadState();
   if (proxy_info_.is_direct()) {
     return ftp_transaction_ ?
         ftp_transaction_->GetLoadState() : LOAD_STATE_IDLE;

net/url_request/url_request_ftp_job.h

@@ -81,7 +81,7 @@ class NET_EXPORT_PRIVATE URLRequestFtpJob : public URLRequestJob {
 
   ProxyResolutionService* proxy_resolution_service_;
   ProxyInfo proxy_info_;
-  ProxyResolutionService::Request* proxy_resolve_request_;
+  std::unique_ptr<ProxyResolutionService::Request> proxy_resolve_request_;
 
   FtpRequestInfo ftp_request_info_;
   std::unique_ptr<FtpTransaction> ftp_transaction_;

services/network/network_context_unittest.cc

@@ -2022,7 +2022,7 @@ TEST_F(NetworkContextTest, NoInitialProxyConfig) {
   // Before there's a proxy configuration, proxy requests should hang.
   net::ProxyInfo proxy_info;
   net::TestCompletionCallback test_callback;
-  net::ProxyResolutionService::Request* request = nullptr;
+  std::unique_ptr<net::ProxyResolutionService::Request> request = nullptr;
   ASSERT_EQ(net::ERR_IO_PENDING, proxy_resolution_service->ResolveProxy(
                                      GURL("http://bar/"), "GET", &proxy_info,
                                      test_callback.callback(), &request,

services/network/proxy_resolving_client_socket.cc

@@ -37,7 +37,6 @@ ProxyResolvingClientSocket::ProxyResolvingClientSocket(
     : network_session_(network_session),
       socket_handle_(std::make_unique<net::ClientSocketHandle>()),
       ssl_config_(ssl_config),
-      proxy_resolve_request_(nullptr),
       url_(url),
       use_tls_(use_tls),
       net_log_(net::NetLogWithSource::Make(network_session_->net_log(),
@@ -117,9 +116,7 @@ int ProxyResolvingClientSocket::Connect(net::CompletionOnceCallback callback) {
 void ProxyResolvingClientSocket::Disconnect() {
   CloseSocket(true /*close_connection*/);
   if (proxy_resolve_request_) {
-    network_session_->proxy_resolution_service()->CancelRequest(
-        proxy_resolve_request_);
-    proxy_resolve_request_ = nullptr;
+    proxy_resolve_request_.reset();
   }
   user_connect_callback_.Reset();
 }

services/network/proxy_resolving_client_socket.h

@@ -127,7 +127,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) ProxyResolvingClientSocket
   std::unique_ptr<net::ClientSocketHandle> socket_handle_;
 
   const net::SSLConfig ssl_config_;
-  net::ProxyResolutionService::Request* proxy_resolve_request_;
+  std::unique_ptr<net::ProxyResolutionService::Request> proxy_resolve_request_;
   net::ProxyInfo proxy_info_;
   const GURL url_;
   const bool use_tls_;