Allow whitelisted bundles to load in Chrome

Allow whitelisted bundles to load in Chrome:

Default Folder X as of CFBundleVersion 4.4.3
MySpeed as of CFBundleVersion 1201
SIMBL (Albert Zeyer's fork) at any CFBundleVersion
Smart Scroll as of CFBundleVersion 3.9

This also removes the prohibition on loading SIMBL plug-ins. If a permitted
SIMBL is present, it is resposnbile for loading plug-ins and provides the
interface for plug-ins to override code.

BUG=95045
TEST=These products should now work in Chrome
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105251

[Nico, 2011-10-12 17:25:59]

http://codereview.chromium.org/8226026/diff/1/chrome/common/mac/cfbundle_bloc...
File chrome/common/mac/cfbundle_blocker.mm (right):

http://codereview.chromium.org/8226026/diff/1/chrome/common/mac/cfbundle_bloc...
chrome/common/mac/cfbundle_blocker.mm:159: const AllowedBundle kAllowedBundles[]
= {
Add a comment saying "// If you want your plugin to be whitelisted, do …"

[Mark Mentovai, 2011-10-12 18:21:36]

Good call. Uploaded a new version with that information in a comment.

[Scott Hess - ex-Googler, 2011-10-12 19:34:34]

The review comment sounds like we've decided that the problem may not be
plug-ins, but the way they inject themselves?

http://codereview.chromium.org/8226026/diff/3002/chrome/common/mac/cfbundle_b...
File chrome/common/mac/cfbundle_blocker.mm (right):

http://codereview.chromium.org/8226026/diff/3002/chrome/common/mac/cfbundle_b...
chrome/common/mac/cfbundle_blocker.mm:175: // dmaclach
Expanding this like other email addresses won't kill anyone, and makes sense in
case of external contributors needing to ask questions.  Sorry dave.

http://codereview.chromium.org/8226026/diff/3002/chrome/common/mac/cfbundle_b...
chrome/common/mac/cfbundle_blocker.mm:196: NSString* identifier_ns =
base::mac::CFToNSCast(identifier_cf);
It might be worthwhile to pull everything outside of CFBundleGetIdentifier()
into a separate function, which could be unit-tested.  Once you've fetched the
bundle id to pass in, I bet fetching the version wouldn't make things materially
slower.

http://codereview.chromium.org/8226026/diff/3002/chrome/common/mac/cfbundle_b...
chrome/common/mac/cfbundle_blocker.mm:207: // identifier.
Would it be reasonable to make "children" be a different id than a straight
match?  Maybe @"com.example.myapp" would match that exactly, but
@"com.example.myapp.*" would match children?

I wasn't going to suggest this, but AFAICT it would only add a line or two of
code to implement.  [I only expected two or three lines, but it _is_ objective-c
:-).]

http://codereview.chromium.org/8226026/diff/3002/chrome/common/mac/cfbundle_b...
chrome/common/mac/cfbundle_blocker.mm:215: range:NSMakeRange(0,
allowed_bundle_id_length)];
-hasPrefix:?  Unless NSLiteralSearch implies correctness differences, I guess. 
Of course this busts my above suggestion, but you might just ignore that anyhow.

Actually, you could pull a -hasPrefix: test up where -isEqualToString: is, and
then qualify success by requiring either matching lengths or next char is a '.'.

http://codereview.chromium.org/8226026/diff/3002/chrome/common/mac/cfbundle_b...
chrome/common/mac/cfbundle_blocker.mm:240: options:NSNumericSearch];
OMG.  I mean, good job, Apple!

[Mark Mentovai, 2011-10-12 23:48:50]

I took most of your suggestions except for the wildcarding one discussed below.
Ready for another look.

> The review comment sounds like we've decided that the problem may not be
> plug-ins, but the way they inject themselves?

We’ve seen problems with both. We’ve seen a larger share of problems caused by
mach_inject. Of these plug-ins that have requested to be unblocked, I’m not
aware of any non-mach_inject-related crashes.

I revised the “how to get yourself into this list” comment to explicitly state
that we don’t tolerate crashes.

http://codereview.chromium.org/8226026/diff/3002/chrome/common/mac/cfbundle_b...
File chrome/common/mac/cfbundle_blocker.mm (right):

http://codereview.chromium.org/8226026/diff/3002/chrome/common/mac/cfbundle_b...
chrome/common/mac/cfbundle_blocker.mm:207: // identifier.
shess wrote:
> Would it be reasonable to make "children" be a different id than a straight
> match?  Maybe @"com.example.myapp" would match that exactly, but
> @"com.example.myapp.*" would match children?
> 
> I wasn't going to suggest this, but AFAICT it would only add a line or two of
> code to implement.  [I only expected two or three lines, but it _is_
objective-c
> :-).]

