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

This reverts commit 2ae89dce.

Reason for revert: net_unittests seems to be consistently failing after your patch landed. First failure is here: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/ios-simulator/35922 . net_unittests hangs when it gets to MultiThreadedProxyResolverTest.CancelCreate

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}

TBR=stevenjb@chromium.org,sky@chromium.org,eroman@chromium.org,boliu@chromium.org,mef@chromium.org,mmenke@chromium.org,lilyhoughton@chromium.org

Change-Id: I17026b358225cc01884be96ca5b69dcd9f885e5d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 479898
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1152368Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578474}

chromeos/dbus/services/proxy_resolution_service_provider.cc

@@ -50,9 +50,6 @@ 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;
 
@@ -163,7 +160,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, &request_ptr->request, nullptr, net::NetLogWithSource());
+      callback, nullptr, 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,7 +46,16 @@ PepperNetworkProxyHost::PepperNetworkProxyHost(BrowserPpapiHostImpl* host,
                  weak_factory_.GetWeakPtr()));
 }
 
-PepperNetworkProxyHost::~PepperNetworkProxyHost() = default;
+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::UIThreadData::UIThreadData() : is_allowed(false) {}
 
@@ -130,7 +139,7 @@ void PepperNetworkProxyHost::TryToSendUnsentRequests() {
     } else {
       // Everything looks valid, so try to resolve the proxy.
       net::ProxyInfo* proxy_info = new net::ProxyInfo;
-      std::unique_ptr<net::ProxyResolutionService::Request> pending_request;
+      net::ProxyResolutionService::Request* pending_request = nullptr;
       base::Callback<void(int)> callback =
           base::Bind(&PepperNetworkProxyHost::OnResolveProxyCompleted,
                      weak_factory_.GetWeakPtr(),
@@ -139,7 +148,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(std::move(pending_request));
+      pending_requests_.push(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,8 +99,7 @@ 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<std::unique_ptr<net::ProxyResolutionService::Request>>
-      pending_requests_;
+  base::queue<net::ProxyResolutionService::Request*> pending_requests_;
 
   base::WeakPtrFactory<PepperNetworkProxyHost> weak_factory_;
 

content/browser/resolve_proxy_msg_helper.cc

@@ -44,28 +44,44 @@ void ResolveProxyMsgHelper::OnResolveProxy(const GURL& url,
     StartPendingRequest();
 }
 
-ResolveProxyMsgHelper::~ResolveProxyMsgHelper() = default;
+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();
+}
 
 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 == pending_requests_.front().request);
+  DCHECK(nullptr == req.request);
 
   if (context_getter_.get()) {
     proxy_resolution_service_ = context_getter_->GetURLRequestContext()->proxy_resolution_service();
@@ -74,26 +90,14 @@ void ResolveProxyMsgHelper::StartPendingRequest() {
 
   // Start the request.
   int result = proxy_resolution_service_->ResolveProxy(
-      pending_requests_.front().url, std::string(), &proxy_info_,
+      req.url, std::string(), &proxy_info_,
       base::BindOnce(&ResolveProxyMsgHelper::OnResolveProxyCompleted,
                      base::Unretained(this)),
-      &pending_requests_.front().request, nullptr, net::NetLogWithSource());
+      &req.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,23 +58,17 @@ 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);
-    PendingRequest(PendingRequest&& pending_request) noexcept;
-    ~PendingRequest();
-
-    PendingRequest& operator=(PendingRequest&& pending_request) noexcept;
+    PendingRequest(const GURL& url, IPC::Message* reply_msg)
+        : url(url), reply_msg(reply_msg), request(NULL) {}
 
     // The URL of the request.
     GURL url;
 
     // Data to pass back to the delegate on completion (we own it until then).
-    std::unique_ptr<IPC::Message> reply_msg;
+    IPC::Message* reply_msg;
 
     // Handle for cancelling the current request if it has started (else NULL).
-    std::unique_ptr<net::ProxyResolutionService::Request> request;
-
-   private:
-    DISALLOW_COPY_AND_ASSIGN(PendingRequest);
+    net::ProxyResolutionService::Request* request;
   };
 
   // Info about the current outstanding proxy request.

content/test/proxy_service_mojo_unittest.cc

@@ -158,11 +158,12 @@ 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(),
-                &request, nullptr, net::NetLogWithSource()));
+                GURL("http://foo"),
+                std::string(),
+                &info, callback.callback(), nullptr,
+                nullptr, net::NetLogWithSource()));
 
   // PAC file fetcher should have a fetch triggered by the first
   // |ResolveProxy()| request.
@@ -180,11 +181,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(),
-                &request, nullptr, test_net_log.bound()));
+                GURL("http://foo"), std::string(),
+                &info, callback.callback(), nullptr,
+                nullptr, test_net_log.bound()));
 
   // PAC file fetcher should have a fetch triggered by the first
   // |ResolveProxy()| request.
@@ -213,11 +214,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(),
-                &request, nullptr, test_net_log.bound()));
+                GURL("http://foo"), std::string(),
+                &info, callback.callback(), nullptr,
+                nullptr, test_net_log.bound()));
 
   // PAC file fetcher should have a fetch triggered by the first
   // |ResolveProxy()| request.
@@ -243,11 +244,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(),
-                &request, nullptr, net::NetLogWithSource()));
+                GURL("http://foo"), std::string(),
+                &info, callback.callback(), nullptr,
+                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_);
-    proxy_resolve_request_.reset();
+    session_->proxy_resolution_service()->CancelRequest(proxy_resolve_request_);
   }
   net_log_.EndEvent(NetLogEventType::HTTP_STREAM_JOB_CONTROLLER);
 }
