[Cronet] change InitializeOnNetworkThread to use URLRequestContextBuilder

components/cronet/ios/cronet_environment.cc

Index: components/cronet/ios/cronet_environment.cc
diff --git a/components/cronet/ios/cronet_environment.cc b/components/cronet/ios/cronet_environment.cc
index 0fef4af2e2266ab840e1b76a457f213ba763f5a6..478892f65f898a421a158d0a7d75d772e4b09d3c 100644
--- a/components/cronet/ios/cronet_environment.cc
+++ b/components/cronet/ios/cronet_environment.cc
@@ -53,6 +53,8 @@
 #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"
@@ -273,113 +275,55 @@ CronetEnvironment::~CronetEnvironment() {
 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;
-
-  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();
 
+  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.

+
+  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() {