Create "persistent memory allocator" for persisting and sharing objects.

base/metrics/persistent_memory_allocator_unittest.cc

Index: base/metrics/persistent_memory_allocator_unittest.cc
diff --git a/base/metrics/persistent_memory_allocator_unittest.cc b/base/metrics/persistent_memory_allocator_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..4476fe2bd3bc5f3a36487b1aea7a44abfe572023
--- /dev/null
+++ b/base/metrics/persistent_memory_allocator_unittest.cc
@@ -0,0 +1,456 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.

[Alexander Potapenko, 2016/01/14 10:54:16]

Dunno if this should be 2016 or it's fine to keep the year in which you started
writing this code.

[bcwhite, 2016/01/19 19:49:39]

Acknowledged.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/metrics/persistent_memory_allocator.h"
+
+#include "base/files/file.h"
+#include "base/files/file_util.h"
+#include "base/files/memory_mapped_file.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/metrics/histogram.h"
+#include "base/rand_util.h"
+#include "base/strings/safe_sprintf.h"
+#include "base/threading/simple_thread.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace {
+
+const uint32_t TEST_MEMORY_SIZE = 1 << 20;   // 1 MiB
+const uint32_t TEST_MEMORY_PAGE = 64 << 10;  // 64 KiB
+const uint32_t TEST_ID = 12345;
+const char TEST_NAME[] = "TestAllocator";
+
+}  // namespace
+
+namespace base {
+
+typedef PersistentMemoryAllocator::Reference Reference;
+
+class PersistentMemoryAllocatorTest : public testing::Test {
+ public:
+  struct TestObject1 {
+    int32_t onething;

[Alexander Potapenko, 2016/01/14 10:54:17]

Not that it makes any difference, but can this be a plain int?

[bcwhite, 2016/01/19 19:49:39]

Done.

+    char oranother;
+  };
+
+  struct TestObject2 {
+    int thiis;
+    long that;
+    float andthe;
+    char other;
+    double thing;
+  };
+
+  PersistentMemoryAllocatorTest() {
+    mem_segment_.reset(new char[TEST_MEMORY_SIZE]);
+  }
+
+  void SetUp() override {
+    allocator_.reset();
+    memset(mem_segment_.get(), 0, TEST_MEMORY_SIZE);
+    allocator_.reset(new PersistentMemoryAllocator(
+        mem_segment_.get(), TEST_MEMORY_SIZE, TEST_MEMORY_PAGE,
+        TEST_ID, TEST_NAME, false));
+    allocator_->CreateHistograms(allocator_->Name());
+  }
+
+  void TearDown() override {
+    allocator_.reset();
+  }
+
+  unsigned CountIterables() {
+    PersistentMemoryAllocator::Iterator iter;
+    uint32_t type;
+    unsigned count = 0;
+    for (allocator_->CreateIterator(&iter);
+         allocator_->GetNextIterable(&iter, &type) != 0;) {
+      count++;
+    }
+    return count;
+  }
+
+  scoped_ptr<char[]> mem_segment_;

[Alexander Potapenko, 2016/01/13 18:34:00]

Should be protected.

[bcwhite, 2016/01/13 21:31:39]

Done.

+  scoped_ptr<PersistentMemoryAllocator> allocator_;
+};
+
+TEST_F(PersistentMemoryAllocatorTest, AllocateAndIterate) {
+  std::string base_name(TEST_NAME);

[Alexander Potapenko, 2016/01/13 18:34:00]

Looks like this test case is testing several scenarios - can you please add
comments before each?

[bcwhite, 2016/01/13 21:31:39]

Done.

+  EXPECT_EQ(TEST_ID, allocator_->Id());
+  EXPECT_TRUE(allocator_->used_histogram_);
+  EXPECT_EQ(base_name + ".UsedKiB",
+            allocator_->used_histogram_->histogram_name());
+  EXPECT_TRUE(allocator_->allocs_histogram_);
+  EXPECT_EQ(base_name + ".Allocs",
+            allocator_->allocs_histogram_->histogram_name());
+
+  PersistentMemoryAllocator::MemoryInfo meminfo0;
+  allocator_->GetMemoryInfo(&meminfo0);
+  EXPECT_EQ(TEST_MEMORY_SIZE, meminfo0.total);
+  EXPECT_GT(meminfo0.total, meminfo0.free);
+
+  Reference block1 = allocator_->Allocate(sizeof(TestObject1), 1);
+  EXPECT_NE(0U, block1);
+  EXPECT_NE(nullptr, allocator_->GetAsObject<TestObject1>(block1, 1));
+  EXPECT_EQ(nullptr, allocator_->GetAsObject<TestObject2>(block1, 1));
+  EXPECT_LE(sizeof(TestObject1), allocator_->GetAllocSize(block1));
+  EXPECT_GE(sizeof(TestObject1) + 7, allocator_->GetAllocSize(block1));

[Alexander Potapenko, 2016/01/14 10:54:16]

Where does this '7' comes from (here and below)?

[bcwhite, 2016/01/19 19:49:39]

Done.  Defined as constant.

+  PersistentMemoryAllocator::MemoryInfo meminfo1;
+  allocator_->GetMemoryInfo(&meminfo1);
+  EXPECT_EQ(meminfo0.total, meminfo1.total);
+  EXPECT_GT(meminfo0.free, meminfo1.free);
+
+  PersistentMemoryAllocator::Iterator iter;
+  uint32_t type;
+  allocator_->CreateIterator(&iter);
+  EXPECT_EQ(0U, allocator_->GetNextIterable(&iter, &type));
+  allocator_->MakeIterable(block1);
+  EXPECT_EQ(block1, allocator_->GetNextIterable(&iter, &type));
+  EXPECT_EQ(1U, type);
+  EXPECT_EQ(0U, allocator_->GetNextIterable(&iter, &type));
+
+  Reference block2 = allocator_->Allocate(sizeof(TestObject2), 2);
+  EXPECT_NE(0U, block2);
+  EXPECT_NE(nullptr, allocator_->GetAsObject<TestObject2>(block2, 2));
+  EXPECT_EQ(nullptr, allocator_->GetAsObject<TestObject2>(block2, 1));
+  EXPECT_LE(sizeof(TestObject2), allocator_->GetAllocSize(block2));
+  EXPECT_GE(sizeof(TestObject2) + 7, allocator_->GetAllocSize(block2));
+  PersistentMemoryAllocator::MemoryInfo meminfo2;
+  allocator_->GetMemoryInfo(&meminfo2);
+  EXPECT_EQ(meminfo1.total, meminfo2.total);
+  EXPECT_GT(meminfo1.free, meminfo2.free);
+
+  allocator_->MakeIterable(block2);
+  EXPECT_EQ(block2, allocator_->GetNextIterable(&iter, &type));
+  EXPECT_EQ(2U, type);
+  EXPECT_EQ(0U, allocator_->GetNextIterable(&iter, &type));
+
+  allocator_->CreateIterator(&iter, block1);
+  EXPECT_EQ(block2, allocator_->GetNextIterable(&iter, &type));
+  EXPECT_EQ(0U, allocator_->GetNextIterable(&iter, &type));
+
+  EXPECT_FALSE(allocator_->IsFull());
+  EXPECT_FALSE(allocator_->IsCorrupt());
+
+  allocator_->UpdateStaticHistograms();
+  scoped_ptr<HistogramSamples> used_samples(
+      allocator_->used_histogram_->SnapshotSamples());
+  EXPECT_TRUE(used_samples);
+  EXPECT_EQ(1, used_samples->TotalCount());
+
+  scoped_ptr<HistogramSamples> allocs_samples(
+      allocator_->allocs_histogram_->SnapshotSamples());
+  EXPECT_TRUE(allocs_samples);
+  EXPECT_EQ(2, allocs_samples->TotalCount());
+  EXPECT_EQ(0, allocs_samples->GetCount(0));
+  EXPECT_EQ(1, allocs_samples->GetCount(sizeof(TestObject1)));
+  EXPECT_EQ(1, allocs_samples->GetCount(sizeof(TestObject2)));
+#if !DCHECK_IS_ON()  // DCHECK builds will die at a NOTREACHED().
+  EXPECT_EQ(0U, allocator_->Allocate(TEST_MEMORY_SIZE + 1, 0));
+  allocs_samples = allocator_->allocs_histogram_->SnapshotSamples();
+  EXPECT_EQ(3, allocs_samples->TotalCount());
+  EXPECT_EQ(1, allocs_samples->GetCount(0));
+#endif
+
+  EXPECT_EQ(2U, allocator_->GetType(block2));
+  allocator_->SetType(block2, 3);
+  EXPECT_EQ(3U, allocator_->GetType(block2));
+  allocator_->SetType(block2, 2);
+  EXPECT_EQ(2U, allocator_->GetType(block2));
+
+  scoped_ptr<PersistentMemoryAllocator> allocator2(
+      new PersistentMemoryAllocator(
+          mem_segment_.get(), TEST_MEMORY_SIZE, TEST_MEMORY_PAGE, 0, "",
+          false));
+  EXPECT_EQ(TEST_ID, allocator2->Id());
+  EXPECT_FALSE(allocator2->used_histogram_);
+  EXPECT_FALSE(allocator2->allocs_histogram_);
+  EXPECT_NE(allocator2->allocs_histogram_, allocator_->allocs_histogram_);
+
+  allocator2->CreateIterator(&iter);
+  EXPECT_EQ(block1, allocator2->GetNextIterable(&iter, &type));
+  EXPECT_EQ(block2, allocator2->GetNextIterable(&iter, &type));
+  EXPECT_EQ(0U, allocator2->GetNextIterable(&iter, &type));
+  EXPECT_NE(nullptr, allocator2->GetAsObject<TestObject1>(block1, 1));
+  EXPECT_NE(nullptr, allocator2->GetAsObject<TestObject2>(block2, 2));
+
+  scoped_ptr<const PersistentMemoryAllocator> allocator3(
+      new PersistentMemoryAllocator(
+          mem_segment_.get(), TEST_MEMORY_SIZE, TEST_MEMORY_PAGE, 0, "", true));
+  EXPECT_EQ(TEST_ID, allocator3->Id());
+  EXPECT_FALSE(allocator3->used_histogram_);
+  EXPECT_FALSE(allocator3->allocs_histogram_);
+
+  allocator3->CreateIterator(&iter);
+  EXPECT_EQ(block1, allocator3->GetNextIterable(&iter, &type));
+  EXPECT_EQ(block2, allocator3->GetNextIterable(&iter, &type));
+  EXPECT_EQ(0U, allocator3->GetNextIterable(&iter, &type));
+  EXPECT_NE(nullptr, allocator3->GetAsObject<TestObject1>(block1, 1));
+  EXPECT_NE(nullptr, allocator3->GetAsObject<TestObject2>(block2, 2));
+}
+
+TEST_F(PersistentMemoryAllocatorTest, PageTest) {
+  Reference block1 = allocator_->Allocate(TEST_MEMORY_PAGE / 2, 1);
+  EXPECT_LT(0U, block1);
+  EXPECT_GT(TEST_MEMORY_PAGE, block1);
+
+  Reference block2 = allocator_->Allocate(TEST_MEMORY_PAGE - 16, 2);

[Alexander Potapenko, 2016/01/14 10:54:16]

What are we testing here? Where does '16' come from? In which case will this
constant change?

[bcwhite, 2016/01/19 19:49:39]

Done.

+  EXPECT_EQ(TEST_MEMORY_PAGE, block2);
+
+  Reference block3 = allocator_->Allocate(99, 3);
+  EXPECT_EQ(2U * TEST_MEMORY_PAGE, block3);
+}
+
+class AllocatorThread : public SimpleThread {
+ public:
+  AllocatorThread(const std::string& name,
+                  void* base,
+                  uint32_t size,
+                  uint32_t page_size)
+      : SimpleThread(name, Options()),
+        count_(0),
+        iterable_(0),
+        allocator_(base, size, page_size, 0, std::string(), false) {}
+
+  void Run() override {
+    for (;;) {
+      uint32_t size = RandInt(1, 99);
+      uint32_t type = RandInt(100, 999);
+      Reference block = allocator_.Allocate(size, type);
+      if (!block)
+        break;
+
+      count_++;
+      if (RandInt(0, 1)) {
+        allocator_.MakeIterable(block);
+        iterable_++;
+      }
+    }
+  }
+
+  unsigned count_;

[Alexander Potapenko, 2016/01/14 10:54:17]

I think it's better to make count_ and iterable_ private and declare
PersistentMemoryAllocatorTest a friend class.

[bcwhite, 2016/01/19 19:49:39]

That would have to be:

FRIEND_TEST_ALL_PREFIXES(PersistentMemoryAllocatorTest, CorruptionTest);
FRIEND_TEST_ALL_PREFIXES(PersistentMemoryAllocatorTest, MaliciousTest);

Added accessor method instead.

+  unsigned iterable_;
+
+ private:
+  PersistentMemoryAllocator allocator_;
+};
+
+TEST_F(PersistentMemoryAllocatorTest, ParallelismTest) {

[Alexander Potapenko, 2016/01/14 10:54:16]

Please add a commend describing what you're testing here.

[bcwhite, 2016/01/19 19:49:39]

Done.

+  void* memory = mem_segment_.get();
+  AllocatorThread t1("t1", memory, TEST_MEMORY_SIZE, TEST_MEMORY_PAGE);

[Alexander Potapenko, 2016/01/13 18:34:01]

How about making t1..t5 an array and employing loops here and below?

[bcwhite, 2016/01/13 21:31:39]

It was more complicated to do so because of the lack of ability to provide
constructor parameters to objects inside an array.  They could be dynamically
allocated but that means scoped_ptrs, etc.  This was easier but I can change it
if you'd like.

[Alexander Potapenko, 2016/01/14 10:54:17]

I just thought this could let you shorten ParallelismTest and CorruptionTest. I
don't insist if there're only two such tests, but in the case you're planning to
add more, it's better to change that code.

[bcwhite, 2016/01/19 19:49:39]

Acknowledged.

+  AllocatorThread t2("t2", memory, TEST_MEMORY_SIZE, TEST_MEMORY_PAGE);
+  AllocatorThread t3("t3", memory, TEST_MEMORY_SIZE, TEST_MEMORY_PAGE);
+  AllocatorThread t4("t4", memory, TEST_MEMORY_SIZE, TEST_MEMORY_PAGE);
+  AllocatorThread t5("t5", memory, TEST_MEMORY_SIZE, TEST_MEMORY_PAGE);
+
+  t1.Start();
+  t2.Start();
+  t3.Start();
+  t4.Start();
+  t5.Start();
+
+  unsigned last_count = 0;
+  do {
+    unsigned count = CountIterables();
+    EXPECT_LE(last_count, count);
+  } while (!allocator_->IsCorrupt() && !allocator_->IsFull());
+
+  t1.Join();
+  t2.Join();
+  t3.Join();
+  t4.Join();
+  t5.Join();
+
+  EXPECT_FALSE(allocator_->IsCorrupt());
+  EXPECT_EQ(CountIterables(),
+            t1.iterable_ + t2.iterable_ + t3.iterable_ + t4.iterable_ +
+            t5.iterable_);
+}
+
+// This test doesn't verify anything other than it doesn't crash.
+TEST_F(PersistentMemoryAllocatorTest, CorruptionTest) {
+  char* memory = mem_segment_.get();
+  AllocatorThread t1("t1", memory, TEST_MEMORY_SIZE, TEST_MEMORY_PAGE);
+  AllocatorThread t2("t2", memory, TEST_MEMORY_SIZE, TEST_MEMORY_PAGE);
+  AllocatorThread t3("t3", memory, TEST_MEMORY_SIZE, TEST_MEMORY_PAGE);
+  AllocatorThread t4("t4", memory, TEST_MEMORY_SIZE, TEST_MEMORY_PAGE);
+  AllocatorThread t5("t5", memory, TEST_MEMORY_SIZE, TEST_MEMORY_PAGE);
+
+  t1.Start();
+  t2.Start();
+  t3.Start();
+  t4.Start();
+  t5.Start();
+
+  do {
+    size_t offset = RandInt(0, TEST_MEMORY_SIZE - 1);
+    char value = RandInt(0, 255);
+    memory[offset] = value;

[Alexander Potapenko, 2016/01/13 18:34:00]

I'm concerned this may lead to test flakiness in the case of an actual crash.

[bcwhite, 2016/01/13 21:31:39]

I'm not sure what you mean by "in the case of an actual crash".  It's supposed
to crash; or rather, it's supposed to *fail* to crash.  Any failure of this
particular test means there's a bug in the code somewhere.

[Alexander Potapenko, 2016/01/14 10:54:16]

My point is that in the case there's a bug in the code somewhere it won't be
detected every time because you write random bytes to the memory, so the test
becomes flaky.

[bcwhite, 2016/01/19 19:49:39]

I know but at least there is indication that something is wrong somewhere which
can then be investigated further.  A stack dump, if present, would also narrow
the search quite significantly.

This test allowed me to find a few bugs when it was first written (and run in a
loop for a while).  I believe it's important to leave present, more as
protection for future changes than in the expectation it will find additional
issues with the current code.

+  } while (!allocator_->IsCorrupt() && !allocator_->IsFull());
+
+  t1.Join();
+  t2.Join();
+  t3.Join();
+  t4.Join();
+  t5.Join();
+
+  CountIterables();
+}
+
+// Attempt to cause crashes or loops by expressly creating dangerous coditions.

[Alexander Potapenko, 2016/01/13 18:34:01]

s/coditions/conditions

[bcwhite, 2016/01/13 21:31:39]

Done.

+TEST_F(PersistentMemoryAllocatorTest, MaliciousTest) {
+  Reference block1 = allocator_->Allocate(sizeof(TestObject1), 1);
+  Reference block2 = allocator_->Allocate(sizeof(TestObject1), 2);
+  Reference block3 = allocator_->Allocate(sizeof(TestObject1), 3);
+  Reference block4 = allocator_->Allocate(sizeof(TestObject1), 3);
+  Reference block5 = allocator_->Allocate(sizeof(TestObject1), 3);
+  allocator_->MakeIterable(block1);
+  allocator_->MakeIterable(block2);
+  allocator_->MakeIterable(block3);
+  allocator_->MakeIterable(block4);
+  allocator_->MakeIterable(block5);
+  EXPECT_EQ(5U, CountIterables());

[Alexander Potapenko, 2016/01/13 18:34:01]

s/5U/5

(here and at other places, it doesn't really improve readability)

[bcwhite, 2016/01/13 21:31:39]

The "U" is required or there are signed/unsigned comparison errors thrown by the
Clang compiler.

[Alexander Potapenko, 2016/01/14 10:54:16]

This is quite strange.
`clang -Wsign-compare` does not complain about a comparison between an unsigned
int returned by a function and a positive integer literal (and it sure should
not).
Are the errors reported because of the EXPECT_EQ macro?

[bcwhite, 2016/01/19 19:49:39]

The errors were always in the EXPECT_ macros.  The Windows build didn't have a
problem with it but various trybots did.

+  EXPECT_FALSE(allocator_->IsCorrupt());
+
+  // Create loop in iterable list and ensure it doesn't hang.
+  uint32_t* header4 = (uint32_t*)(mem_segment_.get() + block4);
+  EXPECT_EQ(block5, header4[3]);
+  header4[3] = block3;
+  CountIterables();  // loop: 1-2-3-4-3

[Alexander Potapenko, 2016/01/13 18:34:01]

Isn't the allocator corrupted already after this line?

[bcwhite, 2016/01/13 21:31:39]

Corruption doesn't stop or prevent iteration.  It's still allowed to try to
retrieve as much information as possible.  So while yes, the flag is set, it
still has to detect the loop.

[Alexander Potapenko, 2016/01/14 10:54:16]

If the flag is already set after the first CountIterables() call, let us perform
the 'EXPECT_TRUE(allocator_->IsCorrupt())' check right after that call.
Otherwise it's hard to tell whether the allocator had been corrupted by the
first assignment or the later ones.

This brings up the questions whether you need the second and third loop checks
(probably yes) and whether you want to setup the blocks before each check (I'd
say yes).

Also, you don't check the return values of CountIterables() calls - what are
your expectations in the case there's a loop?

[bcwhite, 2016/01/19 19:49:39]

Done.
The other loops trials test other edge cases; I'll add some comments.

Iteration isn't allowed to change any state in the persistent memory (because it
could be a read-only segment) so there shouldn't be any behavior differences
when not recreating the entire structure.  I could change it to a parameterized
test, with the loop-to being the parameter, if you'd prefer.
It's undefined.  In the case of a loop, iterable items may get returned multiple
times so there's no way of knowing just what the count will be.  The only
requirement is that it stops.

+  header4[3] = block2;
+  CountIterables();  // loop: 1-2-3-4-2
+  header4[3] = block1;
+  CountIterables();  // loop: 1-2-3-4-1
+  EXPECT_TRUE(allocator_->IsCorrupt());
+}
+
+
+//----- LocalPersistentMemoryAllocator -----------------------------------------
+
+TEST(LocalPersistentMemoryAllocatorTest, CreationTest) {
+  LocalPersistentMemoryAllocator allocator(TEST_MEMORY_SIZE, 42, "");
+  EXPECT_EQ(42U, allocator.Id());
+  EXPECT_NE(0U, allocator.Allocate(24, 1));
+  EXPECT_FALSE(allocator.IsFull());

[Alexander Potapenko, 2016/01/14 10:54:17]

We need a positive test for IsFull() somewhere.

[bcwhite, 2016/01/19 19:49:39]

There's an implicit one in the Parallelism test.  I'll make it explicit.

+  EXPECT_FALSE(allocator.IsCorrupt());
+}
+
+
+//----- FilePersistentMemoryAllocator ------------------------------------------
+
+TEST(FilePersistentMemoryAllocatorTest, CreationTest) {
+  ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  FilePath file_path = temp_dir.path().AppendASCII("persistent_memory");
+
+  PersistentMemoryAllocator::MemoryInfo meminfo1;
+  Reference r123, r456, r789;
+  {
+    LocalPersistentMemoryAllocator local(TEST_MEMORY_SIZE, TEST_ID, "");
+    EXPECT_FALSE(local.IsReadonly());
+    r123 = local.Allocate(123, 123);
+    r456 = local.Allocate(456, 456);
+    r789 = local.Allocate(789, 789);
+    local.MakeIterable(r123);
+    local.SetType(r456, 654);
+    local.MakeIterable(r789);
+    local.GetMemoryInfo(&meminfo1);
+    EXPECT_FALSE(local.IsFull());
+    EXPECT_FALSE(local.IsCorrupt());
+
+    File writer(file_path, File::FLAG_CREATE | File::FLAG_WRITE);
+    ASSERT_TRUE(writer.IsValid());
+    writer.Write(0, (const char*)local.data(), local.used());
+  }
+
+  scoped_ptr<MemoryMappedFile> mmfile(new MemoryMappedFile());
+  mmfile->Initialize(file_path);
+  EXPECT_TRUE(mmfile->IsValid());
+  const size_t mmlength = mmfile->length();
+  EXPECT_GE(meminfo1.total, mmlength);
+
+  FilePersistentMemoryAllocator file(mmfile.release(), 0, "");
+  EXPECT_TRUE(file.IsReadonly());
+  EXPECT_EQ(TEST_ID, file.Id());
+  EXPECT_FALSE(file.IsFull());
+  EXPECT_FALSE(file.IsCorrupt());
+
+  PersistentMemoryAllocator::Iterator iter;
+  uint32_t type;
+  file.CreateIterator(&iter);
+  EXPECT_EQ(r123, file.GetNextIterable(&iter, &type));
+  EXPECT_EQ(r789, file.GetNextIterable(&iter, &type));
+  EXPECT_EQ(0U, file.GetNextIterable(&iter, &type));
+
+  EXPECT_EQ(123U, file.GetType(r123));
+  EXPECT_EQ(654U, file.GetType(r456));
+  EXPECT_EQ(789U, file.GetType(r789));
+
+  PersistentMemoryAllocator::MemoryInfo meminfo2;
+  file.GetMemoryInfo(&meminfo2);
+  EXPECT_GE(meminfo1.total, meminfo2.total);
+  EXPECT_GE(meminfo1.free, meminfo2.free);
+  EXPECT_EQ(mmlength, meminfo2.total);
+  EXPECT_EQ(0U, meminfo2.free);
+}
+
+TEST(FilePersistentMemoryAllocatorTest, AcceptableTest) {
+  ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  FilePath file_path_base = temp_dir.path().AppendASCII("persistent_memory_");
+
+  LocalPersistentMemoryAllocator local(TEST_MEMORY_SIZE, TEST_ID, "");
+  const size_t minsize = local.used();
+  scoped_ptr<char[]> garbage(new char[minsize]);
+  RandBytes(garbage.get(), minsize);
+
+  scoped_ptr<MemoryMappedFile> mmfile;
+  char filename[100];
+  for (size_t filesize = minsize; filesize > 0; --filesize) {
+    strings::SafeSPrintf(filename, "memory_%d_A", filesize);
+    FilePath file_path = temp_dir.path().AppendASCII(filename);
+    ASSERT_FALSE(PathExists(file_path));
+    {
+      File writer(file_path, File::FLAG_CREATE | File::FLAG_WRITE);
+      ASSERT_TRUE(writer.IsValid());
+      writer.Write(0, (const char*)local.data(), filesize);
+    }
+    ASSERT_TRUE(PathExists(file_path));
+
+    mmfile.reset(new MemoryMappedFile());
+    mmfile->Initialize(file_path);
+    EXPECT_EQ(filesize, mmfile->length());
+    if (FilePersistentMemoryAllocator::IsFileAcceptable(*mmfile)) {

[Alexander Potapenko, 2016/01/13 18:34:00]

Can you rephrase this as:
  if (minsize==filesize) {
    ASSERT_TRUE(FilePersistentMemoryAllocator::IsFileAcceptable(*mmfile));
  }

[bcwhite, 2016/01/13 21:31:39]

I need the function to run for all sizes.  It must return True for
minsize==filesize.  For all other cases, either result is allowed.  The purpose
of running it is to ensure it doesn't crash as data becomes invalid.

+      // Just need to make sure it doesn't crash.
+      FilePersistentMemoryAllocator allocator(mmfile.release(), 0, "");

[Alexander Potapenko, 2016/01/13 18:34:00]

This variable is unused, the compiler may warn about it. Do we need it?

[bcwhite, 2016/01/13 21:31:39]

The constructor needs to be run to ensure it can't crash on any data that
IsFileAcceptable says is okay.

Good point about "unused", though.  Would be bad in the compiler optimized out
the call.

[Alexander Potapenko, 2016/01/14 10:54:16]

You can cast the variable to void to prevent that:
  FilePersistentMemoryAllocator allocator(mmfile.release(), 0, "");
  (void)allocator;

[bcwhite, 2016/01/19 19:49:39]

I wasn't aware of that trick.  Done.

+    } else {
+      EXPECT_GT(minsize, filesize);  // Must be acceptable if minsize==filesize.
+    }
+
+#if !DCHECK_IS_ON()  // DCHECK builds will die at a NOTREACHED().
+    strings::SafeSPrintf(filename, "memory_%d_B", filesize);
+    file_path = temp_dir.path().AppendASCII(filename);
+    ASSERT_FALSE(PathExists(file_path));
+    {
+      File writer(file_path, File::FLAG_CREATE | File::FLAG_WRITE);
+      ASSERT_TRUE(writer.IsValid());
+      writer.Write(0, (const char*)garbage.get(), filesize);
+    }
+    ASSERT_TRUE(PathExists(file_path));
+
+    mmfile.reset(new MemoryMappedFile());
+    mmfile->Initialize(file_path);
+    EXPECT_EQ(filesize, mmfile->length());
+    if (FilePersistentMemoryAllocator::IsFileAcceptable(*mmfile)) {
+      // Just need to make sure it doesn't crash.
+      FilePersistentMemoryAllocator allocator(mmfile.release(), 0, "") ;
+      EXPECT_TRUE(allocator.IsCorrupt());  // Gargbage data so it should be.

[Alexander Potapenko, 2016/01/13 18:34:00]

s/Gargbage/Garbage

[bcwhite, 2016/01/13 21:31:39]

Done.

+    } else {
+      EXPECT_GT(minsize, filesize);  // Must be acceptable if minsize==filesize.
+    }
+#endif
+  }
+}
+
+}  // namespace base