Fix partition_alloc unit tests.

base/allocator/partition_allocator/partition_alloc_unittest.cc

 // Copyright (c) 2013 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/allocator/partition_allocator/partition_alloc.h"
 
 #include <stdlib.h>
 #include <string.h>
 
 #include <memory>
@@ skipping 38 matching lines @@
 #else
 const size_t kPointerOffset = kCookieSize;
 const size_t kExtraAllocSize = kCookieSize * 2;
 #endif
 const size_t kRealAllocSize = kTestAllocSize + kExtraAllocSize;
 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();
 }
 
 #if !defined(ARCH_CPU_64_BITS) || defined(OS_POSIX)
 bool SetAddressSpaceLimit() {
 #if !defined(ARCH_CPU_64_BITS)
   // 32 bits => address space is limited already.
   return true;
 #elif defined(OS_POSIX) && !defined(OS_MACOSX)
@@ skipping 1210 matching lines @@
                                          type_name);
     EXPECT_TRUE(ptrs[i]);
     PartitionFreeGeneric(generic_allocator.root(), ptrs[i]);
   }
 
   PartitionFreeGeneric(generic_allocator.root(), ptrs);
 
   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_RepeatedReturnNullDirect DISABLED_RepeatedReturnNullDirect
+#else
+#define MAYBE_RepeatedReturnNullDirect RepeatedReturnNullDirect
+#endif
+TEST(PartitionAllocTest, MAYBE_RepeatedReturnNullDirect) {
+  // A direct-mapped allocation size.
+  DoReturnNullTest(32 * 1024 * 1024);
+}
+
+// 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_RepeatedReturnNull DISABLED_RepeatedReturnNull
 #else
 #define MAYBE_RepeatedReturnNull RepeatedReturnNull
 #endif
 TEST(PartitionAllocTest, MAYBE_RepeatedReturnNull) {
   // A single-slot but non-direct-mapped allocation size.
   DoReturnNullTest(512 * 1024);
 }
 
-// Another "return null" test but for larger, direct-mapped allocations.
-//
-// 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
-#else
-#define MAYBE_RepeatedReturnNullDirect RepeatedReturnNullDirect
-#endif
-TEST(PartitionAllocTest, MAYBE_RepeatedReturnNullDirect) {
-  // A direct-mapped allocation size.
-  DoReturnNullTest(32 * 1024 * 1024);
-}
-
 #endif  // !defined(ARCH_CPU_64_BITS) || defined(OS_POSIX)
 
 // Death tests misbehave on Android, http://crbug.com/643760.
 #if defined(GTEST_HAS_DEATH_TEST) && !defined(OS_ANDROID)
 
 // Make sure that malloc(-1) dies.
 // In the past, we had an integer overflow that would alias malloc(-1) to
 // malloc(0), which is not good.
 TEST(PartitionAllocDeathTest, LargeAllocs) {
   TestSetup();
@@ skipping 741 matching lines @@
     EXPECT_FALSE(page->freelist_head);
 
     PartitionFreeGeneric(generic_allocator.root(), ptr1);
     PartitionFreeGeneric(generic_allocator.root(), ptr2);
   }
 }
 
 }  // namespace base
 
 #endif  // !defined(MEMORY_TOOL_REPLACES_ALLOCATOR)