rlz/mac: Make sure a crashing process doesn't leave a stale lockfile behind.

Don't use NSDistributedLock, which uses a lock that isn't cleaned up on
unexpected program termination, and which strongly recommends to not
call -breakLock (which makes this class fairly pointless).

Instead, use a flock(), which is cleaned up by the OS on process exit.
Suggested by shess@.

BUG=141108

Review URL: https://chromiumcodereview.appspot.com/10823329

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

rlz/lib/rlz_lib_test.cc

@@ -13,6 +13,7 @@
 // The "GGLA" brand is used to test the normal code flow of the code, and the
 // "TEST" brand is used to test the supplementary brand code code flow.
 
+#include "base/eintr_wrapper.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -795,7 +796,9 @@ class ReadonlyRlzDirectoryTest : public RlzLibTestNoMachineState {
 void ReadonlyRlzDirectoryTest::SetUp() {
   RlzLibTestNoMachineState::SetUp();
   // Make the rlz directory non-writeable.
-  chmod(temp_dir_.path().value().c_str(), 0500);
+  int chmod_result = chmod(temp_dir_.path().value().c_str(), 0500);
+  ASSERT_EQ(0, chmod_result);
+
 }
 
 TEST_F(ReadonlyRlzDirectoryTest, WriteFails) {
@@ -821,4 +824,51 @@ TEST_F(ReadonlyRlzDirectoryTest, SupplementaryBrandingDoesNotCrash) {
   EXPECT_FALSE(rlz_lib::RecordProductEvent(rlz_lib::TOOLBAR_NOTIFIER,
       rlz_lib::IE_DEFAULT_SEARCH, rlz_lib::INSTALL));
 }
+
+// Regression test for http://crbug.com/141108
+TEST_F(RlzLibTest, ConcurrentStoreAccessWithProcessExitsWhileLockHeld) {
+  // See the comment at the top of WriteFails.
+  if (!rlz_lib::SupplementaryBranding::GetBrand().empty())
+    return;
+
+  std::vector<pid_t> pids;
+  for (int i = 0; i < 10; ++i) {
+    pid_t pid = fork();
+    ASSERT_NE(-1, pid);
+    if (pid == 0) {
+      // Child.
+      {
+        // SupplementaryBranding is a RAII object for the rlz lock.
+        rlz_lib::SupplementaryBranding branding("TEST");
+
+        // Simulate a crash while holding the lock in some of the children.
+        if (i > 0 && i % 3 == 0)
+          _exit(0);
+
+        // Note: Since this is in a forked child, a failing expectation won't
+        // make the test fail. It does however cause lots of "check failed"
+        // error output. The parent process will then check the exit code
+        // below to make the test fail.
+        bool success = rlz_lib::RecordProductEvent(rlz_lib::TOOLBAR_NOTIFIER,
+            rlz_lib::IE_DEFAULT_SEARCH, rlz_lib::INSTALL);
+        EXPECT_TRUE(success);
+        _exit(success ? 0 : 1);
+      }
+      _exit(0);
+    } else {
+      // Parent.
+      pids.push_back(pid);
+    }
+  }
+
+  int status;
+  for (size_t i = 0; i < pids.size(); ++i) {
+    if (HANDLE_EINTR(waitpid(pids[i], &status, 0)) != -1)
+      EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0);
+  }
+
+  // No child should have the lock at this point, not even the crashed ones.
+  EXPECT_TRUE(rlz_lib::RecordProductEvent(rlz_lib::TOOLBAR_NOTIFIER,
+      rlz_lib::IE_DEFAULT_SEARCH, rlz_lib::INSTALL));
+}
 #endif

rlz/mac/lib/rlz_value_store_mac.mm

@@ -4,6 +4,7 @@
 
 #include "rlz/mac/lib/rlz_value_store_mac.h"
 
+#include "base/eintr_wrapper.h"
 #include "base/mac/foundation_util.h"
 #include "base/file_path.h"
 #include "base/logging.h"
