[Mac] Fix keyword editor-related crashes

[Mac] Fix keyword editor-related crashes

* Adjust the edit buttons after adding a new keyword.
* Don't make NSPointerArray calls which cause exceptions.

Patch from rsesek@chromium.org http://codereview.chromium.org/262028

BUG=23350, 22545
TEST=Preferences-->Manage. Add a search engine. Click "Make Default", delete it ("-)". Crash.

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

[Scott Hess - ex-Googler, 2009-10-22 20:28:57]

Patched over from other CL due to laptop-hit-by-bus clause.  First I patched it
directly over, then I applied changes from my last comments, so you can see both
cases.

[Scott Hess - ex-Googler, 2009-10-22 22:08:29]

[Sorry, I'm positive I added pink, but when I looked again he wasn't there...]

[Scott Hess - ex-Googler, 2009-10-23 13:24:34]

ping.

[pink (ping after 24hrs), 2009-10-23 14:11:00]

lgtm with questions

http://codereview.chromium.org/319006/diff/2001/3002
File chrome/browser/cocoa/keyword_editor_cocoa_controller.mm (right):

http://codereview.chromium.org/319006/diff/2001/3002#newcode67
Line 67: DCHECK_LE(start + length, static_cast<int>([iconImages_ count]));
Seems odd to move this out of a separate function and inline. Why did you choose
to do that?

http://codereview.chromium.org/319006/diff/2001/3002#newcode81
Line 81: if (start == static_cast<int>([iconImages_ count])) {
if we're concerned about throwing, why not wrap in an @try? Or are we now sure
we've gotten all the cases where this is wrong?

[Scott Hess - ex-Googler, 2009-10-23 14:24:28]

http://codereview.chromium.org/319006/diff/2001/3002
File chrome/browser/cocoa/keyword_editor_cocoa_controller.mm (right):

http://codereview.chromium.org/319006/diff/2001/3002#newcode67
Line 67: DCHECK_LE(start + length, static_cast<int>([iconImages_ count]));
On 2009/10/23 14:11:00, pink wrote:
> Seems odd to move this out of a separate function and inline. Why did you
choose
> to do that?

InvalidateIconCache() was a private function only used here and in
OnModelChanged().  Now that it was only used here, pulling it inline makes this
function and the following two all follow the pattern "touch items in my range,
call -modelChanged".

http://codereview.chromium.org/319006/diff/2001/3002#newcode81
Line 81: if (start == static_cast<int>([iconImages_ count])) {
On 2009/10/23 14:11:00, pink wrote:
> if we're concerned about throwing, why not wrap in an @try? Or are we now sure
> we've gotten all the cases where this is wrong?

Catching the exception would still leave the array the wrong size.  I mention
the throw because -[NSArray insertObject:atIndex:] works fine at count, so it's
not obvious why the code does this.

Meta: We don't have the right compile flags to use @try, and I think our
long-term goal should be to throw no exceptions at all.

[Mark Mentovai, 2009-10-23 14:43:18]

Oh!  C++ standard library header bug!  Pick me!  Pick me!

We can use @try, but it's tricky.  Actually, using it is pretty easy if you know
where I keep the magic powder, it's just the explanation that's tricky.

We build without -fexceptions.  This means that we don't support C++ exceptions,
but doesn't directly have any imapct on Objective-C.  The C++ standard library
headers wind up defining "try" and "catch" as macros so that C++ code written
with exceptions in mind (like the standard library itself) can be used with C++
exceptions disabled.  See /usr/include/c++/4.0.0/exception_defines.h.

In Objective-C++ code, depending on the headers that are included, it's possible
to wind up with these macros defined.  This interferes with Objective-C @try and
@catch, and generally sucks.

The solution is, in any .mm file in which you wish to use @try and @catch, to
#undef try and catch after your #includes.

I've fixed this in at least two spots already.  See base/nsimage_cache_mac.mm
and third_party/WebKit/WebCore/WebCorePrefix.h.

[Scott Hess - ex-Googler, 2009-10-23 16:20:52]

I believe that we should leave the repository of "How to catch
NSExceptions" entirely w/in Mark's head.  There are so many
transitions between Cocoa and Chrome code that being able to precisely
target the @try may be impossible in some cases.  You wrap a @try
around a single line of Objective-C code and then find out that four
directories over is something which gets called back to carry water
between Chrome and Cocoa, and suddenly you're masking a @throw across
23 stack frames.  While I think this kind of thing can probably be
dealt with on a case-by-case basis, I think our team is kind of big to
hope that we can deal with this kind of thing culturally.

Actually, do we insurance on Mark?  Between message-loop, gyp, distcc,
exception-handling, and heaven knows what else, I'm worried about a
rogue bus.  NYC is a well-known hotbed of rogue busses.

-scott


On Fri, Oct 23, 2009 at 7:43 AM,  <mark@chromium.org> wrote:
> Oh! =A0C++ standard library header bug! =A0Pick me! =A0Pick me!
>
> We can use @try, but it's tricky. =A0Actually, using it is pretty easy if=
 you
> know
> where I keep the magic powder, it's just the explanation that's tricky.
>
> We build without -fexceptions. =A0This means that we don't support C++
> exceptions,
> but doesn't directly have any imapct on Objective-C. =A0The C++ standard
> library
> headers wind up defining "try" and "catch" as macros so that C++ code
> written
> with exceptions in mind (like the standard library itself) can be used wi=
th
> C++
> exceptions disabled. =A0See /usr/include/c++/4.0.0/exception_defines.h.
>
> In Objective-C++ code, depending on the headers that are included, it's
> possible
> to wind up with these macros defined. =A0This interferes with Objective-C=
 @try
> and
> @catch, and generally sucks.
>
> The solution is, in any .mm file in which you wish to use @try and @catch=
,
> to
> #undef try and catch after your #includes.
>
> I've fixed this in at least two spots already. =A0See
> base/nsimage_cache_mac.mm
> and third_party/WebKit/WebCore/WebCorePrefix.h.
>
> http://codereview.chromium.org/319006
>

[pink (ping after 24hrs), 2009-10-23 16:46:57]

We're already using @try/@catch in certain areas, which is why I asked
the question. I'm fine making sure we fix this particular instance
correctly and not using it.

On Fri, Oct 23, 2009 at 12:20 PM, Scott Hess <shess@chromium.org> wrote:
> I believe that we should leave the repository of "How to catch
> NSExceptions" entirely w/in Mark's head. =A0There are so many
> transitions between Cocoa and Chrome code that being able to precisely
> target the @try may be impossible in some cases. =A0You wrap a @try
> around a single line of Objective-C code and then find out that four
> directories over is something which gets called back to carry water
> between Chrome and Cocoa, and suddenly you're masking a @throw across
> 23 stack frames. =A0While I think this kind of thing can probably be
> dealt with on a case-by-case basis, I think our team is kind of big to
> hope that we can deal with this kind of thing culturally.
>
> Actually, do we insurance on Mark? =A0Between message-loop, gyp, distcc,
> exception-handling, and heaven knows what else, I'm worried about a
> rogue bus. =A0NYC is a well-known hotbed of rogue busses.
>
> -scott
>
>
> On Fri, Oct 23, 2009 at 7:43 AM, =A0<mark@chromium.org> wrote:
>> Oh! =A0C++ standard library header bug! =A0Pick me! =A0Pick me!
>>
>> We can use @try, but it's tricky. =A0Actually, using it is pretty easy i=
f you
>> know
>> where I keep the magic powder, it's just the explanation that's tricky.
>>
>> We build without -fexceptions. =A0This means that we don't support C++
>> exceptions,
>> but doesn't directly have any imapct on Objective-C. =A0The C++ standard
>> library
>> headers wind up defining "try" and "catch" as macros so that C++ code
>> written
>> with exceptions in mind (like the standard library itself) can be used w=
ith
>> C++
>> exceptions disabled. =A0See /usr/include/c++/4.0.0/exception_defines.h.
>>
>> In Objective-C++ code, depending on the headers that are included, it's
>> possible
>> to wind up with these macros defined. =A0This interferes with Objective-=
C @try
>> and
>> @catch, and generally sucks.
>>
>> The solution is, in any .mm file in which you wish to use @try and @catc=
h,
>> to
>> #undef try and catch after your #includes.
>>
>> I've fixed this in at least two spots already. =A0See
>> base/nsimage_cache_mac.mm
>> and third_party/WebKit/WebCore/WebCorePrefix.h.
>>
>> http://codereview.chromium.org/319006
>>
>



--=20
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[Robert Sesek, 2009-10-23 18:07:01]

LGTM. Thanks for taking this over, and sorry for the delay.

[Scott Hess - ex-Googler, 2009-10-23 18:27:18]

In it goes!  Hope your mac gets better :-).

On Fri, Oct 23, 2009 at 11:07 AM,  <rsesek@chromium.org> wrote:
> LGTM. Thanks for taking this over, and sorry for the delay.
>
> http://codereview.chromium.org/319006
>