[Mac] Implement NSObject zombies.

[Mac] Implement NSObject zombies.

Apple's NSZombieEnabled setting makes it easier to catch messages to
freed objects, but is mostly only useful in debugging environments.
This implements a facility like NSZombieEnabled with the following
additions:

 - The number of outstanding zombies can be configured.
 - Classes can opt-in to becoming zombies.
 - C++ destructors are correctly called on Leopard.

The goal is to allow us to enable zombies in certain production builds
to help debug some of the message-after-free bugs we have.

BUG=35590, 24987
TEST=everything

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

[Scott Hess - ex-Googler, 2010-03-03 18:36:22]

I started poking at this last fall as a thought experiment.  I believe that it
is a sound approach to this goal.  Which leaves open the question of whether the
goal is a good goal.  That's why I'm just targeting one reviewer, it should
probably be opened up further if it is deemed reasonable to take forward.

What I'd like to do is enable this for a couple weeks in the devchannel to flush
out some of our browser crashes which manifest as Cocoa calls to
probably-deallocated objects.  So long as the treadmill size is kept low, there
should not be significant performance overhead to running this.  It can be run
with a 0-length treadmill to at least provide additional Breakpad info on crash.

[Avi (use Gerrit), 2010-03-03 19:20:45]

Wow. Truly awesome and evil.

[pink (ping after 24hrs), 2010-03-05 15:14:05]

LGTM, but too many stray TODOs

http://codereview.chromium.org/660411/diff/1/2
File chrome/browser/chrome_browser_application_mac.mm (right):

http://codereview.chromium.org/660411/diff/1/2#newcode161
chrome/browser/chrome_browser_application_mac.mm:161: // Turn all deallocated
Objective-C objects into combies, keeping
zombies?

http://codereview.chromium.org/660411/diff/1/2#newcode163
chrome/browser/chrome_browser_application_mac.mm:163: // TODO(shess): Unwind
this on terminate?
file a bug to track and include the #, or remove the TODO

http://codereview.chromium.org/660411/diff/1/4
File chrome/browser/cocoa/objc_zombie.mm (right):

http://codereview.chromium.org/660411/diff/1/4#newcode19
chrome/browser/cocoa/objc_zombie.mm:19: // a C++ object.
file a bug or remove the todo

http://codereview.chromium.org/660411/diff/1/4#newcode28
chrome/browser/cocoa/objc_zombie.mm:28: - (Class)crZombieWasa;
crZombieWasA ?

http://codereview.chromium.org/660411/diff/1/4#newcode84
chrome/browser/cocoa/objc_zombie.mm:84: // TODO(shess): Port to 64-bit.  I
believe using struct nlist_64
file a bug

http://codereview.chromium.org/660411/diff/1/4#newcode250
chrome/browser/cocoa/objc_zombie.mm:250: // TODO(shess): Maybe pull this out
into a function and don't pollute
either do it, file a bug, or remove the todo

[Avi (use Gerrit), 2010-03-05 15:20:29]

http://codereview.chromium.org/660411/diff/1/4
File chrome/browser/cocoa/objc_zombie.mm (right):

http://codereview.chromium.org/660411/diff/1/4#newcode28
chrome/browser/cocoa/objc_zombie.mm:28: - (Class)crZombieWasa;
On 2010/03/05 15:14:05, pink wrote:
> crZombieWasA ?

No. ObjC objects have a type, an "isa". It's only right that zombies have a
"wasa".

[pink (ping after 24hrs), 2010-03-05 15:26:49]

Hrm, i searched expecting to find "isa" and didn't. Bad AppKiDo. No doughnut.

On Fri, Mar 5, 2010 at 10:20 AM,  <avi@chromium.org> wrote:
>
> http://codereview.chromium.org/660411/diff/1/4
> File chrome/browser/cocoa/objc_zombie.mm (right):
>
> http://codereview.chromium.org/660411/diff/1/4#newcode28
> chrome/browser/cocoa/objc_zombie.mm:28: - (Class)crZombieWasa;
> On 2010/03/05 15:14:05, pink wrote:
>>
>> crZombieWasA ?
>
> No. ObjC objects have a type, an "isa". It's only right that zombies
> have a "wasa".
>
> http://codereview.chromium.org/660411
>



-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[Scott Hess - ex-Googler, 2010-05-12 21:54:20]

OK, now that we have a devchannel, again, pushing this forward another stop.

