Add ScopedObjCClassSwizzler in base/mac, absorbs objc_method_swizzle and ScopedClassSwizzler

Add ScopedObjCClassSwizzler in base/mac, absorbs objc_method_swizzle and ScopedClassSwizzler

ScopedClassSwizzler from ui/test is wanted for new tests where it can't
currently be accessed. It also re-implements a concept in
chrome/common/mac/objc_method_swizzle.*

This change adds base::mac::ScopedObjCClassSwizzler, merges concepts
from objc_method_swizzle, and adjusts chrome_browser_application_mac.mm
to use the new swizzler.

The test from objc_method_swizzle is adapted and extended for the scoped
swizzler.

BUG=378134
TEST=base_unittests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288943

[tapted, 2014-07-21 07:20:52]

Hi Robert - please take a look. This is a follow-up to an earlier CL
(crrev/353643002). The stuff it would have conflicted with has now all landed.

[Robert Sesek, 2014-07-21 15:59:02]

LGTM

https://codereview.chromium.org/345243007/diff/80001/base/test/scoped_class_s...
File base/test/scoped_class_swizzler_mac.h (right):

https://codereview.chromium.org/345243007/diff/80001/base/test/scoped_class_s...
base/test/scoped_class_swizzler_mac.h:14: // Within a given scope, replace the
selector |selector| on |target| with that
We have this code also in //chrome/common/mac/objc_method_swizzle.h in a
namespace called ObjcEvilDoers.

You'll need mark@ for OWNERS approval, so he may have naming ideas, but perhaps
this should be in base::mac::, base::test::, or base::subtle::.

It'd also be nice to consolidate the code.

https://codereview.chromium.org/345243007/diff/80001/ui/gfx/mac/coordinate_co...
File ui/gfx/mac/coordinate_conversion_unittest.mm (right):

https://codereview.chromium.org/345243007/diff/80001/ui/gfx/mac/coordinate_co...
ui/gfx/mac/coordinate_conversion_unittest.mm:21: 
optional: Feel free to omit blank lines around methods in these in-impl files.

[Robert Sesek, 2014-07-21 16:03:14]

+mark for the above question on placement //base, so that you can get feedback
this timezone cycle.

[Mark Mentovai, 2014-07-21 16:09:58]

https://codereview.chromium.org/345243007/diff/80001/base/test/scoped_class_s...
File base/test/scoped_class_swizzler_mac.h (right):

https://codereview.chromium.org/345243007/diff/80001/base/test/scoped_class_s...
base/test/scoped_class_swizzler_mac.h:14: // Within a given scope, replace the
selector |selector| on |target| with that
rsesek wrote:
> We have this code also in //chrome/common/mac/objc_method_swizzle.h in a
> namespace called ObjcEvilDoers.
> 
> You'll need mark@ for OWNERS approval, so he may have naming ideas, but
perhaps
> this should be in base::mac::, base::test::, or base::subtle::.
> 
> It'd also be nice to consolidate the code.

I agree, please do this in base/mac and namespace base::mac so that we can
combine this with the existing code we have to do the same thing.

https://codereview.chromium.org/345243007/diff/80001/base/test/scoped_class_s...
base/test/scoped_class_swizzler_mac.h:16: class ScopedClassSwizzler {
This should say ObjC in its name, and the filename should change as a result.
(scoped_objc_class_swizzler_mac.h?)

[tapted, 2014-07-22 12:53:49]

PTAL - quite a bit changed. (Maybe it's better to land the changes under
src/chrome in a follow-up CL..)

https://codereview.chromium.org/345243007/diff/80001/base/test/scoped_class_s...
File base/test/scoped_class_swizzler_mac.h (right):

https://codereview.chromium.org/345243007/diff/80001/base/test/scoped_class_s...
base/test/scoped_class_swizzler_mac.h:14: // Within a given scope, replace the
selector |selector| on |target| with that
On 2014/07/21 16:09:58, Mark Mentovai wrote:
> rsesek wrote:
> > We have this code also in //chrome/common/mac/objc_method_swizzle.h in a
> > namespace called ObjcEvilDoers.
> > 
> > You'll need mark@ for OWNERS approval, so he may have naming ideas, but
> perhaps
> > this should be in base::mac::, base::test::, or base::subtle::.
> > 
> > It'd also be nice to consolidate the code.
> 
> I agree, please do this in base/mac and namespace base::mac so that we can
> combine this with the existing code we have to do the same thing.

Done. I've merged the implementations. Or, more accurately, replaced the older
objc_method_swizzle.* with scoped_objc_class_swizzler.* after incpororating some
things and stealing (and extending) its unit test.

It's a bit of an overhaul - I tried to make a diff in patchset 5 "everything but
the file moves" to paint a better picture of what changed (to the unit test, and
versus patchset 4 to ScopedClassSwizzler). i.e. the diff patchset 5 vs 4 should
show what's changed still.

