[Mac] Enable CrZombie for all processes.

[Mac] Enable CrZombie for all processes.

For the browser process, this will be overridden by
-[BrowserCrApplication init] (which raises it).

BUG=94551
TEST=Developers don't complain about zombie objects in other processes.

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

[Scott Hess - ex-Googler, 2011-09-01 21:58:22]

I wasn't quite able to figure out whether the zombie and swizzle files should
get chrome::common::mac namespaces or not.  I think at this point we consider
the zombie code to be real, so I'd not be against that, but I'm not entirely
sure about the swizzle code, if anyone every uses that without getting
appropriate review, I REALLY don't want plausible deniability.

http://codereview.chromium.org/7826016/diff/1/chrome/app/chrome_main.cc
File chrome/app/chrome_main.cc (right):

http://codereview.chromium.org/7826016/diff/1/chrome/app/chrome_main.cc#newco...
chrome/app/chrome_main.cc:589: DCHECK(ObjcEvilDoers::ZombieEnable(true, 1000));
This means that only debug builds will do this.  I am not averse to shipping it
to the world, but only in canary form, and I wouldn't mind having a trivial
'drover ...' command-line to accomplish it, if the first canary blows.

[Mark Mentovai, 2011-09-01 22:00:12]

http://codereview.chromium.org/7826016/diff/1/chrome/app/chrome_main.cc
File chrome/app/chrome_main.cc (right):

http://codereview.chromium.org/7826016/diff/1/chrome/app/chrome_main.cc#newco...
chrome/app/chrome_main.cc:589: DCHECK(ObjcEvilDoers::ZombieEnable(true, 1000));
shess wrote:
> This means that only debug builds will do this.  I am not averse to shipping
it
> to the world, but only in canary form, and I wouldn't mind having a trivial
> 'drover ...' command-line to accomplish it, if the first canary blows.

I’m curious about what you’re afraid of. The real benefit to the Objective-C
zombies in the browser has been that they’ve been running on user machines
catching things we never would have caught if we had been left to our own meager
resources.

[Scott Hess - ex-Googler, 2011-09-01 22:03:09]

On 2011/09/01 22:00:12, Mark Mentovai wrote:
> http://codereview.chromium.org/7826016/diff/1/chrome/app/chrome_main.cc
> File chrome/app/chrome_main.cc (right):
> 
>
http://codereview.chromium.org/7826016/diff/1/chrome/app/chrome_main.cc#newco...
> chrome/app/chrome_main.cc:589: DCHECK(ObjcEvilDoers::ZombieEnable(true,
1000));
> shess wrote:
> > This means that only debug builds will do this.  I am not averse to shipping
> it
> > to the world, but only in canary form, and I wouldn't mind having a trivial
> > 'drover ...' command-line to accomplish it, if the first canary blows.
> 
> I’m curious about what you’re afraid of. The real benefit to the Objective-C
> zombies in the browser has been that they’ve been running on user machines
> catching things we never would have caught if we had been left to our own
meager
> resources.

Originally the CrZombie code was enabled this way for the browser, to flush out
problems developers were adding.  After a long while with no complaints about
unusable browsers, it was made the default.  So mostly I just want to make sure
that we aren't shipping something which systemically breaks the world too
widely.  Any problems of this nature should become pretty obvious within a day
or two, I think.

[Mark Mentovai, 2011-09-01 22:07:26]

LGTM

http://codereview.chromium.org/7826016/diff/1/chrome/app/chrome_main.cc
File chrome/app/chrome_main.cc (right):

http://codereview.chromium.org/7826016/diff/1/chrome/app/chrome_main.cc#newco...
chrome/app/chrome_main.cc:589: DCHECK(ObjcEvilDoers::ZombieEnable(true, 1000));
OK, sounds reasonable for this to be debug-only for a week or two.

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

http://codereview.chromium.org/7826016/diff/1/chrome/browser/chrome_browser_a...
chrome/browser/chrome_browser_application_mac.mm:18: #import
"chrome/common/mac/objc_method_swizzle.h"
Sort: chrome < content.

