Remove unused locking functionality from base::SharedMemory.
The only existing consumers of the locking functionality were two unit tests for
SharedMemory.
The first unit test only tested the locking functionality across threads, so I
removed the test.
The second unit test was intended to test cross-process functionality of
base::SharedMemory, and used the locking functionality as a synchronization
method between processes. I rewrote the test to actually test Shared Memory
functionality, as the original test only tested synchronization between
processes, and would have succeeded even if Shared Memory didn't work. I
replaced the synchronization with compare and swap operations.
BUG=466437, 345734
Review URL: https://codereview.chromium.org/1167863002
Cr-Commit-Position: refs/heads/master@{#335135}
CrOS-Libchrome-Original-Commit: 893dadc6e82a64c8135b6f4852ee0960ea62e4a5
diff --git a/base/memory/shared_memory.h b/base/memory/shared_memory.h
index 008bb01..68db65c 100644
--- a/base/memory/shared_memory.h
+++ b/base/memory/shared_memory.h
@@ -257,27 +257,11 @@
return ShareToProcessCommon(process, new_handle, true, SHARE_CURRENT_MODE);
}
- // DEPRECATED (crbug.com/345734):
- // Locks the shared memory.
- //
- // WARNING: on POSIX the memory locking primitive only works across
- // processes, not across threads. The LockDeprecated method is not currently
- // used in inner loops, so we protect against multiple threads in a
- // critical section using a class global lock.
- void LockDeprecated();
-
- // DEPRECATED (crbug.com/345734):
- // Releases the shared memory lock.
- void UnlockDeprecated();
-
private:
-#if defined(OS_POSIX) && !defined(OS_NACL)
-#if !defined(OS_ANDROID)
+#if defined(OS_POSIX) && !defined(OS_NACL) && !defined(OS_ANDROID)
bool PrepareMapFile(ScopedFILE fp, ScopedFD readonly);
bool FilePathForMemoryName(const std::string& mem_name, FilePath* path);
-#endif
- void LockOrUnlockCommon(int function);
-#endif // defined(OS_POSIX) && !defined(OS_NACL)
+#endif // defined(OS_POSIX) && !defined(OS_NACL) && !defined(OS_ANDROID)
enum ShareMode {
SHARE_READONLY,
SHARE_CURRENT_MODE,
@@ -298,32 +282,9 @@
void* memory_;
bool read_only_;
size_t requested_size_;
-#if !defined(OS_POSIX)
- HANDLE lock_;
-#endif
DISALLOW_COPY_AND_ASSIGN(SharedMemory);
};
-
-// DEPRECATED (crbug.com/345734):
-// A helper class that acquires the shared memory lock while
-// the SharedMemoryAutoLockDeprecated is in scope.
-class SharedMemoryAutoLockDeprecated {
- public:
- explicit SharedMemoryAutoLockDeprecated(SharedMemory* shared_memory)
- : shared_memory_(shared_memory) {
- shared_memory_->LockDeprecated();
- }
-
- ~SharedMemoryAutoLockDeprecated() {
- shared_memory_->UnlockDeprecated();
- }
-
- private:
- SharedMemory* shared_memory_;
- DISALLOW_COPY_AND_ASSIGN(SharedMemoryAutoLockDeprecated);
-};
-
} // namespace base
#endif // BASE_MEMORY_SHARED_MEMORY_H_
diff --git a/base/memory/shared_memory_posix.cc b/base/memory/shared_memory_posix.cc
index 35d746e..8aaa9ce 100644
--- a/base/memory/shared_memory_posix.cc
+++ b/base/memory/shared_memory_posix.cc
@@ -4,16 +4,13 @@
#include "base/memory/shared_memory.h"
-#include <errno.h>
#include <fcntl.h>
#include <sys/mman.h>
#include <sys/stat.h>
-#include <sys/types.h>
#include <unistd.h>
#include "base/files/file_util.h"
#include "base/files/scoped_file.h"
-#include "base/lazy_instance.h"
#include "base/logging.h"
#include "base/posix/eintr_wrapper.h"
#include "base/posix/safe_strerror.h"
@@ -21,9 +18,6 @@
#include "base/profiler/scoped_tracker.h"
#include "base/scoped_generic.h"
#include "base/strings/utf_string_conversions.h"
-#include "base/synchronization/lock.h"
-#include "base/threading/platform_thread.h"
-#include "base/threading/thread_restrictions.h"
#if defined(OS_MACOSX)
#include "base/mac/foundation_util.h"
@@ -38,8 +32,6 @@
namespace {
-LazyInstance<Lock>::Leaky g_thread_lock_ = LAZY_INSTANCE_INITIALIZER;
-
struct ScopedPathUnlinkerTraits {
static FilePath* InvalidValue() { return nullptr; }
@@ -414,16 +406,6 @@
}
}
-void SharedMemory::LockDeprecated() {
- g_thread_lock_.Get().Acquire();
- LockOrUnlockCommon(F_LOCK);
-}
-
-void SharedMemory::UnlockDeprecated() {
- LockOrUnlockCommon(F_ULOCK);
- g_thread_lock_.Get().Release();
-}
-
#if !defined(OS_ANDROID)
bool SharedMemory::PrepareMapFile(ScopedFILE fp, ScopedFD readonly_fd) {
DCHECK_EQ(-1, mapped_file_);
@@ -491,25 +473,6 @@
}
#endif // !defined(OS_ANDROID)
-void SharedMemory::LockOrUnlockCommon(int function) {
- DCHECK_GE(mapped_file_, 0);
- while (lockf(mapped_file_, function, 0) < 0) {
- if (errno == EINTR) {
- continue;
- } else if (errno == ENOLCK) {
- // temporary kernel resource exaustion
- base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(500));
- continue;
- } else {
- NOTREACHED() << "lockf() failed."
- << " function:" << function
- << " fd:" << mapped_file_
- << " errno:" << errno
- << " msg:" << base::safe_strerror(errno);
- }
- }
-}
-
bool SharedMemory::ShareToProcessCommon(ProcessHandle process,
SharedMemoryHandle* new_handle,
bool close_self,
diff --git a/base/memory/shared_memory_unittest.cc b/base/memory/shared_memory_unittest.cc
index 6fe5706..7ed8955 100644
--- a/base/memory/shared_memory_unittest.cc
+++ b/base/memory/shared_memory_unittest.cc
@@ -2,6 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include "base/atomicops.h"
#include "base/basictypes.h"
#include "base/memory/scoped_ptr.h"
#include "base/memory/shared_memory.h"
@@ -33,7 +34,7 @@
#endif
static const int kNumThreads = 5;
-#if !defined(OS_IOS) // iOS does not allow multiple processes.
+#if !defined(OS_IOS) && !defined(OS_ANDROID)
static const int kNumTasks = 5;
#endif
@@ -90,56 +91,6 @@
const char* const MultipleThreadMain::s_test_name_ =
"SharedMemoryOpenThreadTest";
-// TODO(port):
-// This test requires the ability to pass file descriptors between processes.
-// We haven't done that yet in Chrome for POSIX.
-#if defined(OS_WIN)
-// Each thread will open the shared memory. Each thread will take the memory,
-// and keep changing it while trying to lock it, with some small pauses in
-// between. Verify that each thread's value in the shared memory is always
-// correct.
-class MultipleLockThread : public PlatformThread::Delegate {
- public:
- explicit MultipleLockThread(int id) : id_(id) {}
- ~MultipleLockThread() override {}
-
- // PlatformThread::Delegate interface.
- void ThreadMain() override {
- const uint32 kDataSize = sizeof(int);
- SharedMemoryHandle handle = NULL;
- {
- SharedMemory memory1;
- EXPECT_TRUE(memory1.CreateNamedDeprecated(
- "SharedMemoryMultipleLockThreadTest", true, kDataSize));
- EXPECT_TRUE(memory1.ShareToProcess(GetCurrentProcess(), &handle));
- // TODO(paulg): Implement this once we have a posix version of
- // SharedMemory::ShareToProcess.
- EXPECT_TRUE(true);
- }
-
- SharedMemory memory2(handle, false);
- EXPECT_TRUE(memory2.Map(kDataSize));
- volatile int* const ptr = static_cast<int*>(memory2.memory());
-
- for (int idx = 0; idx < 20; idx++) {
- memory2.LockDeprecated();
- int i = (id_ << 16) + idx;
- *ptr = i;
- PlatformThread::Sleep(TimeDelta::FromMilliseconds(1));
- EXPECT_EQ(*ptr, i);
- memory2.UnlockDeprecated();
- }
-
- memory2.Close();
- }
-
- private:
- int id_;
-
- DISALLOW_COPY_AND_ASSIGN(MultipleLockThread);
-};
-#endif
-
} // namespace
// Android doesn't support SharedMemory::Open/Delete/
@@ -320,34 +271,6 @@
MultipleThreadMain::CleanUp();
}
-// TODO(port): this test requires the MultipleLockThread class
-// (defined above), which requires the ability to pass file
-// descriptors between processes. We haven't done that yet in Chrome
-// for POSIX.
-#if defined(OS_WIN)
-// Create a set of threads to each open a shared memory segment and write to it
-// with the lock held. Verify that they are always reading/writing consistent
-// data.
-TEST(SharedMemoryTest, Lock) {
- PlatformThreadHandle thread_handles[kNumThreads];
- MultipleLockThread* thread_delegates[kNumThreads];
-
- // Spawn the threads.
- for (int index = 0; index < kNumThreads; ++index) {
- PlatformThreadHandle pth;
- thread_delegates[index] = new MultipleLockThread(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];
- }
-}
-#endif
-
// Allocate private (unique) shared memory with an empty string for a
// name. Make sure several of them don't point to the same thing as
// we might expect if the names are equal.
@@ -647,7 +570,9 @@
shared_memory.Close();
}
-#if !defined(OS_IOS) // iOS does not allow multiple processes.
+// iOS does not allow multiple processes.
+// Android ashmem doesn't support named shared memory.
+#if !defined(OS_IOS) && !defined(OS_ANDROID)
// On POSIX it is especially important we test shmem across processes,
// not just across threads. But the test is enabled on all platforms.
@@ -664,53 +589,61 @@
#if defined(OS_MACOSX)
mac::ScopedNSAutoreleasePool pool;
#endif
- const uint32 kDataSize = 1024;
SharedMemory memory;
- bool rv = memory.CreateNamedDeprecated(s_test_name_, true, kDataSize);
+ bool rv = memory.CreateNamedDeprecated(s_test_name_, true, s_data_size_);
EXPECT_TRUE(rv);
if (rv != true)
errors++;
- rv = memory.Map(kDataSize);
+ rv = memory.Map(s_data_size_);
EXPECT_TRUE(rv);
if (rv != true)
errors++;
int *ptr = static_cast<int*>(memory.memory());
- for (int idx = 0; idx < 20; idx++) {
- memory.LockDeprecated();
- int i = (1 << 16) + idx;
- *ptr = i;
- PlatformThread::Sleep(TimeDelta::FromMilliseconds(10));
- if (*ptr != i)
- errors++;
- memory.UnlockDeprecated();
- }
-
+ // This runs concurrently in multiple processes. Writes need to be atomic.
+ base::subtle::Barrier_AtomicIncrement(ptr, 1);
memory.Close();
return errors;
}
- private:
static const char* const s_test_name_;
+ static const uint32 s_data_size_;
};
const char* const SharedMemoryProcessTest::s_test_name_ = "MPMem";
+const uint32 SharedMemoryProcessTest::s_data_size_ = 1024;
-TEST_F(SharedMemoryProcessTest, Tasks) {
+TEST_F(SharedMemoryProcessTest, SharedMemoryAcrossProcesses) {
SharedMemoryProcessTest::CleanUp();
+ // Create a shared memory region. Set the first word to 0.
+ SharedMemory memory;
+ bool rv = memory.CreateNamedDeprecated(s_test_name_, true, s_data_size_);
+ ASSERT_TRUE(rv);
+ rv = memory.Map(s_data_size_);
+ ASSERT_TRUE(rv);
+ int* ptr = static_cast<int*>(memory.memory());
+ *ptr = 0;
+
+ // Start |kNumTasks| processes, each of which atomically increments the first
+ // word by 1.
Process processes[kNumTasks];
for (int index = 0; index < kNumTasks; ++index) {
processes[index] = SpawnChild("SharedMemoryTestMain");
ASSERT_TRUE(processes[index].IsValid());
}
+ // Check that each process exited correctly.
int exit_code = 0;
for (int index = 0; index < kNumTasks; ++index) {
EXPECT_TRUE(processes[index].WaitForExit(&exit_code));
EXPECT_EQ(0, exit_code);
}
+ // Check that the shared memory region reflects |kNumTasks| increments.
+ ASSERT_EQ(kNumTasks, *ptr);
+
+ memory.Close();
SharedMemoryProcessTest::CleanUp();
}
@@ -718,6 +651,6 @@
return SharedMemoryProcessTest::TaskTestMain();
}
-#endif // !OS_IOS
+#endif // !defined(OS_IOS) && !defined(OS_ANDROID)
} // namespace base