@@ -235,12 +236,14 @@ struct RecursiveCrossProcessLock {
   pthread_mutex_t recursive_lock_;
   pthread_t locking_thread_;
 
-  NSDistributedLock* file_lock_;
+  int file_lock_;
 } g_recursive_lock = {
   // PTHREAD_RECURSIVE_MUTEX_INITIALIZER doesn't exist before 10.7 and is buggy
   // on 10.7 (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51906#c34), so emulate
   // recursive locking with a normal non-recursive mutex.
-  PTHREAD_MUTEX_INITIALIZER
+  PTHREAD_MUTEX_INITIALIZER,
+  0,
+  -1
 };
 
 bool RecursiveCrossProcessLock::TryGetCrossProcessLock(
@@ -266,33 +269,37 @@ bool RecursiveCrossProcessLock::TryGetCrossProcessLock(
     const int kMaxTimeoutMS = 5000;  // Matches windows.
     const int kSleepPerTryMS = 200;
 
-    CHECK(!file_lock_);
-    file_lock_ = [[NSDistributedLock alloc] initWithPath:lock_filename];
+    CHECK(file_lock_ == -1);
+    file_lock_ = open([lock_filename fileSystemRepresentation], O_CREAT, 0666);
+    if (file_lock_ == -1)
+      return false;
 
-    BOOL got_file_lock = NO;
-    int elapsedMS = 0;
-    while (!(got_file_lock = [file_lock_ tryLock]) &&
-           elapsedMS < kMaxTimeoutMS) {
+    int flock_result = -1;
+    int elapsed_ms = 0;
+    while ((flock_result =
+               HANDLE_EINTR(flock(file_lock_, LOCK_EX | LOCK_NB))) == -1 &&
+           errno == EWOULDBLOCK &&
+           elapsed_ms < kMaxTimeoutMS) {
       usleep(kSleepPerTryMS * 1000);
-      elapsedMS += kSleepPerTryMS;
+      elapsed_ms += kSleepPerTryMS;
     }
 
-    if (!got_file_lock) {
-      [file_lock_ release];
-      file_lock_ = nil;
+    if (flock_result == -1) {
+      ignore_result(HANDLE_EINTR(close(file_lock_)));
+      file_lock_ = -1;
       return false;
     }
     return true;
   } else {
-    return file_lock_ != nil;
+    return file_lock_ != -1;
   }
 }
 
 void RecursiveCrossProcessLock::ReleaseLock() {
   if (file_lock_) {
-    [file_lock_ unlock];
-    [file_lock_ release];
-    file_lock_ = nil;
+    ignore_result(HANDLE_EINTR(flock(file_lock_, LOCK_UN)));
+    ignore_result(HANDLE_EINTR(close(file_lock_)));
+    file_lock_ = -1;
   }
 
   locking_thread_ = 0;
@@ -347,7 +354,7 @@ NSString* RlzPlistFilename() {
 // Returns the path of the rlz lock file, also creates the parent directory
 // path if it doesn't exist.
 NSString* RlzLockFilename() {
-  NSString* const kRlzFile = @"lockfile";
+  NSString* const kRlzFile = @"flockfile";
   return [CreateRlzDirectory() stringByAppendingPathComponent:kRlzFile];
 }
 
@@ -416,8 +423,8 @@ ScopedRlzValueStoreLock::~ScopedRlzValueStoreLock() {
   // Check that "store_ set" => "file_lock acquired". The converse isn't true,
   // for example if the rlz data file can't be read.
   if (store_.get())
-    CHECK(g_recursive_lock.file_lock_);
-  if (!g_recursive_lock.file_lock_)
+    CHECK(g_recursive_lock.file_lock_ != -1);
+  if (g_recursive_lock.file_lock_ == -1)
     CHECK(!store_.get());
 
   g_recursive_lock.ReleaseLock();