http://codereview.chromium.org/7826016/diff/1/chrome/browser/chrome_browser_a...
chrome/browser/chrome_browser_application_mac.mm:213:
ObjcEvilDoers::ZombieEnable(true, 10000);
This uses that swap loop, right? Over all of the original 1000 objects (even if
the list is mostly sparse?) That will hopefully not suck.

I wouldn’t be opposed to putting a process-type check into ChromeMain to use
10000 for the browser process, and adding an explicit ZombieEnable call to the
one other file (shared by those tests).

[Scott Hess - ex-Googler, 2011-09-01 23:31:57]

http://codereview.chromium.org/7826016/diff/1/chrome/app/chrome_main.cc
File chrome/app/chrome_main.cc (right):

http://codereview.chromium.org/7826016/diff/1/chrome/app/chrome_main.cc#newco...
chrome/app/chrome_main.cc:589: DCHECK(ObjcEvilDoers::ZombieEnable(true, 1000));
On 2011/09/01 22:07:26, Mark Mentovai wrote:
> OK, sounds reasonable for this to be debug-only for a week or two.

Man, you're too easy :-).  That said, I'm certainly willing to wait until after
the long weekend.

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

http://codereview.chromium.org/7826016/diff/1/chrome/browser/chrome_browser_a...
chrome/browser/chrome_browser_application_mac.mm:18: #import
"chrome/common/mac/objc_method_swizzle.h"
On 2011/09/01 22:07:26, Mark Mentovai wrote:
> Sort: chrome < content.

Done.  I looked right past that distinction.  Would be easier to notice if it
were goose/ versus content/.

http://codereview.chromium.org/7826016/diff/1/chrome/browser/chrome_browser_a...
chrome/browser/chrome_browser_application_mac.mm:213:
ObjcEvilDoers::ZombieEnable(true, 10000);
On 2011/09/01 22:07:26, Mark Mentovai wrote:
> This uses that swap loop, right? Over all of the original 1000 objects (even
if
> the list is mostly sparse?) That will hopefully not suck.

I think I had originally coded up an alternative which did a smart memcpy(), but
decided that handling the case where the overlap was end+beg was maybe a little
aggressive.  Certainly doable, and for something called millions of times per
launch I'd be all for it.

I just instrumented a build with TimeTicks, and for the entire block under the
lock:
  Convert old count 1000 to new count 10000 took 0 ms
I was inclined to just leave it there, but it's easy to go deeper:
  Convert old count 1000 to new count 10000 took 207 us
  Convert old count 1000 to new count 10000 took 200 us
  Convert old count 1000 to new count 10000 took 208 us
For reference, the longest base case is:
  Convert old count 0 to new count 1000 took 133 us
though most of them were between 90 and 100 us.

> I wouldn’t be opposed to putting a process-type check into ChromeMain to use
> 10000 for the browser process, and adding an explicit ZombieEnable call to the
> one other file (shared by those tests).

Hmm.  Would it make sense (or anti-sense) to shift this #if OS_MACOSX block down
to the #if OS_POSIX section?  Then it would have process_type right there.  It
would also be reasonable to push the zombie-enable after the --version check.

[Mark Mentovai, 2011-09-01 23:55:23]

LGTM

http://codereview.chromium.org/7826016/diff/1/chrome/app/chrome_main.cc
File chrome/app/chrome_main.cc (right):

http://codereview.chromium.org/7826016/diff/1/chrome/app/chrome_main.cc#newco...
chrome/app/chrome_main.cc:589: DCHECK(ObjcEvilDoers::ZombieEnable(true, 1000));
shess wrote:
> On 2011/09/01 22:07:26, Mark Mentovai wrote:
> > OK, sounds reasonable for this to be debug-only for a week or two.
> 
> Man, you're too easy :-).  That said, I'm certainly willing to wait until
after
> the long weekend.

Easy? I thought that the question tended more toward the iron-fist end of the
spectrum. I saw you had a TODO here to turn it on, but I pressed you on the
matter anyway. :)

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

http://codereview.chromium.org/7826016/diff/1/chrome/browser/chrome_browser_a...
chrome/browser/chrome_browser_application_mac.mm:18: #import
"chrome/common/mac/objc_method_swizzle.h"
shess wrote:
> On 2011/09/01 22:07:26, Mark Mentovai wrote:
> > Sort: chrome < content.
> 
> Done.  I looked right past that distinction.  Would be easier to notice if it
> were goose/ versus content/.

