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

base/metrics/persistent_memory_allocator_unittest.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // 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"
@@ skipping 427 matching lines @@
   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);
 }
 
+#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);
 
   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)) {
-      // 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
       // filesize < minsize.
       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));
     {
       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());  // Garbage data so it should be.
     } else {
       // For filesize >= minsize, the file must be acceptable. This
       // else clause (file-not-acceptable) should be reached only if
       // filesize < minsize.
       EXPECT_GT(minsize, filesize);
     }
-#endif
   }
 }
+#endif  // !DCHECK_IS_ON()
 
 }  // namespace base