Transition POSIX shmem to use lockf(), not semaphores. Eliminates

resource leak possibility; allows complete sharing of shmem by fd
without name.
Add actual multiprocess shmem/locking unit test.
Fix flaky shmem unit test.

agl: please review.
Amanda: please look over as you've been here before.

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

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

3 files changed, 140 insertions, 58 deletions

diff --git a/base/shared_memory.h b/base/shared_memory.h
index d96299a..ede36c5 100644
--- a/base/shared_memory.h
+++ b/base/shared_memory.h
@@ -24,11 +24,9 @@ typedef HANDLE SharedMemoryHandle;
 typedef HANDLE SharedMemoryLock;
 #elif defined(OS_POSIX)
 typedef int SharedMemoryHandle;
-// TODO(port): these semaphores can leak if we crash, causing
-// autobuilder problems.  Transition to something easier to clean up
-// (e.g. lockf/flock).
-// TODO(port): make sure what we transition to is fast enough.
-typedef sem_t* SharedMemoryLock;
+// On POSIX, the lock is implemented as a lockf() on the mapped file,
+// so no additional member (or definition of SharedMemoryLock) is
+// needed.
 #endif
 
 // Platform abstraction for shared memory.  Provides a C++ wrapper
@@ -123,6 +121,12 @@ class SharedMemory {
   // Lock the shared memory.
   // This is a cross-process lock which may be recursively
   // locked by the same thread.
+  // TODO(port):
+  // WARNING: on POSIX the lock only works across processes, not
+  // across threads.  2 threads in the same process can both grab the
+  // lock at the same time.  There are several solutions for this
+  // (futex, lockf+anon_semaphore) but none are both clean and common
+  // across Mac and Linux.
   void Lock();
 
   // Release the shared memory lock.
@@ -133,6 +137,8 @@ class SharedMemory {
   bool CreateOrOpen(const std::wstring &name, int posix_flags, size_t size);
   bool FilenameForMemoryName(const std::wstring &memname,
                              std::wstring *filename);
+  void LockOrUnlockCommon(int function);
+
 #endif
   bool ShareToProcessCommon(ProcessHandle process,
                             SharedMemoryHandle* new_handle,
@@ -143,9 +149,8 @@ class SharedMemory {
   void*              memory_;
   bool               read_only_;
   size_t             max_size_;
+#if !defined(OS_POSIX)
   SharedMemoryLock   lock_;
-#if defined(OS_POSIX)
-  std::string        sem_name_;
 #endif
 
   DISALLOW_EVIL_CONSTRUCTORS(SharedMemory);
diff --git a/base/shared_memory_posix.cc b/base/shared_memory_posix.cc
index 907ef2d..a28672e 100644
--- a/base/shared_memory_posix.cc
+++ b/base/shared_memory_posix.cc
@@ -10,6 +10,7 @@
 
 #include "base/file_util.h"
 #include "base/logging.h"
+#include "base/platform_thread.h"
 #include "base/string_util.h"
 
 namespace base {
@@ -24,16 +25,14 @@ SharedMemory::SharedMemory()
     : mapped_file_(-1),
       memory_(NULL),
       read_only_(false),
-      max_size_(0),
-      lock_(NULL) {
+      max_size_(0) {
 }
 
 SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only)
     : mapped_file_(handle),
       memory_(NULL),
       read_only_(read_only),
-      max_size_(0),
-      lock_(NULL) {
+      max_size_(0) {
 }
 
 SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only,
@@ -41,8 +40,7 @@ SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only,
     : mapped_file_(handle),
       memory_(NULL),
       read_only_(read_only),
-      max_size_(0),
-      lock_(NULL) {
+      max_size_(0) {
   // We don't handle this case yet (note the ignored parameter); let's die if
   // someone comes calling.
   NOTREACHED();
@@ -50,8 +48,6 @@ SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only,
 
 SharedMemory::~SharedMemory() {
   Close();
-  if (lock_ != NULL)
-    sem_close(lock_);
 }
 
 bool SharedMemory::Create(const std::wstring &name, bool read_only,
@@ -205,19 +201,6 @@ bool SharedMemory::CreateOrOpen(const std::wstring &name,
 
   mapped_file_ = dup(fileno(fp));
   DCHECK(mapped_file_ >= 0);
-
-  mode_t posix_mode =  S_IRUSR | S_IWUSR;  // owner read/write
-  // name_ that includes a tmpdir easily exceeds SEM_NAME_LEN,
-  // so we cannot use it directly as the basis for a sem_name_.
-  sem_name_ = WideToUTF8(name) + kSemaphoreSuffix;
-  lock_ = sem_open(sem_name_.c_str(), O_CREAT, posix_mode, 1);
-  if (lock_ == SEM_FAILED) {
-    close(mapped_file_);
-    mapped_file_ = -1;
-    lock_ = NULL;
-    return false;
-  }
-
   return true;
 }
 
@@ -263,24 +246,33 @@ void SharedMemory::Close() {
     close(mapped_file_);
     mapped_file_ = -1;
   }
+}
 
-  if (lock_) {
-    sem_unlink(sem_name_.c_str());
-    lock_ = NULL;
+void SharedMemory::LockOrUnlockCommon(int function) {
+  DCHECK(mapped_file_ >= 0);
+  while (lockf(mapped_file_, function, 0) < 0) {
+    if (errno == EINTR) {
+      continue;
+    } else if (errno == ENOLCK) {
+      // temporary kernel resource exaustion
+      PlatformThread::Sleep(500);
+      continue;
+    } else {
+      NOTREACHED() << "lockf() failed."
+                   << " function:" << function
+                   << " fd:" << mapped_file_
+                   << " errno:" << errno
+                   << " msg:" << strerror(errno);
+    }
   }
 }
 
 void SharedMemory::Lock() {
-  DCHECK(lock_ != NULL);
-  while(sem_wait(lock_) < 0) {
-    DCHECK(errno == EAGAIN || errno == EINTR);
-  }
+  LockOrUnlockCommon(F_LOCK);
 }
 
 void SharedMemory::Unlock() {
-  DCHECK(lock_ != NULL);
-  int result = sem_post(lock_);
-  DCHECK(result == 0);
+  LockOrUnlockCommon(F_ULOCK);
 }
 
 }  // namespace base
diff --git a/base/shared_memory_unittest.cc b/base/shared_memory_unittest.cc
index bdb6c86..4b1e088 100644
--- a/base/shared_memory_unittest.cc
+++ b/base/shared_memory_unittest.cc
@@ -3,12 +3,15 @@
 // found in the LICENSE file.
 
 #include "base/basictypes.h"
+#include "base/multiprocess_test.h"
 #include "base/platform_thread.h"
 #include "base/scoped_nsautorelease_pool.h"
 #include "base/shared_memory.h"
+#include "base/scoped_ptr.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 static const int kNumThreads = 5;
+static const int kNumTasks = 5;
 
 namespace base {
 
@@ -158,27 +161,40 @@ TEST(SharedMemoryTest, OpenClose) {
   EXPECT_TRUE(rv);
 }
 
-// Create a set of 5 threads to each open a shared memory segment and write to
+// Create a set of N threads to each open a shared memory segment and write to
 // it. Verify that they are always reading/writing consistent data.
 TEST(SharedMemoryTest, MultipleThreads) {
   MultipleThreadMain::CleanUp();
-  PlatformThreadHandle thread_handles[kNumThreads];
-  MultipleThreadMain* thread_delegates[kNumThreads];
-
-  // Spawn the threads.
-  for (int16 index = 0; index < kNumThreads; index++) {
-    PlatformThreadHandle pth;
-    thread_delegates[index] = new MultipleThreadMain(index);
-    EXPECT_TRUE(PlatformThread::Create(0, thread_delegates[index], &pth));
-    thread_handles[index] = pth;
-  }
+  // On POSIX we have a problem when 2 threads try to create the shmem
+  // (a file) at exactly the same time, since create both creates the
+  // file and zerofills it.  We solve the problem for this unit test
+  // (make it not flaky) by starting with 1 thread, then
+  // intentionally don't clean up its shmem before running with
+  // kNumThreads.
+
+  int threadcounts[] = { 1, kNumThreads };
+  for (size_t i = 0; i < sizeof(threadcounts) / sizeof(threadcounts); i++) {
+    int numthreads = threadcounts[i];
+    scoped_array<PlatformThreadHandle> thread_handles;
+    scoped_array<MultipleThreadMain*> thread_delegates;
+
+    thread_handles.reset(new PlatformThreadHandle[numthreads]);
+    thread_delegates.reset(new MultipleThreadMain*[numthreads]);
+
+    // Spawn the threads.
+    for (int16 index = 0; index < numthreads; index++) {
+      PlatformThreadHandle pth;
+      thread_delegates[index] = new MultipleThreadMain(index);
+      EXPECT_TRUE(PlatformThread::Create(0, thread_delegates[index], &pth));
+      thread_handles[index] = pth;
+    }
 
-  // Wait for the threads to finish.
-  for (int index = 0; index < kNumThreads; index++) {
-    PlatformThread::Join(thread_handles[index]);
-    delete thread_delegates[index];
+    // Wait for the threads to finish.
+    for (int index = 0; index < numthreads; index++) {
+      PlatformThread::Join(thread_handles[index]);
+      delete thread_delegates[index];
+    }
   }
-
   MultipleThreadMain::CleanUp();
 }
 
@@ -219,10 +235,10 @@ TEST(SharedMemoryTest, AnonymousPrivate) {
   bool rv;
   const int kDataSize = 8192;
 
-  SharedMemory* memories = new SharedMemory[count];
-  int **pointers = new int*[count];
-  ASSERT_TRUE(memories);
-  ASSERT_TRUE(pointers);
+  scoped_array<SharedMemory> memories(new SharedMemory[count]);
+  scoped_array<int*> pointers(new int*[count]);
+  ASSERT_TRUE(memories.get());
+  ASSERT_TRUE(pointers.get());
 
   for (i = 0; i < count; i++) {
     rv = memories[i].Create(L"", false, true, kDataSize);
@@ -254,6 +270,75 @@ TEST(SharedMemoryTest, AnonymousPrivate) {
   for (int i = 0; i < count; i++) {
     memories[i].Close();
   }
+
+}
+
+
+// On POSIX it is especially important we test shmem across processes,
+// not just across threads.  But the test is enabled on all platforms.
+class SharedMemoryProcessTest : public MultiProcessTest {
+ public:
+
+  static void CleanUp() {
+    SharedMemory memory;
+    memory.Delete(test_name_);
+  }
+
+  static int TaskTestMain() {
+    int errors = 0;
+    ScopedNSAutoreleasePool pool;  // noop if not OSX
+    const int kDataSize = 1024;
+    SharedMemory memory;
+    bool rv = memory.Create(test_name_, false, true, kDataSize);
+    EXPECT_TRUE(rv);
+    if (rv != true)
+      errors++;
+    rv = memory.Map(kDataSize);
+    EXPECT_TRUE(rv);
+    if (rv != true)
+      errors++;
+    int *ptr = static_cast<int*>(memory.memory());
+
+    for (int idx = 0; idx < 20; idx++) {
+      memory.Lock();
+      int i = (1 << 16) + idx;
+      *ptr = i;
+      PlatformThread::Sleep(10);  // Short wait.
+      if (*ptr != i)
+        errors++;
+      memory.Unlock();
+    }
+
+    memory.Close();
+    return errors;
+  }
+
+ private:
+  static const std::wstring test_name_;
+};
+
+const std::wstring SharedMemoryProcessTest::test_name_ = L"MPMem";
+
+
+TEST_F(SharedMemoryProcessTest, Tasks) {
+  SharedMemoryProcessTest::CleanUp();
+
+  base::ProcessHandle handles[kNumTasks];
+  for (int index = 0; index < kNumTasks; ++index) {
+    handles[index] = SpawnChild(L"SharedMemoryTestMain");
+  }
+
+  int exit_code = 0;
+  for (int index = 0; index < kNumTasks; ++index) {
+    EXPECT_TRUE(base::WaitForExitCode(handles[index], &exit_code));
+    EXPECT_TRUE(exit_code == 0);
+  }
+
+  SharedMemoryProcessTest::CleanUp();
+}
+
+MULTIPROCESS_TEST_MAIN(SharedMemoryTestMain) {
+  return SharedMemoryProcessTest::TaskTestMain();
 }