metrics: Connect leak detector to allocator

components/metrics/leak_detector/leak_detector.cc

Index: components/metrics/leak_detector/leak_detector.cc
diff --git a/components/metrics/leak_detector/leak_detector.cc b/components/metrics/leak_detector/leak_detector.cc
index 0fbd868ccfa2df810e08ea523bd7f82b093f07c6..e47e1c0e3a2bc11f131ac56c7a830ff5d53fc6f3 100644
--- a/components/metrics/leak_detector/leak_detector.cc
+++ b/components/metrics/leak_detector/leak_detector.cc
@@ -4,10 +4,128 @@
 
 #include "components/metrics/leak_detector/leak_detector.h"
 
+#include <link.h>
+#include <stdint.h>
+#include <unistd.h>
+
+#include "base/allocator/allocator_extension.h"
+#include "base/logging.h"
+#include "components/metrics/leak_detector/custom_allocator.h"
 #include "content/public/browser/browser_thread.h"
 
 namespace metrics {
 
+using LeakReport = LeakDetector::LeakReport;
+using leak_detector::CustomAllocator;
+using leak_detector::LeakDetectorImpl;
+using InternalLeakReport = LeakDetectorImpl::LeakReport;
+template <typename T>
+using InternalVector = LeakDetectorImpl::InternalVector<T>;
+
+namespace {
+
+// The sampling of allocs and frees is handled internally using integer values,
+// not floating point values. This is the integer value that represents a 100%
+// sampling rate. See |g_sampling_factor|.
+const int kMaxSamplingFactor = 256;
+
+// Create an object of this class to store the current new/delete hooks and
+// then remove them. When this object goes out of scope, it will automatically
+// restore the original hooks if they existed.
+//
+// If multiple instances of this class are created and there are hooks
+// registered, only the first object will save and restore the hook functions.
+// The others will have no effect. However, all concurrent instances MUST be
+// destroyed in reverse relative to their instantiation.
+//
+// This is useful in situations such as:
+// - Calling alloc or free from within a hook function, which would otherwise
+//   result in recursive hook calls.
+// - Calling LOG() when |g_lock| is being held, as LOG will call malloc, which
+//   calls NewHook(), which then attempts to acquire the lock, resulting in it
+//   being blocked.
+class MallocHookDisabler {

[Primiano Tucci (use gerrit), 2016/03/01 18:54:37]

Hmm the design of this seems very prone to races. What happens if a thread is
doing an allocation while you uninstall the hooks here?

From the comment sounds like what you really want here is a scoped thread-local
disabler.
What I would do here is:
 - never uninstall-reinstall the hooks. Keep them always installed.
 - Make this class just increment and decrement a thread local integer variable
(you can use base::ThreadLocalPointer as a counter).
 - When you enter the hooks, check if that variable is >0, if so return (i.e.
make the hook a noop)

[Simon Que, 2016/03/01 19:09:53]

I am aware that uninstalling and reinstalling the hooks incurs a non-negligible
overhead, as I've seen from profiling my code locally. However...
1. InternalAlloc and InternalFree are meant to be called from CustomAllocator,
which is always to be called under lock.
2. I plan to implement a local allocator in the future for small sizes (e.g. <
128 bytes), which make up over 99.99% of the allocations in the leak detector.
When that is implemented, InternalAlloc will only be called in two cases: (a)
For larger sizes (e.g. >= 128 bytes); (b) When the local allocator needs more
memory to fulfill allocs. I've actually already written the code and the
uninstall-reinstall of hooks no longer shows up in perf profiles.

Your suggestion might work but it would also incur that extra overhead in the
hooks, esp if it's using something more exotic like pthread_getspecific(), which
is apparently the internal implementation of base::ThreadLocalPointer.

+ public:
+  MallocHookDisabler()
+      : new_hook_(base::allocator::SetSingleAllocHook(nullptr)),
+        delete_hook_(base::allocator::SetSingleFreeHook(nullptr)) {}
+
+  ~MallocHookDisabler() {
+    if (new_hook_)
+      base::allocator::SetSingleAllocHook(new_hook_);
+    if (delete_hook_)
+      base::allocator::SetSingleFreeHook(delete_hook_);
+  }
+
+ private:
+  base::allocator::AllocHookFunc new_hook_;
+  base::allocator::FreeHookFunc delete_hook_;
+
+  DISALLOW_COPY_AND_ASSIGN(MallocHookDisabler);
+};
+
+// For storing the address range of the Chrome binary in memory.
+struct MappingInfo {
+  uintptr_t addr;
+  size_t size;
+};
+
+// Disables hooks before calling new.
+void* InternalAlloc(size_t size) {
+  MallocHookDisabler disabler;
+  return new char[size];
+}
+
+// Disables hooks before calling delete.
+void InternalFree(void* ptr, size_t /* size */) {
+  MallocHookDisabler disabler;
+  delete[] reinterpret_cast<char*>(ptr);
+}
+
+// Callback for dl_iterate_phdr() to find the Chrome binary mapping.
+int IterateLoadedObjects(struct dl_phdr_info* shared_object,

[Primiano Tucci (use gerrit), 2016/03/01 18:54:37]

I cannot speak for CrOs, but on other platforms this is very error-prone.
Can I ask that, at very least, we make this #ifdef CrOS only? On android this
for sure will fail, so would be great to not end up using this by accident.

My suggestion here would be: do you really need to know the relative offset in
the client chrome? or probably you really need them only server-side. If this is
the case, what I would do here is merely return the offset from a well known
symbol (pick just any of the random function, or define a new one for this
purpose).
Then on server-side you reconstruct the real address by doing the reverse math
based on the knowledge of the virtual address of the known symbol + the offset
from it.
This will not require any client-side black magic.

However, I understandt his might change your design. Personally, as long as this
stays a CrOS only thingy.  Don't know enough of CrOS to say whether this is safe
or not, so, as long as somebody from CrOs is okay with that and we ifdef this
I'm fine.

[Simon Que, 2016/03/01 19:09:53]

I'll get someone from CrOS to take a look. Meanwhile, I'll also look into
specific symbols in the Chrome binary.

[Simon Que, 2016/03/02 06:25:09]

Done.

+                         size_t /* size */,
+                         void* data) {
+  for (int i = 0; i < shared_object->dlpi_phnum; i++) {
+    // Find the ELF segment header that contains the actual code of the Chrome
+    // binary.
+    const ElfW(Phdr)& segment_header = shared_object->dlpi_phdr[i];
+    if (segment_header.p_type == SHT_PROGBITS && segment_header.p_offset == 0 &&
+        data) {
+      MappingInfo* mapping = static_cast<MappingInfo*>(data);
+
+      // Make sure the fields in the ELF header and MappingInfo have the
+      // same size.
+      static_assert(sizeof(mapping->addr) == sizeof(shared_object->dlpi_addr),
+                    "Integer size mismatch between MappingInfo::addr and "
+                    "dl_phdr_info::dlpi_addr.");
+      static_assert(sizeof(mapping->size) == sizeof(segment_header.p_offset),
+                    "Integer size mismatch between MappingInfo::size and "
+                    "ElfW(Phdr)::p_memsz.");
+
+      mapping->addr = shared_object->dlpi_addr + segment_header.p_offset;
+      mapping->size = segment_header.p_memsz;
+      return 1;
+    }
+  }
+  return 0;
+}
+
+// Convert a pointer to a hash value. Returns only the upper eight bits.
+inline uint64_t PointerToHash(const void* ptr) {

[Primiano Tucci (use gerrit), 2016/03/01 18:54:37]

just doublecheckin, do you know that we have a SuperFastHash in base/ ?

[Simon Que, 2016/03/01 19:09:53]

Yes. I plan to further simplify this function by returning the result of the
multiplication without shifting. Instead, the sampling factor (which gets
compared against this value) would be shifted the other way during
initialization, which saves the extra shift each time this function is called.

[Primiano Tucci (use gerrit), 2016/03/01 21:10:28]

ok sounds like you have a plan here. 

[Simon Que, 2016/03/02 06:25:09]

Done.

+  // The input data is the pointer address, not the location in memory pointed
+  // to by the pointer.
+  // The multiplier is taken from Farmhash code:
+  //   https://github.com/google/farmhash/blob/master/src/farmhash.cc
+  const uint64_t kMultiplier = 0x9ddfea08eb382d69ULL;
+  uint64_t value = reinterpret_cast<uint64_t>(ptr) * kMultiplier;
+  return value >> 56;
+}
+
+LeakDetector* g_instance = nullptr;
+
+}  // namespace
+
 LeakDetector::LeakReport::LeakReport() {}
 
 LeakDetector::LeakReport::~LeakReport() {}
@@ -17,11 +135,56 @@ LeakDetector::LeakDetector(float sampling_rate,
                            uint64_t analysis_interval_bytes,
                            uint32_t size_suspicion_threshold,
                            uint32_t call_stack_suspicion_threshold)
-    : weak_factory_(this) {
-  // TODO(sque): Connect this class to LeakDetectorImpl and base::allocator.
+    : total_alloc_size_(0),
+      last_analysis_alloc_size_(0),
+      analysis_interval_bytes_(analysis_interval_bytes),
+      max_call_stack_unwind_depth_(max_call_stack_unwind_depth),
+      sampling_factor_(sampling_rate * kMaxSamplingFactor),
+      weak_factory_(this) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  CHECK(!g_instance) << "Cannot instantiate multiple instances of this class.";
+
+  // Locate the Chrome binary mapping info.
+  MappingInfo mapping;
+  dl_iterate_phdr(IterateLoadedObjects, &mapping);
+  binary_mapping_addr_ = mapping.addr;
+  binary_mapping_size_ = mapping.size;
+
+  CustomAllocator::Initialize(&InternalAlloc, &InternalFree);
+  impl_.reset(new LeakDetectorImpl(mapping.addr, mapping.size,
+                                   size_suspicion_threshold,
+                                   call_stack_suspicion_threshold));
+
+  // Register allocator hook functions.
+  if (base::allocator::SetSingleAllocHook(&AllocHook) != nullptr ||
+      base::allocator::SetSingleFreeHook(&FreeHook) != nullptr) {
+    MallocHookDisabler disabler;
+    LOG(FATAL) << "Overwrote existing callback.";
+  } else if (base::allocator::GetSingleAllocHook() != &AllocHook ||
+             base::allocator::GetSingleFreeHook() != &FreeHook) {
+    MallocHookDisabler disabler;
+    LOG(FATAL) << "Failed to register free callback.";
+    return;
+  }
+
+  g_instance = this;
 }
 
-LeakDetector::~LeakDetector() {}
+LeakDetector::~LeakDetector() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  g_instance = nullptr;
+
+  // Unregister allocator hook functions.
+  if (base::allocator::GetSingleAllocHook() == &AllocHook)
+    base::allocator::SetSingleAllocHook(nullptr);
+  if (base::allocator::GetSingleFreeHook() == &FreeHook)
+    base::allocator::SetSingleFreeHook(nullptr);
+
+  impl_.reset();
+  if (!CustomAllocator::Shutdown()) {
+    LOG(ERROR) << "Memory leak in leak detector, allocated objects remain.";
+  }
+}
 
 void LeakDetector::AddObserver(Observer* observer) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -33,7 +196,122 @@ void LeakDetector::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
