Extract locking behaviour from ProcessSingleton.

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 3f92f84b7750f9ae504decdb130547ec5bbfac46..f14c85efda2b6d6d3cf6324ecb0e70bca7f0dae2 100644
--- a/chrome/browser/chrome_browser_main.cc
+++ b/chrome/browser/chrome_browser_main.cc
@@ -494,6 +494,7 @@ void RegisterComponentsForUpdate(const CommandLine& command_line) {
   cus->Start();
 }
 
+#if !defined(OS_ANDROID)
 bool ProcessSingletonNotificationCallback(
     const CommandLine& command_line,
     const base::FilePath& current_directory) {
@@ -533,6 +534,7 @@ bool ProcessSingletonNotificationCallback(
       command_line, current_directory, startup_profile_dir);
   return true;
 }
+#endif
 
 void LaunchDevToolsHandlerIfNeeded(Profile* profile,
                                    const CommandLine& command_line) {
@@ -608,6 +610,10 @@ ChromeBrowserMainParts::ChromeBrowserMainParts(
       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),
@@ -827,11 +833,10 @@ int ChromeBrowserMainParts::PreCreateThreadsImpl() {
   // 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);
@@ -1069,7 +1074,7 @@ void ChromeBrowserMainParts::PostBrowserStart() {
     chrome_extra_parts_[i]->PostBrowserStart();
 #if !defined(OS_ANDROID)
   // Allow ProcessSingleton to process messages.
-  process_singleton_->Unlock();
+  startup_lock_.Unlock();
 #endif
 }
 
@@ -1244,8 +1249,11 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
     // 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)
@@ -1344,11 +1352,11 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
   // 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);