Move IOThread initialization to happen asynchronously.

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 <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 445 matching lines @@
 }
 
 net::URLRequestContextGetter* IOThread::system_url_request_context_getter() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (!system_url_request_context_getter_.get()) {
     InitSystemRequestContext();
   }
   return system_url_request_context_getter_.get();
 }
 
-void IOThread::Init() {
-  TRACE_EVENT0("startup", "IOThread::Init");
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-
+void IOThread::DoInitTasks() {
 #if defined(USE_NSS) || defined(OS_IOS)
   net::SetMessageLoopForNSSHttpIO();
 #endif
 
-  const CommandLine& command_line = *CommandLine::ForCurrentProcess();
-
   DCHECK(!globals_);
   globals_ = new Globals;
 
+  const CommandLine& command_line = *CommandLine::ForCurrentProcess();
+
   // Add an observer that will emit network change events to the ChromeNetLog.
   // Assuming NetworkChangeNotifier dispatches in FIFO order, we should be
   // logging the network change before other IO thread consumers respond to it.
-  network_change_observer_.reset(
-      new LoggingNetworkChangeObserver(net_log_));
+  network_change_observer_.reset(new LoggingNetworkChangeObserver(net_log_));
 
   // Setup the HistogramWatcher to run on the IO thread.
   net::NetworkChangeNotifier::InitHistogramWatcher();
 
   globals_->extension_event_router_forwarder =
       extension_event_router_forwarder_;
-  ChromeNetworkDelegate* network_delegate =
-      new ChromeNetworkDelegate(extension_event_router_forwarder_,
-                                &system_enable_referrers_);
+  ChromeNetworkDelegate* network_delegate = new ChromeNetworkDelegate(
+      extension_event_router_forwarder_, &system_enable_referrers_);
   if (command_line.HasSwitch(switches::kDisableExtensionsHttpThrottling))
     network_delegate->NeverThrottleRequests();
   globals_->system_network_delegate.reset(network_delegate);
   globals_->host_resolver = CreateGlobalHostResolver(net_log_);
   UpdateDnsClientEnabled();
   globals_->cert_verifier.reset(net::CertVerifier::CreateDefault());
   globals_->transport_security_state.reset(new net::TransportSecurityState());
   globals_->ssl_config_service = GetSSLConfigService();
   if (command_line.HasSwitch(switches::kSpdyProxyAuthOrigin)) {
     spdyproxy_auth_origin_ =
         command_line.GetSwitchValueASCII(switches::kSpdyProxyAuthOrigin);
   } else {
 #if defined(SPDY_PROXY_AUTH_ORIGIN)
     spdyproxy_auth_origin_ = SPDY_PROXY_AUTH_ORIGIN;
 #endif
   }
-  globals_->http_auth_handler_factory.reset(CreateDefaultAuthHandlerFactory(
-      globals_->host_resolver.get()));
+  globals_->http_auth_handler_factory
+      .reset(CreateDefaultAuthHandlerFactory(globals_->host_resolver.get()));

[mmenke, 2013/07/10 18:48:24]

nit:  For all of these, it's much more common across the chrome code base to put
the method on the same name as the class.  I don't think I've ever seen this
indentation style used in chrome, actually, except perhaps where it was
necessary due to variable name length.

[mmenke, 2013/07/10 18:51:04]

"On the same line as the object name", rather.

   globals_->http_server_properties.reset(new net::HttpServerPropertiesImpl);
   // For the ProxyScriptFetcher, we use a direct ProxyService.
-  globals_->proxy_script_fetcher_proxy_service.reset(
-      net::ProxyService::CreateDirectWithNetLog(net_log_));
+  globals_->proxy_script_fetcher_proxy_service
+      .reset(net::ProxyService::CreateDirectWithNetLog(net_log_));
   // In-memory cookie store.
   globals_->system_cookie_store = new net::CookieMonster(NULL, NULL);
   // In-memory server bound cert store.
-  globals_->system_server_bound_cert_service.reset(
-      new net::ServerBoundCertService(
-          new net::DefaultServerBoundCertStore(NULL),
-          base::WorkerPool::GetTaskRunner(true)));
+  globals_->system_server_bound_cert_service
+      .reset(new net::ServerBoundCertService(
+           new net::DefaultServerBoundCertStore(NULL),
+           base::WorkerPool::GetTaskRunner(true)));
   globals_->dns_probe_service.reset(new chrome_browser_net::DnsProbeService());
   globals_->load_time_stats.reset(new chrome_browser_net::LoadTimeStats());
   globals_->host_mapping_rules.reset(new net::HostMappingRules());
-  globals_->http_user_agent_settings.reset(
-      new BasicHttpUserAgentSettings(std::string()));
+  globals_->http_user_agent_settings
+      .reset(new BasicHttpUserAgentSettings(std::string()));
   if (command_line.HasSwitch(switches::kHostRules)) {
     TRACE_EVENT_BEGIN0("startup", "IOThread::Init:SetRulesFromString");
     globals_->host_mapping_rules->SetRulesFromString(
         command_line.GetSwitchValueASCII(switches::kHostRules));
     TRACE_EVENT_END0("startup", "IOThread::Init:SetRulesFromString");
   }
   if (command_line.HasSwitch(switches::kIgnoreCertificateErrors))
     globals_->ignore_certificate_errors = true;
   if (command_line.HasSwitch(switches::kTestingFixedHttpPort)) {
     globals_->testing_fixed_http_port =
         GetSwitchValueAsInt(command_line, switches::kTestingFixedHttpPort);
   }
   if (command_line.HasSwitch(switches::kTestingFixedHttpsPort)) {
     globals_->testing_fixed_https_port =
         GetSwitchValueAsInt(command_line, switches::kTestingFixedHttpsPort);
   }
   globals_->enable_quic.set(ShouldEnableQuic(command_line));
   if (command_line.HasSwitch(switches::kOriginToForceQuicOn)) {
-    net::HostPortPair quic_origin =
-        net::HostPortPair::FromString(
-            command_line.GetSwitchValueASCII(switches::kOriginToForceQuicOn));
+    net::HostPortPair quic_origin = net::HostPortPair::FromString(
+        command_line.GetSwitchValueASCII(switches::kOriginToForceQuicOn));
     if (!quic_origin.IsEmpty()) {
       globals_->origin_to_force_quic_on.set(quic_origin);
     }
   }
-  if (command_line.HasSwitch(
-          switches::kEnableUserAlternateProtocolPorts)) {
+  if (command_line.HasSwitch(switches::kEnableUserAlternateProtocolPorts)) {
     globals_->enable_user_alternate_protocol_ports = true;
   }
   InitializeNetworkOptions(command_line);
 
   net::HttpNetworkSession::Params session_params;
   InitializeNetworkSessionParams(&session_params);
   session_params.net_log = net_log_;
   session_params.proxy_service =
       globals_->proxy_script_fetcher_proxy_service.get();
 
   TRACE_EVENT_BEGIN0("startup", "IOThread::Init:HttpNetworkSession");
   scoped_refptr<net::HttpNetworkSession> network_session(
       new net::HttpNetworkSession(session_params));
   globals_->proxy_script_fetcher_http_transaction_factory
       .reset(new net::HttpNetworkLayer(network_session.get()));
   TRACE_EVENT_END0("startup", "IOThread::Init:HttpNetworkSession");
   scoped_ptr<net::URLRequestJobFactoryImpl> job_factory(
       new net::URLRequestJobFactoryImpl());
   job_factory->SetProtocolHandler(chrome::kDataScheme,
                                   new net::DataProtocolHandler());
   job_factory->SetProtocolHandler(chrome::kFileScheme,
                                   new net::FileProtocolHandler());
 #if !defined(DISABLE_FTP_SUPPORT)
-  globals_->proxy_script_fetcher_ftp_transaction_factory.reset(
-      new net::FtpNetworkLayer(globals_->host_resolver.get()));
+  globals_->proxy_script_fetcher_ftp_transaction_factory
+      .reset(new net::FtpNetworkLayer(globals_->host_resolver.get()));
   job_factory->SetProtocolHandler(
       chrome::kFtpScheme,
       new net::FtpProtocolHandler(
           globals_->proxy_script_fetcher_ftp_transaction_factory.get()));
 #endif
   globals_->proxy_script_fetcher_url_request_job_factory =
       job_factory.PassAs<net::URLRequestJobFactory>();
 
   globals_->throttler_manager.reset(new net::URLRequestThrottlerManager());
   globals_->throttler_manager->set_net_log(net_log_);
   // Always done in production, disabled only for unit tests.
   globals_->throttler_manager->set_enable_thread_checks(true);
 
-  globals_->proxy_script_fetcher_context.reset(
-      ConstructProxyScriptFetcherContext(globals_, net_log_));
+  globals_->proxy_script_fetcher_context
+      .reset(ConstructProxyScriptFetcherContext(globals_, net_log_));
 
-  globals_->network_time_notifier.reset(
-      new net::NetworkTimeNotifier(
-          scoped_ptr<base::TickClock>(new base::DefaultTickClock())));
+  globals_->network_time_notifier.reset(new net::NetworkTimeNotifier(
+      scoped_ptr<base::TickClock>(new base::DefaultTickClock())));
 
   sdch_manager_ = new net::SdchManager();
 
 #if defined(OS_MACOSX) && !defined(OS_IOS)
   // Start observing Keychain events. This needs to be done on the UI thread,
   // as Keychain services requires a CFRunLoop.
-  BrowserThread::PostTask(BrowserThread::UI,
-                          FROM_HERE,
-                          base::Bind(&ObserveKeychainEvents));
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE, base::Bind(&ObserveKeychainEvents));
 #endif
 
   // InitSystemRequestContext turns right around and posts a task back
   // to the IO thread, so we can't let it run until we know the IO
   // thread has started.
   //
   // Note that since we are at BrowserThread::Init time, the UI thread
   // is blocked waiting for the thread to start.  Therefore, posting
   // this task to the main thread's message loop here is guaranteed to
   // get it onto the message loop while the IOThread object still
   // exists.  However, the message might not be processed on the UI
   // thread until after IOThread is gone, so use a weak pointer.
   BrowserThread::PostTask(BrowserThread::UI,
                           FROM_HERE,
                           base::Bind(&IOThread::InitSystemRequestContext,
                                      weak_factory_.GetWeakPtr()));
 }
 
+void IOThread::Init() {
+  TRACE_EVENT0("startup", "IOThread::Init");
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));