For rollout, I propose that it land in a DCHECK() for some baking, then we
remove the DCHECK() for a devchannel release (or two), along with a
high-priority beta-blocking bug to make sure we don't push it to the wrong place
(unless we want to).

I'm not sure what the value for N should be.  I'd imagine 10k only covers a few
seconds of runtime :-).

http://codereview.chromium.org/660411/diff/1/2
File chrome/browser/chrome_browser_application_mac.mm (right):

http://codereview.chromium.org/660411/diff/1/2#newcode161
chrome/browser/chrome_browser_application_mac.mm:161: // Turn all deallocated
Objective-C objects into combies, keeping
On 2010/03/05 15:14:05, pink wrote:
> zombies?

Done.

http://codereview.chromium.org/660411/diff/1/2#newcode163
chrome/browser/chrome_browser_application_mac.mm:163: // TODO(shess): Unwind
this on terminate?
On 2010/03/05 15:14:05, pink wrote:
> file a bug to track and include the #, or remove the TODO

I looked around, and there wasn't a completely obvious place where one would
naturally expect to see no more messages being sent to freed objects.  So I
added a function for ZombieDisable(), but didn't add a call to it anywhere.

http://codereview.chromium.org/660411/diff/1/4
File chrome/browser/cocoa/objc_zombie.mm (right):

http://codereview.chromium.org/660411/diff/1/4#newcode19
chrome/browser/cocoa/objc_zombie.mm:19: // a C++ object.
On 2010/03/05 15:14:05, pink wrote:
> file a bug or remove the todo

Done.  I was thinking RAII and/or nestability and/or clean namespace, but for
something like this I'm hard-pressed to think of a real reason why those are a
real priority.

http://codereview.chromium.org/660411/diff/1/4#newcode28
chrome/browser/cocoa/objc_zombie.mm:28: - (Class)crZombieWasa;
On 2010/03/05 15:20:29, Avi wrote:
> On 2010/03/05 15:14:05, pink wrote:
> > crZombieWasA ?
> 
> No. ObjC objects have a type, an "isa". It's only right that zombies have a
> "wasa".

Yeah, isa -> wasa.  Like hasa.  And none of them make sense pronounced "was
aaa".

http://codereview.chromium.org/660411/diff/1/4#newcode84
chrome/browser/cocoa/objc_zombie.mm:84: // TODO(shess): Port to 64-bit.  I
believe using struct nlist_64
On 2010/03/05 15:14:05, pink wrote:
> file a bug

Done.

http://codereview.chromium.org/660411/diff/1/4#newcode250
chrome/browser/cocoa/objc_zombie.mm:250: // TODO(shess): Maybe pull this out
into a function and don't pollute
On 2010/03/05 15:14:05, pink wrote:
> either do it, file a bug, or remove the todo

Done.

http://codereview.chromium.org/660411/diff/1/4#newcode250
chrome/browser/cocoa/objc_zombie.mm:250: // TODO(shess): Maybe pull this out
into a function and don't pollute
On 2010/03/05 15:14:05, pink wrote:
> either do it, file a bug, or remove the todo

Done, replaced it with a function.

[Scott Hess - ex-Googler, 2010-05-18 19:48:19]

ping?

[pink (ping after 24hrs), 2010-05-18 19:56:56]

lgtm.

What kind of memory impact do we expect to see from 10k zombies?

http://codereview.chromium.org/660411/diff/5001/6003
File chrome/browser/cocoa/objc_zombie.mm (right):

http://codereview.chromium.org/660411/diff/5001/6003#newcode40
chrome/browser/cocoa/objc_zombie.mm:40: DestructFn *g_object_cxxDestruct = NULL;
nit, DestructFn*, per chrome guidelines.

[John Grabowski, 2010-05-18 20:01:55]

On Tue, May 18, 2010 at 12:56 PM, <pinkerton@chromium.org> wrote:

> lgtm.
>
> What kind of memory impact do we expect to see from 10k zombies?
>


'sysctl kern.maxproc' give me 2000 on 10.5 with 12G RAM.
So 10k is well into "zombie apocalypse".
Memory impact is the least of your problems.
Armor your car a la Mad Max.

jrg
<http://support.apple.com/kb/HT3854>

