Fix partition_alloc unit tests.

base/allocator/partition_allocator/partition_alloc_unittest.cc

Index: base/allocator/partition_allocator/partition_alloc_unittest.cc
diff --git a/base/allocator/partition_allocator/partition_alloc_unittest.cc b/base/allocator/partition_allocator/partition_alloc_unittest.cc
index 2ddde105bb4aee707e97750f403c1a017ce5b2c1..1ed26bba972de191589fb5fee45fbeebc599aaec 100644
--- a/base/allocator/partition_allocator/partition_alloc_unittest.cc
+++ b/base/allocator/partition_allocator/partition_alloc_unittest.cc
@@ -56,6 +56,11 @@ const size_t kTestBucketIndex = kRealAllocSize >> kBucketShift;
 const char* type_name = nullptr;
 
 void TestSetup() {
+  // Zero the allocator structs to clear out traces
+  // from previous test.
+  memset(&allocator, 0, sizeof(allocator));
+  memset(&generic_allocator, 0, sizeof(generic_allocator));

[Wez, 2017/02/12 18:51:28]

This looks highly suspect; why trash the instance's fields, rather than creating
a new instance per-test?

[sof, 2017/02/12 20:07:04]

The tests use a set of global allocators, which each test initialize by calling
TestSetup() before referring to them and tidying up. PA doesn't provide a way to
uninitialize their PartitionRoots (or the "allocator" toplevel container structs
themselves), so resetting them by way memset() is the effective means to bring
that about.

Could you rework these unit tests to define their own testing::Test which
packaged up these allocators better per-test? Absolutely. Is it needed to
address this instability? Don't think so.

[Wez, 2017/02/14 00:24:11]

Presumably we're assuming that the tests themselves don't leak, so we should at
least be able to assert that the allocators have no active allocations at test
teardown.
I disagree; we're basically memset()ing over the top of the two structures on
the assumption that they only contain POD structures, without clarifying why
that is assumed to be safe, or why "traces" from prior tests would break things
(though yes, the tests should be hermetic no matter what).

[Wez, 2017/02/14 00:42:50]

OK, looking through the PartitionRoot init code, it seems that nothing is
assuming that the data structure was zero'd, so the fact that things fail if
they're not seems like a legitimate bug.

+
   allocator.init();
   generic_allocator.init();
 }
@@ -1286,35 +1291,43 @@ static void DoReturnNullTest(size_t allocSize) {
   EXPECT_TRUE(ClearAddressSpaceLimit());
 }
 
-// Tests that if an allocation fails in "return null" mode, repeating it doesn't
-// crash, and still returns null. The test tries to allocate 6 GB of memory in
-// 512 kB blocks. On 64-bit POSIX systems, the address space is limited to 6 GB
-// using setrlimit() first.
+// Unit tests that check if an allocation fails in "return null" mode,
+// repeating it doesn't crash, and still returns null. The tests need to
+// stress memory subsystem limits to do so, hence they try to allocate
+// 6 GB of memory, each with a different per-allocation block sizes.
 //
+// On 64-bit POSIX systems, the address space is limited to 6 GB using
+// setrlimit() first.
+
+// Test "return null" for larger, direct-mapped allocations first. As a

[Wez, 2017/02/12 18:51:28]

Tests normally get sharded across all cores, so their ordering is only
approximately followed - is there something causing these tests to be treated
specially?

+// direct-mapped allocation's pages are unmapped and freed on release, this
+// test is performd first for these "return null" tests in order to leave

[Wez, 2017/02/12 18:51:28]

typo: performed

+// sufficient unreserved virtual memory around for the later one(s).
+
 // Disable this test on Android because, due to its allocation-heavy behavior,

[Wez, 2017/02/12 18:51:28]

Looks like it is also disabled on Mac - same reason?

[sof, 2017/02/12 19:52:37]

I believe so; this was just reordering pre-existing comments (as the 2 tests had
to be for reasons given above.)

 // it tends to get OOM-killed rather than pass.
 #if defined(OS_MACOSX) || defined(OS_ANDROID)
-#define MAYBE_RepeatedReturnNull DISABLED_RepeatedReturnNull
+#define MAYBE_RepeatedReturnNullDirect DISABLED_RepeatedReturnNullDirect
 #else
-#define MAYBE_RepeatedReturnNull RepeatedReturnNull
+#define MAYBE_RepeatedReturnNullDirect RepeatedReturnNullDirect
 #endif
-TEST(PartitionAllocTest, MAYBE_RepeatedReturnNull) {
-  // A single-slot but non-direct-mapped allocation size.
-  DoReturnNullTest(512 * 1024);
+TEST(PartitionAllocTest, MAYBE_RepeatedReturnNullDirect) {
+  // A direct-mapped allocation size.
+  DoReturnNullTest(32 * 1024 * 1024);
 }
 
-// Another "return null" test but for larger, direct-mapped allocations.
-//
+// Test "return null" with a 512 kB block size.

[Wez, 2017/02/12 18:51:28]

nit: This comment describes what the test *does* not _why_ it does it, i.e. what
it intends to test. The comment should be something like "verify return null for
a non-direct-mapped allocation size." or similar.

+
 // Disable this test on Android because, due to its allocation-heavy behavior,
 // it tends to get OOM-killed rather than pass.
 #if defined(OS_MACOSX) || defined(OS_ANDROID)
-#define MAYBE_RepeatedReturnNullDirect DISABLED_RepeatedReturnNullDirect
+#define MAYBE_RepeatedReturnNull DISABLED_RepeatedReturnNull
 #else
-#define MAYBE_RepeatedReturnNullDirect RepeatedReturnNullDirect
+#define MAYBE_RepeatedReturnNull RepeatedReturnNull
 #endif
-TEST(PartitionAllocTest, MAYBE_RepeatedReturnNullDirect) {
-  // A direct-mapped allocation size.
-  DoReturnNullTest(32 * 1024 * 1024);
+TEST(PartitionAllocTest, MAYBE_RepeatedReturnNull) {
+  // A single-slot but non-direct-mapped allocation size.
+  DoReturnNullTest(512 * 1024);
 }
 
 #endif  // !defined(ARCH_CPU_64_BITS) || defined(OS_POSIX)