Skip thread-unsafe Windows heaps from MallocDumpProvider.

base/trace_event/malloc_dump_provider.cc

Index: base/trace_event/malloc_dump_provider.cc
diff --git a/base/trace_event/malloc_dump_provider.cc b/base/trace_event/malloc_dump_provider.cc
index 7d0cb579315d4884e00327fa5175b01c5a3824bd..8b60c88968b1f33cbc25c86fbd6c65f88e626bef 100644
--- a/base/trace_event/malloc_dump_provider.cc
+++ b/base/trace_event/malloc_dump_provider.cc
@@ -25,6 +25,8 @@
 #endif
 #if defined(OS_WIN)
 #include <windows.h>
+// See https://msdn.microsoft.com/en-us/library/windows/desktop/aa366703 .
+#define HEAP_LFH 2
 #endif
 
 namespace base {
@@ -103,18 +105,45 @@ struct WinHeapInfo {
   size_t block_count;
 };
 
-bool GetHeapInformation(WinHeapInfo* heap_info,
-                        const std::set<void*>& block_to_skip) {
+bool TryHeapLock(HANDLE heap) {
+  HANDLE main_heap = ::GetProcessHeap();
+
+  // NOTE: crbug.com/665516
+  // We should never try to lock a heap created with HEAP_NO_SERIALIZE flag,
+  // since behaviour of HeapLock is undefined and confirmed to be crashy.
+  // Unfortunately, WinAPI lacks a function to tell the flags heap has been
+  // created with, so we don't account any potentially unsafe heap.
+  ULONG heap_info;
+  if (!::HeapQueryInformation(heap, HeapCompatibilityInformation, &heap_info,
+                              sizeof(heap_info), nullptr)) {
+    return false;
+  }
+  // Low-fragmentation heaps are used by default since Windows Vista and
+  // incompatible with HEAP_NO_SERIALIZE flag, hence there is an indicator.
+  // However, main heap is always accountable even if not LFH because
+  // Windows Runtime should be able to spawn some utility threads implicitly.
+  if (heap_info != HEAP_LFH && heap != main_heap) {
+    return false;
+  }
+
   // NOTE: crbug.com/464430
   // As a part of the Client/Server Runtine Subsystem (CSRSS) lockdown in the
-  // referenced bug, it will invalidate the heap used by CSRSS. The author has
-  // not found a way to clean up an invalid heap handle, so it will be left in
-  // the process's heap list. Therefore we need to support when there is this
-  // invalid heap handle in the heap list.
-  // HeapLock implicitly checks certain aspects of the HEAP structure, such as
-  // the signature. If this passes, we assume that this heap is valid and is
-  // not the one owned by CSRSS.
-  if (!::HeapLock(heap_info->heap_id)) {
+  // referenced bug, it will invalidate the heap used by CSRSS.
+  // HeapLock implicitly checks certain aspects of heap structure.
+  // If this passes, we assume that this heap is valid and is not a heap
+  // owned by CSRSS nor some another corrupted heap.
+  if (!::HeapLock(heap)) {
+    CHECK(heap != main_heap) << "Main WinHeap is not accessible.";
+    return false;
+  } else {
+    return true;
+  }
+}
+
+bool GetWinHeapInformation(WinHeapInfo* heap_info,
+                           const std::set<void*>& block_to_skip) {
+  // Some heaps aren't accountable, see TryHeapLock for details.
+  if (!TryHeapLock(heap_info->heap_id)) {
     return false;
   }
   PROCESS_HEAP_ENTRY heap_entry;
@@ -141,9 +170,6 @@ void WinHeapMemoryDumpImpl(WinHeapInfo* all_heap_info) {
 //     available heaps, but there's no guarantee that that snapshot remains
 //     valid. If a heap disappears between GetProcessHeaps() and HeapWalk()
 //     then chaos should be assumed. This flakyness is acceptable for tracing.
-//   - The MSDN page for HeapLock says: "If the HeapLock function is called on
-//     a heap created with the HEAP_NO_SERIALIZATION flag, the results are
-//     undefined."
 //   - Note that multiple heaps occur on Windows primarily because system and
 //     3rd party DLLs will each create their own private heap. It's possible to
 //     retrieve the heap the CRT allocates from and report specifically on that
@@ -172,20 +198,14 @@ void WinHeapMemoryDumpImpl(WinHeapInfo* all_heap_info) {
   block_to_skip.insert(all_heaps.get());
 
   // Retrieves some metrics about each heap.
-  size_t heap_info_errors = 0;
   for (size_t i = 0; i < number_of_heaps; ++i) {
     WinHeapInfo heap_info = {0};
     heap_info.heap_id = all_heaps[i];
-    if (GetHeapInformation(&heap_info, block_to_skip)) {
+    if (GetWinHeapInformation(&heap_info, block_to_skip)) {
       all_heap_info->allocated_size += heap_info.allocated_size;
       all_heap_info->committed_size += heap_info.committed_size;
       all_heap_info->uncommitted_size += heap_info.uncommitted_size;
       all_heap_info->block_count += heap_info.block_count;
-    } else {
-      ++heap_info_errors;
-      // See notes in GetHeapInformation() but we only expect 1 heap to not be
-      // able to be read.
-      CHECK_EQ(1u, heap_info_errors);
     }
   }
 }