[Mac] Fix keyword editor-related crashes

chrome/browser/cocoa/keyword_editor_cocoa_controller.mm

 // Copyright (c) 2009 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import <Cocoa/Cocoa.h>
 
 #import "base/mac_util.h"
 #include "base/sys_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #import "chrome/browser/cocoa/edit_search_engine_cocoa_controller.h"
@@ skipping 39 matching lines @@
     const std::wstring& url) {
   KeywordEditorController* controller = [controller_ controller];
   if (template_url) {
     controller->ModifyTemplateURL(template_url, title, keyword, url);
   } else {
     controller->AddTemplateURL(title, keyword, url);
   }
 }
 
 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 =
         [controller_ controller]->table_model()->GetIcon(row);
     if (!bitmapIcon.isNull()) {
       image = gfx::SkBitmapToNSImage(bitmapIcon);
       DCHECK(image);
       [iconImages_ replacePointerAtIndex:row withPointer:image];
     }
   }
   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 {
 
 typedef std::map<Profile*,KeywordEditorCocoaController*> ProfileControllerMap;
 
 }  // namespace
 
 @implementation KeywordEditorCocoaController
 
@@ skipping 44 matching lines @@
     observer_.reset(new KeywordEditorModelObserver(self));
     controller_->table_model()->SetObserver(observer_.get());
     controller_->url_model()->AddObserver(observer_.get());
   }
   return self;
 }
 
 - (void)dealloc {
   controller_->table_model()->SetObserver(NULL);
   controller_->url_model()->RemoveObserver(observer_.get());
+  [tableView_ setTarget:nil];
+  observer_.reset();
   [super dealloc];
 }
 
 - (void)awakeFromNib {
   // Make sure the button fits its label, but keep it the same height as the
   // other two buttons.
   [GTMUILocalizerAndLayoutTweaker sizeToFitView:makeDefaultButton_];
   NSSize size = [makeDefaultButton_ frame].size;
   size.height = NSHeight([addButton_ frame]);
   [makeDefaultButton_ setFrameSize:size];
@@ skipping 31 matching lines @@
 - (void)windowDidMove:(NSNotification*)notif {
   if (g_browser_process && g_browser_process->local_state()) {
     NSWindow* window = [self window];
     [window saveWindowPositionToPrefs:g_browser_process->local_state()
                              withPath:prefs::kKeywordEditorWindowPlacement];
   }
 }
 
 - (void)modelChanged {
   [tableView_ reloadData];
+  [self adjustEditingButtons];
 }
 
 - (KeywordEditorController*)controller {
   return controller_.get();
 }
 
 - (void)sheetDidEnd:(NSWindow*)sheet
          returnCode:(NSInteger)code
             context:(void*)context {
   [sheet orderOut:self];
@@ skipping 132 matching lines @@
   if ([selection count] != 1) {
     [makeDefaultButton_ setEnabled:NO];
   } else {
     const TemplateURL& url =
         controller_->table_model()->GetTemplateURL([selection firstIndex]);
     [makeDefaultButton_ setEnabled:controller_->CanMakeDefault(&url)];
   }
 }
 
 @end