diff options
author | shess@chromium.org <shess@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-10-14 22:20:32 +0000 |
---|---|---|
committer | shess@chromium.org <shess@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-10-14 22:20:32 +0000 |
commit | b2841b0739079820ae5a3f405fa777e5109a2e79 (patch) | |
tree | 4395081c3a84401c46936a6f1b89749336ce147c | |
parent | bd9ca3554160c3e4cac9ff289d2713d216fcbd6f (diff) | |
download | chromium_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.h | 10 | ||||
-rw-r--r-- | chrome/browser/process_singleton_mac.cc | 83 | ||||
-rw-r--r-- | chrome/browser/process_singleton_mac_unittest.cc | 149 | ||||
-rw-r--r-- | chrome/chrome_tests.gypi | 1 |
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', |