At least two of the things in the list now need to match both the base ID and
children, so with wildcards, they’d turn into two entries. The other option
would be to have a third field in the struct, either a “bool allow_children” or
an “enum { kExactMatch, KOnlyAllowChildren, kExactMatchOrChildren}”. I think
these are more than a line or two of code, though.

http://codereview.chromium.org/8226026/diff/3002/chrome/common/mac/cfbundle_b...
chrome/common/mac/cfbundle_blocker.mm:240: options:NSNumericSearch];
shess wrote:
> OMG.  I mean, good job, Apple!

I hear that it might not have worked properly in 10.4, but we don’t care.

The new unit test will catch unexpected behavior.

[Scott Hess - ex-Googler, 2011-10-13 00:05:54]

LGTM.

My nits are entirely advisory/philosophical.

http://codereview.chromium.org/8226026/diff/7001/chrome/common/mac/cfbundle_b...
File chrome/common/mac/cfbundle_blocker.mm (right):

http://codereview.chromium.org/8226026/diff/7001/chrome/common/mac/cfbundle_b...
chrome/common/mac/cfbundle_blocker.mm:183: << [path fileSystemRepresentation];
Wonder if this would be cleaner using -[NSString stringWithFormat:] to format
all the OSX stuff together, then one big -UTF8String to log it.   You shouldn't
have added mods, here :-).

http://codereview.chromium.org/8226026/diff/7001/chrome/common/mac/cfbundle_b...
File chrome/common/mac/cfbundle_blocker_unittest.mm (right):

http://codereview.chromium.org/8226026/diff/7001/chrome/common/mac/cfbundle_b...
chrome/common/mac/cfbundle_blocker_unittest.mm:88: }
Thank you for writing the test!  I think IsBundleAllowed() also became clearer
because it could just exit-early rather than maintaining nested state.

That said, I once got yelled at for writing a data-driven test like this.  I
personally consider it reasonable, but figured I should bring it up.  I think
the theory is whether this is really better than a bunch of:

EXPECT_FALSE(IsBundleAllowed(@"com.stclairsoft.DefaultFolderX.osax", @"3.5.4"));

I can kinda see the point.  I think your logging into failure is appropriate to
allow debuggers to see what's happening.

[Mark Mentovai, 2011-10-13 03:23:14]

Thanks for the review!

http://codereview.chromium.org/8226026/diff/7001/chrome/common/mac/cfbundle_b...
File chrome/common/mac/cfbundle_blocker.mm (right):

http://codereview.chromium.org/8226026/diff/7001/chrome/common/mac/cfbundle_b...
chrome/common/mac/cfbundle_blocker.mm:183: << [path fileSystemRepresentation];
shess wrote:
> Wonder if this would be cleaner using -[NSString stringWithFormat:] to format
> all the OSX stuff together, then one big -UTF8String to log it.   You
shouldn't
> have added mods, here :-).

That’d be sorta technically incorrect around the -fileSystemRepresentation bit,
and anyway, this is what LOG(…) << … << …; was born for.

You’re right, I shouldn’t have touched it. :)

http://codereview.chromium.org/8226026/diff/7001/chrome/common/mac/cfbundle_b...
File chrome/common/mac/cfbundle_blocker_unittest.mm (right):

http://codereview.chromium.org/8226026/diff/7001/chrome/common/mac/cfbundle_b...
chrome/common/mac/cfbundle_blocker_unittest.mm:88: }
shess wrote:
> Thank you for writing the test!  I think IsBundleAllowed() also became clearer
> because it could just exit-early rather than maintaining nested state.

I agree that IsBundleAllowed is cleaner this way, and it’s a benefit that it’s
easily testable.

> That said, I once got yelled at for writing a data-driven test like this.  I
> personally consider it reasonable, but figured I should bring it up.  I think
> the theory is whether this is really better than a bunch of:
> 
> EXPECT_FALSE(IsBundleAllowed(@"com.stclairsoft.DefaultFolderX.osax",
@"3.5.4"));
> 
> I can kinda see the point.  I think your logging into failure is appropriate
to
> allow debuggers to see what's happening.

There’s longstanding precedent for the big array. ’Course, some of the precedent
is stuff that I wrote, but that’s one of the perks of being with the project for
a long time. There’s also non-Mark precedent in Chrome.

The chief advantage of writing a bunch of EXPECT_xxx is that they show you the
input and line number, but including the index and inputs overcomes that. I like
writing as much of the test as possible in a real language (like C++ or
Objective-C) and having the silly macro show up in only one location. Writing
EXPECT_xxx(IsBundleAllowed(…, …)); over and over again feels like a lot of
boilerplate over and over, and one of the cardinal rules of programming is that
if you find yourself doing the same thing more than once, maybe you ought to
refactor. Doing it my way eats up less horizontal space and makes it less likely
that things will need to be wrapped “creatively.”