The two swizzlers were bit different: objc_method_swizzle exchanges methods with
different selectors on the same class implementation; scoped_objc_class_swizzler
exchanges methods with the same selectors on different class implementations. I
think the latter is more robust, and it makes
ObjcEvilDoers::GetImplementedInstanceMethod() redundant, so that part is gone.

I've also made the usage in chrome_browser_application_mac.mm "scoped", where it
wasn't previously. I think it should still outlive anything we care about. I
could also just leak it with a CR_DEFINE_STATIC_LOCAL or similar (keeping it in
anonymous::SwizzleInit()). Let me know what you think.

https://codereview.chromium.org/345243007/diff/80001/base/test/scoped_class_s...
base/test/scoped_class_swizzler_mac.h:16: class ScopedClassSwizzler {
On 2014/07/21 16:09:58, Mark Mentovai wrote:
> This should say ObjC in its name, and the filename should change as a result.
> (scoped_objc_class_swizzler_mac.h?)

Done (but dropped the _mac - seems more consistent with other files in base/mac)

https://codereview.chromium.org/345243007/diff/80001/ui/gfx/mac/coordinate_co...
File ui/gfx/mac/coordinate_conversion_unittest.mm (right):

https://codereview.chromium.org/345243007/diff/80001/ui/gfx/mac/coordinate_co...
ui/gfx/mac/coordinate_conversion_unittest.mm:21: 
On 2014/07/21 15:59:01, rsesek wrote:
> optional: Feel free to omit blank lines around methods in these in-impl files.

Done.

[tapted, 2014-07-22 13:01:32]

On 2014/07/22 12:53:49, tapted wrote:
> and it makes
> ObjcEvilDoers::GetImplementedInstanceMethod() redundant, so that part is gone.

(actually, upon reflection, I'm not too sure how true this is. i.e. You could
perhaps still pass the "wrong" [Foo class] argument and swizzle a method at an
unexpected layer, but there might also be a use-case for that. In any case - let
me know if I should add it back :)

[Robert Sesek, 2014-07-22 13:59:21]

On 2014/07/22 12:53:49, tapted wrote:
> PTAL - quite a bit changed. (Maybe it's better to land the changes under
> src/chrome in a follow-up CL..)

Actually, I think it's probably easiest at this point to drop your new test from
the CL and just repurpose this CL for moving the class swizzler to //base.

https://codereview.chromium.org/345243007/diff/80001/base/test/scoped_class_s...
File base/test/scoped_class_swizzler_mac.h (right):

https://codereview.chromium.org/345243007/diff/80001/base/test/scoped_class_s...
base/test/scoped_class_swizzler_mac.h:14: // Within a given scope, replace the
selector |selector| on |target| with that
On 2014/07/22 12:53:49, tapted wrote:
> On 2014/07/21 16:09:58, Mark Mentovai wrote:
> > rsesek wrote:
> > > We have this code also in //chrome/common/mac/objc_method_swizzle.h in a
> > > namespace called ObjcEvilDoers.
> > > 
> > > You'll need mark@ for OWNERS approval, so he may have naming ideas, but
> > perhaps
> > > this should be in base::mac::, base::test::, or base::subtle::.
> > > 
> > > It'd also be nice to consolidate the code.
> > 
> > I agree, please do this in base/mac and namespace base::mac so that we can
> > combine this with the existing code we have to do the same thing.
> 
> Done. I've merged the implementations. Or, more accurately, replaced the older
> objc_method_swizzle.* with scoped_objc_class_swizzler.* after incpororating
some
> things and stealing (and extending) its unit test.
> 
> It's a bit of an overhaul - I tried to make a diff in patchset 5 "everything
but
> the file moves" to paint a better picture of what changed (to the unit test,
and
> versus patchset 4 to ScopedClassSwizzler). i.e. the diff patchset 5 vs 4
should
> show what's changed still.
> 
> The two swizzlers were bit different: objc_method_swizzle exchanges methods
with
> different selectors on the same class implementation;
scoped_objc_class_swizzler
> exchanges methods with the same selectors on different class implementations.
I
> think the latter is more robust, and it makes
> ObjcEvilDoers::GetImplementedInstanceMethod() redundant, so that part is gone.

