From 4a44bc32d152f0583bb196339c2568351573e304 Mon Sep 17 00:00:00 2001
From: "mattm@chromium.org"
 <mattm@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Fri, 28 May 2010 22:22:44 +0000
Subject: ProcessSingleton(all): create the lock immediately after failing to
 connect to an existing process. ProcessSingletonLinux: if creating the lock
 fails, try to notify again.

BUG=44417
TEST=manual

Review URL: http://codereview.chromium.org/2066014

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@48533 0039d316-1c4b-4281-b951-d872f2087c98
---
 chrome/browser/browser_main.cc                   | 19 ++++----
 chrome/browser/process_singleton.h               | 12 ++++-
 chrome/browser/process_singleton_linux.cc        | 42 ++++++++++++++---
 chrome/browser/process_singleton_linux_uitest.cc | 60 ++++++++++++++++++++----
 chrome/browser/process_singleton_mac.cc          |  5 ++
 chrome/browser/process_singleton_win.cc          |  7 +++
 chrome/test/data/valgrind/ui_tests.gtest.txt     |  1 +
 chrome/test/in_process_browser_test.cc           |  6 +++
 8 files changed, 126 insertions(+), 26 deletions(-)

diff --git a/chrome/browser/browser_main.cc b/chrome/browser/browser_main.cc
index df15582..b777843 100644
--- a/chrome/browser/browser_main.cc
+++ b/chrome/browser/browser_main.cc
@@ -996,7 +996,7 @@ int BrowserMain(const MainFunctionParams& parameters) {
     // new one. NotifyOtherProcess will currently give the other process up to
     // 20 seconds to respond. Note that this needs to be done before we attempt
     // to read the profile.
-    switch (process_singleton.NotifyOtherProcess()) {
+    switch (process_singleton.NotifyOtherProcessOrCreate()) {
       case ProcessSingleton::PROCESS_NONE:
         // No process already running, fall through to starting a new one.
         break;
@@ -1011,6 +1011,14 @@ int BrowserMain(const MainFunctionParams& parameters) {
       case ProcessSingleton::PROFILE_IN_USE:
         return ResultCodes::PROFILE_IN_USE;
 
+      case ProcessSingleton::LOCK_ERROR:
+        LOG(ERROR) << "Failed to create a ProcessSingleton for your profile "
+                      "directory. This means that running multiple instances "
+                      "would start multiple browser processes rather than "
+                      "opening a new window in the existing process. Aborting "
+                      "now to avoid profile corruption.";
+        return ResultCodes::PROFILE_IN_USE;
+
       default:
         NOTREACHED();
     }
@@ -1074,15 +1082,6 @@ int BrowserMain(const MainFunctionParams& parameters) {
   if (CheckMachineLevelInstall())
     return ResultCodes::MACHINE_LEVEL_INSTALL_EXISTS;
 
-  if (!process_singleton.Create()) {
-    LOG(ERROR) << "Failed to create a ProcessSingleton for your profile "
-                  "directory. This means that running multiple instances "
-                  "would start multiple browser processes rather than opening "
-                  "a new window in the existing process. Aborting now to "
-                  "avoid profile corruption.";
-    return ResultCodes::PROFILE_IN_USE;
-  }
-
   // Create the TranslateManager singleton.
   Singleton<TranslateManager>::get();
 
diff --git a/chrome/browser/process_singleton.h b/chrome/browser/process_singleton.h
index 4ae0b10..d2ee14f 100644
--- a/chrome/browser/process_singleton.h
+++ b/chrome/browser/process_singleton.h
@@ -37,6 +37,7 @@ class ProcessSingleton : public NonThreadSafe {
     PROCESS_NONE,
     PROCESS_NOTIFIED,
     PROFILE_IN_USE,
+    LOCK_ERROR,
   };
 
   explicit ProcessSingleton(const FilePath& user_data_dir);
@@ -52,11 +53,20 @@ class ProcessSingleton : public NonThreadSafe {
   // first one, so this function won't find it.
   NotifyResult NotifyOtherProcess();
 
+  // Notify another process, if available.  Otherwise sets ourselves as the
+  // singleton instance.  Returns PROCESS_NONE if we became the singleton
+  // instance.
+  NotifyResult NotifyOtherProcessOrCreate();
+
 #if defined(OS_POSIX) && !defined(OS_MACOSX)
   // Exposed for testing.  We use a timeout on Linux, and in tests we want
   // this timeout to be short.
   NotifyResult NotifyOtherProcessWithTimeout(const CommandLine& command_line,
-                                             int timeout_seconds);
+                                             int timeout_seconds,
+                                             bool kill_unresponsive);
+  NotifyResult NotifyOtherProcessWithTimeoutOrCreate(
+      const CommandLine& command_line,
+      int timeout_seconds);
 #endif
 
   // Sets ourself up as the singleton instance.  Returns true on success.  If
diff --git a/chrome/browser/process_singleton_linux.cc b/chrome/browser/process_singleton_linux.cc
index fdf4c2b..2a1a01c 100644
--- a/chrome/browser/process_singleton_linux.cc
+++ b/chrome/browser/process_singleton_linux.cc
@@ -648,12 +648,14 @@ ProcessSingleton::~ProcessSingleton() {
 
 ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcess() {
   return NotifyOtherProcessWithTimeout(*CommandLine::ForCurrentProcess(),
-                                       kTimeoutInSeconds);
+                                       kTimeoutInSeconds,
+                                       true);
 }
 
 ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcessWithTimeout(
     const CommandLine& cmd_line,
-    int timeout_seconds) {
+    int timeout_seconds,
+    bool kill_unresponsive) {
   DCHECK_GE(timeout_seconds, 0);
 
   int socket;
@@ -710,7 +712,7 @@ ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcessWithTimeout(
 
     if (retries == timeout_seconds) {
       // Retries failed.  Kill the unresponsive chrome process and continue.
-      if (!KillProcessByLockPath(lock_path_.value()))
+      if (!kill_unresponsive || !KillProcessByLockPath(lock_path_.value()))
         return PROFILE_IN_USE;
       return PROCESS_NONE;
     }
@@ -741,7 +743,7 @@ ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcessWithTimeout(
   // Send the message
   if (!WriteToSocket(socket, to_send.data(), to_send.length())) {
     // Try to kill the other process, because it might have been dead.
-    if (!KillProcessByLockPath(lock_path_.value()))
+    if (!kill_unresponsive || !KillProcessByLockPath(lock_path_.value()))
       return PROFILE_IN_USE;
     return PROCESS_NONE;
   }
@@ -757,7 +759,7 @@ ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcessWithTimeout(
 
   // Failed to read ACK, the other process might have been frozen.
   if (len <= 0) {
-    if (!KillProcessByLockPath(lock_path_.value()))
+    if (!kill_unresponsive || !KillProcessByLockPath(lock_path_.value()))
       return PROFILE_IN_USE;
     return PROCESS_NONE;
   }
@@ -775,6 +777,34 @@ ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcessWithTimeout(
   return PROCESS_NOTIFIED;
 }
 
+ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcessOrCreate() {
+  return NotifyOtherProcessWithTimeoutOrCreate(
+      *CommandLine::ForCurrentProcess(),
+      kTimeoutInSeconds);
+}
+
+ProcessSingleton::NotifyResult
+ProcessSingleton::NotifyOtherProcessWithTimeoutOrCreate(
+    const CommandLine& command_line,
+    int timeout_seconds) {
+  NotifyResult result = NotifyOtherProcessWithTimeout(command_line,
+                                                      timeout_seconds, true);
+  if (result != PROCESS_NONE)
+    return result;
+  if (Create())
+    return PROCESS_NONE;
+  // If the Create() failed, try again to notify. (It could be that another
+  // instance was starting at the same time and managed to grab the lock before
+  // we did.)
+  // This time, we don't want to kill anything if we aren't successful, since we
+  // aren't going to try to take over the lock ourselves.
+  result = NotifyOtherProcessWithTimeout(command_line, timeout_seconds, false);
+  if (result != PROCESS_NONE)
+    return result;
+
+  return LOCK_ERROR;
+}
+
 bool ProcessSingleton::Create() {
   int sock;
   sockaddr_un addr;
@@ -796,8 +826,6 @@ bool ProcessSingleton::Create() {
     if (ReadLink(lock_path_.value()) != symlink_content) {
       // If we failed to create the lock, most likely another instance won the
       // startup race.
-      // TODO(mattm): If the other instance is on the same host, we could try
-      // to notify it rather than just failing.
       errno = saved_errno;
       PLOG(ERROR) << "Failed to create " << lock_path_.value();
       return false;
diff --git a/chrome/browser/process_singleton_linux_uitest.cc b/chrome/browser/process_singleton_linux_uitest.cc
index ef42966..3f7ddd8 100644
--- a/chrome/browser/process_singleton_linux_uitest.cc
+++ b/chrome/browser/process_singleton_linux_uitest.cc
@@ -30,14 +30,14 @@ namespace {
 
 typedef UITest ProcessSingletonLinuxTest;
 
-// A helper method to call ProcessSingleton::NotifyOtherProcess().
-// |url| will be added to CommandLine for current process, so that it can be
-// sent to browser process by ProcessSingleton::NotifyOtherProcess().
-ProcessSingleton::NotifyResult NotifyOtherProcess(const std::string& url,
-                                                  int timeout_ms) {
+ProcessSingleton* CreateProcessSingleton() {
   FilePath user_data_dir;
   PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
 
+  return new ProcessSingleton(user_data_dir);
+}
+
+CommandLine CommandLineForUrl(const std::string& url) {
   // Hack: mutate the current process's command line so we don't show a dialog.
   // Note that this only works if we have no loose values on the command line,
   // but that's fine for unit tests.  In a UI test we disable error dialogs
@@ -48,11 +48,28 @@ ProcessSingleton::NotifyResult NotifyOtherProcess(const std::string& url,
 
   CommandLine new_cmd_line(*cmd_line);
   new_cmd_line.AppendLooseValue(ASCIIToWide(url));
+  return new_cmd_line;
+}
 
-  ProcessSingleton process_singleton(user_data_dir);
+// A helper method to call ProcessSingleton::NotifyOtherProcess().
+// |url| will be added to CommandLine for current process, so that it can be
+// sent to browser process by ProcessSingleton::NotifyOtherProcess().
+ProcessSingleton::NotifyResult NotifyOtherProcess(const std::string& url,
+                                                  int timeout_ms) {
+  scoped_ptr<ProcessSingleton> process_singleton(CreateProcessSingleton());
+  return process_singleton->NotifyOtherProcessWithTimeout(
+      CommandLineForUrl(url), timeout_ms / 1000, true);
+}
 
-  return process_singleton.NotifyOtherProcessWithTimeout(
-      new_cmd_line, timeout_ms / 1000);
+// A helper method to call ProcessSingleton::NotifyOtherProcessOrCreate().
+// |url| will be added to CommandLine for current process, so that it can be
+// sent to browser process by ProcessSingleton::NotifyOtherProcessOrCreate().
+ProcessSingleton::NotifyResult NotifyOtherProcessOrCreate(
+    const std::string& url,
+    int timeout_ms) {
+  scoped_ptr<ProcessSingleton> process_singleton(CreateProcessSingleton());
+  return process_singleton->NotifyOtherProcessWithTimeoutOrCreate(
+      CommandLineForUrl(url), timeout_ms / 1000);
 }
 
 }  // namespace
@@ -188,3 +205,30 @@ TEST_F(ProcessSingletonLinuxTest, NotifyOtherProcessDifferingHost) {
   EXPECT_EQ(ProcessSingleton::PROFILE_IN_USE,
             NotifyOtherProcess(url, action_timeout_ms()));
 }
+
+// Test that we fail when lock says process is on another host and we can't
+// notify it over the socket.
+TEST_F(ProcessSingletonLinuxTest, NotifyOtherProcessOrCreate_DifferingHost) {
+  base::ProcessId pid = browser_process_id();
+
+  ASSERT_GE(pid, 1);
+
+  // Kill the browser process, so that it does not respond on the socket.
+  kill(pid, SIGKILL);
+  // Wait for a while to make sure the browser process is actually killed.
+  EXPECT_FALSE(CrashAwareSleep(sleep_timeout_ms()));
+
+  FilePath lock_path = user_data_dir().Append(chrome::kSingletonLockFilename);
+  EXPECT_EQ(0, unlink(lock_path.value().c_str()));
+  EXPECT_EQ(0, symlink("FAKEFOOHOST-1234", lock_path.value().c_str()));
+
+  std::string url("about:blank");
+  EXPECT_EQ(ProcessSingleton::PROFILE_IN_USE,
+            NotifyOtherProcessOrCreate(url, action_timeout_ms()));
+}
+
+// Test that Create fails when another browser is using the profile directory.
+TEST_F(ProcessSingletonLinuxTest, CreateFailsWithExistingBrowser) {
+  scoped_ptr<ProcessSingleton> process_singleton(CreateProcessSingleton());
+  EXPECT_FALSE(process_singleton->Create());
+}
diff --git a/chrome/browser/process_singleton_mac.cc b/chrome/browser/process_singleton_mac.cc
index 50148a9..23d317c 100644
--- a/chrome/browser/process_singleton_mac.cc
+++ b/chrome/browser/process_singleton_mac.cc
@@ -32,6 +32,11 @@ ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcess() {
   return PROCESS_NONE;
 }
 
+ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcessOrCreate() {
+  // This space intentionally left blank.
+  return PROCESS_NONE;
+}
+
 bool ProcessSingleton::Create() {
   // This space intentionally left blank.
   return true;
diff --git a/chrome/browser/process_singleton_win.cc b/chrome/browser/process_singleton_win.cc
index 09dec13..6454f66 100644
--- a/chrome/browser/process_singleton_win.cc
+++ b/chrome/browser/process_singleton_win.cc
@@ -156,6 +156,13 @@ ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcess() {
   return PROCESS_NONE;
 }
 
+ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcessOrCreate() {
+  NotifyResult result = NotifyOtherProcess();
+  if (result != PROCESS_NONE)
+    return result;
+  return Create() ? PROCESS_NONE : PROFILE_IN_USE;
+}
+
 // For windows, there is no need to call Create() since the call is made in
 // the constructor but to avoid having more platform specific code in
 // browser_main.cc we tolerate a second call which will do nothing.
diff --git a/chrome/test/data/valgrind/ui_tests.gtest.txt b/chrome/test/data/valgrind/ui_tests.gtest.txt
index a61c639..04c8f85 100644
--- a/chrome/test/data/valgrind/ui_tests.gtest.txt
+++ b/chrome/test/data/valgrind/ui_tests.gtest.txt
@@ -32,6 +32,7 @@ TabRestoreUITest.*
 # See http://crbug.com/25176
 ProcessSingletonLinuxTest.NotifyOtherProcessFailure
 ProcessSingletonLinuxTest.NotifyOtherProcessDifferingHost
+ProcessSingletonLinuxTest.NotifyOtherProcessOrCreate_DifferingHost
 
 # These tests fail under valgrind.
 # See http://crbug.com/29579.
diff --git a/chrome/test/in_process_browser_test.cc b/chrome/test/in_process_browser_test.cc
index 9939108..976a893 100644
--- a/chrome/test/in_process_browser_test.cc
+++ b/chrome/test/in_process_browser_test.cc
@@ -95,6 +95,12 @@ void InProcessBrowserTest::SetUp() {
       "argument and try again.";
   ASSERT_TRUE(file_util::DieFileDie(user_data_dir, true));
 
+  // Recreate the user data dir. (PathService::Get guarantees that the directory
+  // exists if it returns true, but it only actually checks on the first call,
+  // the rest are cached.  Thus we need to recreate it ourselves to not break
+  // the PathService guarantee.)
+  ASSERT_TRUE(file_util::CreateDirectory(user_data_dir));
+
   // The unit test suite creates a testingbrowser, but we want the real thing.
   // Delete the current one. We'll install the testing one in TearDown.
   delete g_browser_process;
-- 
cgit v1.1