IOThread: Move more stuff into system context's URLRequestContextStore.

chrome/browser/io_thread.cc

Index: chrome/browser/io_thread.cc
diff --git a/chrome/browser/io_thread.cc b/chrome/browser/io_thread.cc
index 6ef847c2fc088e65420e7eabacf800a35342aa8a..ef55b09e42c1608ef0962c4afee548d54931736a 100644
--- a/chrome/browser/io_thread.cc
+++ b/chrome/browser/io_thread.cc
@@ -551,14 +551,6 @@ void IOThread::Init() {
           new chrome_browser_data_usage::TabIdAnnotator()),
       std::move(data_use_amortizer)));
 
-  std::unique_ptr<ChromeNetworkDelegate> chrome_network_delegate(
-      new ChromeNetworkDelegate(extension_event_router_forwarder(),
-                                &system_enable_referrers_));
-  // By default, data usage is considered off the record.
-  chrome_network_delegate->set_data_use_aggregator(
-      globals_->data_use_aggregator.get(),
-      true /* is_data_usage_off_the_record */);
-
 #if defined(OS_ANDROID)
   globals_->external_data_use_observer.reset(
       new chrome::android::ExternalDataUseObserver(
@@ -567,10 +559,6 @@ void IOThread::Init() {
           BrowserThread::GetTaskRunnerForThread(BrowserThread::UI)));
 #endif  // defined(OS_ANDROID)
 
-  globals_->system_network_delegate =
-      globals_->data_use_ascriber->CreateNetworkDelegate(
-          std::move(chrome_network_delegate), GetMetricsDataUseForwarder());
-
   globals_->host_resolver = CreateGlobalHostResolver(net_log_);
 
   std::map<std::string, std::string> network_quality_estimator_params;
@@ -612,8 +600,6 @@ void IOThread::Init() {
   globals_->cert_verifier = net::CertVerifier::CreateDefault();
 #endif
 
-  globals_->transport_security_state.reset(new net::TransportSecurityState());
-
   std::vector<scoped_refptr<const net::CTLogVerifier>> ct_logs(
       net::ct::CreateLogVerifiersForKnownLogs());
 
@@ -631,21 +617,13 @@ void IOThread::Init() {
   // Register the ct_tree_tracker_ as observer for verified SCTs.
   globals_->cert_transparency_verifier->SetObserver(ct_tree_tracker_.get());
 
-  globals_->ct_policy_enforcer.reset(new net::CTPolicyEnforcer());
-  params_.ct_policy_enforcer = globals_->ct_policy_enforcer.get();

[Randy Smith (Not in Mondays), 2017/05/11 18:05:42]

I spent some time trying to convince myself that this was safe and failed; want
to help me out?  The problems I see are:

* params_ is used in CreateHttpNetworkSession() in
ProfileIOData::InitializeInternal().  I'll admit I didn't check that the
HttpNetworkSession references HttpNetworkSession::Params::ct_policy_enforcer,
but if it doesn't, that seems like it's surprising enough to be unwise to rely
on.

* params_ is reconfigured after having various things set in in by
ConfigureParamsFromFieldTrialAndCommandLine().  Even if that function doesn't
alter the ct_policy_enforcer, it seems like it could easily in the future, and
avoiding using params for transmitting the ct_policy_enforcer seems a mistake.

[mmenke, 2017/05/11 18:15:31]

The last line in this method calls ConstructSystemRequestContext() (Note that
the first CL in this change, still out for review, moves the
ConstructSystemRequestContext call there.  On trunk, this CL would not be safe,
in isolation), and nothing in the method calls into ProfileIOData, since this
isn't a profile-scoped class.
It doesn't make any sense for a command line flag to affect only the IOThread's
CtPolicyEnforcer.  So it would affect the CtPolicyEnforcer globally, which is
the one that's owned by the SystemURLRequestContext.  Also note that the
net::URLRequestContextBuilder::SetHttpNetworkSessionComponents call overrides
that member of HttpNetworkSession params, anyways, so it's effectively ignored.

Regardless, that method won't exist in the future - I'm going to need to unify
the logic to set params so I can send the individual values we need over to the
network service, so that's going away (And we can't pass an entire
CtPolicyEnforcer across processes), anyways.  I'll have to clean this up before
I can switch over to a builder, anyways, since I can't pass it a raw
HttpNetworkSession::Params, since it creates half of the things that are needed
to populate them.

[Randy Smith (Not in Mondays), 2017/05/11 21:06:10]

The above doesn't seem relevant to me, since
ProfileIOData::CreateHttpNetworkSession reaches into
io_thread->NetworkSessionParams(), which is params_.  But your comment below
about URLRequestContextBuilder::SetupHttpNetworkSessionComponents() nuking the
ct_policy_enforcer resolves my concerns about potential behavior change here.  
I'd rather not have the code this confusing and suggestive that it is a behavior
change, but given that you're heading for nuking the relevant pieces in the near
future, I won't worry about it in the short term.
Ah!  Ok, I was assuming it was an IN parameter, and yep, it doesn't look like it
is.  

[mmenke, 2017/05/11 21:12:08]

We call IOThread::Init when we create the IO thread, when we start it.  So
ProfileIOData cannot do anything on the IOThread before IOThread::Init is
called, and IOThread::Init has always created globals, and now creates the
SystemURLRequestContext as well.

Anyhow, I agree that this is a mess.  Currently, URLRequestContextBuilders own
everything passed to them.  So when moving ProfileIOData over to using
URLRequestContextBuilder (After IOThread), it should become clearer what is
shared, and I'll expect in the network service for those to be globals.  I'm not
doing it here mostly because globals that are part of the system request context
and those that aren't are hopelessly intertwined, and I think it's simplest not
to worry about which is what (At least right now).

-
-  globals_->ssl_config_service = GetSSLConfigService();
-
   CreateDefaultAuthHandlerFactory();
-  globals_->http_server_properties.reset(new net::HttpServerPropertiesImpl());
   // For the ProxyScriptFetcher, we use a direct ProxyService.
   globals_->proxy_script_fetcher_proxy_service =
       net::ProxyService::CreateDirectWithNetLog(net_log_);
   globals_->dns_probe_service.reset(new chrome_browser_net::DnsProbeService());
   globals_->host_mapping_rules.reset(new net::HostMappingRules());
   params_.host_mapping_rules = globals_->host_mapping_rules.get();
-  globals_->http_user_agent_settings.reset(
-      new net::StaticHttpUserAgentSettings(std::string(), GetUserAgent()));
   if (command_line.HasSwitch(switches::kHostRules)) {
     TRACE_EVENT_BEGIN0("startup", "IOThread::InitAsync:SetRulesFromString");
     globals_->host_mapping_rules->SetRulesFromString(
@@ -871,13 +849,23 @@ void IOThread::ConstructSystemRequestContext() {
   context->set_enable_brotli(globals_->enable_brotli);
   context->set_name("system");
 
-  context->set_http_user_agent_settings(
-      globals_->http_user_agent_settings.get());
-  context->set_network_delegate(globals_->system_network_delegate.get());
+  context_storage->set_http_user_agent_settings(
+      base::MakeUnique<net::StaticHttpUserAgentSettings>(std::string(),
+                                                         GetUserAgent()));
+  std::unique_ptr<ChromeNetworkDelegate> chrome_network_delegate(
+      new ChromeNetworkDelegate(extension_event_router_forwarder(),
+                                &system_enable_referrers_));
+  // By default, data usage is considered off the record.
+  chrome_network_delegate->set_data_use_aggregator(
+      globals_->data_use_aggregator.get(),
+      true /* is_data_usage_off_the_record */);
+  context_storage->set_network_delegate(
+      globals_->data_use_ascriber->CreateNetworkDelegate(
+          std::move(chrome_network_delegate), GetMetricsDataUseForwarder()));
   context->set_net_log(net_log_);
   context->set_host_resolver(globals_->host_resolver.get());
 
-  context->set_ssl_config_service(globals_->ssl_config_service.get());
+  context_storage->set_ssl_config_service(GetSSLConfigService());
   context->set_http_auth_handler_factory(
       globals_->http_auth_handler_factory.get());
 
@@ -891,15 +879,17 @@ void IOThread::ConstructSystemRequestContext() {
   context->cookie_store()->SetChannelIDServiceID(
       context->channel_id_service()->GetUniqueID());
 
-  context->set_transport_security_state(
-      globals_->transport_security_state.get());
+  context_storage->set_transport_security_state(
+      base::MakeUnique<net::TransportSecurityState>());
 
-  context->set_http_server_properties(globals_->http_server_properties.get());
+  context_storage->set_http_server_properties(
+      base::MakeUnique<net::HttpServerPropertiesImpl>());
 
   context->set_cert_verifier(globals_->cert_verifier.get());
   context->set_cert_transparency_verifier(
       globals_->cert_transparency_verifier.get());
-  context->set_ct_policy_enforcer(globals_->ct_policy_enforcer.get());
+  context_storage->set_ct_policy_enforcer(
+      base::MakeUnique<net::CTPolicyEnforcer>());
 
   TRACE_EVENT_BEGIN0("startup",
                      "IOThread::Init:ProxyScriptFetcherRequestContext");
@@ -912,9 +902,8 @@ void IOThread::ConstructSystemRequestContext() {
       *base::CommandLine::ForCurrentProcess();
   context_storage->set_proxy_service(ProxyServiceFactory::CreateProxyService(
       net_log_, globals_->proxy_script_fetcher_context.get(),
-      globals_->system_network_delegate.get(),
-      std::move(system_proxy_config_service_), command_line,
-      WpadQuickCheckEnabled(), PacHttpsUrlStrippingEnabled()));
+      context->network_delegate(), std::move(system_proxy_config_service_),
+      command_line, WpadQuickCheckEnabled(), PacHttpsUrlStrippingEnabled()));
 
   net::HttpNetworkSession::Params system_params(params_);
   net::URLRequestContextBuilder::SetHttpNetworkSessionComponents(
@@ -1034,11 +1023,13 @@ net::URLRequestContext* IOThread::ConstructProxyScriptFetcherContext(
   context->set_host_resolver(globals->host_resolver.get());
   context->set_cert_verifier(globals->cert_verifier.get());
   context->set_transport_security_state(
-      globals->transport_security_state.get());
+      globals->system_request_context->transport_security_state());
   context->set_cert_transparency_verifier(
       globals->cert_transparency_verifier.get());
-  context->set_ct_policy_enforcer(globals->ct_policy_enforcer.get());
-  context->set_ssl_config_service(globals->ssl_config_service.get());
+  context->set_ct_policy_enforcer(
+      globals->system_request_context->ct_policy_enforcer());
+  context->set_ssl_config_service(
+      globals->system_request_context->ssl_config_service());
   context->set_http_auth_handler_factory(
       globals->http_auth_handler_factory.get());
   context->set_proxy_service(globals->proxy_script_fetcher_proxy_service.get());
@@ -1049,10 +1040,11 @@ net::URLRequestContext* IOThread::ConstructProxyScriptFetcherContext(
   context->set_cookie_store(globals->system_request_context->cookie_store());
   context->set_channel_id_service(
       globals->system_request_context->channel_id_service());
-  context->set_network_delegate(globals->system_network_delegate.get());
+  context->set_network_delegate(context->network_delegate());
   context->set_http_user_agent_settings(
-      globals->http_user_agent_settings.get());
-  context->set_http_server_properties(globals->http_server_properties.get());
+      globals->system_request_context->http_user_agent_settings());
+  context->set_http_server_properties(
+      globals->system_request_context->http_server_properties());
 
   context->set_enable_brotli(globals->enable_brotli);