[awong, 2013/07/10 18:45:47]

This confuses me...

My understanding of the original code was:

(1) UI BrowserThread does "new IOThread()"
(2) UI BrowserThread does effectively "PostTask(UI, FROM_HERE,
Bind(&IOThread::Init)" via BrowserThreadDelegate.
(3) IO BrowserThread runs IOThread::Init.

Now we know that we can post tasks to the IO BrowserThread that assume the
global IOThread object is initialized.

In the modified code, we haven't changed the behavior of the UI thread at all.

What we do change is in (3) we now delay the initialization of the IOThread
object on the IO BrowserThread such that there is a gap in time where IOThread
is not initialized.  This seems wrong.

+
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&IOThread::DoInitTasks, base::Unretained(this)));
+}
+
 void IOThread::CleanUp() {
   base::debug::LeakTracker<SafeBrowsingURLRequestContext>::CheckForLeaks();
 
   delete sdch_manager_;
   sdch_manager_ = NULL;
 
 #if defined(USE_NSS) || defined(OS_IOS)
   net::ShutdownNSSHttpIO();
 #endif
 
   system_url_request_context_getter_ = NULL;
 
   // Release objects that the net::URLRequestContext could have been pointing
   // to.
 
   // This must be reset before the ChromeNetLog is destroyed.
   network_change_observer_.reset();
 
   system_proxy_config_service_.reset();
 
   delete globals_;
   globals_ = NULL;
 
   base::debug::LeakTracker<SystemURLRequestContextGetter>::CheckForLeaks();
 }
 
 void IOThread::InitializeNetworkOptions(const CommandLine& command_line) {
   if (command_line.HasSwitch(switches::kEnableFileCookies)) {
     // Enable cookie storage for file:// URLs.  Must do this before the first
     // Profile (and therefore the first CookieMonster) is created.

[mmenke, 2013/07/10 18:48:24]

After this CL, this can actually be done after the creation of the profile. 
Actually has to be done before URLRequestContexts are initialized on the IO
thread (Both the system one, and the per-profile ones), so we're still good. 
Comment is just more strict than what's needed.

     net::CookieMonster::EnableFileScheme();
   }
 
   // If "spdy.disabled" preference is controlled via policy, then skip use-spdy
   // command line flags.
   if (is_spdy_disabled_by_policy_)
     return;
 
   if (command_line.HasSwitch(switches::kEnableIPPooling))
     globals_->enable_spdy_ip_pooling.set(true);
@@ skipping 295 matching lines @@
       base::FieldTrialList::FindFullName(kQuicFieldTrialName);
 
   if (command_line.HasSwitch(switches::kDisableQuic))
     return false;
 
   if (command_line.HasSwitch(switches::kEnableQuic))
     return true;
 
   return quic_trial_group == kQuicFieldTrialEnabledGroupName;
 }