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..c641d7b8782cfe6485f7fbcb57ca188cd98d486b 100644
--- a/components/metrics/leak_detector/leak_detector.cc
+++ b/components/metrics/leak_detector/leak_detector.cc
@@ -4,10 +4,139 @@
 
 #include "components/metrics/leak_detector/leak_detector.h"
 
+#include <stdint.h>
+#include <unistd.h>
+
+#include <algorithm>  // for std::min
+
+#include "base/allocator/allocator_extension.h"
+#include "base/logging.h"
+#include "base/synchronization/lock.h"
+#include "components/metrics/leak_detector/custom_allocator.h"
 #include "content/public/browser/browser_thread.h"
 
+#if defined(OS_CHROMEOS)
+#include <link.h>  // for dl_iterate_phdr
+#else
+#error "Getting binary mapping info is not supported on this platform."
+#endif  // defined(OS_CHROMEOS)
+
 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 {
+
+// 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.
+//
+// This class is not thread-safe and should be called under lock.
+class AllocatorHookDisabler {
+ public:
+  AllocatorHookDisabler() {
+    base::allocator::SetHooks(nullptr, nullptr);
+  }
+
+  ~AllocatorHookDisabler() {
+    base::allocator::SetHooks(alloc_hook_, free_hook_);
+  }
+
+  static base::allocator::AllocHookFunc alloc_hook_;

[Primiano Tucci (use gerrit), 2016/03/03 16:23:44]

It seems that this variable is only ever set to be = to &AllocHook;
Why don't you directly SetHooks(&AllocHook, &FreeHook) in the dtor, instead of
keeping a copy of something you already know in these statics?

[Simon Que, 2016/03/04 01:59:18]

That would require AllocHook/FreeHook to be public, or defined as a non-member
function locally. It'll depends on how the rest of the code is structured, based
on the rest of this discussion. Let's revisit this later when we have the rest
of the design laid out.

[Simon Que, 2016/03/04 02:55:19]

One argument in favor of the current design: what happens when the LeakDetector
dtor has unregistered the hooks, but there are still some other threads that
have not finished executing in the hooks? If they instantiate and destroy one of
these, they must not re-install the hooks.

If you think that's a good reason to keep it this way, I can add a comment here
explaining the rationale.

[Primiano Tucci (use gerrit), 2016/03/04 06:57:35]

That's why I was suggesting to have a boolean state to keep track of that.

[Primiano Tucci (use gerrit), 2016/03/04 06:57:35]

I am not sure I follow. AllocHook and FreeHook here are already static functions
here.
As I suggested in my previous comment, they don't even need to be such, they
could be just functions in the anonymous namespace.
Even if you want to keep them static functions of LeakDetector, very likely what
you want is the disabler to be an internal class of the LeakDetector, or just
use friend.

+  static base::allocator::FreeHookFunc free_hook_;
+
+  private:
+  DISALLOW_COPY_AND_ASSIGN(AllocatorHookDisabler);
+};
+
+base::allocator::AllocHookFunc AllocatorHookDisabler::alloc_hook_ = nullptr;
+base::allocator::FreeHookFunc AllocatorHookDisabler::free_hook_ = nullptr;
+
+// Disables hooks before calling new.
+void* InternalAlloc(size_t size) {
+  AllocatorHookDisabler disabler;
+  return new char[size];
+}
+
+// Disables hooks before calling delete.
+void InternalFree(void* ptr, size_t /* size */) {
+  AllocatorHookDisabler disabler;
+  delete[] reinterpret_cast<char*>(ptr);
+}
+
+#if defined(OS_CHROMEOS)
+// For storing the address range of the Chrome binary in memory.
+struct MappingInfo {
+  uintptr_t addr;
+  size_t size;
+};
+
+// Callback for dl_iterate_phdr() to find the Chrome binary mapping.
+int IterateLoadedObjects(struct dl_phdr_info* shared_object,
+                         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;
+}
+#endif  // defined(OS_CHROMEOS)
+
+// Convert a pointer to a hash value. Returns only the upper eight bits.
+inline uint64_t PointerToHash(const void* ptr) {
+  // 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;
+  return reinterpret_cast<uint64_t>(ptr) * kMultiplier;
+}
+
+// A singleton instance of LeakDetector, used by the hook functions, which are
+// static members of LeakDetector.
+LeakDetector* g_instance = nullptr;
+
+// For atomic access to |g_instance| -- the pointer, not the pointed-to object.

[Primiano Tucci (use gerrit), 2016/03/03 17:51:30]

You say "not the pointed-to object" but in reality you use this lock (and need
it) to avoid concurrent accesses to the LeaakDetectorImpl class.

+base::Lock* g_instance_ptr_lock = nullptr;

[Primiano Tucci (use gerrit), 2016/03/03 16:23:44]

This lock as is is not really solving any problem. You are preventing that you
access an inconsistent g_instance ptr but now you moved the race to the lock.
How can prevent to access an inconsistent lock if you hit a hook function while
you are destroying the LeakDetector (and nulling the lock)

I think what you really want is the lock here to be static. What I typically see
in these cases in chromium is 

base::LazyInstance<base::Lock>::Leaky g_instance_ptr_lock =
LAZY_INSTANCE_INITIALIZER;

[Simon Que, 2016/03/04 01:59:18]

What we really need here is a one-shot rwlock. I'm not sure that a standard lock
is the best option since it has to cover all the code that uses |g_instance| --
meaning it locks a large section of code.

If you're averse to using a pthread_rwlock, I can make my own -- it's easy
because the only writer is the dtor. Replace this with an int counter that
represents the number of threads using g_instance. Each time the hook function
runs, increment the counter, and then decrement on exit.

In the dtor, the hooks will be unregistered first. Then the dtor will wait for
the counter to reach 0. The dtor will end up waiting for any threads currently
in the hook to finish, and there will be no more hook calls.

Then, I can change hook code to use the lock in fewer places since there there
are no longer any thread contention issues with |g_instance| as a ptr.

[Primiano Tucci (use gerrit), 2016/03/04 06:57:35]

Is not that I am adverse. As usually in chrome the pattern is: is there
precedent for doing this? If not why?
base does not expose a rwlock. Neither I can find any non-third_party code which
uses pthread_rwlock.
There are a bunch of discussions on chromium-dev about the topic. 
Yes, and now you run into another small problem: how do you make the concurrent
accesses to this counter sane? Now you need another lock for the counter.
So you have a lock for the counter, you have a counter and now you want a method
to wait for that: I think you have just re-invented base::ConditionVariable.

+
+}  // namespace
+
 LeakDetector::LeakReport::LeakReport() {}
 
 LeakDetector::LeakReport::~LeakReport() {}
