Have content/ create and destroy its own threads.

chrome/browser/chrome_browser_main.cc

Index: chrome/browser/chrome_browser_main.cc
diff --git a/chrome/browser/chrome_browser_main.cc b/chrome/browser/chrome_browser_main.cc
index 30cf27cd159a480e41640dab75d21f5bbdce9a99..88fc605e416e55be9c5b18c66e1eec3098e4f438 100644
--- a/chrome/browser/chrome_browser_main.cc
+++ b/chrome/browser/chrome_browser_main.cc
@@ -74,7 +74,6 @@
 #include "chrome/browser/shell_integration.h"
 #include "chrome/browser/translate/translate_manager.h"
 #include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/browser_init.h"
 #include "chrome/browser/ui/webui/chrome_url_data_manager_backend.h"
 #include "chrome/browser/web_resource/gpu_blacklist_updater.h"
 #include "chrome/common/child_process_logging.h"
@@ -94,6 +93,7 @@
 #include "content/browser/renderer_host/resource_dispatcher_host.h"
 #include "content/common/child_process.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/io_thread_delegate.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/main_function_params.h"
 #include "grit/app_locale_settings.h"
@@ -308,23 +308,6 @@ void InitializeURLRequestThrottlerManager(net::NetLog* net_log) {
   net::URLRequestThrottlerManager::GetInstance()->set_net_log(net_log);
 }
 
