Use process_singleton_linux on Mac.

chrome/browser/process_singleton_posix.cc

Index: chrome/browser/process_singleton_posix.cc
diff --git a/chrome/browser/process_singleton_linux.cc b/chrome/browser/process_singleton_posix.cc
similarity index 92%
rename from chrome/browser/process_singleton_linux.cc
rename to chrome/browser/process_singleton_posix.cc
index 54d7db296f329d29016c8f5ba35d64bbefadd778..8ff2efdcb7105b9e8de751d5b093b922efbbbd72 100644
--- a/chrome/browser/process_singleton_linux.cc
+++ b/chrome/browser/process_singleton_posix.cc
@@ -74,7 +74,6 @@
 #include "base/threading/platform_thread.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
-#include "chrome/browser/ui/process_singleton_dialog_linux.h"
 #include "chrome/common/chrome_constants.h"
 #include "content/public/browser/browser_thread.h"
 #include "grit/chromium_strings.h"
@@ -82,6 +81,10 @@
 #include "net/base/net_util.h"
 #include "ui/base/l10n/l10n_util.h"
 
+#if defined(OS_LINUX)
+#include "chrome/browser/ui/process_singleton_dialog_linux.h"
+#endif
+
 #if defined(TOOLKIT_VIEWS) && !defined(OS_CHROMEOS)
 #include "ui/views/linux_ui/linux_ui.h"
 #endif
@@ -298,14 +301,24 @@ bool DisplayProfileInUseError(const base::FilePath& lock_path,
                               const std::string& hostname,
                               int pid) {
   base::string16 error = l10n_util::GetStringFUTF16(
-      IDS_PROFILE_IN_USE_LINUX,
+      IDS_PROFILE_IN_USE_POSIX,
       base::IntToString16(pid),
       base::ASCIIToUTF16(hostname));
+  LOG(ERROR) << base::SysWideToNativeMB(base::UTF16ToWide(error)).c_str();
+
+  if (g_disable_prompt)
+    return false;
+
+#if defined(OS_LINUX)
   base::string16 relaunch_button_text = l10n_util::GetStringUTF16(
       IDS_PROFILE_IN_USE_LINUX_RELAUNCH);
-  LOG(ERROR) << base::SysWideToNativeMB(base::UTF16ToWide(error)).c_str();
-  if (!g_disable_prompt)
-    return ShowProcessSingletonDialog(error, relaunch_button_text);
+  return ShowProcessSingletonDialog(error, relaunch_button_text);
+#elif defined(OS_MACOSX)
+  // On Mac, always usurp the lock.
+  return true;
+#endif
+
+  NOTREACHED();
   return false;
 }
 
@@ -393,6 +406,54 @@ bool ConnectSocket(ScopedSocket* socket,
   }
 }
 