@@ -17,11 +146,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),

[Primiano Tucci (use gerrit), 2016/03/03 16:23:44]

Can you reset  binary_mapping_addr_ and  binary_mapping_size_ here? If someone
in future takes this piece of code and doesn't realize the #ifdef part below
that would be left uninitialized

[Simon Que, 2016/03/04 01:59:18]

Done.

+      last_analysis_alloc_size_(0),
+      analysis_interval_bytes_(analysis_interval_bytes),
+      max_call_stack_unwind_depth_(max_call_stack_unwind_depth),
+      sampling_factor_(std::min(sampling_rate, 1.0f) * UINT64_MAX),
+      weak_factory_(this) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  CHECK(!g_instance) << "Cannot instantiate multiple instances of this class.";

[Primiano Tucci (use gerrit), 2016/03/03 16:23:44]

don't you need the g_instance_ptr_lock to make this check?

[Simon Que, 2016/03/04 01:59:18]

The DCHECK above should ensure that the ctor is always on the same thread.

[Primiano Tucci (use gerrit), 2016/03/04 06:57:35]

Acknowledged.

+
+#if defined(OS_CHROMEOS)
+  // Locate the Chrome binary mapping info.
+  MappingInfo mapping;
+  dl_iterate_phdr(IterateLoadedObjects, &mapping);
+  binary_mapping_addr_ = mapping.addr;
+  binary_mapping_size_ = mapping.size;
+#endif  // defined(OS_CHROMEOS)
+
+  // InternalAlloc() and InternalFree() are not thread-safe, but neither is
+  // CustomAllocator. CustomAllocator should always be accessed under lock.
+  // There is no need to further lock InternalAlloc()/-Free().
+  CustomAllocator::Initialize(&InternalAlloc, &InternalFree);
+  impl_.reset(new LeakDetectorImpl(mapping.addr, mapping.size,
+                                   size_suspicion_threshold,
+                                   call_stack_suspicion_threshold));
+
+  g_instance_ptr_lock = new base::Lock;
+  g_instance = this;
+
+  AllocatorHookDisabler::alloc_hook_ = &AllocHook;
+  AllocatorHookDisabler::free_hook_ = &FreeHook;
+  // Register allocator hook functions.
+  base::allocator::SetHooks(&AllocHook, &FreeHook);
 }
 