Oh, how you goad me!

http://codereview.chromium.org/7826016/diff/1/chrome/browser/chrome_browser_a...
chrome/browser/chrome_browser_application_mac.mm:213:
ObjcEvilDoers::ZombieEnable(true, 10000);
shess wrote:
>   Convert old count 1000 to new count 10000 took 207 us

I guess that sounds about right.

> Hmm.  Would it make sense (or anti-sense) to shift this #if OS_MACOSX block
down
> to the #if OS_POSIX section?  Then it would have process_type right there.  It
> would also be reasonable to push the zombie-enable after the --version check.

In this tiny POSIX-specific block of main, I don’t think the ordering of most
steps matters that much.

(Except that there are probably some small subtle relative-ordering things. I
put EnableCFBundleBlocker after SetUpBundleOverrides intentionally: while
EnableCFBundleBlocker itself doesn’t depend on bundle overrides, I wasn’t
positive that everything called by our blocker override function was free of
uses of the bundle override stuff. If I inverted the order of the two calls, I
was afraid someone would try to insert something in the middle that might try to
load a bundle.)

That said, it seems like the change you’re looking to make wouldn’t have any
ordering concerns of this sort, so pick something that sounds good and let’s
roll with it. Practically, I think of everything in that BasicStartupComplete
function running at pretty much the same time in the context of the greater
ChromeMain scheme.

[Scott Hess - ex-Googler, 2011-09-02 00:03:21]

Since I got a LGTM, I think I'll combine rearrange-the-startup with
remove-the-DCHECK?  I have a different actually-crashing-right-now bug I'm going
to try to finish up today...

[Mark Mentovai, 2011-09-02 00:05:53]

Yeah, that’s fine. I’d probably like to look at the code that does go in, but if
I’m not around, it’s fine to check this in and I’ll look at it after it lands. I
think we understand each other on where this will end up, so the worst that
further review would turn up would just be style nits, and who cares, we can fix
those in a follow-up.

[Scott Hess - ex-Googler, 2011-09-02 17:01:15]

On 2011/09/02 00:03:21, shess wrote:
> Since I got a LGTM, I think I'll combine rearrange-the-startup with
> remove-the-DCHECK?  I have a different actually-crashing-right-now bug I'm
going
> to try to finish up today...

After some time with trys failing due to LKGR being stale, then from not using
--revision correctly, now I find that check-deps is failing because the moved
files depend on breakpad (moved to chrome/common, breakpad in chrome/app). 
FFFUUU, NOW I remember why I hadn't moved it all out of ui/cocoa earlier!

I can't think of any reason other than prettiness to have the files in common/
rather than app/, and chrome_main is in app/, so that seems like the path of
least resistance.  WDYT?

[Sad that path-changing was right there was half the work of the CL.]

[Mark Mentovai, 2011-09-02 17:06:22]

I guess you could put it in chrome/app, but then I’d want it in chrome/app/mac
so we can have the right sort of OWNERS file for it.

At this point it might be easier to just edit chrome/common/mac/DEPS?

That breakpad thing is kind of bogus, but these directories are so
spaghetti-ish.

[Scott Hess - ex-Googler, 2011-09-02 19:50:56]

On 2011/09/02 17:06:22, Mark Mentovai wrote:
> I guess you could put it in chrome/app, but then I’d want it in chrome/app/mac
> so we can have the right sort of OWNERS file for it.
> 
> At this point it might be easier to just edit chrome/common/mac/DEPS?
> 
> That breakpad thing is kind of bogus, but these directories are so
> spaghetti-ish.

OK, I'm going to add the DEPS change, with the reasoning that being able to set
a crash key for debugging purposes is probably something which makes sense to be
in chrome/common/ in the long term - if not content/common/, or maybe even base/
(obviously with hooks and empty stubs in the latter cases).  I'll include a bug
to that end.