Yes, I like the Donor pattern.

> I've also made the usage in chrome_browser_application_mac.mm "scoped", where
it
> wasn't previously. I think it should still outlive anything we care about. I
> could also just leak it with a CR_DEFINE_STATIC_LOCAL or similar (keeping it
in
> anonymous::SwizzleInit()). Let me know what you think.

I think I'd prefer the NSException swizzler to be a CR_DEFINE_STATIC_LOCAL, just
in case any NSExceptions are thrown during shutdown.

[tapted, 2014-07-22 23:41:44]

On 2014/07/22 13:59:21, rsesek wrote:
> On 2014/07/22 12:53:49, tapted wrote:
> > PTAL - quite a bit changed. (Maybe it's better to land the changes under
> > src/chrome in a follow-up CL..)
> 
> Actually, I think it's probably easiest at this point to drop your new test
from
> the CL and just repurpose this CL for moving the class swizzler to //base.

Done.

https://codereview.chromium.org/345243007/diff/80001/base/test/scoped_class_s...
File base/test/scoped_class_swizzler_mac.h (right):

https://codereview.chromium.org/345243007/diff/80001/base/test/scoped_class_s...
base/test/scoped_class_swizzler_mac.h:14: // Within a given scope, replace the
selector |selector| on |target| with that
On 2014/07/22 13:59:21, rsesek wrote:
> I think I'd prefer the NSException swizzler to be a CR_DEFINE_STATIC_LOCAL,
just
> in case any NSExceptions are thrown during shutdown.

Done.

[Robert Sesek, 2014-07-23 14:21:00]

LGTM

