components/metrics: Add runtime memory leak detector

components/metrics/leak_detector/call_stack_manager.cc

Index: components/metrics/leak_detector/call_stack_manager.cc
diff --git a/components/metrics/leak_detector/call_stack_manager.cc b/components/metrics/leak_detector/call_stack_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..c175c3ed67e4116c91056e194c2136e7a9602a02
--- /dev/null
+++ b/components/metrics/leak_detector/call_stack_manager.cc
@@ -0,0 +1,67 @@
+// 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 "components/metrics/leak_detector/call_stack_manager.h"
+
+#include <string.h>   // For memset.
+
+#include <algorithm>  // For std::copy.
+#include <new>
+
+#include "base/hash.h"
+#include "components/metrics/leak_detector/custom_allocator.h"
+
+namespace metrics {
+namespace leak_detector {
+
+CallStackManager::CallStackManager() {}
+
+CallStackManager::~CallStackManager() {
+  for (CallStack* call_stack : call_stacks_) {
+    CustomAllocator::Free(call_stack->stack,
+                          call_stack->depth * sizeof(*call_stack->stack));

[jar (doing other things), 2015/11/14 02:58:35]

nit: Perhaps a tiny comment indicating why the structures pointed to (or are
they just addresses into the stack segment??) are not deleted.

nit: personal preference/cleanliness: I find it is less error prone to clear out
pointers as I free areas.  In this case, it seems nice to follow this Free with:
call_stack->stack = NULL;
call_stack->depth = 0;

[Simon Que, 2015/11/17 00:28:47]

They are just addresses into the stack segment -- the addresses themselves are
the data values.

If this is not clear, I wonder if I'm better off storing the addresses as an
array of uintptr_t's.
Done

+    CustomAllocator::Free(call_stack, sizeof(CallStack));
+  }
+  call_stacks_.clear();
+}
+
+const CallStack* CallStackManager::GetCallStack(
+    int depth, const void* const stack[]) {
+  // Temporarily create a call stack object for lookup in |call_stacks_|.
+  CallStack temp;
+  temp.depth = depth;
+  temp.stack = const_cast<const void**>(stack);

[jar (doing other things), 2015/11/14 02:58:36]

nit: perhaps it would be nicer to have declared the |stack| member as:

const void*const *

and then I think you wouldn't need the cast here.

[Simon Que, 2015/11/17 00:28:48]

The problem is that we wouldn't be able to make a copy of the stack on line 54.
The |stack| member would need to be non-const. OTOH, I use this pointer
assignment to avoid a full-stack copy in case the CallStack object has already
been created.

+  // This is the only place where the call stack's hash is computed. This value
+  // can be reused in the created object to avoid further hash computation.
+  temp.hash =
+      base::Hash(reinterpret_cast<const char*>(stack), sizeof(*stack) * depth);
+
+  auto iter = call_stacks_.find(&temp);
+  if (iter != call_stacks_.end())
+    return *iter;
+
+  // Since |call_stacks_| stores CallStack pointers rather than actual objects,
+  // create new call objects manually here.
+  CallStack* call_stack =

[jar (doing other things), 2015/11/14 02:58:36]

nit: Would it have been nicer to declare CallStack as a class, including a
constructor (so you don't need to clear the memory via memset), and probably a
class specific operator new and operator delete (so you don't need to keep
mentioning your custom allocator)?

[Simon Que, 2015/11/17 00:28:47]

Actually the memset is not necessary, removed.

+      new(CustomAllocator::Allocate(sizeof(CallStack))) CallStack;
+  memset(call_stack, 0, sizeof(*call_stack));
+  call_stack->depth = depth;
+  call_stack->hash = temp.hash;  // Don't run the hash function again.
+  call_stack->stack =
+      reinterpret_cast<const void**>(
+          CustomAllocator::Allocate(sizeof(*stack) * depth));
+  std::copy(stack, stack + depth, call_stack->stack);
+
+  call_stacks_.insert(call_stack);
+  return call_stack;
+}
+
+bool CallStackManager::CallStackPointerEqual::operator() (
+    const CallStack* c1, const CallStack* c2) const {
+  return c1->depth == c2->depth &&

[jar (doing other things), 2015/11/14 02:58:36]

nit: Is it worth also validating that the hash matches, as that should be much
cheaper than the full compare?... or are you presuming that we'd never be called
if the hashes didn't match?

[Simon Que, 2015/11/17 00:28:48]

Done.

+         std::equal(c1->stack, c1->stack + c1->depth, c2->stack);
+}
+
+}  // namespace leak_detector
+}  // namespace metrics