On heap tracking datastructure overflow, degrade instead of CHECK()

base/trace_event/heap_profiler_allocation_register.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/trace_event/heap_profiler_allocation_register.h"
 
 #include <algorithm>
 
 #include "base/trace_event/trace_event_memory_overhead.h"
 
@@ skipping 63 matching lines @@
     : AllocationRegister(kAllocationCapacity, kBacktraceCapacity) {}
 
 AllocationRegister::AllocationRegister(size_t allocation_capacity,
                                        size_t backtrace_capacity)
     : allocations_(allocation_capacity),
       backtraces_(backtrace_capacity) {}
 
 AllocationRegister::~AllocationRegister() {
 }
 
-void AllocationRegister::Insert(const void* address,
+bool AllocationRegister::Insert(const void* address,
                                 size_t size,
                                 const AllocationContext& context) {
   DCHECK(address != nullptr);
   if (size == 0) {
-    return;
+    return false;
   }
 
   AllocationInfo info = {
       size,
       context.type_name,
       InsertBacktrace(context.backtrace)
   };
 
   // Try to insert the allocation.
   auto index_and_flag = allocations_.Insert(address, info);
-  if (!index_and_flag.second) {
+  if (!index_and_flag.second &&
+      index_and_flag.first != AllocationMap::kInvalidKVIndex) {
     // |address| is already there - overwrite the allocation info.
     auto& old_info = allocations_.Get(index_and_flag.first).second;
     RemoveBacktrace(old_info.backtrace_index);
     old_info = info;
   }

[Primiano Tucci (use gerrit), 2017/03/31 19:05:47]

minor thing, but  this branch should return true, as the insert technically
succeeded here (by replacing the old entry).

+
+  return index_and_flag.second;
 }
 
 void AllocationRegister::Remove(const void* address) {
   auto index = allocations_.Find(address);
   if (index == AllocationMap::kInvalidKVIndex) {
     return;
   }
 
   const AllocationInfo& info = allocations_.Get(index).second;
   RemoveBacktrace(info.backtrace_index);
@@ skipping 26 matching lines @@
   size_t allocated = sizeof(AllocationRegister);
   size_t resident = sizeof(AllocationRegister)
                     + allocations_.EstimateUsedMemory()
                     + backtraces_.EstimateUsedMemory();
   overhead->Add("AllocationRegister", allocated, resident);
 }
 
 AllocationRegister::BacktraceMap::KVIndex AllocationRegister::InsertBacktrace(
     const Backtrace& backtrace) {
   auto index = backtraces_.Insert(backtrace, 0).first;
-  auto& backtrace_and_count = backtraces_.Get(index);
-  backtrace_and_count.second++;
+  if (index != BacktraceMap::kInvalidKVIndex) {
+    auto& backtrace_and_count = backtraces_.Get(index);
+    backtrace_and_count.second++;
+  }
   return index;
 }
 
 void AllocationRegister::RemoveBacktrace(BacktraceMap::KVIndex index) {
   auto& backtrace_and_count = backtraces_.Get(index);

[Primiano Tucci (use gerrit), 2017/03/31 19:05:47]

I think now you created a bug here.
If you run out of backtraces_ space but not allocations, you can end up with an
allocation which is mapped to backtrace index -1, which will BOOM in the Get
here (tell me again how useless is that guard page now :P).

   if (--backtrace_and_count.second == 0) {
     // Backtrace is not referenced anymore - remove it.
     backtraces_.Remove(index);
   }
 }
 
 AllocationRegister::Allocation AllocationRegister::GetAllocation(
     AllocationMap::KVIndex index) const {
   const auto& address_and_info = allocations_.Get(index);
   const auto& backtrace_and_count = backtraces_.Get(
       address_and_info.second.backtrace_index);
   return {
       address_and_info.first,
       address_and_info.second.size,
       AllocationContext(
           backtrace_and_count.first,
           address_and_info.second.type_name)
   };
 }
 
 }  // namespace trace_event
 }  // namespace base