Extract locking behaviour from ProcessSingleton.

chrome/browser/chrome_browser_main.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/chrome_browser_main.h"
 
 #if defined(TOOLKIT_GTK)
 #include <gtk/gtk.h>
 #endif
 
@@ skipping 476 matching lines @@
   // CRLSetFetcher attempts to load a CRL set from either the local disk or
   // network.
   if (!command_line.HasSwitch(switches::kDisableCRLSets))
     g_browser_process->crl_set_fetcher()->StartInitialLoad(cus);
 
   RegisterPnaclComponent(cus, command_line);
 
   cus->Start();
 }
 
+#if !defined(OS_ANDROID)
 bool ProcessSingletonNotificationCallback(
     const CommandLine& command_line,
     const base::FilePath& current_directory) {
   // Drop the request if the browser process is already in shutdown path.
   if (!g_browser_process || g_browser_process->IsShuttingDown())
     return false;
 
   g_browser_process->PlatformSpecificCommandLineProcessing(command_line);
 
   // TODO(erikwright): Consider removing this - AFAIK it is no longer used.
@@ skipping 19 matching lines @@
 
   base::FilePath user_data_dir =
       g_browser_process->profile_manager()->user_data_dir();
   base::FilePath startup_profile_dir =
       GetStartupProfilePath(user_data_dir, command_line);
 
   StartupBrowserCreator::ProcessCommandLineAlreadyRunning(
       command_line, current_directory, startup_profile_dir);
   return true;
 }
