[Mac] Capture -dealloc backtrace to log with CrZombie messages.

chrome/browser/ui/cocoa/objc_zombie.mm

Index: chrome/browser/ui/cocoa/objc_zombie.mm
diff --git a/chrome/browser/ui/cocoa/objc_zombie.mm b/chrome/browser/ui/cocoa/objc_zombie.mm
index 3e097363e031ed24c1ce2f58e37655ba15e56c52..7acb8a27767e7cde94b4694cf254a8d1e06b00d6 100644
--- a/chrome/browser/ui/cocoa/objc_zombie.mm
+++ b/chrome/browser/ui/cocoa/objc_zombie.mm
@@ -5,11 +5,13 @@
 #import "chrome/browser/ui/cocoa/objc_zombie.h"
 
 #include <dlfcn.h>
+#include <execinfo.h>
 #include <mach-o/dyld.h>
 #include <mach-o/nlist.h>
 
 #import <objc/objc-class.h>
 
+#include "base/debug/stack_trace.h"
 #include "base/logging.h"
 #include "base/mac/mac_util.h"
 #include "base/metrics/histogram.h"
@@ -35,6 +37,12 @@
 
 namespace {
 
+// The depth of backtrace to store with zombies.  This directly influences
+// the amount of memory required to track zombies, so should be kept as
+// small as is useful.  Unfortunately, too small and it won't poke through
+// deep autorelease and event loop stacks.
+const int kBacktraceDepth = 20;
+
 // Function which destroys the contents of an object without freeing
 // the object.  On 10.5 this is |object_cxxDestruct()|, which
 // traverses the class hierarchy to run the C++ destructors.  On 10.6
@@ -71,6 +79,8 @@ size_t g_zombieIndex = 0;
 typedef struct {
   id object;   // The zombied object.
   Class wasa;  // Value of |object->isa| before we replaced it.
+  void* trace[kBacktraceDepth];  // Backtrace at point of deallocation.
+  int traceDepth;                // Actual depth of trace[].
 } ZombieRecord;
 
 ZombieRecord* g_zombies = NULL;
@@ -192,6 +202,7 @@ void ZombieDealloc(id self, SEL _cmd) {
   // The new record to swap into |g_zombies|.  If |g_zombieCount| is
   // zero, then |self| will be freed immediately.
   ZombieRecord zombieToFree = {self, wasa};
+  zombieToFree.traceDepth = backtrace(zombieToFree.trace, kBacktraceDepth);

[Mark Mentovai, 2011/08/28 15:40:01]

Caution!

backtrace may not be implemented in a way that’s able to walk through stack
frames created by code built with -fomit-frame-pointer. We’re not currently
using -fomit-frame-pointer because performance testing showed no real advantage
last time I tried it, but given the short-term compiler switch and other plans
that may come together in the medium or long term, if ongoing testing showed a
performance advantage from -fomit-frame-pointer, I wouldn’t consider this use of
backtrace something that would block turning -fomit-frame-pointer on for release
builds.

This isn’t worth worrying about now and probably isn’t worth worrying about in
the future either, but it is worth being aware of.

[Scott Hess - ex-Googler, 2011/08/29 20:13:36]

Hmm.  That perhaps argues for taking the overhead of declaring a StackTrace()
instance and calling it's API to grab a subset of the backtrace.

[Later]  Unscientific profiling indicates that at 40000 zombies straight
backtrace() adds 13ms of overhead, while a StackTrace() instance adds 20ms. 
Those are in release mode (debug mode was 17ms and 30ms).  So I'll include a
version using StackTrace(), as a separate patch in case you want to question it.

 
   // Don't involve the lock when creating zombies without a treadmill.
   if (g_zombieCount > 0) {
@@ -213,23 +224,28 @@ void ZombieDealloc(id self, SEL _cmd) {
     object_dispose(zombieToFree.object);
 }
 
-// Attempt to determine the original class of zombie |object|.
-Class ZombieWasa(id object) {
-  // Fat zombies can hold onto their |wasa| past the point where the
-  // object was actually freed.  Note that to arrive here at all,
-  // |object|'s memory must still be accessible.
-  if (object_getClass(object) == g_fatZombieClass)
-    return static_cast<CrFatZombie*>(object)->wasa;
-
-  // For instances which weren't big enough to store |wasa|, check if
-  // the object is still on the treadmill.
+// Search the treadmill for |object| and fill in |*record| if found.
+// Returns YES if found.
+BOOL GetZombieRecord(id object, ZombieRecord* record) {
+  // Holding the lock is reasonable because this should be fast, and
+  // the process is going to crash presently anyhow.
   base::AutoLock pin(lock_);
   for (size_t i=0; i < g_zombieCount; ++i) {

[Mark Mentovai, 2011/08/28 15:40:01]

Style nit (existing code): spaces around the =.

[Scott Hess - ex-Googler, 2011/08/29 20:13:36]

Done.

-    if (g_zombies[i].object == object)
-      return g_zombies[i].wasa;
+    if (g_zombies[i].object == object) {
+      *record = g_zombies[i];
+      return YES;
+    }
   }
+  return NO;
+}
+
+// Dump the symbols.  This is pulled out into a function to make it
+// easy to use DCHECK to dump only in debug builds.
+BOOL DumpDeallocTrace(const void* const* array, int size) {
+  DLOG(INFO) << "Backtrace from -dealloc:";

[Mark Mentovai, 2011/08/28 15:40:01]

base::debug::StackTrace::PrintBacktrace outputs to std::cerr. Since this message
will identify the trace that follows, maybe it should go to std::cerr (or
fprintf(stderr)) instead of DLOG(INFO) as well.

[Scott Hess - ex-Googler, 2011/08/29 20:13:36]

Done.  But it feels funny.

+  base::debug::StackTrace(array, size).PrintBacktrace();
 
-  return Nil;
+  return YES;
 }
 
 // Log a message to a freed object.  |wasa| is the object's original
@@ -238,8 +254,19 @@ Class ZombieWasa(id object) {
 // dispatch-related method which is being invoked to send |aSelector|
 // (for instance, -respondsToSelector:).
 void ZombieObjectCrash(id object, SEL aSelector, SEL viaSelector) {
-  Class wasa = ZombieWasa(object);
+  ZombieRecord record;
+  BOOL found = GetZombieRecord(object, &record);
+
+  // The object's class can be in the zombie record, but if that is
+  // not available it can also be in the object itself (in most cases).
+  Class wasa = Nil;
+  if (found) {
+    wasa = record.wasa;
+  } else if (object_getClass(object) == g_fatZombieClass) {
+    wasa = static_cast<CrFatZombie*>(object)->wasa;
+  }
   const char* wasaName = (wasa ? class_getName(wasa) : "<unknown>");
+
   NSString* aString =
       [NSString stringWithFormat:@"Zombie <%s: %p> received -%s",
                                  wasaName, object, sel_getName(aSelector)];
@@ -248,12 +275,33 @@ void ZombieObjectCrash(id object, SEL aSelector, SEL viaSelector) {
     aString = [aString stringByAppendingFormat:@" (via -%s)", viaName];
   }
 
-  // Set a value for breakpad to report, then crash.
+  // Set a value for breakpad to report.
   SetCrashKeyValue(@"zombie", aString);
-  LOG(ERROR) << [aString UTF8String];
+
+  // Hex-encode the backtrace and tuck it into a breakpad key.
+  NSString* deallocTrace = @"<unknown>";
+  if (found && record.traceDepth > 0) {
+    NSMutableArray* hexBacktrace =
+        [NSMutableArray arrayWithCapacity:record.traceDepth];
+    for (int i = 0; i < record.traceDepth; ++i) {
+      NSString* s = [NSString stringWithFormat:@"%p", record.trace[i]];
+      [hexBacktrace addObject:s];
+    }
+    deallocTrace = [hexBacktrace componentsJoinedByString:@" "];
+  }
+  SetCrashKeyValue(@"zombie_dealloc_bt", deallocTrace);

[Mark Mentovai, 2011/08/28 15:40:01]

Looks like Breakpad keys and values have a maximum byte length of 255 with the
current implementation. For 32-bit pointers, this string will have a maximum
length of 219. This is 100% fine. I’m just pointing it out because:

1. If we do a 64-bit thing, the string’s maximum length of 379 would pose a
problem here.

2. If we want kBacktraceDepth to be more than 23, we’d need to be aware of this
problem too.

It’s possible to do a better job with a more compact encoding, or letting the
trace string spill out across multiple Breakpad key-value pairs. None of this is
necessary at this point. In fact, please don’t. But it’s probably worth
commenting on. The comment may be better placed up where we define
kBacktraceDepth than being hidden here.

[Scott Hess - ex-Googler, 2011/08/29 20:13:36]

I added some documentation around this to the kBacktraceDepth declaraction.  I
agree that we can worry about this later - perhaps after we determine if this is
even useful :-).

+
+  // Log -dealloc backtrace in debug builds then crash with a useful
+  // stack trace.
+  if (found && record.traceDepth) {

[Mark Mentovai, 2011/08/28 15:40:01]

For consistency’s sake, you checked |traceDepth > 0| on line 283 but just
|traceDepth| here. They should match.

|>0| is correct. traceDepth contains the result of the backtrace call. I don’t
see any way for it to return negative right now based on the read of the current
implementation, but it is a (signed) int, so the possibility technically exists,
and using a >0 check here wont’t harm anything.

[Scott Hess - ex-Googler, 2011/08/29 20:13:36]

Done.

+    DCHECK(DumpDeallocTrace(record.trace, record.traceDepth));
+  } else {
+    DLOG(INFO) << "Unable to generate backtrace from -dealloc.";
+  }
+  DLOG(FATAL) << [aString UTF8String];
 
   // This is how about:crash is implemented.  Using instead of
-  // |baes::debug::BreakDebugger()| or |LOG(FATAL)| to make the top of
+  // |base::debug::BreakDebugger()| or |LOG(FATAL)| to make the top of
   // stack more immediately obvious in crash dumps.
   int* zero = NULL;
   *zero = 0;