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..a2a33d74e32d46afba641ad980ddcb70e8a35101 100644
--- a/components/metrics/leak_detector/leak_detector.cc
+++ b/components/metrics/leak_detector/leak_detector.cc
@@ -4,45 +4,279 @@
 
 #include "components/metrics/leak_detector/leak_detector.h"
 
+#include <stdint.h>
+
+#include "base/allocator/allocator_extension.h"
+#include "base/bind.h"
+#include "base/lazy_instance.h"
+#include "base/logging.h"
+#include "base/numerics/safe_conversions.h"
+#include "base/threading/thread_local.h"
+#include "components/metrics/leak_detector/custom_allocator.h"
+#include "components/metrics/leak_detector/leak_detector_impl.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 InternalLeakReport = leak_detector::LeakDetectorImpl::LeakReport;
+template <typename T>
+using InternalVector = leak_detector::LeakDetectorImpl::InternalVector<T>;
+
+namespace {
+
+#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);

[Will Harris, 2016/03/15 04:13:35]

reinterpret_cast for pointers

[Simon Que, 2016/03/15 06:39:31]

Done.

+
+      // 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;
+}
+
+// Converts a vector of leak reports generated by LeakDetectorImpl
+// (InternalLeakReport) to a vector of leak reports suitable for sending to
+// LeakDetector's observers (LeakReport).
+void GetReportsForObservers(
+    const InternalVector<InternalLeakReport>& leak_reports,
+    std::vector<LeakReport>* reports_for_observers) {
+  reports_for_observers->clear();
+  reports_for_observers->resize(leak_reports.size());
+  for (const InternalLeakReport& report : leak_reports) {
+    reports_for_observers->push_back(LeakReport());
+    LeakReport* new_report = &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]));
+    }
+  }
+}
+
+// The only instance of LeakDetector that should be used.
+base::LazyInstance<LeakDetector>::Leaky g_instance = LAZY_INSTANCE_INITIALIZER;
+
+// Thread-specific flag indicating that one of the alloc hooks have already been
+// entered. Used to handle recursive hook calls. Anything allocated when this
+// flag is set should also be freed when this flag is set.
+base::LazyInstance<base::ThreadLocalBoolean>::Leaky g_entered_hook_tls;
+
+}  // namespace
+
 LeakDetector::LeakReport::LeakReport() {}
 
 LeakDetector::LeakReport::~LeakReport() {}
 
-LeakDetector::LeakDetector(float sampling_rate,
-                           size_t max_call_stack_unwind_depth,
-                           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.
+// static
+LeakDetector* LeakDetector::GetInstance() {
+  return &g_instance.Get();

[Will Harris, 2016/03/15 04:13:35]

.Pointer()

[Simon Que, 2016/03/15 06:39:31]

Done.

 }
 
-LeakDetector::~LeakDetector() {}
+void LeakDetector::Init(float sampling_rate,
+                        size_t max_call_stack_unwind_depth,
+                        uint64_t analysis_interval_bytes,
+                        uint32_t size_suspicion_threshold,
+                        uint32_t call_stack_suspicion_threshold) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(sampling_rate > 0) << "Sampling rate cannot be zero or negative.";
+
+  sampling_factor_ = base::saturated_cast<uint64_t>(sampling_rate * UINT64_MAX);
+
+  analysis_interval_bytes_ = analysis_interval_bytes;
+  max_call_stack_unwind_depth_ = max_call_stack_unwind_depth;
+
+  MappingInfo mapping = {0};
+#if defined(OS_CHROMEOS)
+  // Locate the Chrome binary mapping info.
+  dl_iterate_phdr(IterateLoadedObjects, &mapping);
+#endif  // defined(OS_CHROMEOS)
+
+  // CustomAllocator can use the default allocator, as long as the hook
+  // functions can handle recursive calls.
+  leak_detector::CustomAllocator::Initialize();
+
+  // The initialization should be done only once. Check for this by examining
+  // whether |impl_| has already been initialized.
+  CHECK(!impl_.get()) << "Cannot initialize LeakDetector more than once!";
+  impl_.reset(
+      new leak_detector::LeakDetectorImpl(mapping.addr, mapping.size,

[Will Harris, 2016/03/15 04:13:35]

mapping.addr and mapping.size might be 0 here, should the leak detector still be
instantiated in that case?

[Simon Que, 2016/03/15 06:39:31]

Yes. If that's the case, call stacks in leak reports will simply have the
original addresses, not offsets.

+                                          size_suspicion_threshold,
+                                          call_stack_suspicion_threshold));
+
+  // Register allocator hook functions.
+  base::allocator::SetHooks(&AllocHook, &FreeHook);
+}
 
 void LeakDetector::AddObserver(Observer* observer) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  base::AutoLock lock(observers_lock_);
   observers_.AddObserver(observer);
 }
 
 void LeakDetector::RemoveObserver(Observer* observer) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  base::AutoLock lock(observers_lock_);
   observers_.RemoveObserver(observer);
 }
 