[And then I'll ignore the bug, in hopes that "long term" implies that "someone"
will fix the horrible breakpad situation so we can make things like scoped crash
keys cross-platform.  The crash key stuff really shouldn't be Mac-specific.]

[Mark Mentovai, 2011-09-02 19:54:35]

"Someone."

[Scott Hess - ex-Googler, 2011-09-02 20:05:42]

On 2011/09/02 19:50:56, shess wrote:
> On 2011/09/02 17:06:22, Mark Mentovai wrote:
> > I guess you could put it in chrome/app, but then I’d want it in
chrome/app/mac
> > so we can have the right sort of OWNERS file for it.
> > 
> > At this point it might be easier to just edit chrome/common/mac/DEPS?
> > 
> > That breakpad thing is kind of bogus, but these directories are so
> > spaghetti-ish.
> 
> OK, I'm going to add the DEPS change, with the reasoning that being able to
set
> a crash key for debugging purposes is probably something which makes sense to
be
> in chrome/common/ in the long term - if not content/common/, or maybe even
base/
> (obviously with hooks and empty stubs in the latter cases).  I'll include a
bug
> to that end.
> 
> [And then I'll ignore the bug, in hopes that "long term" implies that
"someone"
> will fix the horrible breakpad situation so we can make things like scoped
crash
> keys cross-platform.  The crash key stuff really shouldn't be Mac-specific.]

Per usual, immediately after posting this I was going to enter the bug
explaining why, and determined that the swizzle code only depends on breakpad
because someone included the file.  It doesn't appear to be necessary.

So, now I'm backtracking.  An argument can be made that objc_method_swizzle.h
provides infrastructure which may sometimes be necessary for correct operation
of the system (the "It should be in the same directory as cfbundle_blocker.h"
argument), while objc_zombie.h modifies the environment in ways which are not
necessary for correct operation.

Sometimes I wish I just didn't look closely at stuff, you know?

[Scott Hess - ex-Googler, 2011-09-02 22:57:50]

On 2011/09/02 20:05:42, shess wrote:
> On 2011/09/02 19:50:56, shess wrote:
> > On 2011/09/02 17:06:22, Mark Mentovai wrote:
> > > I guess you could put it in chrome/app, but then I’d want it in
> chrome/app/mac
> > > so we can have the right sort of OWNERS file for it.
> > > 
> > > At this point it might be easier to just edit chrome/common/mac/DEPS?
> > > 
> > > That breakpad thing is kind of bogus, but these directories are so
> > > spaghetti-ish.
> > 
> > OK, I'm going to add the DEPS change, with the reasoning that being able to
> set
> > a crash key for debugging purposes is probably something which makes sense
to
> be
> > in chrome/common/ in the long term - if not content/common/, or maybe even
> base/
> > (obviously with hooks and empty stubs in the latter cases).  I'll include a
> bug
> > to that end.
> > 
> > [And then I'll ignore the bug, in hopes that "long term" implies that
> "someone"
> > will fix the horrible breakpad situation so we can make things like scoped
> crash
> > keys cross-platform.  The crash key stuff really shouldn't be Mac-specific.]
> 
> Per usual, immediately after posting this I was going to enter the bug
> explaining why, and determined that the swizzle code only depends on breakpad
> because someone included the file.  It doesn't appear to be necessary.
> 
> So, now I'm backtracking.  An argument can be made that objc_method_swizzle.h
> provides infrastructure which may sometimes be necessary for correct operation
> of the system (the "It should be in the same directory as cfbundle_blocker.h"
> argument), while objc_zombie.h modifies the environment in ways which are not
> necessary for correct operation.
> 
> Sometimes I wish I just didn't look closely at stuff, you know?

And, I'm going with the DEPS change.  chrome/app/ is a nightmare, do not want.

[Mark Mentovai, 2011-09-03 16:33:27]

LGTM

http://codereview.chromium.org/7826016/diff/8001/chrome/common/mac/DEPS
File chrome/common/mac/DEPS (right):

http://codereview.chromium.org/7826016/diff/8001/chrome/common/mac/DEPS#newcode6
chrome/common/mac/DEPS:6: "+third_party/mach_override",
Put a blank line before this so that it’s clear that your comment doesn’t apply
to mach_override.