+#endif
 
 void LaunchDevToolsHandlerIfNeeded(Profile* profile,
                                    const CommandLine& command_line) {
   if (command_line.HasSwitch(::switches::kRemoteDebuggingPort)) {
     std::string port_str =
         command_line.GetSwitchValueASCII(::switches::kRemoteDebuggingPort);
     int port;
     if (base::StringToInt(port_str, &port) && port > 0 && port < 65535) {
       std::string frontend_str;
       if (command_line.HasSwitch(::switches::kRemoteDebuggingFrontend)) {
@@ skipping 55 matching lines @@
 
 ChromeBrowserMainParts::ChromeBrowserMainParts(
     const content::MainFunctionParams& parameters)
     : parameters_(parameters),
       parsed_command_line_(parameters.command_line),
       result_code_(content::RESULT_CODE_NORMAL_EXIT),
       startup_watcher_(new StartupTimeBomb()),
       shutdown_watcher_(new ShutdownWatcherHelper()),
       startup_timer_(new performance_monitor::StartupTimer()),
       browser_field_trials_(parameters.command_line),
+#if !defined(OS_ANDROID)
+      startup_lock_(base::Bind(&ProcessSingletonNotificationCallback)),
+      modal_dialog_lock_(startup_lock_.AsNotificationCallback()),
+#endif
       record_search_engine_(false),
       translate_manager_(NULL),
       profile_(NULL),
       run_message_loop_(true),
       notify_result_(ProcessSingleton::PROCESS_NONE),
       do_first_run_tasks_(false),
       local_state_(NULL),
       restart_last_session_(false) {
   // If we're running tests (ui_task is non-null).
   if (parameters.ui_task) {
@@ skipping 199 matching lines @@
       << "Must be able to get user data directory!";
 #endif
 
   // Whether this is First Run. |do_first_run_tasks_| should be prefered to this
   // unless the desire is actually to know whether this is really First Run
   // (i.e., even if --no-first-run is passed).
   bool is_first_run = false;
   // Android's first run is done in Java instead of native.
 #if !defined(OS_ANDROID)
 
+  // Pass command-line invocations through modal_dialog_lock_ and
+  // startup_lock_ before handling them ourselves.
   process_singleton_.reset(new ProcessSingleton(
-      user_data_dir_, base::Bind(&ProcessSingletonNotificationCallback)));
-  // Ensure ProcessSingleton won't process messages too early. It will be
-  // unlocked in PostBrowserStart().
-  process_singleton_->Lock(NULL);
+      user_data_dir_, modal_dialog_lock_.AsNotificationCallback()));
 
   bool force_first_run =
       parsed_command_line().HasSwitch(switches::kForceFirstRun);
   bool force_skip_first_run_tasks =
       (!force_first_run &&
        parsed_command_line().HasSwitch(switches::kNoFirstRun));
 
   is_first_run =
       (force_first_run || first_run::IsChromeFirstRun()) &&
       !ProfileManager::IsImportProcess(parsed_command_line());
@@ skipping 217 matching lines @@
 void ChromeBrowserMainParts::PostBrowserStart() {
 #if !defined(OS_ANDROID)
   if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kVisitURLs))
     RunPageCycler();
 #endif
 
   for (size_t i = 0; i < chrome_extra_parts_.size(); ++i)
     chrome_extra_parts_[i]->PostBrowserStart();
 #if !defined(OS_ANDROID)
   // Allow ProcessSingleton to process messages.
-  process_singleton_->Unlock();
+  startup_lock_.Unlock();
 #endif
 }
 
 #if !defined(OS_ANDROID)
 void ChromeBrowserMainParts::RunPageCycler() {
   CommandLine* command_line = CommandLine::ForCurrentProcess();
   // We assume a native desktop for tests, but we will need to find a way to
   // get the proper host desktop type once we start running these tests in ASH.
   Browser* browser = chrome::FindBrowserWithProfile(
       profile_, chrome::HOST_DESKTOP_TYPE_NATIVE);
@@ skipping 154 matching lines @@
   std::string try_chrome =
       parsed_command_line().GetSwitchValueASCII(switches::kTryChromeAgain);
   if (!try_chrome.empty()) {
 #if defined(OS_WIN)
     // Setup.exe has determined that we need to run a retention experiment
     // and has lauched chrome to show the experiment UI. It is guaranteed that
     // no other Chrome is currently running as the process singleton was
     // sucessfully grabbed above.
     int try_chrome_int;
     base::StringToInt(try_chrome, &try_chrome_int);
-    TryChromeDialogView::Result answer =
-        TryChromeDialogView::Show(try_chrome_int, process_singleton_.get());
+    TryChromeDialogView::Result answer = TryChromeDialogView::Show(
+        try_chrome_int,
+        base::Bind(
+            &ProcessSingletonModalDialogLock::SetActiveModalDialog,
+            base::Unretained(&modal_dialog_lock_)));
     if (answer == TryChromeDialogView::NOT_NOW)
       return chrome::RESULT_CODE_NORMAL_EXIT_CANCEL;
     if (answer == TryChromeDialogView::UNINSTALL_CHROME)
       return chrome::RESULT_CODE_NORMAL_EXIT_EXP2;
     // At this point the user is willing to try chrome again.
     if (answer == TryChromeDialogView::TRY_CHROME_AS_DEFAULT) {
       // Only set in the unattended case, the interactive case is Windows 8.
       if (ShellIntegration::CanSetAsDefaultBrowser() ==
           ShellIntegration::SET_DEFAULT_UNATTENDED)
         ShellIntegration::SetAsDefaultBrowser();
@@ skipping 78 matching lines @@
   // Create an instance of GpuModeManager to watch gpu mode pref change.
   g_browser_process->gpu_mode_manager();
 
 #if !defined(OS_ANDROID)
   // Show the First Run UI if this is the first time Chrome has been run on
   // this computer, or we're being compelled to do so by a command line flag.
   // 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 (do_first_run_tasks_) {
+    DCHECK(startup_lock_.locked());

[Nico, 2013/03/28 04:50:09]

How bad is it if this fails? Should this be a CHECK instead?

[gab, 2013/03/28 13:45:53]

The process singleton is already locked all the way until the end of
PostBrowserStart().

In fact the extra locking in AutoImport was wrong as the Lock() didn't have a
stacking behavior (and thus unlocking would fully unlock right at the end of
AutoImport; way too early -- this refactoring might in fact fix some
mysterious/undiagnosed/hard-to-repro rendez-vous crashes).

It's a good thing this extra locking/unlocking was removed, but I don't feel
replacing it by a DCHECK here has much value (otherwise might as well DCHECK for
the lock in every other part of the Chrome initialization...).

[erikwright (departed), 2013/04/04 01:39:51]

I added this because I wanted to demonstrate that the code change is safe (i.e.,
that it's not necessary to call Lock() inside AutoImport()).

As gab says, you could argue for putting such a check all over the
initialization code, so I'll remove it.

     first_run::AutoImport(profile_,
                           master_prefs_->homepage_defined,
                           master_prefs_->do_import_items,
-                          master_prefs_->dont_import_items,
-                          process_singleton_.get());
+                          master_prefs_->dont_import_items);
     // Note: this can pop the first run consent dialog on linux.
     first_run::DoPostImportTasks(profile_, master_prefs_->make_chrome_default);
 
     browser_process_->profile_manager()->OnImportFinished(profile_);
 
     if (!master_prefs_->suppress_first_run_default_browser_prompt) {
       browser_creator_->set_show_main_browser_window(
           !chrome::ShowFirstRunDefaultBrowserPrompt(profile_));
     } else {
       browser_creator_->set_is_default_browser_dialog_suppressed(true);
@@ skipping 452 matching lines @@
     if (base::win::GetVersion() <= base::win::VERSION_XP)
       uma_name += "_XP";
 
     uma_name += "_PreRead_";
     uma_name += pre_read_percentage;
     AddPreReadHistogramTime(uma_name.c_str(), time);
   }
 #endif
 #endif
 }