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

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

Logs a subset of the stack backtrace from the point of -dealloc into a
breakpad key to help debug messages to CrZombie objects.  This
productionizes the basic approach taken to debug a few recent zombie
object crashes.

BUG=none
TEST=monitor crash server.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98858

[Scott Hess - ex-Googler, 2011-08-26 22:50:05]

Happy to take whoever isn't currently being blown about like Dorothy.

I tested this by injecting code to dladdr() the trace[] items and print the
dli_fname and dli_fbase, then fed that to atos -s (my atos is ancient and has no
-l), and it worked.  This'll warrant some sort of script to scrape info from
minidumps and symbolize, but I'm inclined to let that wait for live crashes to
debug.

[Scott Hess - ex-Googler, 2011-08-26 22:54:37]

On 2011/08/26 22:50:05, shess wrote:
> Happy to take whoever isn't currently being blown about like Dorothy.
> 
> I tested this by injecting code to dladdr() the trace[] items and print the
> dli_fname and dli_fbase, then fed that to atos -s (my atos is ancient and has
no
> -l), and it worked.  This'll warrant some sort of script to scrape info from
> minidumps and symbolize, but I'm inclined to let that wait for live crashes to
> debug.

Additionally, I instrumented the code to use base::TimeTicks to aggregate how
long backtrace() was taking.  After 40,000 zombies it indicated 14ms.  This was
a debug build, but that probably shouldn't make a huge difference in this case.

[Of course, the additional memory will also have a slight cost, but that's hard
to measure.]

[Mark Mentovai, 2011-08-27 00:29:02]

http://codereview.chromium.org/7766013/diff/1/base/debug/stack_trace.cc
File base/debug/stack_trace.cc (right):

http://codereview.chromium.org/7766013/diff/1/base/debug/stack_trace.cc#newco...
base/debug/stack_trace.cc:12: memcpy(trace_, trace, count * sizeof(trace[0]));
This is very dangerous. trace_ only has room for kMaxTraces pointers.

http://codereview.chromium.org/7766013/diff/1/base/debug/stack_trace.h
File base/debug/stack_trace.h (right):

http://codereview.chromium.org/7766013/diff/1/base/debug/stack_trace.h#newcode31
base/debug/stack_trace.h:31: StackTrace(const void* const* trace, int count);
Since this is part of the public interface now, you kind of need to say what the
trace argument is all about. I can see from the implementation file that it’s
dumped right into trace_, but that doesn’t really help someone who doesn’t know
any better because the contents of trace_ isn’t defined below other than to say
that it’s an array of up to kMaxTraces pointers.

http://codereview.chromium.org/7766013/diff/1/chrome/browser/ui/cocoa/objc_zo...
File chrome/browser/ui/cocoa/objc_zombie.mm (right):

http://codereview.chromium.org/7766013/diff/1/chrome/browser/ui/cocoa/objc_zo...
chrome/browser/ui/cocoa/objc_zombie.mm:44: static const int kBacktraceDepth =
20;
Since you’re in an anonymous namespace, the |static| is extraneous.

http://codereview.chromium.org/7766013/diff/1/chrome/browser/ui/cocoa/objc_zo...
chrome/browser/ui/cocoa/objc_zombie.mm:82: void* trace[kBacktraceDepth];  //
Backtrace at point of deallocation.
Wow, zombie memory explosion! How many zombies do we let hang around, again?

http://codereview.chromium.org/7766013/diff/1/chrome/browser/ui/cocoa/objc_zo...
chrome/browser/ui/cocoa/objc_zombie.mm:203: ZombieRecord zombieToFree = {self,
wasa, {0}};
This zeroes the whole trace array out. Do we need that?

http://codereview.chromium.org/7766013/diff/1/chrome/browser/ui/cocoa/objc_zo...
chrome/browser/ui/cocoa/objc_zombie.mm:204: backtrace(zombieToFree.trace,
kBacktraceDepth);
I’d feel much more comfortable if you stored the return value of backtrace (the
number of frames) rather than guessing at it elsewhere. You’re already using 80
extra bytes (32-bit) for the backtrace, what’s 4 more?

http://codereview.chromium.org/7766013/diff/1/chrome/browser/ui/cocoa/objc_zo...
chrome/browser/ui/cocoa/objc_zombie.mm:285: NSString* deallocTrace =
@"<unknown>";
Nit: remember to use this_style names because you’re not in an Objective-C
@implementation or @interface or @protocol.