>
>
> http://codereview.chromium.org/660411/diff/5001/6003
>
> File chrome/browser/cocoa/objc_zombie.mm (right):
>
> http://codereview.chromium.org/660411/diff/5001/6003#newcode40
> chrome/browser/cocoa/objc_zombie.mm:40: DestructFn *g_object_cxxDestruct
> = NULL;
> nit, DestructFn*, per chrome guidelines.
>
>
> http://codereview.chromium.org/660411/show
>

[pink (ping after 24hrs), 2010-05-18 20:04:11]

you misunderstand. These are NSZombie objects, not zombie processes.

On Tue, May 18, 2010 at 4:01 PM, John Grabowski <jrg@chromium.org> wrote:
>
>
> On Tue, May 18, 2010 at 12:56 PM, <pinkerton@chromium.org> wrote:
>>
>> lgtm.
>>
>> What kind of memory impact do we expect to see from 10k zombies?
>
>
> 'sysctl kern.maxproc' give me 2000 on 10.5 with 12G RAM.
> So 10k is well into "zombie apocalypse".
> Memory impact is the least of your problems.
> Armor your car a la Mad Max.
> jrg
>
>>
>> http://codereview.chromium.org/660411/diff/5001/6003
>> File chrome/browser/cocoa/objc_zombie.mm (right):
>>
>> http://codereview.chromium.org/660411/diff/5001/6003#newcode40
>> chrome/browser/cocoa/objc_zombie.mm:40: DestructFn *g_object_cxxDestruct
>> = NULL;
>> nit, DestructFn*, per chrome guidelines.
>>
>> http://codereview.chromium.org/660411/show
>
>



-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[Scott Hess - ex-Googler, 2010-05-18 20:37:58]

On 2010/05/18 19:56:56, pink wrote:
> What kind of memory impact do we expect to see from 10k zombies?

I instrumented the code and it varied between 320k and 350k.  So not quite
negligible, but also not totally egregious.  Unfortunately, it was turning over
the treadmill every few seconds, which means this might have to be tuned upward
to catch more.  I'm somewhat thinking it might make sense to have a mode where
system classes aren't zombied.

I could adapt my instrumentation into a histogram to monitor memory usage, but
it would probably be more reasonable to just modify the code to cap memory use
directly.

On Tue, May 18, 2010 at 4:01 PM, John Grabowski <jrg@chromium.org> wrote:
> 'sysctl kern.maxproc' give me 2000 on 10.5 with 12G RAM.
> So 10k is well into "zombie apocalypse".
> Memory impact is the least of your problems.
> Armor your car a la Mad Max.

Zombies r in ur mini-kitchin eatin ur brainz.

http://codereview.chromium.org/660411/diff/5001/6003
File chrome/browser/cocoa/objc_zombie.mm (right):

http://codereview.chromium.org/660411/diff/5001/6003#newcode40
chrome/browser/cocoa/objc_zombie.mm:40: DestructFn *g_object_cxxDestruct = NULL;
On 2010/05/18 19:56:57, pink wrote:
> nit, DestructFn*, per chrome guidelines.

Done.

[John Grabowski, 2010-05-18 20:38:09]

Ah.  Less exciting then.

jrg

On Tue, May 18, 2010 at 1:04 PM, Mike Pinkerton <pinkerton@chromium.org>wrote:

> you misunderstand. These are NSZombie objects, not zombie processes.
>
> On Tue, May 18, 2010 at 4:01 PM, John Grabowski <jrg@chromium.org> wrote:
> >
> >
> > On Tue, May 18, 2010 at 12:56 PM, <pinkerton@chromium.org> wrote:
> >>
> >> lgtm.
> >>
> >> What kind of memory impact do we expect to see from 10k zombies?
> >
> >
> > 'sysctl kern.maxproc' give me 2000 on 10.5 with 12G RAM.
> > So 10k is well into "zombie apocalypse".
> > Memory impact is the least of your problems.
> > Armor your car a la Mad Max.
> > jrg
> >
> >>
> >> http://codereview.chromium.org/660411/diff/5001/6003
> >> File chrome/browser/cocoa/objc_zombie.mm (right):
> >>
> >> http://codereview.chromium.org/660411/diff/5001/6003#newcode40
> >> chrome/browser/cocoa/objc_zombie.mm:40: DestructFn
> *g_object_cxxDestruct
> >> = NULL;
> >> nit, DestructFn*, per chrome guidelines.
> >>
> >> http://codereview.chromium.org/660411/show
> >
> >
>
>
>
> --
> Mike Pinkerton
> Mac Weenie
> pinkerton@google.com
>