metrics: Connect leak detector to allocator

components/metrics/leak_detector/leak_detector.cc

 // Copyright 2016 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/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 = reinterpret_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;
+}
+
+// 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.Pointer();
+}
+
+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, size_suspicion_threshold,
+      call_stack_suspicion_threshold));
+
+  // Register allocator hook functions.
+  base::allocator::SetHooks(&AllocHook, &FreeHook);
+}
+
+void LeakDetector::AddObserver(Observer* observer) {
+  base::AutoLock lock(observers_lock_);
+  observers_.AddObserver(observer);
+}
+
+void LeakDetector::RemoveObserver(Observer* observer) {
+  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());

[Primiano Tucci (use gerrit), 2016/03/15 11:49:24]

I don't think this DCHECK can possibly be hit.
the thread_checker_ is intiialized here in the ctor. When constructed, it
automatically sets the TID internally, so this condition will be necessarily
true.

In other words, if you expand the ThreadChecker ctor + this line, this reads as:
thread_checker.valid_thread_id_ = current_id
DCHECK(thread_checker.valid_thread_id == current_tid)

It makes sense, instead, that you keep the check elsewhere

[Simon Que, 2016/03/15 18:33:42]

Done.

 }
 
 LeakDetector::~LeakDetector() {}
 
-void LeakDetector::AddObserver(Observer* observer) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  observers_.AddObserver(observer);
-}
-
-void LeakDetector::RemoveObserver(Observer* observer) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  observers_.RemoveObserver(observer);
+// 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 = GetInstance();
+
+  // 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();

[Primiano Tucci (use gerrit), 2016/03/15 11:49:23]

Just a minor stylistic nit, I usually see this as a forced scope + autolock:
{
  AutoLock lock(detector->alloc_size_lock_);
  detector->total_alloc_size_ += size;
}

this prevents that if somebody touches your code later they don't screw up the
locking (e.g. mistakenly cutting away the release).

ditto below

[Simon Que, 2016/03/15 18:33:42]

Done.

+  detector->total_alloc_size_ += size;
+  detector->alloc_size_lock_.Release();

[Primiano Tucci (use gerrit), 2016/03/15 11:49:24]

Note that you write here the total_alloc_size_ in a lock, but re-read it below
on line 219 outside the lock, which is potentially racy.
I think you want to store the total_alloc_size here within the lock, so
everything stays atomic.

[Simon Que, 2016/03/15 18:33:42]

I'm not that concerned about it since |total_alloc_size_| never decreases.
Nonetheless, I'll include that in the lock.

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

[Primiano Tucci (use gerrit), 2016/03/15 11:49:24]

You have two fine grained locks here (alloc_size_lock_ and impl_lock). I think
you introduced them to reduce contemption, however the length of the critical
section seems minimal for both.
IMHO it would be more readable and also more performant if you had only one
lock, and did everything inside this if under that lock.
Rationale: now, on every malloc that > threshold, you have to [acquire, release,
acquire, release]. If you had only one lock could do that only once. I am pretty
confident that if you profile this most of the time is spent for the
acquire/release themselves, not for the number crunching you do inside :)
But that's really up to you.

[Simon Que, 2016/03/15 18:33:42]

Done.

+
+    // Pass leak reports to observers.
+    std::vector<LeakReport> leak_reports_for_observers;
+    GetReportsForObservers(leak_reports, &leak_reports_for_observers);
+    detector->NotifyObservers(leak_reports_for_observers);
+  }
+
+  entered_hook.Set(false);
+}
+
+// static
+void LeakDetector::FreeHook(const void* ptr) {
+  LeakDetector* detector = GetInstance();
+  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) {

[Primiano Tucci (use gerrit), 2016/03/15 11:49:24]

Suggestion for a future improvement, observer_list_threadsafe.h gives lot of
this threadsafe  + posttask stuff for free :)

[Simon Que, 2016/03/15 18:33:42]

Done. Added a TODO.

+  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));
   }
 }
 
 }  // namespace metrics