[Mac] Fix keyword editor-related crashes

chrome/browser/cocoa/keyword_editor_cocoa_controller.mm

Index: chrome/browser/cocoa/keyword_editor_cocoa_controller.mm
diff --git a/chrome/browser/cocoa/keyword_editor_cocoa_controller.mm b/chrome/browser/cocoa/keyword_editor_cocoa_controller.mm
index 032ffbb757c4f2b6979e525596871052973863bb..86b7e5642cd1390af6372fe85a288a2d3ae556f4 100644
--- a/chrome/browser/cocoa/keyword_editor_cocoa_controller.mm
+++ b/chrome/browser/cocoa/keyword_editor_cocoa_controller.mm
@@ -57,35 +57,54 @@ void KeywordEditorModelObserver::OnEditedKeyword(
 }
 
 void KeywordEditorModelObserver::OnModelChanged() {
-  InvalidateIconCache(0, [iconImages_ count]);
   int count = [controller_ controller]->table_model()->RowCount();
+  [iconImages_ setCount:0];
   [iconImages_ setCount:count];
   [controller_ modelChanged];
 }
 
 void KeywordEditorModelObserver::OnItemsChanged(int start, int length) {
-  InvalidateIconCache(start, length);
+  DCHECK_LE(start + length, static_cast<int>([iconImages_ count]));

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

Seems odd to move this out of a separate function and inline. Why did you choose
to do that?

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

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".

+  for (int i = start; i < (start + length); ++i) {
+    [iconImages_ replacePointerAtIndex:i withPointer:NULL];
+  }
+  DCHECK_EQ([controller_ controller]->table_model()->RowCount(),
+            static_cast<int>([iconImages_ count]));
   [controller_ modelChanged];
 }
 
 void KeywordEditorModelObserver::OnItemsAdded(int start, int length) {
   DCHECK_LE(start, static_cast<int>([iconImages_ count]));
-  for (int i = 0; i < length; ++i) {
-    [iconImages_ insertPointer:NULL atIndex:start];  // Values slide down.
+
+  // -[NSPointerArray insertPointer:atIndex:] throws if index == count.
+  // Instead expand the array with NULLs.
+  if (start == static_cast<int>([iconImages_ count])) {

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

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]

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.

+    [iconImages_ setCount:start + length];
+  } else {
+    for (int i = 0; i < length; ++i) {
+      [iconImages_ insertPointer:NULL atIndex:start];  // Values slide up.
+    }
   }
+  DCHECK_EQ([controller_ controller]->table_model()->RowCount(),
+            static_cast<int>([iconImages_ count]));
   [controller_ modelChanged];
 }
 
 void KeywordEditorModelObserver::OnItemsRemoved(int start, int length) {
   DCHECK_LE(start + length, static_cast<int>([iconImages_ count]));
   for (int i = 0; i < length; ++i) {
-    [iconImages_ removePointerAtIndex:start];  // Values slide up.
+    [iconImages_ removePointerAtIndex:start];  // Values slide down.
   }
+  DCHECK_EQ([controller_ controller]->table_model()->RowCount(),
+            static_cast<int>([iconImages_ count]));
   [controller_ modelChanged];
 }
 
 NSImage* KeywordEditorModelObserver::GetImageForRow(int row) {
-  DCHECK(row >= 0 && row < static_cast<int>([iconImages_ count]));
+  DCHECK_EQ([controller_ controller]->table_model()->RowCount(),
+            static_cast<int>([iconImages_ count]));
+  DCHECK_GE(row, 0);
+  DCHECK_LT(row, static_cast<int>([iconImages_ count]));
   NSImage* image = static_cast<NSImage*>([iconImages_ pointerAtIndex:row]);
   if (!image) {
     const SkBitmap bitmapIcon =
@@ -99,13 +118,6 @@ NSImage* KeywordEditorModelObserver::GetImageForRow(int row) {
   return image;
 }
 
-void KeywordEditorModelObserver::InvalidateIconCache(int start, int length) {
-  DCHECK_LE(start + length, static_cast<int>([iconImages_ count]));
-  for (int i = start; i < (start + length); ++i) {
-    [iconImages_ replacePointerAtIndex:i withPointer:NULL];
-  }
-}
-
 // KeywordEditorCocoaController -----------------------------------------------
 
 namespace {
@@ -170,6 +182,8 @@ typedef std::map<Profile*,KeywordEditorCocoaController*> ProfileControllerMap;
 - (void)dealloc {
   controller_->table_model()->SetObserver(NULL);
   controller_->url_model()->RemoveObserver(observer_.get());
+  [tableView_ setTarget:nil];
+  observer_.reset();
   [super dealloc];
 }
 
@@ -221,6 +235,7 @@ typedef std::map<Profile*,KeywordEditorCocoaController*> ProfileControllerMap;
 
 - (void)modelChanged {
   [tableView_ reloadData];
+  [self adjustEditingButtons];
 }
 
 - (KeywordEditorController*)controller {