Linux: fix startup race between creating the SingletonLock and listening on SingletonSocket.

BUG=39922
TEST=see bug

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@43801 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 85 insertions, 45 deletions

diff --git a/chrome/browser/browser_main.cc b/chrome/browser/browser_main.cc
index 9d4b5c4..a993769 100644
--- a/chrome/browser/browser_main.cc
+++ b/chrome/browser/browser_main.cc
@@ -973,7 +973,14 @@ int BrowserMain(const MainFunctionParams& parameters) {
   if (CheckMachineLevelInstall())
     return ResultCodes::MACHINE_LEVEL_INSTALL_EXISTS;
 
-  process_singleton.Create();
+  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 d7fd0f5..4ae0b10 100644
--- a/chrome/browser/process_singleton.h
+++ b/chrome/browser/process_singleton.h
@@ -59,8 +59,10 @@ class ProcessSingleton : public NonThreadSafe {
                                              int timeout_seconds);
 #endif
 
-  // Sets ourself up as the singleton instance.
-  void Create();
+  // Sets ourself up as the singleton instance.  Returns true on success.  If
+  // false is returned, we are not the singleton instance and the caller must
+  // exit.
+  bool Create();
 
   // Clear any lock state during shutdown.
   void Cleanup();
diff --git a/chrome/browser/process_singleton_linux.cc b/chrome/browser/process_singleton_linux.cc
index 10d6321..a012aea 100644
--- a/chrome/browser/process_singleton_linux.cc
+++ b/chrome/browser/process_singleton_linux.cc
@@ -48,9 +48,11 @@
 #include "base/basictypes.h"
 #include "base/command_line.h"
 #include "base/eintr_wrapper.h"
+#include "base/file_path.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/path_service.h"
+#include "base/platform_thread.h"
 #include "base/process_util.h"
 #include "base/safe_strerror_posix.h"
 #include "base/stl_util-inl.h"
@@ -235,10 +237,14 @@ bool UnlinkPath(const std::string& path) {
 }
 
 // Extract the hostname and pid from the lock symlink.
-std::string ParseLockPath(const std::string& path,
-                          std::string* hostname,
-                          int* pid) {
+// Returns true if the lock existed.
+bool ParseLockPath(const std::string& path,
+                   std::string* hostname,
+                   int* pid) {
   std::string real_path = ReadLink(path);
+  if (real_path.empty())
+    return false;
+
   std::string::size_type pos = real_path.rfind('-');
 
   // If the path is not a symbolic link, or doesn't contain what we expect,
@@ -246,7 +252,7 @@ std::string ParseLockPath(const std::string& path,
   if (pos == std::string::npos) {
     *hostname = "";
     *pid = -1;
-    return "";
+    return true;
   }
 
   *hostname = real_path.substr(0, pos);
@@ -255,7 +261,7 @@ std::string ParseLockPath(const std::string& path,
   if (!StringToInt(pid_str, pid))
     *pid = -1;
 
-  return real_path;
+  return true;
 }
 
 void DisplayProfileInUseError(const std::string& lock_path,
@@ -274,21 +280,6 @@ void DisplayProfileInUseError(const std::string& lock_path,
 #endif
 }
 
-// Check if the lock is on a different host.  If so, return false.  If not,
-// unlink the lock file and return true.
-bool CheckLockHostnameAndCleanup(const std::string& path) {
-  std::string hostname;
-  int pid;
-  ParseLockPath(path, &hostname, &pid);
-
-  if (!hostname.empty() && hostname != net::GetHostName()) {
-    DisplayProfileInUseError(path, hostname, pid);
-    return false;
-  }
-  UnlinkPath(path);
-  return true;
-}
-
 // Extract the process's pid from a symbol link path and if it is on
 // the same host, kill the process, unlink the lock file and return true.
 // If the process is on a different host, return false.
@@ -635,6 +626,8 @@ ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcess() {
 ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcessWithTimeout(
     const CommandLine& cmd_line,
     int timeout_seconds) {
+  DCHECK_GE(timeout_seconds, 0);
+
   int socket;
   sockaddr_un addr;
   SetupSocket(socket_path_.value(), &socket, &addr);
@@ -642,18 +635,55 @@ ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcessWithTimeout(
   // It'll close the socket automatically when exiting this method.
   SocketCloser socket_closer(socket);
 
-  // Connecting to the socket
-  int ret = HANDLE_EINTR(connect(socket,
-                                 reinterpret_cast<sockaddr*>(&addr),
-                                 sizeof(addr)));
-  if (ret < 0) {
-    // TODO(mattm): there is a bit of a race here if another instance on the
-    // same host is in Create() and has created the lock but not attached to the
-    // socket.  Our CheckLockHostnameAndCleanup call will clean up their lock
-    // and allow us to create a new one.
-    if (!CheckLockHostnameAndCleanup(lock_path_.value()))
+  for (int retries = 0; retries <= timeout_seconds; ++retries) {
+    // Connecting to the socket
+    int ret = HANDLE_EINTR(connect(socket,
+                                   reinterpret_cast<sockaddr*>(&addr),
+                                   sizeof(addr)));
+    if (ret == 0)
+      break;
+
+    // If we're in a race with another process, they may be in Create() and have
+    // created the lock but not attached to the socket.  So we check if the
+    // process with the pid from the lockfile is currently running and is a
+    // chrome browser.  If so, we loop and try again for |timeout_seconds|.
+
+    std::string hostname;
+    int pid;
+    if (!ParseLockPath(lock_path_.value(), &hostname, &pid)) {
+      // No lockfile exists.
+      return PROCESS_NONE;
+    }
+
+    if (hostname.empty()) {
+      // Invalid lockfile.
+      UnlinkPath(lock_path_.value());
+      return PROCESS_NONE;
+    }
+
+    if (hostname != net::GetHostName()) {
+      // Locked by process on another host.
+      DisplayProfileInUseError(lock_path_.value(), hostname, pid);
       return PROFILE_IN_USE;
-    return PROCESS_NONE;  // Tell the caller there's nobody to notify.
+    }
+
+    FilePath other_chrome_path(base::GetProcessExecutablePath(pid));
+    if (other_chrome_path.empty() ||
+        other_chrome_path.BaseName() !=
+        FilePath::FromWStringHack(chrome::kBrowserProcessExecutableName)) {
+      // Orphaned lockfile (no process with pid, or non-chrome process.)
+      UnlinkPath(lock_path_.value());
+      return PROCESS_NONE;
+    }
+
+    if (retries == timeout_seconds) {
+      // Retries failed.  Kill the unresponsive chrome process and continue.
+      if (!KillProcessByLockPath(lock_path_.value()))
+        return PROFILE_IN_USE;
+      return PROCESS_NONE;
+    }
+
+    PlatformThread::Sleep(1000 /* ms */);
   }
 
   timeval timeout = {timeout_seconds, 0};
@@ -713,7 +743,7 @@ ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcessWithTimeout(
   return PROCESS_NOTIFIED;
 }
 
-void ProcessSingleton::Create() {
+bool ProcessSingleton::Create() {
   int sock;
   sockaddr_un addr;
 
@@ -737,7 +767,8 @@ void ProcessSingleton::Create() {
       // 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(FATAL) << "Failed to create " << lock_path_.value();
+      PLOG(ERROR) << "Failed to create " << lock_path_.value();
+      return false;
     }
   }
 
@@ -746,13 +777,9 @@ void ProcessSingleton::Create() {
   UnlinkPath(socket_path_.value());
 
   if (bind(sock, reinterpret_cast<sockaddr*>(&addr), sizeof(addr)) < 0) {
-    PLOG(ERROR) << "bind() failed";
-    LOG(ERROR) << "SingletonSocket failed to create a socket in your home "
-                  "directory. This means that running multiple instances of "
-                  "the Chrome binary will start multiple browser process "
-                  "rather than opening a new window in the existing process.";
+    PLOG(ERROR) << "Failed to bind() " << socket_path_.value();
     CloseSocket(sock);
-    return;
+    return false;
   }
 
   if (listen(sock, 5) < 0)
@@ -767,6 +794,8 @@ void ProcessSingleton::Create() {
     watcher_.get(),
     &ProcessSingleton::LinuxWatcher::StartListening,
     sock));
+
+  return true;
 }
 
 void ProcessSingleton::Cleanup() {
diff --git a/chrome/browser/process_singleton_mac.cc b/chrome/browser/process_singleton_mac.cc
index c1edbb5..50148a9 100644
--- a/chrome/browser/process_singleton_mac.cc
+++ b/chrome/browser/process_singleton_mac.cc
@@ -32,8 +32,9 @@ ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcess() {
   return PROCESS_NONE;
 }
 
-void ProcessSingleton::Create() {
+bool ProcessSingleton::Create() {
   // This space intentionally left blank.
+  return true;
 }
 
 void ProcessSingleton::Cleanup() {
diff --git a/chrome/browser/process_singleton_win.cc b/chrome/browser/process_singleton_win.cc
index 4e8de16..09dec13 100644
--- a/chrome/browser/process_singleton_win.cc
+++ b/chrome/browser/process_singleton_win.cc
@@ -159,10 +159,10 @@ ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcess() {
 // 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.
-void ProcessSingleton::Create() {
+bool ProcessSingleton::Create() {
   DCHECK(!remote_window_);
   if (window_)
-    return;
+    return true;
 
   HINSTANCE hinst = GetModuleHandle(NULL);
 
@@ -185,6 +185,7 @@ void ProcessSingleton::Create() {
   DCHECK(window_);
 
   win_util::SetWindowUserData(window_, this);
+  return true;
 }
 
 void ProcessSingleton::Cleanup() {