@@ -155,7 +155,8 @@ void HttpStreamFactory::JobController::Preconnect(int num_streams) {
 LoadState HttpStreamFactory::JobController::GetLoadState() const {
   DCHECK(request_);
   if (next_state_ == STATE_RESOLVE_PROXY_COMPLETE)
-    return proxy_resolve_request_->GetLoadState();
+    return session_->proxy_resolution_service()->GetLoadState(
+        proxy_resolve_request_);
   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_;
-  std::unique_ptr<ProxyResolutionService::Request> proxy_resolve_request_;
+  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,11 +23,9 @@
 #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;
@@ -41,7 +39,9 @@ namespace net {
 
 class DhcpPacFileFetcher;
 class NetLog;
+class NetLogWithSource;
 class ProxyDelegate;
+class ProxyResolver;
 class ProxyResolverFactory;
 class PacFileData;
 class PacFileFetcher;
@@ -123,17 +123,7 @@ class NET_EXPORT ProxyResolutionService
   ~ProxyResolutionService() override;
 
   // Used to track proxy resolution requests that complete asynchronously.
-  class Request {
-   public:
-    virtual ~Request() = default;
-    virtual LoadState GetLoadState() const = 0;
-
-   protected:
-    Request() = default;
-
-   private:
-    DISALLOW_COPY_AND_ASSIGN(Request);
-  };
+  class 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
@@ -145,9 +135,10 @@ 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,
-  // which occurs when the unique pointer to it is deleted (by leaving scope or
-  // otherwise).  |request| must not be NULL.
+  // 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.
   //
   // We use the three possible proxy access types in the following order,
   // doing fallback if one doesn't work.  See "pac_script_decider.h"
@@ -161,7 +152,7 @@ class NET_EXPORT ProxyResolutionService
                    const std::string& method,
                    ProxyInfo* results,
                    CompletionOnceCallback callback,
-                   std::unique_ptr<Request>* request,
+                   Request** request,
                    ProxyDelegate* proxy_delegate,
                    const NetLogWithSource& net_log);
 
@@ -194,6 +185,12 @@ 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(
@@ -315,11 +312,11 @@ class NET_EXPORT ProxyResolutionService
                            UpdateConfigAfterFailedAutodetect);
   FRIEND_TEST_ALL_PREFIXES(ProxyResolutionServiceTest,
                            UpdateConfigFromPACToDirect);
+  friend class Request;
   class InitProxyResolver;
   class PacFileDeciderPoller;
-  class RequestImpl;
 
-  typedef std::set<RequestImpl*> PendingRequests;
+  typedef std::set<scoped_refptr<Request>> PendingRequests;
 
   enum State {
     STATE_NONE,
@@ -358,7 +355,7 @@ class NET_EXPORT ProxyResolutionService
                          const std::string& method,
                          ProxyInfo* results,
                          CompletionOnceCallback callback,
-                         std::unique_ptr<Request>* request,
+                         Request** request,
                          ProxyDelegate* proxy_delegate,
                          const NetLogWithSource& net_log);
 
@@ -371,10 +368,10 @@ class NET_EXPORT ProxyResolutionService
   void SetReady();
 
   // Returns true if |pending_requests_| contains |req|.
-  bool ContainsPendingRequest(RequestImpl* req);
+  bool ContainsPendingRequest(Request* req);
 
   // Removes |req| from the list of pending requests.
-  void RemovePendingRequest(RequestImpl* req);
+  void RemovePendingRequest(Request* req);
 
   // Called when proxy resolution has completed (either synchronously or
   // asynchronously). Handles logging the result, and cleaning out
@@ -475,10 +472,6 @@ 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,6 +46,7 @@ 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),
@@ -128,7 +129,8 @@ void URLRequestFtpJob::Start() {
 
 void URLRequestFtpJob::Kill() {
   if (proxy_resolve_request_) {
-    proxy_resolve_request_.reset();
+    proxy_resolution_service_->CancelRequest(proxy_resolve_request_);
+    proxy_resolve_request_ = nullptr;
   }
   if (ftp_transaction_)
     ftp_transaction_.reset();
@@ -280,7 +282,7 @@ void URLRequestFtpJob::RestartTransactionWithAuth() {
 
 LoadState URLRequestFtpJob::GetLoadState() const {
   if (proxy_resolve_request_)
-    return proxy_resolve_request_->GetLoadState();
+    return proxy_resolution_service_->GetLoadState(proxy_resolve_request_);
   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_;
-  std::unique_ptr<ProxyResolutionService::Request> proxy_resolve_request_;
+  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;
-  std::unique_ptr<net::ProxyResolutionService::Request> request = nullptr;
+  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,6 +37,7 @@ 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(),
@@ -116,7 +117,9 @@ int ProxyResolvingClientSocket::Connect(net::CompletionOnceCallback callback) {
 void ProxyResolvingClientSocket::Disconnect() {
   CloseSocket(true /*close_connection*/);
   if (proxy_resolve_request_) {
-    proxy_resolve_request_.reset();
+    network_session_->proxy_resolution_service()->CancelRequest(
+        proxy_resolve_request_);
+    proxy_resolve_request_ = nullptr;
   }
   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_;
-  std::unique_ptr<net::ProxyResolutionService::Request> proxy_resolve_request_;
+  net::ProxyResolutionService::Request* proxy_resolve_request_;
   net::ProxyInfo proxy_info_;
   const GURL url_;
   const bool use_tls_;