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

chrome/browser/io_thread.cc

 // Copyright (c) 2012 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 "chrome/browser/io_thread.h"
 
 #include <utility>
 #include <vector>
 
 #include "base/base64.h"
@@ skipping 533 matching lines @@
 #endif  // defined(OS_ANDROID)
 
   globals_->data_use_ascriber =
       base::MakeUnique<data_use_measurement::ChromeDataUseAscriber>();
 
   globals_->data_use_aggregator.reset(new data_usage::DataUseAggregator(
       std::unique_ptr<data_usage::DataUseAnnotator>(
           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(
           globals_->data_use_aggregator.get(),
           BrowserThread::GetTaskRunnerForThread(BrowserThread::IO),
           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;
   variations::GetVariationParams(kNetworkQualityEstimatorFieldTrialName,
                                  &network_quality_estimator_params);
 
   if (command_line.HasSwitch(switches::kForceEffectiveConnectionType)) {
     const std::string force_ect_value =
         base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
             switches::kForceEffectiveConnectionType);
@@ skipping 21 matching lines @@
   UpdateDnsClientEnabled();
 #if defined(OS_CHROMEOS)
   // Creates a CertVerifyProc that doesn't allow any profile-provided certs.
   globals_->cert_verifier = base::MakeUnique<net::CachingCertVerifier>(
       base::MakeUnique<net::MultiThreadedCertVerifier>(
           new chromeos::CertVerifyProcChromeOS()));
 #else
   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());
 
   globals_->ct_logs.assign(ct_logs.begin(), ct_logs.end());
 
   net::MultiLogCTVerifier* ct_verifier = new net::MultiLogCTVerifier();
   globals_->cert_transparency_verifier.reset(ct_verifier);
   // Add built-in logs
   ct_verifier->AddLogs(globals_->ct_logs);
 
   ct_tree_tracker_.reset(new certificate_transparency::TreeStateTracker(
       globals_->ct_logs, net_log_));
   // Register the ct_tree_tracker_ as observer for new STHs.
   RegisterSTHObserver(ct_tree_tracker_.get());
   // 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(
         command_line.GetSwitchValueASCII(switches::kHostRules));
     TRACE_EVENT_END0("startup", "IOThread::InitAsync:SetRulesFromString");
   }
   globals_->enable_brotli =
       base::FeatureList::IsEnabled(features::kBrotliEncoding);
   params_.enable_token_binding =
       base::FeatureList::IsEnabled(features::kTokenBinding);
@@ skipping 205 matching lines @@
   globals_->system_request_context_storage =
       base::MakeUnique<net::URLRequestContextStorage>(context);
   net::URLRequestContextStorage* context_storage =
       globals_->system_request_context_storage.get();
 
   context->set_network_quality_estimator(
       globals_->network_quality_estimator.get());
   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());
 
   // In-memory cookie store.
   context_storage->set_cookie_store(
       content::CreateCookieStore(content::CookieStoreConfig()));
   // In-memory channel ID store.
   context_storage->set_channel_id_service(
       base::MakeUnique<net::ChannelIDService>(
           new net::DefaultChannelIDStore(nullptr)));
   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");
   globals_->proxy_script_fetcher_context.reset(
       ConstructProxyScriptFetcherContext(globals_, params_, net_log_));
   TRACE_EVENT_END0("startup",
                    "IOThread::Init:ProxyScriptFetcherRequestContext");
 
   const base::CommandLine& command_line =
       *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(
       context, &system_params);
 
   context_storage->set_http_network_session(
       base::MakeUnique<net::HttpNetworkSession>(system_params));
   context_storage->set_http_transaction_factory(
       base::MakeUnique<net::HttpNetworkLayer>(
           context_storage->http_network_session()));
@@ skipping 99 matching lines @@
 // static
 net::URLRequestContext* IOThread::ConstructProxyScriptFetcherContext(
     IOThread::Globals* globals,
     const net::HttpNetworkSession::Params& params,
     net::NetLog* net_log) {
   net::URLRequestContext* context = new net::URLRequestContext;
   context->set_net_log(net_log);
   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());
 
   context->set_job_factory(
       globals->proxy_script_fetcher_url_request_job_factory.get());
 
   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);
 
   net::HttpNetworkSession::Params session_params(params);
   net::URLRequestContextBuilder::SetHttpNetworkSessionComponents(
       context, &session_params);
 
   globals->proxy_script_fetcher_http_network_session.reset(
       new net::HttpNetworkSession(session_params));
   globals->proxy_script_fetcher_http_transaction_factory.reset(
@@ skipping 27 matching lines @@
 
   // TODO(rtenneti): We should probably use HttpServerPropertiesManager for the
   // system URLRequestContext too. There's no reason this should be tied to a
   // profile.
   return context;
 }
 
 metrics::UpdateUsagePrefCallbackType IOThread::GetMetricsDataUseForwarder() {
   return base::Bind(&UpdateMetricsUsagePrefsOnUIThread);
 }