+LeakDetector::LeakDetector() : total_alloc_size_(0),
+                               last_analysis_alloc_size_(0),
+                               analysis_interval_bytes_(0),
+                               max_call_stack_unwind_depth_(0),
+                               sampling_factor_(0) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+}
+
+LeakDetector::~LeakDetector() {}
+
+// static
+void LeakDetector::AllocHook(const void* ptr, size_t size) {
+  base::ThreadLocalBoolean& entered_hook = g_entered_hook_tls.Get();
+  if (entered_hook.Get())
+    return;
+
+  LeakDetector* detector = &g_instance.Get();

[Will Harris, 2016/03/15 04:13:35]

.Pointer()

[Simon Que, 2016/03/15 06:39:31]

Done.

+
+  // Add the allocation size to the total size across all threads. This should
+  // be done before sampling because it is more accurate than attempting to
+  // extrapolate it from the post-sampling total size.
+  detector->alloc_size_lock_.Acquire();
+  detector->total_alloc_size_ += size;
+  detector->alloc_size_lock_.Release();
+
+  if (!detector->ShouldSample(ptr))
+    return;
+
+  entered_hook.Set(true);
+
+  // Get stack trace if necessary.
+  std::vector<void*> stack;
+  int depth = 0;
+  if (detector->impl_->ShouldGetStackTraceForSize(size)) {
+    stack.resize(detector->max_call_stack_unwind_depth_);
+    depth = base::allocator::GetCallStack(stack.data(), stack.size());
+  }
+
+  detector->impl_lock_.Acquire();

[Will Harris, 2016/03/15 04:13:35]

what is the performance hit like for having a lock around every alloc and free?

[Simon Que, 2016/03/15 06:39:31]

The overhead of the leak detector at 1/256 sampling is 2.0%. This is designed
for clarity over performance. I do plan to optimize it in a later CL.

I've measured some optimizations and have the overhead at 1.1% per process. It
may be possible to optimize it even more.

+  detector->impl_->RecordAlloc(ptr, size, depth, stack.data());
+  detector->impl_lock_.Release();
+
+  {
+    // The internal memory of |stack| should be freed before setting
+    // |entered_hook| to false at the end of this function. Free it here by
+    // moving the internal memory to a temporary variable that will go out of
+    // scope.
+    std::vector<void*> dummy_stack;
+    dummy_stack.swap(stack);
+  }
+
+  // Check for leaks after |analysis_interval_bytes_| bytes have been
+  // allocated since the last time that was done.
+  const auto& total_alloc_size = detector->total_alloc_size_;
+  const auto& analysis_interval_bytes = detector->analysis_interval_bytes_;
+  if (total_alloc_size >
+      detector->last_analysis_alloc_size_ + analysis_interval_bytes) {
+    // Try to maintain regular intervals of size |analysis_interval_bytes_|.
+    detector->alloc_size_lock_.Acquire();
+    detector->last_analysis_alloc_size_ =
+        total_alloc_size - total_alloc_size % analysis_interval_bytes;
+    detector->alloc_size_lock_.Release();
+
+    InternalVector<InternalLeakReport> leak_reports;
+    detector->impl_lock_.Acquire();
+    detector->impl_->TestForLeaks(&leak_reports);
+    detector->impl_lock_.Release();
+
+    // Pass leak reports to observers.
+    std::vector<LeakReport> leak_reports_for_observers;
+    GetReportsForObservers(leak_reports, &leak_reports_for_observers);

[Will Harris, 2016/03/15 04:13:35]

two threads can call GetReportsForObservers at the same time, I think?

[Simon Que, 2016/03/15 06:39:31]

Both arguments to this function are local. tcmalloc itself is handling the heap
allocation of |leak_reports_for_observers|. Is there another safety issue?

[Will Harris, 2016/03/15 06:49:42]

yes sorry you're right, I did not see that GetReportsForObservers was in anon
namespace.

+    detector->NotifyObservers(leak_reports_for_observers);
+  }
+
+  entered_hook.Set(false);
+}
+
+// static
+void LeakDetector::FreeHook(const void* ptr) {
+  LeakDetector* detector = &g_instance.Get();

[Will Harris, 2016/03/15 04:13:35]

.Pointer()

[Simon Que, 2016/03/15 06:39:31]

Done.

+  if (!detector->ShouldSample(ptr))
+    return;
+
+  base::ThreadLocalBoolean& entered_hook = g_entered_hook_tls.Get();
+  if (entered_hook.Get())
+    return;
+
+  entered_hook.Set(true);
+
+  detector->impl_lock_.Acquire();
+  detector->impl_->RecordFree(ptr);
+  detector->impl_lock_.Release();
+
+  entered_hook.Set(false);
+}
+
+inline bool LeakDetector::ShouldSample(const void* ptr) const {
+  return PointerToHash(ptr) < sampling_factor_;
+}
+
 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,
-        base::Bind(&LeakDetector::NotifyObservers, weak_factory_.GetWeakPtr(),
+        base::Bind(&LeakDetector::NotifyObservers, base::Unretained(this),
                    reports));
     return;
   }
 
   for (const LeakReport& report : reports) {
+    base::AutoLock lock(observers_lock_);
     FOR_EACH_OBSERVER(Observer, observers_, OnLeakFound(report));
   }
 }