Make GCM's ConnectionFactoryImpl uses ProxyResolvingClientSocketFactory

ProxyResolvingClientSocketFactory takes care of creating a new
HttpNetworkSession that is based on a given net::URLRequestContext.

This CL switches GCM's ConnectionFactoryImpl to using
ProxyResolvingClientSocketFactory, so there is less code duplication. This
will makes it easier to standardize usage of ProxyResolvingClientSocket to
prepare for servicifying it.

Bug: 844187
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I06b1f6b7c6fc7e8c458b24f2ee3dcc5664bd4c21
Reviewed-on: https://chromium-review.googlesource.com/1101058
Commit-Queue: Helen Li <xunjieli@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Reviewed-by: Nicolas Zea <zea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567674}

components/gcm_driver/gcm_client_impl.cc

@@ -41,8 +41,6 @@
 #include "google_apis/gcm/monitoring/gcm_stats_recorder.h"
 #include "google_apis/gcm/protocol/checkin.pb.h"
 #include "google_apis/gcm/protocol/mcs.pb.h"
-#include "net/http/http_network_session.h"
-#include "net/http/http_transaction_factory.h"
 #include "net/url_request/url_request_context.h"
 #include "url/gurl.h"
 
@@ -280,12 +278,10 @@ std::unique_ptr<MCSClient> GCMInternalsBuilder::BuildMCSClient(
 std::unique_ptr<ConnectionFactory> GCMInternalsBuilder::BuildConnectionFactory(
     const std::vector<GURL>& endpoints,
     const net::BackoffEntry::Policy& backoff_policy,
-    net::HttpNetworkSession* gcm_network_session,
-    net::HttpNetworkSession* http_network_session,
+    net::URLRequestContext* url_request_context,
     GCMStatsRecorder* recorder) {
-  return base::WrapUnique<ConnectionFactory>(
-      new ConnectionFactoryImpl(endpoints, backoff_policy, gcm_network_session,
-                                http_network_session, nullptr, recorder));
+  return base::WrapUnique<ConnectionFactory>(new ConnectionFactoryImpl(
+      endpoints, backoff_policy, url_request_context, recorder));
 }
 
 GCMClientImpl::CheckinInfo::CheckinInfo()
@@ -342,16 +338,6 @@ void GCMClientImpl::Initialize(
   DCHECK(delegate);
 
   url_request_context_getter_ = url_request_context_getter;
-  const net::HttpNetworkSession::Params* network_session_params =
-      url_request_context_getter_->GetURLRequestContext()->
-          GetNetworkSessionParams();
-  DCHECK(network_session_params);
-  const net::HttpNetworkSession::Context* network_session_context =
-      url_request_context_getter_->GetURLRequestContext()
-          ->GetNetworkSessionContext();
-  network_session_.reset(new net::HttpNetworkSession(*network_session_params,
-                                                     *network_session_context));
-
   chrome_build_info_ = chrome_build_info;
 
   gcm_store_.reset(
@@ -508,13 +494,8 @@ void GCMClientImpl::InitializeMCSClient() {
   if (fallback_endpoint.is_valid())
     endpoints.push_back(fallback_endpoint);
   connection_factory_ = internals_builder_->BuildConnectionFactory(
-      endpoints,
-      GetGCMBackoffPolicy(),
-      network_session_.get(),
-      url_request_context_getter_->GetURLRequestContext()
-          ->http_transaction_factory()
-          ->GetSession(),
-      &recorder_);
+      endpoints, GetGCMBackoffPolicy(),
+      url_request_context_getter_->GetURLRequestContext(), &recorder_);
   connection_factory_->SetConnectionListener(this);
   mcs_client_ = internals_builder_->BuildMCSClient(
       chrome_build_info_.version, clock_, connection_factory_.get(),

components/gcm_driver/gcm_client_impl.h

@@ -43,7 +43,7 @@ class DataMessageStanza;
 }  // namespace mcs_proto
 
 namespace net {
-class HttpNetworkSession;
+class URLRequestContext;
 }  // namespace net
 
 namespace gcm {
@@ -69,8 +69,7 @@ class GCMInternalsBuilder {
   virtual std::unique_ptr<ConnectionFactory> BuildConnectionFactory(
       const std::vector<GURL>& endpoints,
       const net::BackoffEntry::Policy& backoff_policy,
-      net::HttpNetworkSession* gcm_network_session,
-      net::HttpNetworkSession* http_network_session,
+      net::URLRequestContext* url_request_context,
       GCMStatsRecorder* recorder);
 };
 
@@ -363,7 +362,6 @@ class GCMClientImpl
   // resetting and loading from the store again and again.
   bool gcm_store_reset_;
 
-  std::unique_ptr<net::HttpNetworkSession> network_session_;
   std::unique_ptr<ConnectionFactory> connection_factory_;
   scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
 

components/gcm_driver/gcm_client_impl_unittest.cc

@@ -226,8 +226,7 @@ class FakeGCMInternalsBuilder : public GCMInternalsBuilder {
   std::unique_ptr<ConnectionFactory> BuildConnectionFactory(
       const std::vector<GURL>& endpoints,
       const net::BackoffEntry::Policy& backoff_policy,
-      net::HttpNetworkSession* gcm_network_session,
-      net::HttpNetworkSession* http_network_session,
+      net::URLRequestContext* url_request_context,
       GCMStatsRecorder* recorder) override;
 
  private:
@@ -257,8 +256,7 @@ std::unique_ptr<ConnectionFactory>
 FakeGCMInternalsBuilder::BuildConnectionFactory(
     const std::vector<GURL>& endpoints,
     const net::BackoffEntry::Policy& backoff_policy,
-    net::HttpNetworkSession* gcm_network_session,
-    net::HttpNetworkSession* http_network_session,
+    net::URLRequestContext* url_request_context,
     GCMStatsRecorder* recorder) {
   return base::WrapUnique<ConnectionFactory>(new FakeConnectionFactory());
 }

google_apis/gcm/engine/connection_factory_impl.cc

@@ -16,7 +16,6 @@
 #include "google_apis/gcm/monitoring/gcm_stats_recorder.h"
 #include "google_apis/gcm/protocol/mcs.pb.h"
 #include "net/base/net_errors.h"
-#include "net/http/http_network_session.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/proxy_fallback.h"
 #include "net/log/net_log_source_type.h"
@@ -25,6 +24,7 @@
 #include "net/socket/client_socket_pool_manager.h"
 #include "net/ssl/ssl_config_service.h"
 #include "services/network/proxy_resolving_client_socket.h"
+#include "services/network/proxy_resolving_client_socket_factory.h"
 
 namespace gcm {
 
@@ -52,16 +52,15 @@ bool ShouldRestorePreviousBackoff(const base::TimeTicks& login_time,
 ConnectionFactoryImpl::ConnectionFactoryImpl(
     const std::vector<GURL>& mcs_endpoints,
     const net::BackoffEntry::Policy& backoff_policy,
-    net::HttpNetworkSession* gcm_network_session,
-    net::HttpNetworkSession* http_network_session,
-    net::NetLog* net_log,
+    net::URLRequestContext* url_request_context,
     GCMStatsRecorder* recorder)
     : mcs_endpoints_(mcs_endpoints),
       next_endpoint_(0),
       last_successful_endpoint_(0),
       backoff_policy_(backoff_policy),
-      gcm_network_session_(gcm_network_session),
-      http_network_session_(http_network_session),
+      socket_factory_(
+          std::make_unique<network::ProxyResolvingClientSocketFactory>(
+              url_request_context)),
       connecting_(false),
       waiting_for_backoff_(false),
       waiting_for_network_online_(false),
@@ -70,8 +69,6 @@ ConnectionFactoryImpl::ConnectionFactoryImpl(
       listener_(NULL),
       weak_ptr_factory_(this) {
   DCHECK_GE(mcs_endpoints_.size(), 1U);
-  DCHECK(!http_network_session_ ||
-         (gcm_network_session_ != http_network_session_));
 }
 
 ConnectionFactoryImpl::~ConnectionFactoryImpl() {
@@ -319,11 +316,7 @@ void ConnectionFactoryImpl::StartConnection() {
   connecting_ = true;
   GURL current_endpoint = GetCurrentEndpoint();
   recorder_->RecordConnectionInitiated(current_endpoint.host());
-  UpdateFromHttpNetworkSession();
-  net::SSLConfig ssl_config;
-  gcm_network_session_->ssl_config_service()->GetSSLConfig(&ssl_config);
-  socket_ = std::make_unique<network::ProxyResolvingClientSocket>(
-      gcm_network_session_, ssl_config, current_endpoint, true /*use_tls*/);
+  socket_ = socket_factory_->CreateSocket(current_endpoint, true /*use_tls*/);
   int status = socket_->Connect(base::BindRepeating(
       &ConnectionFactoryImpl::OnConnectDone, weak_ptr_factory_.GetWeakPtr()));
   if (status != net::ERR_IO_PENDING)
@@ -472,15 +465,4 @@ void ConnectionFactoryImpl::CloseSocket() {
   socket_ = nullptr;
 }
 
-void ConnectionFactoryImpl::UpdateFromHttpNetworkSession() {
-  if (!http_network_session_ || !http_network_session_->http_auth_cache())
-    return;
-
-  gcm_network_session_->http_auth_cache()->UpdateAllFrom(
-      *http_network_session_->http_auth_cache());
-
-  if (!http_network_session_->IsQuicEnabled())
-    gcm_network_session_->DisableQuic();
-}
-
 }  // namespace gcm

google_apis/gcm/engine/connection_factory_impl.h

@@ -22,11 +22,11 @@
 
 namespace network {
 class ProxyResolvingClientSocket;
+class ProxyResolvingClientSocketFactory;
 }
 
 namespace net {
-class HttpNetworkSession;
-class NetLog;
+class URLRequestContext;
 }
 
 namespace gcm {
@@ -37,20 +37,12 @@ class GCM_EXPORT ConnectionFactoryImpl :
     public ConnectionFactory,
     public net::NetworkChangeNotifier::NetworkChangeObserver {
  public:
-  // |http_network_session| is an optional network session to use as a source
-  // for proxy auth credentials (via its HttpAuthCache). |gcm_network_session|
-  // is the network session through which GCM connections should be made, and
-  // must not be the same as |http_network_session|.
-  //
   // The caller is responsible for making sure the ConnectionFactoryImpl is
-  // destroyed before the |gcm_network_session| and |http_network_session|.
-  ConnectionFactoryImpl(
-      const std::vector<GURL>& mcs_endpoints,
-      const net::BackoffEntry::Policy& backoff_policy,
-      net::HttpNetworkSession* gcm_network_session,
-      net::HttpNetworkSession* http_network_session,
-      net::NetLog* net_log,
-      GCMStatsRecorder* recorder);
+  // destroyed before the |url_request_context|.
+  ConnectionFactoryImpl(const std::vector<GURL>& mcs_endpoints,
+                        const net::BackoffEntry::Policy& backoff_policy,
+                        net::URLRequestContext* url_request_context,
+                        GCMStatsRecorder* recorder);
   ~ConnectionFactoryImpl() override;
 
   // ConnectionFactory implementation.
@@ -147,11 +139,8 @@ class GCM_EXPORT ConnectionFactoryImpl :
   const net::BackoffEntry::Policy backoff_policy_;
 
   // ---- net:: components for establishing connections. ----
-  // Network session for creating new GCM connections.
-  net::HttpNetworkSession* gcm_network_session_;
-  // HTTP Network session. If set, is used for extracting proxy auth
-  // credentials. If nullptr, is ignored.
-  net::HttpNetworkSession* http_network_session_;
+  // Socket factory for creating new GCM connections.
+  std::unique_ptr<network::ProxyResolvingClientSocketFactory> socket_factory_;
   // The handle to the socket for the current connection, if one exists.
   std::unique_ptr<network::ProxyResolvingClientSocket> socket_;
   // Current backoff entry.

google_apis/gcm/engine/connection_factory_impl_unittest.cc

@@ -16,8 +16,8 @@
 #include "google_apis/gcm/engine/fake_connection_handler.h"
 #include "google_apis/gcm/monitoring/fake_gcm_stats_recorder.h"
 #include "net/base/backoff_entry.h"
-#include "net/http/http_network_session.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
+#include "net/url_request/url_request_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 class Policy;
@@ -86,7 +86,8 @@ void WriteContinuation() {
 // backoff policy.
 class TestConnectionFactoryImpl : public ConnectionFactoryImpl {
  public:
-  TestConnectionFactoryImpl(const base::Closure& finished_callback);
+  TestConnectionFactoryImpl(net::URLRequestContext* request_context,
+                            const base::Closure& finished_callback);
   ~TestConnectionFactoryImpl() override;
 
   void InitializeFactory();
@@ -141,12 +142,11 @@ class TestConnectionFactoryImpl : public ConnectionFactoryImpl {
 };
 
 TestConnectionFactoryImpl::TestConnectionFactoryImpl(
+    net::URLRequestContext* request_context,
     const base::Closure& finished_callback)
     : ConnectionFactoryImpl(BuildEndpoints(),
                             net::BackoffEntry::Policy(),
-                            NULL,
-                            NULL,
-                            NULL,
+                            request_context,
                             &dummy_recorder_),
       connect_result_(net::ERR_UNEXPECTED),
       num_expected_attempts_(0),
@@ -274,16 +274,23 @@ class ConnectionFactoryImplTest
  private:
   void ConnectionsComplete();
 
-  TestConnectionFactoryImpl factory_;
   base::MessageLoop message_loop_;
+
+  // Dummy request context that is not used to make network requests, and is
+  // added to make ProxyResolvingClientSocketFactory to not DCHECK on a null
+  // context.
+  net::TestURLRequestContext request_context_;
+
+  TestConnectionFactoryImpl factory_;
   std::unique_ptr<base::RunLoop> run_loop_;
 
   GURL connected_server_;
 };
 
 ConnectionFactoryImplTest::ConnectionFactoryImplTest()
-    : factory_(base::Bind(&ConnectionFactoryImplTest::ConnectionsComplete,
-                         base::Unretained(this))),
+    : factory_(&request_context_,
+               base::Bind(&ConnectionFactoryImplTest::ConnectionsComplete,
+                          base::Unretained(this))),
       run_loop_(new base::RunLoop()) {
   factory()->SetConnectionListener(this);
   factory()->Initialize(

google_apis/gcm/tools/mcs_probe.cc

@@ -216,7 +216,6 @@ class MCSProbe {
  private:
   void CheckIn();
   void InitializeNetworkState();
-  void BuildNetworkSession();
 
   void LoadCallback(std::unique_ptr<GCMStore::LoadResult> load_result);
   void UpdateCallback(bool success);
@@ -305,14 +304,13 @@ MCSProbe::~MCSProbe() {
 void MCSProbe::Start() {
   file_thread_.Start();
   InitializeNetworkState();
-  BuildNetworkSession();
   std::vector<GURL> endpoints(
       1, GURL("https://" +
               net::HostPortPair(server_host_, server_port_).ToString()));
 
   connection_factory_ = std::make_unique<ConnectionFactoryImpl>(
-      endpoints, kDefaultBackoffPolicy, network_session_.get(), nullptr,
-      &net_log_, &recorder_);
+      endpoints, kDefaultBackoffPolicy,
+      url_request_context_getter_->GetURLRequestContext(), &recorder_);
   gcm_store_ = std::make_unique<GCMStoreImpl>(
       gcm_store_path_, file_thread_.task_runner(),
       std::make_unique<FakeEncryptor>());
@@ -389,30 +387,6 @@ void MCSProbe::InitializeNetworkState() {
       net::ProxyResolutionService::CreateDirectWithNetLog(&net_log_);
 }
 
-void MCSProbe::BuildNetworkSession() {
-  net::HttpNetworkSession::Params session_params;
-  session_params.ignore_certificate_errors = true;
-  session_params.testing_fixed_http_port = 0;
-  session_params.testing_fixed_https_port = 0;
-
-  net::HttpNetworkSession::Context session_context;
-  session_context.host_resolver = host_resolver_.get();
-  session_context.cert_verifier = cert_verifier_.get();
-  session_context.channel_id_service = system_channel_id_service_.get();
-  session_context.transport_security_state = transport_security_state_.get();
-  session_context.cert_transparency_verifier =
-      cert_transparency_verifier_.get();
-  session_context.ct_policy_enforcer = ct_policy_enforcer_.get();
-  session_context.ssl_config_service = new net::SSLConfigServiceDefaults();
-  session_context.http_auth_handler_factory = http_auth_handler_factory_.get();
-  session_context.http_server_properties = http_server_properties_.get();
-  session_context.net_log = &net_log_;
-  session_context.proxy_resolution_service = proxy_resolution_service_.get();
-
-  network_session_ = std::make_unique<net::HttpNetworkSession>(session_params,
-                                                               session_context);
-}
-
 void MCSProbe::ErrorCallback() {
   LOG(INFO) << "MCS error happened";
 }

services/network/proxy_resolving_client_socket_factory.cc

@@ -59,6 +59,11 @@ ProxyResolvingClientSocketFactory::ProxyResolvingClientSocketFactory(
     session_params.enable_http2 = reference_params->enable_http2;
     session_params.enable_http2_alternative_service =
         reference_params->enable_http2_alternative_service;
+    // Note that ProxyResolvingClientSocket uses either
+    // net::InitSocketHandleForTlsConnect() or
+    // net::InitSocketHandleForRawConnect() to establish connections through
+    // socket pools. QUIC's connection establishment is in another path, so
+    // enabling QUIC won't do anything here.
   }
 
   network_session_ = std::make_unique<net::HttpNetworkSession>(session_params,