New test and off-by-one fix for data persisted to disk.

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
index 8b98ee4cd44406d8fd80a3ca9e9e4a418250bb05..572eada6ec3eb230795cc0706e6a16c581263c12 100644
--- a/base/metrics/persistent_memory_allocator_unittest.cc
+++ b/base/metrics/persistent_memory_allocator_unittest.cc
@@ -445,12 +445,15 @@ TEST(FilePersistentMemoryAllocatorTest, CreationTest) {
   EXPECT_EQ(0U, meminfo2.free);
 }
 
+#if !DCHECK_IS_ON()  // DCHECK builds die at a various debug checks.

[Alexei Svitkine (slow), 2016/02/08 21:43:26]

Aren't most of our tests run with DCHECKs? So this is disabling testing for
majority of our infrastructure.

Which DCHECKs does this hit? Can the code be changed to avoid hitting those
DCHECKs instead of not running this code if DCHECKs are on?

[bcwhite, 2016/02/10 16:28:58]

I think all the bot-tests run off a release build.

The checks being hit are the "hardening" code that protects against malicious
actors.  This test ends up producing invalid memory segments which hit those
NOTREACHED() statements.  This is intentional, but we need to verify that it
does the right thing by handling the situation whereas in a real build, we want
to be notified immediately that something happened that shouldn't in a
well-behaved system.

[Alexei Svitkine (slow), 2016/02/10 16:35:34]

I believe the bots run Release with DCHECK on.

I would prefer a solution that lets us run these tests with DCHECKs enabled.
Perhaps the NOTREACHED() can be done by the caller when it gets a return value
that indicates a failure? Or the test can mock out a function that otherwise
would do a NOTREACHED()?

[bcwhite, 2016/02/10 16:54:09]

Complicated.  Result to the caller is always "good".  Production code should
never have to call "IsCorrupt()" or check the return values any more that usual.

I could create a "IgnoreDChecksForTesting()" that sets a flag and then change
NOTREACHED() to DCHECK(testing).  Sound reasonable?

 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, "");
+  local.Allocate(1, 1);
+  local.Allocate(11, 11);
   const size_t minsize = local.used();
   scoped_ptr<char[]> garbage(new char[minsize]);
   RandBytes(garbage.get(), minsize);
@@ -472,9 +475,25 @@ TEST(FilePersistentMemoryAllocatorTest, AcceptableTest) {
     mmfile->Initialize(file_path);
     EXPECT_EQ(filesize, mmfile->length());
     if (FilePersistentMemoryAllocator::IsFileAcceptable(*mmfile)) {
-      // Just need to make sure it doesn't crash.
+      // Make sure construction doesn't crash.
       FilePersistentMemoryAllocator allocator(mmfile.release(), 0, "");
-      (void)allocator;  // Ensure compiler can't optimize-out above variable.
+      // Also make sure that iteration doesn't crash.
+      PersistentMemoryAllocator::Iterator iter;
+      allocator.CreateIterator(&iter);
+      for (;;) {
+        Reference ref = allocator.GetNextIterable(&iter, 0);
+        if (!ref)
+          break;
+        const char* data = allocator.GetAsObject<char>(ref, 0);
+        uint32_t type = allocator.GetType(ref);
+        size_t size = allocator.GetAllocSize(ref);
+        // Ensure compiler can't optimize-out above variables.
+        (void)data;
+        (void)type;
+        (void)size;
+        // Ensure that corruption-detected flag gets properly set.
+        EXPECT_EQ(filesize != minsize, allocator.IsCorrupt());
+      }
     } else {
       // For filesize >= minsize, the file must be acceptable. This
       // else clause (file-not-acceptable) should be reached only if
@@ -482,7 +501,6 @@ TEST(FilePersistentMemoryAllocatorTest, AcceptableTest) {
       EXPECT_LT(filesize, minsize);
     }
 
-#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));
@@ -506,8 +524,8 @@ TEST(FilePersistentMemoryAllocatorTest, AcceptableTest) {
       // filesize < minsize.
       EXPECT_GT(minsize, filesize);
     }
-#endif
   }
 }
+#endif  // !DCHECK_IS_ON()
 
 }  // namespace base