http://codereview.chromium.org/7766013/diff/1/chrome/browser/ui/cocoa/objc_zo...
chrome/browser/ui/cocoa/objc_zombie.mm:289: while (realDepth > 0 &&
!record.trace[realDepth - 1]) {
This is what I was talking about before. It’s kind of cheesey.

http://codereview.chromium.org/7766013/diff/1/chrome/browser/ui/cocoa/objc_zo...
chrome/browser/ui/cocoa/objc_zombie.mm:303:
SetCrashKeyValue(@"zombie_dealloc_bt", deallocTrace);
Might we also want to record the thread that did the -dealloc?

http://codereview.chromium.org/7766013/diff/1/chrome/browser/ui/cocoa/objc_zo...
chrome/browser/ui/cocoa/objc_zombie.mm:308: DLOG(FATAL) << [aString UTF8String];
This won’t get a chance to DLOG(FATAL) because the DCHECK is already fatal. (Or
am I wrong?)

[Scott Hess - ex-Googler, 2011-08-27 05:15:40]

Changes and a hang-up at the end.

http://codereview.chromium.org/7766013/diff/1/base/debug/stack_trace.cc
File base/debug/stack_trace.cc (right):

http://codereview.chromium.org/7766013/diff/1/base/debug/stack_trace.cc#newco...
base/debug/stack_trace.cc:12: memcpy(trace_, trace, count * sizeof(trace[0]));
On 2011/08/27 00:29:02, Mark Mentovai wrote:
> This is very dangerous. trace_ only has room for kMaxTraces pointers.

Oops, capped that.

http://codereview.chromium.org/7766013/diff/1/base/debug/stack_trace.h
File base/debug/stack_trace.h (right):

http://codereview.chromium.org/7766013/diff/1/base/debug/stack_trace.h#newcode31
base/debug/stack_trace.h:31: StackTrace(const void* const* trace, int count);
On 2011/08/27 00:29:02, Mark Mentovai wrote:
> Since this is part of the public interface now, you kind of need to say what
the
> trace argument is all about. I can see from the implementation file that it’s
> dumped right into trace_, but that doesn’t really help someone who doesn’t
know
> any better because the contents of trace_ isn’t defined below other than to
say
> that it’s an array of up to kMaxTraces pointers.

OK, I'll document it as accepting an array of instruction pointers such as
returned by Addresses().

Given that, would it make sense to have the zombie code generate an instance and
store results from Addresses(), or to continue using backtrace() directly?  I'm
a bit paranoid about overhead, but I find that a hard decision (it's called a
lot, arguing for low overhead, and this class of code is hardly abstract in the
first place).

http://codereview.chromium.org/7766013/diff/1/chrome/browser/ui/cocoa/objc_zo...
File chrome/browser/ui/cocoa/objc_zombie.mm (right):

http://codereview.chromium.org/7766013/diff/1/chrome/browser/ui/cocoa/objc_zo...
chrome/browser/ui/cocoa/objc_zombie.mm:44: static const int kBacktraceDepth =
20;
On 2011/08/27 00:29:02, Mark Mentovai wrote:
> Since you’re in an anonymous namespace, the |static| is extraneous.

Done.  Doh.

http://codereview.chromium.org/7766013/diff/1/chrome/browser/ui/cocoa/objc_zo...
chrome/browser/ui/cocoa/objc_zombie.mm:82: void* trace[kBacktraceDepth];  //
Backtrace at point of deallocation.
On 2011/08/27 00:29:02, Mark Mentovai wrote:
> Wow, zombie memory explosion! How many zombies do we let hang around, again?

10,000 at this time.  From past instrumentation of things, this seems to wire
down about 3M.  This change will add a bit under 1M additional, but the constant
can easily be tuned down if we find that the extra frames aren't providing solid
benefits.  The current depth was chosen by intuition from triaging crashes - 5
is too small, 10 feels borderline, 20 is probably too much, but not way too
much.

http://codereview.chromium.org/7766013/diff/1/chrome/browser/ui/cocoa/objc_zo...
chrome/browser/ui/cocoa/objc_zombie.mm:203: ZombieRecord zombieToFree = {self,
wasa, {0}};
On 2011/08/27 00:29:02, Mark Mentovai wrote:
> This zeroes the whole trace array out. Do we need that?

We did if I wasn't keeping the count :-).

http://codereview.chromium.org/7766013/diff/1/chrome/browser/ui/cocoa/objc_zo...
chrome/browser/ui/cocoa/objc_zombie.mm:204: backtrace(zombieToFree.trace,
kBacktraceDepth);
On 2011/08/27 00:29:02, Mark Mentovai wrote:
> I’d feel much more comfortable if you stored the return value of backtrace
(the
> number of frames) rather than guessing at it elsewhere. You’re already using
80
> extra bytes (32-bit) for the backtrace, what’s 4 more?

OK.  I was mainly thinking in terms of whether we might set the depth to 5 (or
0), but it's reasonable to worry about that when it happens.  I had also
considered whether we might store the backtraces as a separate circular buffer
from the objects, so that it could contain info for fewer zombies, but that
feels kind of scary for now.

http://codereview.chromium.org/7766013/diff/1/chrome/browser/ui/cocoa/objc_zo...
chrome/browser/ui/cocoa/objc_zombie.mm:285: NSString* deallocTrace =
@"<unknown>";
On 2011/08/27 00:29:02, Mark Mentovai wrote:
> Nit: remember to use this_style names because you’re not in an Objective-C
> @implementation or @interface or @protocol.

OK ... as-is, the file is pretty much Objc-style.  The _ are from the runtime
API and g_.  Not sure where to take that.

http://codereview.chromium.org/7766013/diff/1/chrome/browser/ui/cocoa/objc_zo...
chrome/browser/ui/cocoa/objc_zombie.mm:289: while (realDepth > 0 &&
!record.trace[realDepth - 1]) {
On 2011/08/27 00:29:02, Mark Mentovai wrote:
> This is what I was talking about before. It’s kind of cheesey.

It's totally cheesy!  Be happy I didn't try to delta-encode the EIP values :-).

http://codereview.chromium.org/7766013/diff/1/chrome/browser/ui/cocoa/objc_zo...
chrome/browser/ui/cocoa/objc_zombie.mm:303:
SetCrashKeyValue(@"zombie_dealloc_bt", deallocTrace);
On 2011/08/27 00:29:02, Mark Mentovai wrote:
> Might we also want to record the thread that did the -dealloc?

Hmm.  Hmmmmmmm.  I think I'm willing to leave that for later.  There are some
cases of zombies on other threads, but I can't remember a case where crossing
threads was really relevant to the problem.  That said, pretty much all the
problems we've seen were on the UI thread.

http://codereview.chromium.org/7766013/diff/1/chrome/browser/ui/cocoa/objc_zo...
chrome/browser/ui/cocoa/objc_zombie.mm:308: DLOG(FATAL) << [aString UTF8String];
On 2011/08/27 00:29:02, Mark Mentovai wrote:
> This won’t get a chance to DLOG(FATAL) because the DCHECK is already fatal.
(Or
> am I wrong?)

DumpDeallocTrace() returns YES.  What I want is to say "Do this in debug build,
but get rid of it all in production build" without having to replicate the logic
to do that (and take the chance that the logic would not match the reality). 
This way I'm relying on people who change the logging code consistently changing
everything.

http://codereview.chromium.org/7766013/diff/5001/base/debug/stack_trace.cc
File base/debug/stack_trace.cc (right):

http://codereview.chromium.org/7766013/diff/5001/base/debug/stack_trace.cc#ne...
base/debug/stack_trace.cc:14: count = std::min(count, kMaxTraces);
OK, so weird one ... on my 10.6 machine at work running xcode 3.2.1 (yes,
ancient), this compiles.  On my home 10.6 machine running xcode 3.2.6, I get:

Undefined symbols:
  "base::debug::StackTrace::kMaxTraces", referenced from:
      __ZN4base5debug10StackTrace10kMaxTracesE$non_lazy_ptr in
libbase.a(stack_trace.o)
     (maybe you meant: __ZN4base5debug10StackTrace10kMaxTracesE$non_lazy_ptr)
ld: symbol(s) not found

The ways I've worked around it thus far:

 - count = std::min(count, static_cast<int>(kMaxTraces));
 - const int i = kMaxTraces; count = std::min(count, i);
 - if (count > kMaxTraces) count = kMaxTraces;

My best guess is that as coded, the compiler requires a reference to real
memory, which isn't present anywhere.  The work-arounds allow the value to be
inlined everywhere.  Since it's used to size the array, I can't just move the
definition into the implementation file.

There is one more way to solve it:
 - do min(sizeof(trace_), count * sizeof(trace[0])) for memcpy.

This seems least sketchy, as it directly targets the goal (don't overrun
memory!), but I wanted to throw up this discussion in case I've forgotten a C++
case that easily resolves it.

[Scott Hess - ex-Googler, 2011-08-27 05:44:49]

http://codereview.chromium.org/7766013/diff/5001/base/debug/stack_trace.cc
File base/debug/stack_trace.cc (right):

http://codereview.chromium.org/7766013/diff/5001/base/debug/stack_trace.cc#ne...
base/debug/stack_trace.cc:14: count = std::min(count, kMaxTraces);
On 2011/08/27 05:15:40, shess wrote:
> OK, so weird one ... on my 10.6 machine at work running xcode 3.2.1 (yes,
> ancient), this compiles.  On my home 10.6 machine running xcode 3.2.6, I get:
> 
> Undefined symbols:
>   "base::debug::StackTrace::kMaxTraces", referenced from:
>       __ZN4base5debug10StackTrace10kMaxTracesE$non_lazy_ptr in
> libbase.a(stack_trace.o)
>      (maybe you meant: __ZN4base5debug10StackTrace10kMaxTracesE$non_lazy_ptr)
> ld: symbol(s) not found
> 
> The ways I've worked around it thus far:
> 
>  - count = std::min(count, static_cast<int>(kMaxTraces));
>  - const int i = kMaxTraces; count = std::min(count, i);
>  - if (count > kMaxTraces) count = kMaxTraces;
> 
> My best guess is that as coded, the compiler requires a reference to real
> memory, which isn't present anywhere.  The work-arounds allow the value to be
> inlined everywhere.  Since it's used to size the array, I can't just move the
> definition into the implementation file.
> 
> There is one more way to solve it:
>  - do min(sizeof(trace_), count * sizeof(trace[0])) for memcpy.
> 
> This seems least sketchy, as it directly targets the goal (don't overrun
> memory!), but I wanted to throw up this discussion in case I've forgotten a
C++
> case that easily resolves it.

http://en.wikipedia.org/wiki/One_Definition_Rule#Definitions_of_static_const_...
Seems to describe this exact case.  I think.  Of the first workarounds only the
second looks really valid per that description, but the sizeof(trace_)
workaround would still seem to target the goal well, so I'm putting that up.

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

LGTM.

Most of the comments are more “food for thought” than anything else.

http://codereview.chromium.org/7766013/diff/5001/base/debug/stack_trace.cc
File base/debug/stack_trace.cc (right):

http://codereview.chromium.org/7766013/diff/5001/base/debug/stack_trace.cc#ne...
base/debug/stack_trace.cc:14: count = std::min(count, kMaxTraces);
I know that you found an answer to your question, but I think that what’s really
going on here is that std::min accepts &reference arguments, and there isn’t
necessarily anything for the reference to refer to when you’re using an integral
static const member variable.

Yes, this is kind of stupid for integral const &references. The compiler COULD
just spit out an int where it needs it and refer to it. But since it isn’t
(wasn’t?) required by the standard, I guess it just wasn’t ever done. Compilers
are actually more lenient in this area than the standard allows them to be, and
a couple of your proposed workarounds took advantage of that fact. I think the
workaround you ultimately used is cleanest, though. And it doesn’t violate the
standard, to boot.

http://codereview.chromium.org/7766013/diff/8001/base/debug/stack_trace.cc
File base/debug/stack_trace.cc (right):

http://codereview.chromium.org/7766013/diff/8001/base/debug/stack_trace.cc#ne...
base/debug/stack_trace.cc:7: #include <algorithm>
C system headers before C++ system headers: <string.h> before <algorithm> with a
blank line between them.

http://codereview.chromium.org/7766013/diff/8001/chrome/browser/ui/cocoa/objc...
File chrome/browser/ui/cocoa/objc_zombie.mm (right):

http://codereview.chromium.org/7766013/diff/8001/chrome/browser/ui/cocoa/objc...
chrome/browser/ui/cocoa/objc_zombie.mm:205: zombieToFree.traceDepth =
backtrace(zombieToFree.trace, kBacktraceDepth);
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.

http://codereview.chromium.org/7766013/diff/8001/chrome/browser/ui/cocoa/objc...
chrome/browser/ui/cocoa/objc_zombie.mm:215: std::swap(zombieToFree,
g_zombies[g_zombieIndex]);
As ZombieRecord grows, it may begin to make sense to build it in the g_zombies
array rather than building it separately and having to do all of these copies to
effect a swap.

http://codereview.chromium.org/7766013/diff/8001/chrome/browser/ui/cocoa/objc...
chrome/browser/ui/cocoa/objc_zombie.mm:233: for (size_t i=0; i < g_zombieCount;
++i) {
Style nit (existing code): spaces around the =.

http://codereview.chromium.org/7766013/diff/8001/chrome/browser/ui/cocoa/objc...
chrome/browser/ui/cocoa/objc_zombie.mm:245: DLOG(INFO) << "Backtrace from
-dealloc:";
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.

http://codereview.chromium.org/7766013/diff/8001/chrome/browser/ui/cocoa/objc...
chrome/browser/ui/cocoa/objc_zombie.mm:292:
SetCrashKeyValue(@"zombie_dealloc_bt", deallocTrace);
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.

http://codereview.chromium.org/7766013/diff/8001/chrome/browser/ui/cocoa/objc...
chrome/browser/ui/cocoa/objc_zombie.mm:296: if (found && record.traceDepth) {
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]

OK, I folded in some of your food-for-thought, so another pass is in order.  I
could go either way on converting between backtrace() and a StackTrace()
instance - really, the performance impact is so slight, and when measuring it I
was measuring just the capture of the trace, not the increment due to
maintaining the additional ZombieRecord data, so the impact is slightly less
than indicated.

http://codereview.chromium.org/7766013/diff/5001/base/debug/stack_trace.cc
File base/debug/stack_trace.cc (right):

http://codereview.chromium.org/7766013/diff/5001/base/debug/stack_trace.cc#ne...
base/debug/stack_trace.cc:14: count = std::min(count, kMaxTraces);
On 2011/08/28 15:40:01, Mark Mentovai wrote:
> I know that you found an answer to your question, but I think that what’s
really
> going on here is that std::min accepts &reference arguments, and there isn’t
> necessarily anything for the reference to refer to when you’re using an
integral
> static const member variable.
> 
> Yes, this is kind of stupid for integral const &references. The compiler COULD
> just spit out an int where it needs it and refer to it. But since it isn’t
> (wasn’t?) required by the standard, I guess it just wasn’t ever done.
Compilers
> are actually more lenient in this area than the standard allows them to be,
and
> a couple of your proposed workarounds took advantage of that fact. I think the
> workaround you ultimately used is cleanest, though. And it doesn’t violate the
> standard, to boot.

Unfortunately, the code still has to set |count_| to something, so I can't
escape that easily.  I switched it to use min(count, arraysize(trace_)).  Sigh.

http://codereview.chromium.org/7766013/diff/8001/base/debug/stack_trace.cc
File base/debug/stack_trace.cc (right):

http://codereview.chromium.org/7766013/diff/8001/base/debug/stack_trace.cc#ne...
base/debug/stack_trace.cc:7: #include <algorithm>
On 2011/08/28 15:40:01, Mark Mentovai wrote:
> C system headers before C++ system headers: <string.h> before <algorithm> with
a
> blank line between them.

Done.

http://codereview.chromium.org/7766013/diff/8001/base/debug/stack_trace.h
File base/debug/stack_trace.h (right):

http://codereview.chromium.org/7766013/diff/8001/base/debug/stack_trace.h#new...
base/debug/stack_trace.h:63: int count_;
Having this and kMaxTraces be int seems wrong, would you mind if I switch to
size_t while I'm in here?  The code itself seems unclear on the difference
between int and size_t (f/i, Addresses() has if(count_) and has a size_t*
parameter).  I could land it as a followup CL, in the interests of landing the
rest sooner rather than later.

http://codereview.chromium.org/7766013/diff/8001/chrome/browser/ui/cocoa/objc...
File chrome/browser/ui/cocoa/objc_zombie.mm (right):

http://codereview.chromium.org/7766013/diff/8001/chrome/browser/ui/cocoa/objc...
chrome/browser/ui/cocoa/objc_zombie.mm:205: zombieToFree.traceDepth =
backtrace(zombieToFree.trace, kBacktraceDepth);
On 2011/08/28 15:40:01, Mark Mentovai wrote:
> 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.

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.

http://codereview.chromium.org/7766013/diff/8001/chrome/browser/ui/cocoa/objc...
chrome/browser/ui/cocoa/objc_zombie.mm:215: std::swap(zombieToFree,
g_zombies[g_zombieIndex]);
On 2011/08/28 15:40:01, Mark Mentovai wrote:
> As ZombieRecord grows, it may begin to make sense to build it in the g_zombies
> array rather than building it separately and having to do all of these copies
to
> effect a swap.

To my mind thread-safety argues against that.  Or at least sets the bar higher
for where the transition makes sense.  As-is, since it's POD, the swap should be
pretty cheap.  Also note that the swap isn't only getting the new zombie onto
the treadmill, it's pulling an old zombie out for disposal.

http://codereview.chromium.org/7766013/diff/8001/chrome/browser/ui/cocoa/objc...
chrome/browser/ui/cocoa/objc_zombie.mm:233: for (size_t i=0; i < g_zombieCount;
++i) {
On 2011/08/28 15:40:01, Mark Mentovai wrote:
> Style nit (existing code): spaces around the =.

Done.

http://codereview.chromium.org/7766013/diff/8001/chrome/browser/ui/cocoa/objc...
chrome/browser/ui/cocoa/objc_zombie.mm:245: DLOG(INFO) << "Backtrace from
-dealloc:";
On 2011/08/28 15:40:01, Mark Mentovai wrote:
> 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.

Done.  But it feels funny.

http://codereview.chromium.org/7766013/diff/8001/chrome/browser/ui/cocoa/objc...
chrome/browser/ui/cocoa/objc_zombie.mm:292:
SetCrashKeyValue(@"zombie_dealloc_bt", deallocTrace);
On 2011/08/28 15:40:01, Mark Mentovai wrote:
> 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.

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 :-).

http://codereview.chromium.org/7766013/diff/8001/chrome/browser/ui/cocoa/objc...
chrome/browser/ui/cocoa/objc_zombie.mm:296: if (found && record.traceDepth) {
On 2011/08/28 15:40:01, Mark Mentovai wrote:
> 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.

Done.

[Scott Hess - ex-Googler, 2011-08-29 20:30:49]

http://codereview.chromium.org/7766013/diff/13001/chrome/browser/ui/cocoa/obj...
File chrome/browser/ui/cocoa/objc_zombie.mm (right):

http://codereview.chromium.org/7766013/diff/13001/chrome/browser/ui/cocoa/obj...
chrome/browser/ui/cocoa/objc_zombie.mm:216: zombieToFree.traceDepth *
sizeof(zombieToFree.trace[0]));
I'm already looking at this and thinking that if the implementation using a
StackTrace implementation is desired, it would be reasonable to convert the
swap() into a manual operation to remove the additional memcpy().

[Mark Mentovai, 2011-08-29 20:37:02]

I would LGTM this. I don’t think I really have a strong preference between
StackTrace and backtrace.

http://codereview.chromium.org/7766013/diff/13001/chrome/browser/ui/cocoa/obj...
File chrome/browser/ui/cocoa/objc_zombie.mm (right):

http://codereview.chromium.org/7766013/diff/13001/chrome/browser/ui/cocoa/obj...
chrome/browser/ui/cocoa/objc_zombie.mm:214: if (zombieToFree.traceDepth)
{braces}

http://codereview.chromium.org/7766013/diff/13001/chrome/browser/ui/cocoa/obj...
chrome/browser/ui/cocoa/objc_zombie.mm:216: zombieToFree.traceDepth *
sizeof(zombieToFree.trace[0]));
shess wrote:
> I'm already looking at this and thinking that if the implementation using a
> StackTrace implementation is desired, it would be reasonable to convert the
> swap() into a manual operation to remove the additional memcpy().

Yup.

[Scott Hess - ex-Googler, 2011-08-29 21:53:04]

On 2011/08/29 20:37:02, Mark Mentovai wrote:
> I would LGTM this. I don’t think I really have a strong preference between
> StackTrace and backtrace.

OK, in the interests of landing rather than accumulating "One more thing", I'm
going to revert to the backtrace() implementation, and get it into the canary.