Fix 2 bugs in OOP HP implementation. Re-enable tests on Windows.

The re-entrancy flag was missing for PartitionAlloc and Oilpan hooks. There was
an extra call to AllocatorShimLogFree() in HookFree().

Change-Id: Ida86b9e0e3ba33679ba353baccd47f5fcf93685c
Bug: 813117
TBR: primiano@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/924885
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537554}

chrome/browser/profiling_host/memlog_browsertest.cc

@@ -77,13 +77,7 @@ class MemlogBrowserTest : public InProcessBrowserTest,
 
 // Ensure invocations via TracingController can generate a valid JSON file with
 // expected data.
-// Crashes on Win only.  http://crbug.com/813117
-#if defined(OS_WIN)
-#define MAYBE_EndToEnd DISABLED_EndToEnd
-#else
-#define MAYBE_EndToEnd EndToEnd
-#endif
-IN_PROC_BROWSER_TEST_P(MemlogBrowserTest, MAYBE_EndToEnd) {
+IN_PROC_BROWSER_TEST_P(MemlogBrowserTest, EndToEnd) {
   LOG(INFO) << "Memlog mode: " << static_cast<int>(GetParam().mode);
   LOG(INFO) << "Memlog stack mode: " << static_cast<int>(GetParam().stack_mode);
   LOG(INFO) << "Started via command line flag: "

chrome/browser/profiling_host/profiling_test_driver.cc

@@ -688,10 +688,8 @@ void ProfilingTestDriver::MakeTestAllocations() {
   leaks_.reserve(2 * kMallocAllocCount + 1 + kPartitionAllocSize);
 
   {
-    DisableAllocationTrackingForCurrentThreadForTesting();
     TRACE_HEAP_PROFILER_API_SCOPED_TASK_EXECUTION event(kMallocTypeTag);
     TRACE_EVENT0(kTestCategory, kMallocEvent);
-    EnableAllocationTrackingForCurrentThreadForTesting();
 
     for (int i = 0; i < kMallocAllocCount; ++i) {
       leaks_.push_back(new char[kMallocAllocSize]);
@@ -699,9 +697,7 @@ void ProfilingTestDriver::MakeTestAllocations() {
   }
 
   {
-    DisableAllocationTrackingForCurrentThreadForTesting();
     TRACE_EVENT0(kTestCategory, kPAEvent);
-    EnableAllocationTrackingForCurrentThreadForTesting();
 
     for (int i = 0; i < kPartitionAllocCount; ++i) {
       leaks_.push_back(static_cast<char*>(partition_allocator_.root()->Alloc(
@@ -710,9 +706,7 @@ void ProfilingTestDriver::MakeTestAllocations() {
   }
 
   {
-    DisableAllocationTrackingForCurrentThreadForTesting();
     TRACE_EVENT0(kTestCategory, kVariadicEvent);
-    EnableAllocationTrackingForCurrentThreadForTesting();
 
     for (int i = 0; i < kVariadicAllocCount; ++i) {
       leaks_.push_back(new char[i + 8000]);  // Variadic allocation.

chrome/common/profiling/memlog_allocator_shim.cc

@@ -355,7 +355,6 @@ void HookFree(const AllocatorDispatch* self, void* address, void* context) {
   next->free_function(next, address, context);
 
   if (LIKELY(!reentering)) {
-    AllocatorShimLogFree(address);
     g_prevent_reentrancy.Pointer()->Set(false);
   }
 }
@@ -432,19 +431,37 @@ AllocatorDispatch g_memlog_hooks = {
 #endif  // BUILDFLAG(USE_ALLOCATOR_SHIM)
 
 void HookPartitionAlloc(void* address, size_t size, const char* type) {
-  AllocatorShimLogAlloc(AllocatorType::kPartitionAlloc, address, size, type);
+  // If this is our first time passing through, set the reentrancy bit.
+  if (LIKELY(!g_prevent_reentrancy.Pointer()->Get())) {
+    g_prevent_reentrancy.Pointer()->Set(true);
+    AllocatorShimLogAlloc(AllocatorType::kPartitionAlloc, address, size, type);
+    g_prevent_reentrancy.Pointer()->Set(false);
+  }
 }
 
 void HookPartitionFree(void* address) {
-  AllocatorShimLogFree(address);
+  // If this is our first time passing through, set the reentrancy bit.
+  if (LIKELY(!g_prevent_reentrancy.Pointer()->Get())) {
+    g_prevent_reentrancy.Pointer()->Set(true);
+    AllocatorShimLogFree(address);
+    g_prevent_reentrancy.Pointer()->Set(false);
+  }
 }
 
 void HookGCAlloc(uint8_t* address, size_t size, const char* type) {
-  AllocatorShimLogAlloc(AllocatorType::kOilpan, address, size, type);
+  if (LIKELY(!g_prevent_reentrancy.Pointer()->Get())) {
+    g_prevent_reentrancy.Pointer()->Set(true);
+    AllocatorShimLogAlloc(AllocatorType::kOilpan, address, size, type);
+    g_prevent_reentrancy.Pointer()->Set(false);
+  }
 }
 
 void HookGCFree(uint8_t* address) {
-  AllocatorShimLogFree(address);
+  if (LIKELY(!g_prevent_reentrancy.Pointer()->Get())) {
+    g_prevent_reentrancy.Pointer()->Set(true);
+    AllocatorShimLogFree(address);
+    g_prevent_reentrancy.Pointer()->Set(false);
+  }
 }
 
 // Updates an existing in_memory buffer with frame data. If a frame contains a
@@ -815,12 +832,4 @@ void SetOnInitAllocatorShimCallbackForTesting(
   *g_on_init_allocator_shim_task_runner_.Pointer() = task_runner;
 }
 
-void DisableAllocationTrackingForCurrentThreadForTesting() {
-  g_prevent_reentrancy.Pointer()->Set(true);
-}
-
-void EnableAllocationTrackingForCurrentThreadForTesting() {
-  g_prevent_reentrancy.Pointer()->Set(false);
-}
-
 }  // namespace profiling

chrome/common/profiling/memlog_allocator_shim.h

@@ -61,9 +61,6 @@ void SetOnInitAllocatorShimCallbackForTesting(
     base::OnceClosure callback,
     scoped_refptr<base::TaskRunner> task_runner);
 
-void DisableAllocationTrackingForCurrentThreadForTesting();
-void EnableAllocationTrackingForCurrentThreadForTesting();
-
 }  // namespace profiling
 
 #endif  // CHROME_COMMON_PROFILING_MEMLOG_ALLOCATOR_SHIM_H_