From 893dadc6e82a64c8135b6f4852ee0960ea62e4a5 Mon Sep 17 00:00:00 2001 From: erikchen Date: Thu, 18 Jun 2015 14:48:33 -0700 Subject: [PATCH] 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} --- base/memory/shared_memory.h | 43 +----------- base/memory/shared_memory_nacl.cc | 8 --- base/memory/shared_memory_posix.cc | 37 ---------- base/memory/shared_memory_unittest.cc | 125 ++++++++-------------------------- base/memory/shared_memory_win.cc | 35 ++-------- 5 files changed, 36 insertions(+), 212 deletions(-) diff --git a/base/memory/shared_memory.h b/base/memory/shared_memory.h index 008bb0106688..68db65c2f322 100644 --- a/base/memory/shared_memory.h +++ b/base/memory/shared_memory.h @@ -257,27 +257,11 @@ class BASE_EXPORT SharedMemory { 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 @@ class BASE_EXPORT SharedMemory { 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_nacl.cc b/base/memory/shared_memory_nacl.cc index 26dd4a380d6b..4a1fa11141bf 100644 --- a/base/memory/shared_memory_nacl.cc +++ b/base/memory/shared_memory_nacl.cc @@ -139,14 +139,6 @@ void SharedMemory::Close() { } } -void SharedMemory::LockDeprecated() { - NOTIMPLEMENTED(); -} - -void SharedMemory::UnlockDeprecated() { - NOTIMPLEMENTED(); -} - bool SharedMemory::ShareToProcessCommon(ProcessHandle process, SharedMemoryHandle *new_handle, bool close_self, diff --git a/base/memory/shared_memory_posix.cc b/base/memory/shared_memory_posix.cc index 35d746e5394e..8aaa9ce44389 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 #include #include #include -#include #include #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 base { namespace { -LazyInstance::Leaky g_thread_lock_ = LAZY_INSTANCE_INITIALIZER; - struct ScopedPathUnlinkerTraits { static FilePath* InvalidValue() { return nullptr; } @@ -414,16 +406,6 @@ void SharedMemory::Close() { } } -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 @@ bool SharedMemory::FilePathForMemoryName(const std::string& mem_name, } #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 6fe57060e6fc..7ed8955525ec 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 @@ class MultipleThreadMain : public PlatformThread::Delegate { 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(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 @@ TEST(SharedMemoryTest, MultipleThreads) { 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 @@ TEST(SharedMemoryTest, MapMinimumAlignment) { 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 @@ class SharedMemoryProcessTest : public MultiProcessTest { #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(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(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 @@ MULTIPROCESS_TEST_MAIN(SharedMemoryTestMain) { return SharedMemoryProcessTest::TaskTestMain(); } -#endif // !OS_IOS +#endif // !defined(OS_IOS) && !defined(OS_ANDROID) } // namespace base diff --git a/base/memory/shared_memory_win.cc b/base/memory/shared_memory_win.cc index eacf0d6ccefc..b1f85da93356 100644 --- a/base/memory/shared_memory_win.cc +++ b/base/memory/shared_memory_win.cc @@ -32,8 +32,7 @@ SharedMemory::SharedMemory() memory_(NULL), read_only_(false), mapped_size_(0), - requested_size_(0), - lock_(NULL) { + requested_size_(0) { } SharedMemory::SharedMemory(const std::wstring& name) @@ -42,7 +41,6 @@ SharedMemory::SharedMemory(const std::wstring& name) read_only_(false), requested_size_(0), mapped_size_(0), - lock_(NULL), name_(name) { } @@ -51,18 +49,17 @@ SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only) memory_(NULL), read_only_(read_only), requested_size_(0), - mapped_size_(0), - lock_(NULL) { + mapped_size_(0) { } -SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only, +SharedMemory::SharedMemory(SharedMemoryHandle handle, + bool read_only, ProcessHandle process) : mapped_file_(NULL), memory_(NULL), read_only_(read_only), requested_size_(0), - mapped_size_(0), - lock_(NULL) { + mapped_size_(0) { ::DuplicateHandle(process, handle, GetCurrentProcess(), &mapped_file_, read_only_ ? FILE_MAP_READ : FILE_MAP_READ | @@ -73,8 +70,6 @@ SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only, SharedMemory::~SharedMemory() { Unmap(); Close(); - if (lock_ != NULL) - CloseHandle(lock_); } // static @@ -270,26 +265,6 @@ void SharedMemory::Close() { } } -void SharedMemory::LockDeprecated() { - if (lock_ == NULL) { - std::wstring name = name_; - name.append(L"lock"); - lock_ = CreateMutex(NULL, FALSE, name.c_str()); - if (lock_ == NULL) { - DPLOG(ERROR) << "Could not create mutex."; - NOTREACHED(); - return; // There is nothing good we can do here. - } - } - DWORD result = WaitForSingleObject(lock_, INFINITE); - DCHECK_EQ(result, WAIT_OBJECT_0); -} - -void SharedMemory::UnlockDeprecated() { - DCHECK(lock_ != NULL); - ReleaseMutex(lock_); -} - SharedMemoryHandle SharedMemory::handle() const { return mapped_file_; } -- 2.11.4.GIT