+#if defined(OS_MACOSX)
+bool ReplaceOldSingletonLock(const base::FilePath& symlink_content,
+                             const base::FilePath& lock_path) {
+  // Try taking an flock(2) on the file. Failure means the lock is taken so we
+  // should quit.
+  int lock_fd = HANDLE_EINTR(open(lock_path.value().c_str(),
+                             O_RDWR | O_CREAT | O_SYMLINK, 0644));

[Scott Hess - ex-Googler, 2014/04/24 21:54:43]

This indentation is wrong, the O_RDWR is w/in the open().

[jackhou1, 2014/04/28 05:20:16]

Done.

+  if (lock_fd == -1) {
+    PLOG(ERROR) << "Could not open singleton lock";
+    return false;
+  }
+
+  int rc = HANDLE_EINTR(flock(lock_fd, LOCK_EX|LOCK_NB));
+  if (rc == -1) {
+    if (errno == EWOULDBLOCK) {
+      LOG(ERROR) << "Singleton lock held by old process.";
+    } else {
+      PLOG(ERROR) << "Error locking singleton lock";
+    }

[Scott Hess - ex-Googler, 2014/04/24 21:54:43]

Close lock_fd, or use a ScopedFD on it.

Actually, use a ScopedFD on it, someone should close it!

[jackhou1, 2014/04/28 05:20:16]

Done.

+    return false;
+  }
+
+  // Successfully taking the lock means we can replace it with the a new symlink
+  // lock. We never flock() the lock file from now on. I.e. we assume that an
+  // old version of Chrome will not run with the same user data dir after this
+  // version has run.

[Scott Hess - ex-Googler, 2014/04/24 21:54:43]

What happens if this case occurs?  Is the user's browser stuck, or does it just
give up and optimistically run anyhow?

[jackhou1, 2014/04/28 05:20:16]

The older version of Chrome will replace the symlink as if it didn't exist and
continue running. 

+  base::ScopedTempDir temp_lock_dir;
+  if (!temp_lock_dir.CreateUniqueTempDirUnderPath(lock_path.DirName())) {

[Scott Hess - ex-Googler, 2014/04/24 21:54:43]

My guess is that this code will never crash.  BUT, if some other thread crashes
between here and the rename, then this potentially leaves random cruft in the
profile.  I think just choosing a fixed path (like lock_path with ".tmp"
appended) would be nicer, because that will keep trying until it succeeds.

I _think_ SymlinkPath() will work right if this scenario happens, given the
double-check it does on failure.

[jackhou1, 2014/04/28 05:20:16]

Putting it in the same directory ensures it's on the same volume, otherwise
rename() doesn't work. However, mattm made a good point that replacing the lock
instead of just deleting it doesn't buy much, so I'll just do that.

[Scott Hess - ex-Googler, 2014/04/28 17:41:39]

Sounds good.

+    LOG(ERROR) << "Could not create temp_lock_dir";
+    return false;
+  }
+  base::FilePath temp_lock_path =
+      temp_lock_dir.path().Append(chrome::kSingletonLockFilename);
+  if (!SymlinkPath(symlink_content, temp_lock_path)) {
+    LOG(ERROR) << "Could not create temp symlink.";
+    return false;
+  }
+
+  rc = rename(temp_lock_path.value().c_str(), lock_path.value().c_str());

[mattm, 2014/04/24 22:58:51]

Is all of this even necessary? The rest of the code already assumes that
creating the symlink can race, so why not just unlink the old lockfile and try
again to create the new one the normal way?

[Scott Hess - ex-Googler, 2014/04/25 15:26:54]

It's atomically locking against the previous browser version.

[jackhou1, 2014/04/28 05:20:16]

Changed to just deleting the old SingletonLock.

+  if (rc == -1) {
+    PLOG(ERROR) << "Could not replace singleton lock with symlink";
+    return false;
+  }
+
+  return true;
+}
+#endif  // defined(OS_MACOSX)
+
 }  // namespace
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -883,9 +944,15 @@ bool ProcessSingleton::Create() {
   // Create symbol link before binding the socket, to ensure only one instance
   // can have the socket open.
   if (!SymlinkPath(symlink_content, lock_path_)) {
-      // If we failed to create the lock, most likely another instance won the
-      // startup race.
-      return false;
+    // TODO(jackhou): Remove this case once this code is stable on Mac.

[Scott Hess - ex-Googler, 2014/04/24 21:54:43]

Log a tracking bug for this, and reference it here.  After one release cycle I
think it could be something like unlinking on failure and trying again.  But
since it doesn't work at all if the existing file is there, I think it probably
has to keep some sort of handler forever (I don't think someone keeping an old
Chrome running for a long time and launching in parallel is likely, but it is
likely that someone will try to run Chrome after a long period of no launches).

I find myself wondering what happens if someone creates something that
persistently fails.  The SymlinkPath() code doesn't appear to check for specific
codes known to happen when failing the race, so it's possible there are a bunch
of other errors which actually should lead to unlink.

[jackhou1, 2014/04/28 05:20:16]

Done. http://crbug.com/367612

Yeah, it's probably ok to delete any non-symlink lock file on linux. I'll look
into making that the default when resolving that bug.

+#if defined(OS_MACOSX)
+    // On Mac, the lock could be held by the old process singleton code.
+    if (errno == EEXIST && !base::IsLink(lock_path_))
+      return ReplaceOldSingletonLock(symlink_content, lock_path_);

[mattm, 2014/04/24 22:58:51]

Unconditionally returning here doesn't seem right. If the old lock was
successfully replaced, you still need to create the socket and stuff below.

[jackhou1, 2014/04/28 05:20:16]

Fixed. Also updated test.

+#endif
+    // If we failed to create the lock, most likely another instance won the
+    // startup race.
+    return false;
   }
 
   // Create the socket file somewhere in /tmp which is usually mounted as a
@@ -895,6 +962,13 @@ bool ProcessSingleton::Create() {
     LOG(ERROR) << "Failed to create socket directory.";
     return false;
   }
+
+  // Check that the directory was created with the correct permissions.
+  int dir_mode = 0;
+  CHECK(base::GetPosixFilePermissions(socket_dir_.path(), &dir_mode) &&
+        dir_mode == base::FILE_PERMISSION_USER_MASK)
+      << "Temp directory mode is not 700: " << std::oct << dir_mode;
+
   // Setup the socket symlink and the two cookies.
   base::FilePath socket_target_path =
       socket_dir_.path().Append(chrome::kSingletonSocketFilename);