[Cronet] change InitializeOnNetworkThread to use URLRequestContextBuilder

components/cronet/ios/cronet_environment.cc

 // Copyright 2016 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 "components/cronet/ios/cronet_environment.h"
 
 #include <utility>
 
 #include "base/at_exit.h"
 #include "base/atomicops.h"
@@ skipping 35 matching lines @@
 #include "net/http/http_util.h"
 #include "net/log/net_log.h"
 #include "net/log/net_log_capture_mode.h"
 #include "net/log/write_to_file_net_log_observer.h"
 #include "net/proxy/proxy_service.h"
 #include "net/socket/ssl_client_socket.h"
 #include "net/ssl/channel_id_service.h"
 #include "net/ssl/default_channel_id_store.h"
 #include "net/ssl/ssl_config_service_defaults.h"
 #include "net/url_request/static_http_user_agent_settings.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_builder.h"
 #include "net/url_request/url_request_context_storage.h"
 #include "net/url_request/url_request_job_factory_impl.h"
 #include "url/scheme_host_port.h"
 #include "url/url_util.h"
 
 namespace {
 
 base::AtExitManager* g_at_exit_ = nullptr;
 net::NetworkChangeNotifier* g_network_change_notifier = nullptr;
 // MessageLoop on the main thread.
@@ skipping 200 matching lines @@
                                  base::Unretained(this)));
 }
 
 CronetEnvironment::~CronetEnvironment() {
 // net::HTTPProtocolHandlerDelegate::SetInstance(nullptr);
 }
 
 void CronetEnvironment::InitializeOnNetworkThread() {
   DCHECK(network_io_thread_->task_runner()->BelongsToCurrentThread());
   base::FeatureList::InitializeInstance(std::string(), std::string());
-  // TODO(mef): Use net:UrlRequestContextBuilder instead of manual build.
-  main_context_.reset(new net::URLRequestContext);
-  main_context_->set_net_log(net_log_.get());
 
   if (user_agent_partial_)
     user_agent_ = web::BuildUserAgentFromProduct(user_agent_);
 
-  main_context_->set_http_user_agent_settings(
-      new net::StaticHttpUserAgentSettings(accept_language_, user_agent_));
-
-  main_context_->set_ssl_config_service(new net::SSLConfigServiceDefaults);
-  main_context_->set_transport_security_state(
-      new net::TransportSecurityState());
-  http_server_properties_.reset(new net::HttpServerPropertiesImpl());
-  main_context_->set_http_server_properties(http_server_properties_.get());
-
-  // TODO(rdsmith): Note that the ".release()" calls below are leaking
-  // the objects in question; this should be fixed by having an object
-  // corresponding to URLRequestContextStorage that actually owns those
-  // objects.  See http://crbug.com/523858.
-  std::unique_ptr<net::MappedHostResolver> mapped_host_resolver(
-      new net::MappedHostResolver(
-          net::HostResolver::CreateDefaultResolver(nullptr)));
-
-  mapped_host_resolver->SetRulesFromString(host_resolver_rules_);
-  main_context_->set_host_resolver(mapped_host_resolver.release());
-
   if (!cert_verifier_)
     cert_verifier_ = net::CertVerifier::CreateDefault();

[mmenke, 2016/11/16 16:41:20]

Not needed:  URLRequestContextConfig will do this by default.  Maybe remove this
line and rename cert_verifier_ to mock_cert_verifier_ (And rename the setter as
well)?  That's what URLRequestContextConfig calls it, and that's the only reason
we need to keep code here to set it.

[lilyhoughton, 2016/11/22 19:44:01]

Done.

-  main_context_->set_cert_verifier(cert_verifier_.get());
-
-  std::unique_ptr<net::MultiLogCTVerifier> ct_verifier =
-      base::MakeUnique<net::MultiLogCTVerifier>();
-  ct_verifier->AddLogs(net::ct::CreateLogVerifiersForKnownLogs());
-  main_context_->set_cert_transparency_verifier(ct_verifier.release());
-  main_context_->set_ct_policy_enforcer(new net::CTPolicyEnforcer());
-
-  main_context_->set_http_auth_handler_factory(
-      net::HttpAuthHandlerRegistryFactory::CreateDefault(
-          main_context_->host_resolver())
-          .release());
-  main_context_->set_proxy_service(
-      net::ProxyService::CreateUsingSystemProxyResolver(
-          std::move(proxy_config_service_), 0, nullptr)
-          .release());
 
   // Cache
   base::FilePath cache_path;
   if (!PathService::Get(base::DIR_CACHE, &cache_path))
     return;
   cache_path = cache_path.Append(FILE_PATH_LITERAL("cronet"));
-  std::unique_ptr<net::HttpCache::DefaultBackend> main_backend(
-      new net::HttpCache::DefaultBackend(net::DISK_CACHE,
-                                         net::CACHE_BACKEND_SIMPLE, cache_path,
-                                         0,  // Default cache size.
-                                         network_cache_thread_->task_runner()));
 
-  net::HttpNetworkSession::Params params;
+  std::unique_ptr<URLRequestContextConfig> config(new URLRequestContextConfig(

[mmenke, 2016/11/16 16:41:20]

include components/cronet/url_request_context_config.h?

[lilyhoughton, 2016/11/22 19:44:01]

This was in cronet_environment.h.  That file didn't use it, however, so I've
moved it into this one.

+      quic_enabled_, "", http2_enabled_, false, URLRequestContextConfig::DISK,
+      0, false, cache_path.value(), user_agent_, "{}", "", "", "", "",
+      std::move(cert_verifier_), false, true, ""));

[mmenke, 2016/11/16 16:41:20]

Should have comments labeling all the inscrutable arguments here.  i.e.

true /* should_take_mondays_off */,
"" /* answer_to_question_of_life_and_death */,
"{}" /* etc */

[lilyhoughton, 2016/11/22 19:44:01]

Done.

 
-  params.host_resolver = main_context_->host_resolver();
-  params.cert_verifier = main_context_->cert_verifier();
-  params.cert_transparency_verifier =
-      main_context_->cert_transparency_verifier();
-  params.ct_policy_enforcer = main_context_->ct_policy_enforcer();
-  params.channel_id_service = main_context_->channel_id_service();
-  params.transport_security_state = main_context_->transport_security_state();
-  params.proxy_service = main_context_->proxy_service();
-  params.ssl_config_service = main_context_->ssl_config_service();
-  params.http_auth_handler_factory = main_context_->http_auth_handler_factory();
-  params.http_server_properties = main_context_->http_server_properties();
-  params.net_log = main_context_->net_log();
-  params.enable_http2 = http2_enabled();
-  params.enable_quic = quic_enabled();
+  net::URLRequestContextBuilder context_builder;
 
+  context_builder.set_accept_language(accept_language_);
+
+  config->ConfigureURLRequestContextBuilder(&context_builder, net_log_.get(),
+                                            file_thread_.get()->task_runner());
+
+  std::unique_ptr<net::MappedHostResolver> mapped_host_resolver(
+      new net::MappedHostResolver(
+          net::HostResolver::CreateDefaultResolver(nullptr)));

[mmenke, 2016/11/16 16:41:20]

nullptr -> net_log_.get() (That was a bug in the old code)

[lilyhoughton, 2016/11/22 19:44:01]

Done.

What's the bug / would it be nontrivial to add a test case for it?

[mmenke, 2016/11/22 19:50:42]

It means we wouldn't have been emitting DNS lookup events to the NetLog.  I
suspect it's not worth testing that all individual classes are hooked up to
logging infrastructure, given that no one has noticed this in how many ever
years this has been broken, and it doesn't result in user-visible failures.

+
+  mapped_host_resolver->SetRulesFromString(host_resolver_rules_);
+  context_builder.set_host_resolver(std::move(mapped_host_resolver));
+
+  std::unordered_set<std::string> quic_host_whitelist;
+  std::unique_ptr<net::HttpServerProperties> http_server_properties(
+      new net::HttpServerPropertiesImpl());
   for (const auto& quic_hint : quic_hints_) {
     net::AlternativeService alternative_service(net::AlternateProtocol::QUIC,
                                                 "", quic_hint.port());
     url::SchemeHostPort quic_hint_server("https", quic_hint.host(),
                                          quic_hint.port());
-    main_context_->http_server_properties()->SetAlternativeService(
+    http_server_properties->SetAlternativeService(
         quic_hint_server, alternative_service, base::Time::Max());
-    params.quic_host_whitelist.insert(quic_hint.host());
+    quic_host_whitelist.insert(quic_hint.host());
   }
 
-  if (!params.channel_id_service) {
-    // The main context may not have a ChannelIDService, since it is lazily
-    // constructed. If not, build an ephemeral ChannelIDService with no backing
-    // disk store.
-    // TODO(ellyjones): support persisting ChannelID.
-    params.channel_id_service =
-        new net::ChannelIDService(new net::DefaultChannelIDStore(NULL),
-                                  base::WorkerPool::GetTaskRunner(true));
-  }
+  context_builder.SetHttpServerProperties(std::move(http_server_properties));
+  context_builder.set_quic_host_whitelist(quic_host_whitelist);
 
-  // TODO(mmenke):  These really shouldn't be leaked.
-  //                See https://crbug.com/523858.
-  net::HttpNetworkSession* http_network_session =
-      new net::HttpNetworkSession(params);
-  net::HttpCache* main_cache =
-      new net::HttpCache(http_network_session, std::move(main_backend),
-                         true /* set_up_quic_server_info */);
-  main_context_->set_http_transaction_factory(main_cache);
-
-  net::URLRequestJobFactoryImpl* job_factory =
-      new net::URLRequestJobFactoryImpl;
-  main_context_->set_job_factory(job_factory);
-  main_context_->set_net_log(net_log_.get());
+  main_context_ = context_builder.Build();
 }
 
 std::string CronetEnvironment::user_agent() {
   const net::HttpUserAgentSettings* user_agent_settings =
       main_context_->http_user_agent_settings();
   if (!user_agent_settings) {
     return nullptr;
   }
 
   return user_agent_settings->GetUserAgent();
 }
 
 std::vector<uint8_t> CronetEnvironment::GetHistogramDeltas() {
   base::StatisticsRecorder::Initialize();
   std::vector<uint8_t> data;
   if (!HistogramManager::GetInstance()->GetDeltas(&data))
     return std::vector<uint8_t>();
   return data;
 }
 
 }  // namespace cronet