-// Creates key child threads. We need to do this explicitly since
-// BrowserThread::PostTask silently deletes a posted task if the target message
-// loop isn't created.
-void CreateChildThreads(BrowserProcessImpl* process) {
-  process->db_thread();
-  process->file_thread();
-  process->process_launcher_thread();
-  process->cache_thread();
-  process->io_thread();
-#if defined(OS_CHROMEOS)
-  process->web_socket_proxy_thread();
-#endif
-  // Create watchdog thread after creating all other threads because it will
-  // watch the other threads and they must be running.
-  process->watchdog_thread();
-}
-
 // Returns the new local state object, guaranteed non-NULL.
 PrefService* InitializeLocalState(const CommandLine& parsed_command_line,
                                   bool is_first_run) {
@@ -675,7 +658,12 @@ ChromeBrowserMainParts::ChromeBrowserMainParts(
       translate_manager_(NULL),
       profile_(NULL),
       run_message_loop_(true),
-      notify_result_(ProcessSingleton::PROCESS_NONE) {
+      notify_result_(ProcessSingleton::PROCESS_NONE),
+      is_first_run_(false),
+      first_run_ui_bypass_(false),
+      metrics_(NULL),
+      local_state_(NULL),
+      restart_last_session_(false) {
   // If we're running tests (ui_task is non-null).
   if (parameters.ui_task)
     browser_defaults::enable_help_app = false;
@@ -1178,33 +1166,37 @@ void ChromeBrowserMainParts::PreMainMessageLoopStart() {
 void ChromeBrowserMainParts::PostMainMessageLoopStart() {
 }
 
-void ChromeBrowserMainParts::PreMainMessageLoopRun() {
+content::IOThreadDelegate* ChromeBrowserMainParts::PreMainMessageLoopRun() {
   result_code_ = PreMainMessageLoopRunImpl();
+  return browser_process_->io_thread();
+}
+
+void ChromeBrowserMainParts::PreMainMessageLoopRunThreadsCreated() {
+  result_code_ = PreMainMessageLoopRunThreadsCreatedImpl();
 }
 
 int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
   run_message_loop_ = false;
-  FilePath user_data_dir;
 #if defined(OS_WIN)
-  PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
+  PathService::Get(chrome::DIR_USER_DATA, &user_data_dir_);
 #else
   // Getting the user data dir can fail if the directory isn't
   // creatable, for example; on Windows in code below we bring up a
   // dialog prompting the user to pick a different directory.
   // However, ProcessSingleton needs a real user_data_dir on Mac/Linux,
   // so it's better to fail here than fail mysteriously elsewhere.
-  CHECK(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir))
+  CHECK(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir_))
       << "Must be able to get user data directory!";
 #endif
 
-  process_singleton_.reset(new ProcessSingleton(user_data_dir));
+  process_singleton_.reset(new ProcessSingleton(user_data_dir_));
 
-  bool is_first_run = FirstRun::IsChromeFirstRun() ||
+  is_first_run_ = FirstRun::IsChromeFirstRun() ||
       parsed_command_line().HasSwitch(switches::kFirstRun);
 
 #if defined(USE_AURA)
   // No first run on Aura. See crbug.com/99439.
-  is_first_run = false;
+  is_first_run_ = false;
 #endif
 
   if (parsed_command_line().HasSwitch(switches::kImport) ||
@@ -1213,7 +1205,7 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
     // instantiated (as it makes a net::URLRequest and we don't have an IO
     // thread, see bug #1292702).
     browser_process_.reset(new FirstRunBrowserProcess(parsed_command_line()));
-    is_first_run = false;
+    is_first_run_ = false;
   } else {
     browser_process_.reset(new BrowserProcessImpl(parsed_command_line()));
   }
@@ -1231,8 +1223,8 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
   // tabs.
   g_browser_process->tab_closeable_state_watcher();
 
-  PrefService* local_state = InitializeLocalState(parsed_command_line(),
-                                                  is_first_run);
+  local_state_ = InitializeLocalState(parsed_command_line(),
+                                      is_first_run_);

[robertshield, 2011/11/14 04:15:55]

It looks like local_state_ and metrics_ will be dangling pointers once
g_browser_process is destroyed, which happens in FinalCleanup() iiuc.

Should these two be explicitly NULLed out at some point?

 
 #if defined(USE_LINUX_BREAKPAD)
   // Needs to be called after we have chrome::DIR_USER_DATA and
@@ -1240,7 +1232,7 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
   g_browser_process->file_thread()->message_loop()->PostTask(FROM_HERE,
       new GetLinuxDistroTask());
 
-  if (IsCrashReportingEnabled(local_state))
+  if (IsCrashReportingEnabled(local_state_))
     InitCrashReporter();
 #endif
 
@@ -1256,7 +1248,7 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
     g_browser_process->SetApplicationLocale(l10n_util::GetLocaleOverride());
 #else
     const std::string locale =
-        local_state->GetString(prefs::kApplicationLocale);
+        local_state_->GetString(prefs::kApplicationLocale);
     // On a POSIX OS other than ChromeOS, the parameter that is passed to the
     // method InitSharedInstance is ignored.
     const std::string loaded_locale =
@@ -1316,19 +1308,19 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
   // browser's profile_manager object is created, but after ResourceBundle
   // is initialized.
   master_prefs_.reset(new FirstRun::MasterPrefs);
-  bool first_run_ui_bypass = false;  // True to skip first run UI.
-  if (is_first_run) {
-    first_run_ui_bypass =
-        !FirstRun::ProcessMasterPreferences(user_data_dir, master_prefs_.get());
+  first_run_ui_bypass_ = false;  // True to skip first run UI.
+  if (is_first_run_) {
+    first_run_ui_bypass_ = !FirstRun::ProcessMasterPreferences(
+        user_data_dir_, master_prefs_.get());
     AddFirstRunNewTabs(browser_init_.get(), master_prefs_->new_tabs);
 
     // If we are running in App mode, we do not want to show the importer
     // (first run) UI.
-    if (!first_run_ui_bypass &&
+    if (!first_run_ui_bypass_ &&
         (parsed_command_line().HasSwitch(switches::kApp) ||
          parsed_command_line().HasSwitch(switches::kAppId) ||
          parsed_command_line().HasSwitch(switches::kNoFirstRun)))
-      first_run_ui_bypass = true;
+      first_run_ui_bypass_ = true;
   }
 
   // TODO(viettrungluu): why don't we run this earlier?
@@ -1337,12 +1329,12 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
 
   // Enable print preview once for supported platforms.
 #if defined(GOOGLE_CHROME_BUILD)
-  local_state->RegisterBooleanPref(prefs::kPrintingPrintPreviewEnabledOnce,
+  local_state_->RegisterBooleanPref(prefs::kPrintingPrintPreviewEnabledOnce,
                                    false,
                                    PrefService::UNSYNCABLE_PREF);
-  if (!local_state->GetBoolean(prefs::kPrintingPrintPreviewEnabledOnce)) {
-    local_state->SetBoolean(prefs::kPrintingPrintPreviewEnabledOnce, true);
-    about_flags::SetExperimentEnabled(local_state, "print-preview", true);
+  if (!local_state_->GetBoolean(prefs::kPrintingPrintPreviewEnabledOnce)) {
+    local_state_->SetBoolean(prefs::kPrintingPrintPreviewEnabledOnce, true);
+    about_flags::SetExperimentEnabled(local_state_, "print-preview", true);
   }
 #endif
 
@@ -1356,7 +1348,7 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
 #endif
 
   // Convert active labs into switches. Modifies the current command line.
-  about_flags::ConvertFlagsToSwitches(local_state,
+  about_flags::ConvertFlagsToSwitches(local_state_,
                                       CommandLine::ForCurrentProcess());
 
   // Reset the command line in the crash report details, since we may have
@@ -1375,7 +1367,7 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
 
   // Now the command line has been mutated based on about:flags, we can
   // set up metrics and initialize field trials.
-  MetricsService* metrics = SetupMetricsAndFieldTrials(local_state);
+  metrics_ = SetupMetricsAndFieldTrials(local_state_);
 
 #if defined(USE_WEBKIT_COMPOSITOR)
   // We need to ensure WebKit has been initialized before we start the WebKit
@@ -1387,9 +1379,9 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
   // for the uninstall metrics if this is our first run. This only actually
   // gets used if the user has metrics reporting enabled at uninstall time.
   int64 install_date =
-      local_state->GetInt64(prefs::kUninstallMetricsInstallDate);
+      local_state_->GetInt64(prefs::kUninstallMetricsInstallDate);
   if (install_date == 0) {
-    local_state->SetInt64(prefs::kUninstallMetricsInstallDate,
+    local_state_->SetInt64(prefs::kUninstallMetricsInstallDate,
                           base::Time::Now().ToTimeT());
   }
 
@@ -1403,7 +1395,17 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
   SecKeychainAddCallback(&KeychainCallback, 0, NULL);
 #endif
 
-  CreateChildThreads(browser_process_.get());
+  // This initializes the IOThread object.  This needs to happen
+  // before the thread itself is created.
+  browser_process_->CreateIOThreadState();
+
+  return content::RESULT_CODE_NORMAL_EXIT;
+}
+
+int ChromeBrowserMainParts::PreMainMessageLoopRunThreadsCreatedImpl() {
+  // Create watchdog thread after creating all other threads because it will
+  // watch the other threads and they must be running.
+  browser_process_->watchdog_thread();
 
 #if defined(OS_CHROMEOS)
   // Now that the file thread exists we can record our stats.
@@ -1545,13 +1547,13 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
   }
 #endif
 
-  if (is_first_run) {
+  if (is_first_run_) {
     // Warn the ProfileManager that an import process will run, possibly
     // locking the WebDataService directory of the next Profile created.
     g_browser_process->profile_manager()->SetWillImport();
   }
 
-  profile_ = CreateProfile(parameters(), user_data_dir, parsed_command_line());
+  profile_ = CreateProfile(parameters(), user_data_dir_, parsed_command_line());
   if (!profile_)
     return content::RESULT_CODE_NORMAL_EXIT;
 
@@ -1630,8 +1632,8 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
   // Note that this be done _after_ the PrefService is initialized and all
   // preferences are registered, since some of the code that the importer
   // touches reads preferences.
-  if (is_first_run) {
-    if (!first_run_ui_bypass) {
+  if (is_first_run_) {
+    if (!first_run_ui_bypass_) {
       FirstRun::AutoImport(profile_,
                            master_prefs_->homepage_defined,
                            master_prefs_->do_import_items,
@@ -1647,9 +1649,9 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
       // If stats reporting was turned on by the first run dialog then toggle
       // the pref.
       if (GoogleUpdateSettings::GetCollectStatsConsent())
-        local_state->SetBoolean(prefs::kMetricsReportingEnabled, true);
+        local_state_->SetBoolean(prefs::kMetricsReportingEnabled, true);
 #endif  // OS_POSIX
-    }  // if (!first_run_ui_bypass)
+    }  // if (!first_run_ui_bypass_)
 
     Browser::SetNewHomePagePrefs(profile_->GetPrefs());
     g_browser_process->profile_manager()->OnImportFinished(profile_);
@@ -1768,8 +1770,8 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
 #endif
 
   HandleTestParameters(parsed_command_line());
-  RecordBreakpadStatusUMA(metrics);
-  about_flags::RecordUMAStatistics(local_state);
+  RecordBreakpadStatusUMA(metrics_);
+  about_flags::RecordUMAStatistics(local_state_);
   LanguageUsageMetrics::RecordAcceptLanguages(
       profile_->GetPrefs()->GetString(prefs::kAcceptLanguages));
   LanguageUsageMetrics::RecordApplicationLanguage(
@@ -1780,7 +1782,7 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
 #endif
 
 #if defined(OS_CHROMEOS)
-  metrics->StartExternalMetrics();
+  metrics_->StartExternalMetrics();
 
   // Initialize the audio handler on ChromeOS.
   chromeos::AudioHandler::Initialize();
@@ -1813,7 +1815,7 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
 #if defined(OS_WIN)
   // We check this here because if the profile is OTR (chromeos possibility)
   // it won't still be accessible after browser is destroyed.
-  record_search_engine_ = is_first_run && !profile_->IsOffTheRecord();
+  record_search_engine_ = is_first_run_ && !profile_->IsOffTheRecord();
 #endif
 
   // ChildProcess:: is a misnomer unless you consider context.  Use
@@ -1876,9 +1878,10 @@ void ChromeBrowserMainParts::StartBrowserOrUITask() {
 
     // We are in regular browser boot sequence. Open initial tabs and enter the
     // main message loop.
+    int result_code;
     if (browser_init_->Start(parsed_command_line(), FilePath(), profile_,
-                             &result_code_)) {
-#if defined(OS_WIN) || (defined(OS_LINUX) && !defined(OS_CHROMEOS))
+                             &result_code)) {
+#if (defined(OS_WIN) || defined(OS_LINUX)) && !defined(OS_CHROMEOS)
       // Initialize autoupdate timer. Timer callback costs basically nothing
       // when browser is not in persistent mode, so it's OK to let it ride on
       // the main thread. This needs to be done here because we don't want
@@ -2049,10 +2052,23 @@ void ChromeBrowserMainParts::PostMainMessageLoopRun() {
   chromeos::AudioHandler::Shutdown();
 #endif
 
+  restart_last_session_ = browser_shutdown::ShutdownPreThreadsStop();
+  browser_process_->StartTearDown();
+}
+
+void ChromeBrowserMainParts::PreStopThread(BrowserThread::ID identifier) {
+  browser_process_->PreStopThread(identifier);
+}
+
+void ChromeBrowserMainParts::PostStopThread(BrowserThread::ID identifier) {
+  browser_process_->PostStopThread(identifier);
+}
+
+void ChromeBrowserMainParts::FinalCleanup() {
   // browser_shutdown takes care of deleting browser_process, so we need to
   // release it.
   ignore_result(browser_process_.release());

[robertshield, 2011/11/14 04:15:55]

This isn't new to this CL, but why does ChromeBrowserMainParts bother having a
scoped_ptr on the BrowserProcessImpl instance?

Afaict, ChromeBrowserMainParts is never responsible for deleting the
BrowserProcessImpl instance, making the ownership implied by the scoped_ptr
misleading at best.

In fact, it looks like it would be an error for browser_process_ to be destroyed
as a result of this ChromeBrowserMainParts instance being destroyed without
FinalCleanup() having been called. (Maybe a CHECK in ~ChromeBrowserMainParts()
to ensure that FinalCleanup HAS been called?)

Perhaps browser_process_ should be removed and existing usages replaced with 
g_browser_process?

-  browser_shutdown::Shutdown();
+  browser_shutdown::ShutdownPostThreadsStop(restart_last_session_);
   master_prefs_.reset();
   process_singleton_.reset();