Make changing a record's type an atomic operation.

Accessing the type_id is generally thread-safe within the PMA
code because it is only accessed once the memory is owned to
the exclusion of any other allocator. The same can't be said,
however, once access to it is given to a caller and so
protection needs to be added so that callers can't introduce
race conditions due to their inter-process thread-unsafety.

BUG=546019

Review-Url: https://codereview.chromium.org/2034813003
Cr-Commit-Position: refs/heads/master@{#397711}

base/metrics/persistent_histogram_allocator.cc

@@ -481,7 +481,7 @@ void PersistentHistogramAllocator::FinalizeHistogram(Reference ref,
   // two to be created. The allocator does not support releasing the
   // acquired memory so just change the type to be empty.
   else
-    memory_allocator_->SetType(ref, 0);
+    memory_allocator_->ChangeType(ref, 0, kTypeIdHistogram);
 }
 
 void PersistentHistogramAllocator::MergeHistogramToStatisticsRecorder(

base/metrics/persistent_memory_allocator.cc

@@ -84,8 +84,8 @@ const uint32_t PersistentMemoryAllocator::kAllocAlignment = 8;
 struct PersistentMemoryAllocator::BlockHeader {
   uint32_t size;       // Number of bytes in this block, including header.
   uint32_t cookie;     // Constant value indicating completed allocation.
-  uint32_t type_id;    // A number provided by caller indicating data type.
-  std::atomic<uint32_t> next;  // Pointer to the next block when iterating.
+  std::atomic<uint32_t> type_id;  // Arbitrary number indicating data type.
+  std::atomic<uint32_t> next;     // Pointer to the next block when iterating.
 };
 
 // The shared metadata exists once at the top of the memory segment to
@@ -194,7 +194,7 @@ PersistentMemoryAllocator::Iterator::GetNext(uint32_t* type_return) {
     // "strong" compare-exchange is used because failing unnecessarily would
     // mean repeating some fairly costly validations above.
     if (last_record_.compare_exchange_strong(last, next)) {
-      *type_return = block->type_id;
+      *type_return = block->type_id.load(std::memory_order_relaxed);
       break;
     }
   }
@@ -301,7 +301,7 @@ PersistentMemoryAllocator::PersistentMemoryAllocator(
         shared_meta()->queue.next.load(std::memory_order_relaxed) != 0 ||
         first_block->size != 0 ||
         first_block->cookie != 0 ||
-        first_block->type_id != 0 ||
+        first_block->type_id.load(std::memory_order_relaxed) != 0 ||
         first_block->next != 0) {
       // ...or something malicious has been playing with the metadata.
       SetCorrupt();
@@ -428,15 +428,20 @@ uint32_t PersistentMemoryAllocator::GetType(Reference ref) const {
   const volatile BlockHeader* const block = GetBlock(ref, 0, 0, false, false);
   if (!block)
     return 0;
-  return block->type_id;
+  return block->type_id.load(std::memory_order_relaxed);
 }
 
-void PersistentMemoryAllocator::SetType(Reference ref, uint32_t type_id) {
+bool PersistentMemoryAllocator::ChangeType(Reference ref,
+                                           uint32_t to_type_id,
+                                           uint32_t from_type_id) {
   DCHECK(!readonly_);
   volatile BlockHeader* const block = GetBlock(ref, 0, 0, false, false);
   if (!block)
-    return;
-  block->type_id = type_id;
+    return false;
+
+  // This is a "strong" exchange because there is no loop that can retry in
+  // the wake of spurious failures possible with "weak" exchanges.
+  return block->type_id.compare_exchange_strong(from_type_id, to_type_id);
 }
 
 PersistentMemoryAllocator::Reference PersistentMemoryAllocator::Allocate(
@@ -550,7 +555,7 @@ PersistentMemoryAllocator::Reference PersistentMemoryAllocator::AllocateImpl(
     // writing beyond the allocated space and into unallocated space.
     if (block->size != 0 ||
         block->cookie != kBlockCookieFree ||
-        block->type_id != 0 ||
+        block->type_id.load(std::memory_order_relaxed) != 0 ||
         block->next.load(std::memory_order_relaxed) != 0) {
       SetCorrupt();
       return kReferenceNull;
@@ -558,7 +563,7 @@ PersistentMemoryAllocator::Reference PersistentMemoryAllocator::AllocateImpl(
 
     block->size = size;
     block->cookie = kBlockCookieAllocated;
-    block->type_id = type_id;
+    block->type_id.store(type_id, std::memory_order_relaxed);
     return freeptr;
   }
 }
@@ -690,8 +695,10 @@ PersistentMemoryAllocator::GetBlock(Reference ref, uint32_t type_id,
       return nullptr;
     if (ref != kReferenceQueue && block->cookie != kBlockCookieAllocated)
       return nullptr;
-    if (type_id != 0 && block->type_id != type_id)
+    if (type_id != 0 &&
+        block->type_id.load(std::memory_order_relaxed) != type_id) {
       return nullptr;
+    }
   }
 
   // Return pointer to block data.

base/metrics/persistent_memory_allocator.h

@@ -241,9 +241,11 @@ class BASE_EXPORT PersistentMemoryAllocator {
 
   // Access the internal "type" of an object. This generally isn't necessary
   // but can be used to "clear" the type and so effectively mark it as deleted
-  // even though the memory stays valid and allocated.
+  // even though the memory stays valid and allocated. Changing the type is
+  // an atomic compare/exchange and so requires knowing the existing value.
+  // It will return false if the existing type is not what is expected.
   uint32_t GetType(Reference ref) const;
-  void SetType(Reference ref, uint32_t type_id);
+  bool ChangeType(Reference ref, uint32_t to_type_id, uint32_t from_type_id);
 
   // Reserve space in the memory segment of the desired |size| and |type_id|.
   // A return value of zero indicates the allocation failed, otherwise the

base/metrics/persistent_memory_allocator_unittest.cc

@@ -181,9 +181,9 @@ TEST_F(PersistentMemoryAllocatorTest, AllocateAndIterate) {
 
   // Check that an objcet's type can be changed.
   EXPECT_EQ(2U, allocator_->GetType(block2));
-  allocator_->SetType(block2, 3);
+  allocator_->ChangeType(block2, 3, 2);
   EXPECT_EQ(3U, allocator_->GetType(block2));
-  allocator_->SetType(block2, 2);
+  allocator_->ChangeType(block2, 2, 3);
   EXPECT_EQ(2U, allocator_->GetType(block2));
 
   // Create second allocator (read/write) using the same memory segment.
@@ -511,7 +511,7 @@ TEST(SharedPersistentMemoryAllocatorTest, CreationTest) {
     r456 = local.Allocate(456, 456);
     r789 = local.Allocate(789, 789);
     local.MakeIterable(r123);
-    local.SetType(r456, 654);
+    local.ChangeType(r456, 654, 456);
     local.MakeIterable(r789);
     local.GetMemoryInfo(&meminfo1);
     EXPECT_FALSE(local.IsFull());
@@ -599,7 +599,7 @@ TEST(FilePersistentMemoryAllocatorTest, CreationTest) {
     r456 = local.Allocate(456, 456);
     r789 = local.Allocate(789, 789);
     local.MakeIterable(r123);
-    local.SetType(r456, 654);
+    local.ChangeType(r456, 654, 456);
     local.MakeIterable(r789);
     local.GetMemoryInfo(&meminfo1);
     EXPECT_FALSE(local.IsFull());