-LeakDetector::~LeakDetector() {}
+LeakDetector::~LeakDetector() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // Unregister allocator hook functions.
+  base::allocator::SetHooks(nullptr, nullptr);
+  AllocatorHookDisabler::alloc_hook_ = nullptr;
+  AllocatorHookDisabler::free_hook_ = nullptr;
+
+  delete g_instance_ptr_lock;
+  g_instance_ptr_lock = 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 +207,94 @@ void LeakDetector::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
+// static
+void LeakDetector::AllocHook(const void* ptr, size_t size) {
+  if (!ptr)
+    return;
+
+  base::AutoLock lock(*g_instance_ptr_lock);

[Primiano Tucci (use gerrit), 2016/03/03 16:23:44]

At this point you already have to take a lock every time you enter this (and the
free hooks).
Can't you leverage the g_isntance check (or the ShouldSample check) to bypass
the hooks without having to uninstall and reinstall them in the scoped enabler.

+
+  if (!g_instance || !g_instance->ShouldSample(ptr))
+    return;
+
+  g_instance->total_alloc_size_ += size;
+
+  // Must be modified under lock as it uses the shared resources of
+  // CustomAllocator.
+  InternalVector<void*> stack;
+  int depth = 0;
+  if (g_instance->impl_->ShouldGetStackTraceForSize(size)) {
+    stack.resize(g_instance->max_call_stack_unwind_depth_);
+    depth = base::allocator::GetCallStack(stack.data(), stack.size());
+  }
+
+  g_instance->RecordAlloc(ptr, size, depth, stack.data());
+}
+
+// static
+void LeakDetector::FreeHook(const void* ptr) {
+  if (!ptr)
+    return;
+
+  base::AutoLock lock(*g_instance_ptr_lock);
+
+  if (!g_instance || !g_instance->ShouldSample(ptr))
+    return;
+
+  g_instance->impl_->RecordFree(ptr);
+}
+
+inline bool LeakDetector::ShouldSample(const void* ptr) const {
+  return PointerToHash(ptr) < 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

[Primiano Tucci (use gerrit), 2016/03/03 17:51:30]

You can make this comment more explicit by adding a :

g_lock->AssertAcquired() (which is a noop in release, but still gives you a
dcheck)

+  // resources in CustomAllocator.
+  InternalVector<InternalLeakReport> leak_reports;
+
+  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.

[Primiano Tucci (use gerrit), 2016/03/03 17:51:30]

Suggestion (maybe for a future CL): probably would be less intrusive if you did
all this leak analysis below in a posted task, which would give you two
benefits:
you would avoid races by being sure that everything happens on the same thread,
and will avoid to block random threads during their allocation fastpaths

[Simon Que, 2016/03/04 01:59:18]

Added a TODO.

+  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;

[Primiano Tucci (use gerrit), 2016/03/03 17:51:30]

isn't std::vector cause mallocs and going to re-enter the LeakDetector?

[Simon Que, 2016/03/04 01:59:18]

Yes. Recursive calls to the allocator are actually OK as long as they don't
cause a deadlock -- e.g. called from an unlocked location.

This code was originally written as an unlocked location.

[Primiano Tucci (use gerrit), 2016/03/04 06:57:35]

How can you say that re-entrant calls to RecordAlloc are OK?

1) You are using std::vector here, how can you be sure that std::vector is
reentrancy-safe  in the allocator path? That is an implementation detail.

2) Below you are invoking some observers. How can you be sure that the observers
are reentrancy-safe if you don't know who they are?

3) In the code below you are doing a vector.push_back(). How is that
re-entrant-safe? (push_back while you are pushing_back in the same vector?)

+  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]));
+    }
+  }
+
+  // Pass leak reports to observers. The observers must be called outside of the
+  // locked area to avoid slowdown.
+  NotifyObservers(leak_reports_for_observers);

[Primiano Tucci (use gerrit), 2016/03/03 17:51:30]

NotifyObserver does all sort of thigs (PosTask, invoking a list of observers).
How do you guarantee that they don't re-enter the hooks? PostTask very likely
will do some malloc. Similarly you have no control of your observers.
All this seems to work only if none of that malloc's

+}
+
 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,