However it occurs to me that this is going to break the way the crash server
buckets NSExceptions (https://goto.google.com/rsisx). I guess we could probably
drop the (Cr part in the search prefix.

[tapted, 2014-07-23 23:57:17]

mark: wdyt?

On 2014/07/23 14:21:00, rsesek wrote:
> However it occurs to me that this is going to break the way the crash server
> buckets NSExceptions (https://goto.google.com/rsisx). I guess we could
probably
> drop the (Cr part in the search prefix.

Wow - good point! Maybe I can roll this into an official build locally and
generate a test dump... Although the dSYMs won't be on the crash server - hm.
I'll keep an eye out for the changes wrt this in any case - (not 100% sure how
the new prefixes will look yet).

[tapted, 2014-07-25 06:59:56]

On 2014/07/23 23:57:17, tapted wrote:
> mark: wdyt?
> 
> On 2014/07/23 14:21:00, rsesek wrote:
> > However it occurs to me that this is going to break the way the crash server
> > buckets NSExceptions (https://goto.google.com/rsisx). I guess we could
> probably
> > drop the (Cr part in the search prefix.
> 
> Wow - good point! Maybe I can roll this into an official build locally and
> generate a test dump... Although the dSYMs won't be on the crash server - hm.
> I'll keep an eye out for the changes wrt this in any case - (not 100% sure how
> the new prefixes will look yet).

So I generated a crash - http://go/crash/0457cb564cf09c2b . But of course no
symbols on the server. However, the Product Data has `nsexception` set, which
the swizzled handler adds to the crash data [note below]. The crash server code
is super unfamiliar to me, but I wonder whether instead of the `HasPrefixString`
check, it could instead check for a non-empty value for this product data key
(i.e. `!report.pdata().cocoa_exception.empty() ==> return..
mac::FindNSExceptionThrower(..)` etc. )?

https://codereview.chromium.org/345243007/diff/400001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/345243007/diff/400001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_application_mac.mm:90:
base::debug::SetCrashKeyValue(crash_keys::mac::kNSException, value);
note

[Robert Sesek, 2014-07-25 17:21:13]

On 2014/07/25 06:59:56, tapted wrote:
> On 2014/07/23 23:57:17, tapted wrote:
> > mark: wdyt?
> > 
> > On 2014/07/23 14:21:00, rsesek wrote:
> > > However it occurs to me that this is going to break the way the crash
server
> > > buckets NSExceptions (https://goto.google.com/rsisx). I guess we could
> > probably
> > > drop the (Cr part in the search prefix.
> > 
> > Wow - good point! Maybe I can roll this into an official build locally and
> > generate a test dump... Although the dSYMs won't be on the crash server -
hm.
> > I'll keep an eye out for the changes wrt this in any case - (not 100% sure
how
> > the new prefixes will look yet).
> 
> So I generated a crash - http://go/crash/0457cb564cf09c2b . But of course no
> symbols on the server. However, the Product Data has `nsexception` set, which
> the swizzled handler adds to the crash data [note below]. The crash server
code
> is super unfamiliar to me, but I wonder whether instead of the
`HasPrefixString`
> check, it could instead check for a non-empty value for this product data key
> (i.e. `!report.pdata().cocoa_exception.empty() ==> return..
> mac::FindNSExceptionThrower(..)` etc. )?

The crash processor typically makes heuristic decisions based on the stack
trace, rather than product data (in fact, I don't think we have any product-data
based selection, though we do use that data for enhancing the stack-based
selection.

I wonder if it would make sense to have an alternate ctor that provides the
category-based swizzle support, in addition to the constructor that provides
donor-based swizzle support.

[tapted, 2014-07-28 23:03:01]

mark: (ping) what do you think is the best way to proceed?

I think we have

- add an extra constructor for ScopedObjCClassSwizzler to do method swizzling on
the same class type (by different method names) and leave the crash server
unchanged (https://goto.google.com/rsisx)

- change crash server to bucket by the product data, looking for
!cocoa_exception.empty()

- change crash server to bucket by just the prefxix: "-[NSException"



On 2014/07/25 17:21:13, rsesek wrote:
> On 2014/07/25 06:59:56, tapted wrote:
> > On 2014/07/23 23:57:17, tapted wrote:
> > > mark: wdyt?
> > > 
> > > On 2014/07/23 14:21:00, rsesek wrote:
> > > > However it occurs to me that this is going to break the way the crash
> server
> > > > buckets NSExceptions (https://goto.google.com/rsisx). I guess we could
> > > probably
> > > > drop the (Cr part in the search prefix.
> > > 
> > > Wow - good point! Maybe I can roll this into an official build locally and
> > > generate a test dump... Although the dSYMs won't be on the crash server -
> hm.
> > > I'll keep an eye out for the changes wrt this in any case - (not 100% sure
> how
> > > the new prefixes will look yet).
> > 
> > So I generated a crash - http://go/crash/0457cb564cf09c2b . But of course no
> > symbols on the server. However, the Product Data has `nsexception` set,
which
> > the swizzled handler adds to the crash data [note below]. The crash server
> code
> > is super unfamiliar to me, but I wonder whether instead of the
> `HasPrefixString`
> > check, it could instead check for a non-empty value for this product data
key
> > (i.e. `!report.pdata().cocoa_exception.empty() ==> return..
> > mac::FindNSExceptionThrower(..)` etc. )?
> 
> The crash processor typically makes heuristic decisions based on the stack
> trace, rather than product data (in fact, I don't think we have any
product-data
> based selection, though we do use that data for enhancing the stack-based
> selection.
> 
> I wonder if it would make sense to have an alternate ctor that provides the
> category-based swizzle support, in addition to the constructor that provides
> donor-based swizzle support.

[Robert Sesek, 2014-07-29 01:46:36]

On 2014/07/25 17:21:13, rsesek wrote:
> On 2014/07/25 06:59:56, tapted wrote:
> > On 2014/07/23 23:57:17, tapted wrote:
> > > mark: wdyt?
> > > 
> > > On 2014/07/23 14:21:00, rsesek wrote:
> > > > However it occurs to me that this is going to break the way the crash
> server
> > > > buckets NSExceptions (https://goto.google.com/rsisx). I guess we could
> > > probably
> > > > drop the (Cr part in the search prefix.
> > > 
> > > Wow - good point! Maybe I can roll this into an official build locally and
> > > generate a test dump... Although the dSYMs won't be on the crash server -
> hm.
> > > I'll keep an eye out for the changes wrt this in any case - (not 100% sure
> how
> > > the new prefixes will look yet).
> > 
> > So I generated a crash - http://go/crash/0457cb564cf09c2b . But of course no
> > symbols on the server. However, the Product Data has `nsexception` set,
which
> > the swizzled handler adds to the crash data [note below]. The crash server
> code
> > is super unfamiliar to me, but I wonder whether instead of the
> `HasPrefixString`
> > check, it could instead check for a non-empty value for this product data
key
> > (i.e. `!report.pdata().cocoa_exception.empty() ==> return..
> > mac::FindNSExceptionThrower(..)` etc. )?
> 
> The crash processor typically makes heuristic decisions based on the stack
> trace, rather than product data (in fact, I don't think we have any
product-data
> based selection, though we do use that data for enhancing the stack-based
> selection.
> 
> I wonder if it would make sense to have an alternate ctor that provides the
> category-based swizzle support, in addition to the constructor that provides
> donor-based swizzle support.

This should be really easy to do, and it means not changing the crash server.
(Not that I'm opposed to changing the crash server – it's just more work).

The old signature is Swizzle(Class, SEL, SEL) and the new one is Swizzle(Class,
Class, SEL). Both of these are just specializations of Swizzle(Class, SEL,
Class, SEL), which could be implemented in a helper function (e.g. a delegated
constructor). Since @selector() has to be used at the callsite, it will be
obvious which constructor is being used.

[Mark Mentovai, 2014-07-29 12:58:59]

Robert’s suggestion, the one to use two Class-SEL pairs, is the most general and
sounds like a good idea to me.

[tapted, 2014-07-31 13:01:33]

PTAL - I've added the constructor overload, some extra tests, and rolled back
some of the changes in chrome_browser_application_mac.mm

[Robert Sesek, 2014-07-31 13:57:00]

LGTM!

[Mark Mentovai, 2014-07-31 14:07:26]

LGTM

[tapted, 2014-07-31 23:32:15]

+sky for OWNERS in ui/events/* and
chrome/browser/chrome_browser_application_mac.mm

[sky, 2014-08-04 19:44:20]

LGTM

[tapted, 2014-08-04 23:16:01]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-08-04 23:19:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tapted@chromium.org/345243007/480001

[commit-bot: I haz the power, 2014-08-05 01:13:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[commit-bot: I haz the power, 2014-08-05 01:16:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-05 01:16:45]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tapted, 2014-08-12 03:24:41]

The following should be fixed by r288888, so I'm giving this a crack at the CQ
again. Thanks for your patience :)

** Presubmit ERRORS **
These files spam the console log with LOG(INFO):
  chrome/common/mac/objc_zombie.mm

[tapted, 2014-08-12 03:25:02]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-08-12 03:27:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tapted@chromium.org/345243007/480001

[commit-bot: I haz the power, 2014-08-12 03:32:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-12 03:34:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-12 03:34:27]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tapted, 2014-08-12 04:57:35]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-08-12 04:58:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tapted@chromium.org/345243007/520001

[commit-bot: I haz the power, 2014-08-12 09:50:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-12 12:03:55]

Change committed as 288943