metrics: Remove SharedPersistentMemoryAllocator

SharedPersistentMemoryAllocator is based on obsolete shared memory classes and
is being replaced by {Writable,ReadOnly}SharedMemoryAllocator.

SharedPersistentMemoryAllocator is still used in base/debug/activity_tracker.h
and base/debug/activity_analyzer.h.

This CL converts these last clients of the SharedPersistentMemoryAllocator to
the new allocators and removes SharedPersistentMemoryAllocator.

Bug: 920183
Change-Id: Ic789dc79a5798135a43bd59bbaaa4d2977c96cbf
Reviewed-on: https://chromium-review.googlesource.com/c/1481297Reviewed-by: Brian White <bcwhite@chromium.org>
Commit-Queue: Alex Ilin <alexilin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635471}

base/debug/activity_analyzer.cc

@@ -135,25 +135,15 @@ std::unique_ptr<GlobalActivityAnalyzer> GlobalActivityAnalyzer::CreateWithFile(
 // static
 std::unique_ptr<GlobalActivityAnalyzer>
 GlobalActivityAnalyzer::CreateWithSharedMemory(
-    std::unique_ptr<SharedMemory> shm) {
-  if (shm->mapped_size() == 0 ||
-      !SharedPersistentMemoryAllocator::IsSharedMemoryAcceptable(*shm)) {
+    base::ReadOnlySharedMemoryMapping mapping) {
+  if (!mapping.IsValid() ||
+      !ReadOnlySharedPersistentMemoryAllocator::IsSharedMemoryAcceptable(
+          mapping)) {
     return nullptr;
   }
-  return CreateWithAllocator(std::make_unique<SharedPersistentMemoryAllocator>(
-      std::move(shm), 0, StringPiece(), /*readonly=*/true));
-}
-
-// static
-std::unique_ptr<GlobalActivityAnalyzer>
-GlobalActivityAnalyzer::CreateWithSharedMemoryHandle(
-    const SharedMemoryHandle& handle,
-    size_t size) {
-  std::unique_ptr<SharedMemory> shm(
-      new SharedMemory(handle, /*readonly=*/true));
-  if (!shm->Map(size))
-    return nullptr;
-  return CreateWithSharedMemory(std::move(shm));
+  return CreateWithAllocator(
+      std::make_unique<ReadOnlySharedPersistentMemoryAllocator>(
+          std::move(mapping), 0, StringPiece()));
 }
 
 int64_t GlobalActivityAnalyzer::GetFirstProcess() {

base/debug/activity_analyzer.h

@@ -13,6 +13,7 @@
 
 #include "base/base_export.h"
 #include "base/debug/activity_tracker.h"
+#include "base/memory/shared_memory_mapping.h"
 
 namespace base {
 namespace debug {
@@ -150,13 +151,7 @@ class BASE_EXPORT GlobalActivityAnalyzer {
 
   // Like above but accesses an allocator in a mapped shared-memory segment.
   static std::unique_ptr<GlobalActivityAnalyzer> CreateWithSharedMemory(
-      std::unique_ptr<SharedMemory> shm);
-
-  // Like above but takes a handle to an existing shared memory segment and
-  // maps it before creating the tracker.
-  static std::unique_ptr<GlobalActivityAnalyzer> CreateWithSharedMemoryHandle(
-      const SharedMemoryHandle& handle,
-      size_t size);
+      base::ReadOnlySharedMemoryMapping mapping);
 
   // Iterates over all known valid processes and returns their PIDs or zero
   // if there are no more. Calls to GetFirstProcess() will perform a global

base/debug/activity_analyzer_unittest.cc

@@ -15,6 +15,7 @@
 #include "base/files/memory_mapped_file.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/ptr_util.h"
+#include "base/memory/read_only_shared_memory_region.h"
 #include "base/pending_task.h"
 #include "base/process/process.h"
 #include "base/stl_util.h"
@@ -217,25 +218,19 @@ TEST_F(ActivityAnalyzerTest, GlobalAnalyzerConstruction) {
 }
 
 TEST_F(ActivityAnalyzerTest, GlobalAnalyzerFromSharedMemory) {
-  SharedMemoryHandle handle1;
-  SharedMemoryHandle handle2;
-
-  {
-    std::unique_ptr<SharedMemory> shmem(new SharedMemory());
-    ASSERT_TRUE(shmem->CreateAndMapAnonymous(kMemorySize));
-    handle1 = shmem->handle().Duplicate();
-    ASSERT_TRUE(handle1.IsValid());
-    handle2 = shmem->handle().Duplicate();
-    ASSERT_TRUE(handle2.IsValid());
-  }
-
-  GlobalActivityTracker::CreateWithSharedMemoryHandle(handle1, kMemorySize, 0,
-                                                      "", 3);
+  base::MappedReadOnlyRegion shm =
+      base::ReadOnlySharedMemoryRegion::Create(kMemorySize);
+  ASSERT_TRUE(shm.IsValid());
+  base::WritableSharedMemoryMapping rw_mapping = std::move(shm.mapping);
+  base::ReadOnlySharedMemoryMapping ro_mapping = shm.region.Map();
+  ASSERT_TRUE(ro_mapping.IsValid());
+
+  GlobalActivityTracker::CreateWithSharedMemory(std::move(rw_mapping), 0, "",
+                                                3);
   GlobalActivityTracker::Get()->process_data().SetString("foo", "bar");
 
   std::unique_ptr<GlobalActivityAnalyzer> analyzer =
-      GlobalActivityAnalyzer::CreateWithSharedMemoryHandle(handle2,
-                                                           kMemorySize);
+      GlobalActivityAnalyzer::CreateWithSharedMemory(std::move(ro_mapping));
 
   const int64_t pid = analyzer->GetFirstProcess();
   ASSERT_NE(0, pid);

base/debug/activity_tracker.cc

@@ -1305,34 +1305,21 @@ bool GlobalActivityTracker::CreateWithLocalMemory(size_t size,
 
 // static
 bool GlobalActivityTracker::CreateWithSharedMemory(
-    std::unique_ptr<SharedMemory> shm,
+    base::WritableSharedMemoryMapping mapping,
     uint64_t id,
     StringPiece name,
     int stack_depth) {
-  if (shm->mapped_size() == 0 ||
-      !SharedPersistentMemoryAllocator::IsSharedMemoryAcceptable(*shm)) {
+  if (!mapping.IsValid() ||
+      !WritableSharedPersistentMemoryAllocator::IsSharedMemoryAcceptable(
+          mapping)) {
     return false;
   }
-  CreateWithAllocator(std::make_unique<SharedPersistentMemoryAllocator>(
-                          std::move(shm), id, name, false),
+  CreateWithAllocator(std::make_unique<WritableSharedPersistentMemoryAllocator>(
+                          std::move(mapping), id, name),
                       stack_depth, 0);
   return true;
 }
 
-// static
-bool GlobalActivityTracker::CreateWithSharedMemoryHandle(
-    const SharedMemoryHandle& handle,
-    size_t size,
-    uint64_t id,
-    StringPiece name,
-    int stack_depth) {
-  std::unique_ptr<SharedMemory> shm(
-      new SharedMemory(handle, /*readonly=*/false));
-  if (!shm->Map(size))
-    return false;
-  return CreateWithSharedMemory(std::move(shm), id, name, stack_depth);
-}
-
 // static
 void GlobalActivityTracker::SetForTesting(
     std::unique_ptr<GlobalActivityTracker> tracker) {

base/debug/activity_tracker.h

@@ -27,7 +27,7 @@
 #include "base/compiler_specific.h"
 #include "base/gtest_prod_util.h"
 #include "base/location.h"
-#include "base/memory/shared_memory.h"
+#include "base/memory/shared_memory_mapping.h"
 #include "base/metrics/persistent_memory_allocator.h"
 #include "base/process/process_handle.h"
 #include "base/strings/string_piece.h"
@@ -907,20 +907,12 @@ class BASE_EXPORT GlobalActivityTracker {
                                     int64_t process_id);
 
   // Like above but internally creates an allocator using a shared-memory
-  // segment. The segment must already be mapped into the local memory space.
-  static bool CreateWithSharedMemory(std::unique_ptr<SharedMemory> shm,
+  // segment that is already mapped into the local memory space.
+  static bool CreateWithSharedMemory(base::WritableSharedMemoryMapping mapping,
                                      uint64_t id,
                                      StringPiece name,
                                      int stack_depth);
 
-  // Like above but takes a handle to an existing shared memory segment and
-  // maps it before creating the tracker.
-  static bool CreateWithSharedMemoryHandle(const SharedMemoryHandle& handle,
-                                           size_t size,
-                                           uint64_t id,
-                                           StringPiece name,
-                                           int stack_depth);
-
   // Gets the global activity-tracker or null if none exists.
   static GlobalActivityTracker* Get() {
     return reinterpret_cast<GlobalActivityTracker*>(

base/metrics/persistent_memory_allocator.cc

@@ -1013,30 +1013,6 @@ void LocalPersistentMemoryAllocator::DeallocateLocalMemory(void* memory,
 #endif
 }
 
-//----- SharedPersistentMemoryAllocator ----------------------------------------
-
-SharedPersistentMemoryAllocator::SharedPersistentMemoryAllocator(
-    std::unique_ptr<SharedMemory> memory,
-    uint64_t id,
-    base::StringPiece name,
-    bool read_only)
-    : PersistentMemoryAllocator(
-          Memory(static_cast<uint8_t*>(memory->memory()), MEM_SHARED),
-          memory->mapped_size(),
-          0,
-          id,
-          name,
-          read_only),
-      shared_memory_(std::move(memory)) {}
-
-SharedPersistentMemoryAllocator::~SharedPersistentMemoryAllocator() = default;
-
-// static
-bool SharedPersistentMemoryAllocator::IsSharedMemoryAcceptable(
-    const SharedMemory& memory) {
-  return IsMemoryAcceptable(memory.memory(), memory.mapped_size(), 0, false);
-}
-
 //----- WritableSharedPersistentMemoryAllocator --------------------------------
 
 WritableSharedPersistentMemoryAllocator::

base/metrics/persistent_memory_allocator.h

@@ -23,7 +23,6 @@ namespace base {
 
 class HistogramBase;
 class MemoryMappedFile;
-class SharedMemory;
 
 // Simple allocator for pieces of a memory block that may be persistent
 // to some storage or shared across multiple processes. This class resides
@@ -711,32 +710,6 @@ class BASE_EXPORT LocalPersistentMemoryAllocator
 };
 
 
-// This allocator takes a shared-memory object and performs allocation from
-// it. The memory must be previously mapped via Map() or MapAt(). The allocator
-// takes ownership of the memory object.
-class BASE_EXPORT SharedPersistentMemoryAllocator
-    : public PersistentMemoryAllocator {
- public:
-  SharedPersistentMemoryAllocator(std::unique_ptr<SharedMemory> memory,
-                                  uint64_t id,
-                                  base::StringPiece name,
-                                  bool read_only);
-  ~SharedPersistentMemoryAllocator() override;
-
-  SharedMemory* shared_memory() { return shared_memory_.get(); }
-
-  // Ensure that the memory isn't so invalid that it would crash when passing it
-  // to the allocator. This doesn't guarantee the data is valid, just that it
-  // won't cause the program to abort. The existing IsCorrupt() call will handle
-  // the rest.
-  static bool IsSharedMemoryAcceptable(const SharedMemory& memory);
-
- private:
-  std::unique_ptr<SharedMemory> shared_memory_;
-
-  DISALLOW_COPY_AND_ASSIGN(SharedPersistentMemoryAllocator);
-};
-
 // This allocator takes a writable shared memory mapping object and performs
 // allocation from it. The allocator takes ownership of the mapping object.
 class BASE_EXPORT WritableSharedPersistentMemoryAllocator

base/metrics/persistent_memory_allocator_unittest.cc

@@ -10,7 +10,9 @@
 #include "base/files/file_util.h"
 #include "base/files/memory_mapped_file.h"
 #include "base/files/scoped_temp_dir.h"
-#include "base/memory/shared_memory.h"
+#include "base/memory/read_only_shared_memory_region.h"
+#include "base/memory/shared_memory_mapping.h"
+#include "base/memory/writable_shared_memory_region.h"
 #include "base/metrics/histogram.h"
 #include "base/rand_util.h"
 #include "base/strings/safe_sprintf.h"
@@ -608,20 +610,20 @@ TEST(LocalPersistentMemoryAllocatorTest, CreationTest) {
   EXPECT_FALSE(allocator.IsCorrupt());
 }
 
-
-//----- SharedPersistentMemoryAllocator ----------------------------------------
+//----- {Writable,ReadOnly}SharedPersistentMemoryAllocator ---------------------
 
 TEST(SharedPersistentMemoryAllocatorTest, CreationTest) {
-  SharedMemoryHandle shared_handle_1;
-  SharedMemoryHandle shared_handle_2;
+  base::WritableSharedMemoryRegion rw_region =
+      base::WritableSharedMemoryRegion::Create(TEST_MEMORY_SIZE);
+  ASSERT_TRUE(rw_region.IsValid());
 
   PersistentMemoryAllocator::MemoryInfo meminfo1;
   Reference r123, r456, r789;
   {
-    std::unique_ptr<SharedMemory> shmem1(new SharedMemory());
-    ASSERT_TRUE(shmem1->CreateAndMapAnonymous(TEST_MEMORY_SIZE));
-    SharedPersistentMemoryAllocator local(std::move(shmem1), TEST_ID, "",
-                                          false);
+    base::WritableSharedMemoryMapping mapping = rw_region.Map();
+    ASSERT_TRUE(mapping.IsValid());
+    WritableSharedPersistentMemoryAllocator local(std::move(mapping), TEST_ID,
+                                                  "");
     EXPECT_FALSE(local.IsReadonly());
     r123 = local.Allocate(123, 123);
     r456 = local.Allocate(456, 456);
@@ -632,19 +634,20 @@ TEST(SharedPersistentMemoryAllocatorTest, CreationTest) {
     local.GetMemoryInfo(&meminfo1);
     EXPECT_FALSE(local.IsFull());
     EXPECT_FALSE(local.IsCorrupt());
-
-    shared_handle_1 = local.shared_memory()->handle().Duplicate();
-    ASSERT_TRUE(shared_handle_1.IsValid());
-    shared_handle_2 = local.shared_memory()->handle().Duplicate();
-    ASSERT_TRUE(shared_handle_2.IsValid());
   }
 
-  // Read-only test.
-  std::unique_ptr<SharedMemory> shmem2(new SharedMemory(shared_handle_1,
-                                                        /*readonly=*/true));
-  ASSERT_TRUE(shmem2->Map(TEST_MEMORY_SIZE));
+  // Create writable and read-only mappings of the same region.
+  base::WritableSharedMemoryMapping rw_mapping = rw_region.Map();
+  ASSERT_TRUE(rw_mapping.IsValid());
+  base::ReadOnlySharedMemoryRegion ro_region =
+      base::WritableSharedMemoryRegion::ConvertToReadOnly(std::move(rw_region));
+  ASSERT_TRUE(ro_region.IsValid());
+  base::ReadOnlySharedMemoryMapping ro_mapping = ro_region.Map();
+  ASSERT_TRUE(ro_mapping.IsValid());
 
-  SharedPersistentMemoryAllocator shalloc2(std::move(shmem2), 0, "", true);
+  // Read-only test.
+  ReadOnlySharedPersistentMemoryAllocator shalloc2(std::move(ro_mapping), 0,
+                                                   "");
   EXPECT_TRUE(shalloc2.IsReadonly());
   EXPECT_EQ(TEST_ID, shalloc2.Id());
   EXPECT_FALSE(shalloc2.IsFull());
@@ -666,11 +669,8 @@ TEST(SharedPersistentMemoryAllocatorTest, CreationTest) {
   EXPECT_EQ(meminfo1.free, meminfo2.free);
 
   // Read/write test.
-  std::unique_ptr<SharedMemory> shmem3(new SharedMemory(shared_handle_2,
-                                                        /*readonly=*/false));
-  ASSERT_TRUE(shmem3->Map(TEST_MEMORY_SIZE));
-
-  SharedPersistentMemoryAllocator shalloc3(std::move(shmem3), 0, "", false);
+  WritableSharedPersistentMemoryAllocator shalloc3(std::move(rw_mapping), 0,
+                                                   "");
   EXPECT_FALSE(shalloc3.IsReadonly());
   EXPECT_EQ(TEST_ID, shalloc3.Id());
   EXPECT_FALSE(shalloc3.IsFull());