summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorshess@chromium.org <shess@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2010-10-14 22:20:32 +0000
committershess@chromium.org <shess@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2010-10-14 22:20:32 +0000
commitb2841b0739079820ae5a3f405fa777e5109a2e79 (patch)
tree4395081c3a84401c46936a6f1b89749336ce147c
parentbd9ca3554160c3e4cac9ff289d2713d216fcbd6f (diff)
downloadchromium_src-b2841b0739079820ae5a3f405fa777e5109a2e79.zip
chromium_src-b2841b0739079820ae5a3f405fa777e5109a2e79.tar.gz
chromium_src-b2841b0739079820ae5a3f405fa777e5109a2e79.tar.bz2
[Mac] Lock profile against multiple uses.
Mac uses Launch Services to make sure one Chrome is started from the GUI, and AppleEvents and Launch Services to pass in URLs. When a developer or test launches multiple browsers against a profile, it fails in mysterious ways. This adds a profile lock to flush out any hidden assumptions. BUG=22163,58986,59061 TEST=Run Chromium twice with same --user-data-dir. Second instances shouldn't launch. Review URL: http://codereview.chromium.org/3777003 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@62659 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r--chrome/browser/process_singleton.h10
-rw-r--r--chrome/browser/process_singleton_mac.cc83
-rw-r--r--chrome/browser/process_singleton_mac_unittest.cc149
-rw-r--r--chrome/chrome_tests.gypi1
4 files changed, 234 insertions, 9 deletions
diff --git a/chrome/browser/process_singleton.h b/chrome/browser/process_singleton.h
index 57dd446..c8f34ed 100644
--- a/chrome/browser/process_singleton.h
+++ b/chrome/browser/process_singleton.h
@@ -13,7 +13,7 @@
#endif
#include "base/basictypes.h"
-#if defined(USE_X11)
+#if defined(USE_X11) || defined(OS_MACOSX)
#include "base/file_path.h"
#endif
#include "base/logging.h"
@@ -152,6 +152,14 @@ class ProcessSingleton : public NonThreadSafe {
// because it posts messages between threads.
class LinuxWatcher;
scoped_refptr<LinuxWatcher> watcher_;
+#elif defined(OS_MACOSX)
+ // Path in file system to the lock.
+ FilePath lock_path_;
+
+ // File descriptor associated with the lockfile, valid between
+ // |Create()| and |Cleanup()|. Two instances cannot have a lock on
+ // the same file at the same time.
+ int lock_fd_;
#endif
DISALLOW_COPY_AND_ASSIGN(ProcessSingleton);
diff --git a/chrome/browser/process_singleton_mac.cc b/chrome/browser/process_singleton_mac.cc
index b6d2498..994efca 100644
--- a/chrome/browser/process_singleton_mac.cc
+++ b/chrome/browser/process_singleton_mac.cc
@@ -2,8 +2,24 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include <errno.h>
+#include <fcntl.h>
+#include <sys/file.h>
+
#include "chrome/browser/process_singleton.h"
+#include "base/eintr_wrapper.h"
+#include "base/file_util.h"
+#include "base/metrics/histogram.h"
+#include "chrome/common/chrome_constants.h"
+
+namespace {
+
+// From "man 2 intro".
+const int kMaxErrno = EOPNOTSUPP;
+
+} // namespace
+
// This class is used to funnel messages to a single instance of the browser
// process. This is needed for several reasons on other platforms.
//
@@ -17,16 +33,20 @@
// Neither of those cases apply on the Mac. Launch Services ensures that there
// is only one instance of the process, and we get URLs to open via AppleEvents
// and, once again, the Launch Services system. We have no need to manage this
-// ourselves.
+// ourselves. An exclusive lock is used to flush out anyone making incorrect
+// assumptions.
ProcessSingleton::ProcessSingleton(const FilePath& user_data_dir)
: locked_(false),
- foreground_window_(NULL) {
- // This space intentionally left blank.
+ foreground_window_(NULL),
+ lock_path_(user_data_dir.Append(chrome::kSingletonLockFilename)),
+ lock_fd_(-1) {
}
ProcessSingleton::~ProcessSingleton() {
- // This space intentionally left blank.
+ // Make sure the lock is released. Process death will also release
+ // it, even if this is not called.
+ Cleanup();
}
ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcess() {
@@ -35,15 +55,62 @@ ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcess() {
}
ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcessOrCreate() {
- // This space intentionally left blank.
- return PROCESS_NONE;
+ // Windows tries NotifyOtherProcess() first.
+ return Create() ? PROCESS_NONE : PROFILE_IN_USE;
}
+// Attempt to acquire an exclusive lock on an empty file in the
+// profile directory. Returns |true| if it gets the lock.
+// TODO(shess): The older code always returned |true|. Monitor the
+// histograms and convert the marked failure cases to |false| once
+// it's clear that it is safe to do. http://crbug.com/58986
+// TODO(shess): Rather than logging failures, popup an alert. Punting
+// that for now because it would require confidence that this code is
+// never called in a situation where an alert wouldn't work.
+// http://crbug.com/59061
bool ProcessSingleton::Create() {
- // This space intentionally left blank.
+ DCHECK_EQ(-1, lock_fd_) << "lock_path_ is already open.";
+
+ lock_fd_ = HANDLE_EINTR(open(lock_path_.value().c_str(),
+ O_RDONLY | O_CREAT, 0644));
+ if (lock_fd_ == -1) {
+ const int capture_errno = errno;
+ DPCHECK(lock_fd_ != -1) << "Unexpected failure opening profile lockfile";
+ UMA_HISTOGRAM_ENUMERATION("ProcessSingleton.OpenError",
+ capture_errno, kMaxErrno);
+ // TODO(shess): Change to |false|.
+ return true;
+ }
+
+ // Acquire an exclusive lock in non-blocking fashion. If the lock
+ // is already held, this will return |EWOULDBLOCK|.
+ int rc = HANDLE_EINTR(flock(lock_fd_, LOCK_EX|LOCK_NB));
+ if (rc == -1) {
+ const int capture_errno = errno;
+ DPCHECK(errno == EWOULDBLOCK)
+ << "Unexpected failure locking profile lockfile";
+
+ Cleanup();
+
+ // Other errors indicate something crazy is happening.
+ if (capture_errno != EWOULDBLOCK) {
+ UMA_HISTOGRAM_ENUMERATION("ProcessSingleton.LockError",
+ capture_errno, kMaxErrno);
+ // TODO(shess): Change to |false|.
+ return true;
+ }
+
+ // The file is open by another process and locked.
+ LOG(ERROR) << "Unable to obtain profile lock.";
+ return false;
+ }
+
return true;
}
void ProcessSingleton::Cleanup() {
- // This space intentionally left blank.
+ // Closing the file also releases the lock.
+ if (lock_fd_ != -1)
+ HANDLE_EINTR(close(lock_fd_));
+ lock_fd_ = -1;
}
diff --git a/chrome/browser/process_singleton_mac_unittest.cc b/chrome/browser/process_singleton_mac_unittest.cc
new file mode 100644
index 0000000..bdbeff6
--- /dev/null
+++ b/chrome/browser/process_singleton_mac_unittest.cc
@@ -0,0 +1,149 @@
+// Copyright (c) 2010 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 <errno.h>
+#include <fcntl.h>
+#include <sys/file.h>
+
+#include "chrome/browser/process_singleton.h"
+
+#include "base/eintr_wrapper.h"
+#include "base/file_util.h"
+#include "base/path_service.h"
+#include "chrome/common/chrome_constants.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/test/testing_profile.h"
+#include "testing/platform_test.h"
+
+namespace {
+
+class ProcessSingletonMacTest : public PlatformTest {
+ public:
+ virtual void SetUp() {
+ PlatformTest::SetUp();
+
+ // Put the lock in a temporary directory. Doesn't need to be a
+ // full profile to test this code.
+ temp_dir_.CreateUniqueTempDir();
+ ASSERT_TRUE(temp_dir_.IsValid());
+ lock_path_ = temp_dir_.path().Append(chrome::kSingletonLockFilename);
+ }
+
+ virtual void TearDown() {
+ PlatformTest::TearDown();
+
+ // Verify that the lock was released.
+ EXPECT_FALSE(IsLocked());
+ }
+
+ // Return |true| if the file exists and is locked. Forces a failure
+ // in the containing test in case of error condition.
+ bool IsLocked() {
+ int fd = HANDLE_EINTR(open(lock_path_.value().c_str(), O_RDONLY));
+ if (fd == -1) {
+ EXPECT_EQ(ENOENT, errno) << "Unexpected error opening lockfile.";
+ return false;
+ }
+
+ file_util::ScopedFD auto_close(&fd);
+
+ int rc = HANDLE_EINTR(flock(fd, LOCK_EX|LOCK_NB));
+
+ // Got the lock, so it wasn't already locked. Close releases.
+ if (rc != -1)
+ return false;
+
+ // Someone else has the lock.
+ if (errno == EWOULDBLOCK)
+ return true;
+
+ EXPECT_EQ(EWOULDBLOCK, errno) << "Unexpected error acquiring lock.";
+ return false;
+ }
+
+ ScopedTempDir temp_dir_;
+ FilePath lock_path_;
+};
+
+// Test that the base case doesn't blow up.
+TEST_F(ProcessSingletonMacTest, Basic) {
+ ProcessSingleton ps(temp_dir_.path());
+ EXPECT_FALSE(IsLocked());
+ EXPECT_TRUE(ps.Create());
+ EXPECT_TRUE(IsLocked());
+ ps.Cleanup();
+ EXPECT_FALSE(IsLocked());
+}
+
+// The destructor should release the lock.
+TEST_F(ProcessSingletonMacTest, DestructorReleases) {
+ EXPECT_FALSE(IsLocked());
+ {
+ ProcessSingleton ps(temp_dir_.path());
+ EXPECT_TRUE(ps.Create());
+ EXPECT_TRUE(IsLocked());
+ }
+ EXPECT_FALSE(IsLocked());
+}
+
+// Multiple singletons should interlock appropriately.
+TEST_F(ProcessSingletonMacTest, Interlock) {
+ ProcessSingleton ps1(temp_dir_.path());
+ ProcessSingleton ps2(temp_dir_.path());
+
+ // Windows and Linux use a command-line flag to suppress this, but
+ // it is on a sub-process so the scope is contained. Rather than
+ // add additional API to process_singleton.h in an #ifdef, just tell
+ // the reader what to expect and move on.
+ LOG(ERROR) << "Expect two failures to obtain the lock.";
+
+ // When |ps1| has the lock, |ps2| cannot get it.
+ EXPECT_FALSE(IsLocked());
+ EXPECT_TRUE(ps1.Create());
+ EXPECT_TRUE(IsLocked());
+ EXPECT_FALSE(ps2.Create());
+ ps1.Cleanup();
+
+ // And when |ps2| has the lock, |ps1| cannot get it.
+ EXPECT_FALSE(IsLocked());
+ EXPECT_TRUE(ps2.Create());
+ EXPECT_TRUE(IsLocked());
+ EXPECT_FALSE(ps1.Create());
+ ps2.Cleanup();
+ EXPECT_FALSE(IsLocked());
+}
+
+// Like |Interlock| test, but via |NotifyOtherProcessOrCreate()|.
+TEST_F(ProcessSingletonMacTest, NotifyOtherProcessOrCreate) {
+ ProcessSingleton ps1(temp_dir_.path());
+ ProcessSingleton ps2(temp_dir_.path());
+
+ // Windows and Linux use a command-line flag to suppress this, but
+ // it is on a sub-process so the scope is contained. Rather than
+ // add additional API to process_singleton.h in an #ifdef, just tell
+ // the reader what to expect and move on.
+ LOG(ERROR) << "Expect two failures to obtain the lock.";
+
+ // When |ps1| has the lock, |ps2| cannot get it.
+ EXPECT_FALSE(IsLocked());
+ EXPECT_EQ(ProcessSingleton::PROCESS_NONE, ps1.NotifyOtherProcessOrCreate());
+ EXPECT_TRUE(IsLocked());
+ EXPECT_EQ(ProcessSingleton::PROFILE_IN_USE, ps2.NotifyOtherProcessOrCreate());
+ ps1.Cleanup();
+
+ // And when |ps2| has the lock, |ps1| cannot get it.
+ EXPECT_FALSE(IsLocked());
+ EXPECT_EQ(ProcessSingleton::PROCESS_NONE, ps2.NotifyOtherProcessOrCreate());
+ EXPECT_TRUE(IsLocked());
+ EXPECT_EQ(ProcessSingleton::PROFILE_IN_USE, ps1.NotifyOtherProcessOrCreate());
+ ps2.Cleanup();
+ EXPECT_FALSE(IsLocked());
+}
+
+// TODO(shess): Test that the lock is released when the process dies.
+// DEATH_TEST? I don't know. If the code to communicate between
+// browser processes is ever written, this all would need to be tested
+// more like the other platforms, in which case it would be easy.
+
+} // namespace
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 2bf6aa8..e4324f4 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1387,6 +1387,7 @@
'browser/printing/print_dialog_cloud_unittest.cc',
'browser/printing/print_job_unittest.cc',
'browser/process_info_snapshot_mac_unittest.cc',
+ 'browser/process_singleton_mac_unittest.cc',
'browser/profile_manager_unittest.cc',
'browser/renderer_host/audio_renderer_host_unittest.cc',
'browser/renderer_host/gtk_im_context_wrapper_unittest.cc',