+// static
+void LeakDetector::AllocHook(const void* ptr, size_t size) {
+  CHECK(g_instance);

[Primiano Tucci (use gerrit), 2016/03/01 21:10:28]

How are you preventing to hit this check when an allocation happens while
shutting down the ~LeakDetector.
In other words, how do you prevent that:

Thread A (who owns the Leak Detector) writes:
  g_instance = nullptr;
  SetHook (nullptr)

and Thread B:
 invokes AllocHook between the two instructions above?

note that even if you swap the order of execution of thread A that is still not
going to be thread safe, unless you use locks there and here in the check.

Probably here you want just:
if (!g_instance)
  return

[Simon Que, 2016/03/02 06:25:09]

Done

+
+  {
+    base::AutoLock lock(g_instance->lock_);
+    g_instance->total_alloc_size_ += size;
+
+    if (!ptr || !g_instance->ShouldSample(ptr))
+      return;
+  }
+
+  auto& impl = g_instance->impl_;
+
+  // Must be modified under lock as it uses the shared resources of
+  // CustomAllocator.
+  InternalVector<void*> stack;
+
+  // Take the stack trace outside the critical section.
+  // |LeakDetectorImpl::ShouldGetStackTraceForSize()| is const; there is no

[Primiano Tucci (use gerrit), 2016/03/01 21:10:28]

what happens if you are here while you destroy the impl on another thread?

+  // need for a lock.
+  int depth = 0;
+  if (impl->ShouldGetStackTraceForSize(size)) {
+    {
+      base::AutoLock lock(g_instance->lock_);
+      stack.resize(g_instance->max_call_stack_unwind_depth_);
+    }
+    depth = base::allocator::GetCallStack(stack.data(), stack.size(), 0);
+  }
+
+  g_instance->RecordAlloc(ptr, size, depth, stack.data());

[Primiano Tucci (use gerrit), 2016/03/01 21:10:28]

in the header of this class you say that this class is not thread safe, but here
you call RecordAlloc without any lock

+
+  {
+    base::AutoLock lock(g_instance->lock_);
+
+    // InternalVectors must be cleaned up under lock, so we can't wait for them
+    // to go out of scope.
+    // std::vector::clear() still leaves reserved memory inside that will be
+    // cleaned up by the destructor when it goes out of scope. And
+    // vector::shrink_to_fit() is not allowed to be used yet. Instead swap
+    // out the contents to a local container that is cleaned up when it goes
+    // out of scope.
+    InternalVector<void*> dummy_stack;
+    stack.swap(dummy_stack);
+  }
+}
+
+// static
+void LeakDetector::FreeHook(const void* ptr) {
+  if (!ptr || !g_instance->ShouldSample(ptr))
+    return;
+
+  base::AutoLock lock(g_instance->lock_);
+  g_instance->impl_->RecordFree(ptr);
+}
+
+inline bool LeakDetector::ShouldSample(const void* ptr) const {
+  return PointerToHash(ptr) < static_cast<uint64_t>(sampling_factor_);
+}
+
+void LeakDetector::RecordAlloc(const void* ptr,
+                               size_t size,
+                               int depth,
+                               void** call_stack) {
+  // This should be modified only with a lock because it uses the shared
+  // resources in CustomAllocator.
+  InternalVector<InternalLeakReport> leak_reports;
+
+  {
+    base::AutoLock lock(lock_);
+    impl_->RecordAlloc(ptr, size, depth, call_stack);
+
+    // Check for leaks after |analysis_interval_bytes_| bytes have been
+    // allocated since the last time that was done. Should be called with a lock
+    // since it:
+    // - Modifies the global variable |g_last_analysis_alloc_size|.
+    // - Updates internals of |*impl|.
+    // - Possibly generates a vector of LeakReports using CustomAllocator.
+    if (total_alloc_size_ >
+        last_analysis_alloc_size_ + analysis_interval_bytes_) {
+      // Try to maintain regular intervals of size |analysis_interval_bytes_|.
+      last_analysis_alloc_size_ =
+          total_alloc_size_ - total_alloc_size_ % analysis_interval_bytes_;
+      impl_->TestForLeaks(&leak_reports);
+    }
+  }
+
+  std::vector<LeakReport> leak_reports_for_observers;
+  leak_reports_for_observers.reserve(leak_reports.size());
+  for (const InternalLeakReport& report : leak_reports) {
+    leak_reports_for_observers.push_back(LeakReport());
+
+    LeakReport* new_report = &leak_reports_for_observers.back();
+    new_report->alloc_size_bytes = report.alloc_size_bytes();
+    if (!report.call_stack().empty()) {
+      new_report->call_stack.resize(report.call_stack().size());
+      memcpy(new_report->call_stack.data(), report.call_stack().data(),
+             report.call_stack().size() * sizeof(report.call_stack()[0]));
+    }
+  }
+
+  {
+    base::AutoLock lock(lock_);
+    InternalVector<InternalLeakReport> dummy_leak_reports;
+    leak_reports.swap(dummy_leak_reports);
+  }
+
+  // Pass leak reports to observers. The observers must be called outside of the
+  // locked area to avoid slowdown.
+  NotifyObservers(leak_reports_for_observers);
+}
+
 void LeakDetector::NotifyObservers(const std::vector<LeakReport>& reports) {
+  if (reports.empty())
+    return;
+
   if (!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
     content::BrowserThread::PostTask(
         content::BrowserThread::UI, FROM_HERE,