Bookmark context menus reference bookmark IDs, not pointers, in case

they go away while the menu is open.

BUG=34522

TEST=\
1) Confirm bookmark context menus continue to work (e.g. rename, delete).
2) Repeat the "sync and delete while context menu" is open test from the
  bug; make sure the op is ignored (and we don't crash).

Review URL: http://codereview.chromium.org/1041003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@41903 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 138 insertions, 56 deletions

diff --git a/chrome/browser/cocoa/bookmark_bar_controller.h b/chrome/browser/cocoa/bookmark_bar_controller.h
index eac35fa..f3bf44a 100644
--- a/chrome/browser/cocoa/bookmark_bar_controller.h
+++ b/chrome/browser/cocoa/bookmark_bar_controller.h
@@ -316,7 +316,7 @@ willAnimateFromState:(bookmarks::VisualState)oldState
 - (NSMenu*)offTheSideMenu;
 - (NSButton*)offTheSideButton;
 - (NSButton*)otherBookmarksButton;
-- (BookmarkNode*)nodeFromMenuItem:(id)sender;
+- (const BookmarkNode*)nodeFromMenuItem:(id)sender;
 - (void)updateTheme:(ThemeProvider*)themeProvider;
 - (BookmarkBarFolderController*)folderController;
 - (BookmarkButton*)buttonForDroppingOnAtPoint:(NSPoint)point;
diff --git a/chrome/browser/cocoa/bookmark_bar_controller.mm b/chrome/browser/cocoa/bookmark_bar_controller.mm
index 411e08a..80ced64 100644
--- a/chrome/browser/cocoa/bookmark_bar_controller.mm
+++ b/chrome/browser/cocoa/bookmark_bar_controller.mm
@@ -950,7 +950,9 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
   if (![item isKindOfClass:[NSMenuItem class]])
     return YES;
 
-  BookmarkNode* node = [self nodeFromMenuItem:item];
+  const BookmarkNode* node = [self nodeFromMenuItem:item];
+  if (!node)
+    return NO;
 
   // If this is the bar menu, we only have things to do if there are
   // buttons.  If this is a folder button menu, we only have things to
@@ -1183,22 +1185,27 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
 // -[BookmarkButtonCell menu].
 
 - (IBAction)openBookmarkInNewForegroundTab:(id)sender {
-  BookmarkNode* node = [self nodeFromMenuItem:sender];
-  [self openURL:node->GetURL() disposition:NEW_FOREGROUND_TAB];
+  const BookmarkNode* node = [self nodeFromMenuItem:sender];
+  if (node)
+    [self openURL:node->GetURL() disposition:NEW_FOREGROUND_TAB];
 }
 
 - (IBAction)openBookmarkInNewWindow:(id)sender {
-  BookmarkNode* node = [self nodeFromMenuItem:sender];
-  [self openURL:node->GetURL() disposition:NEW_WINDOW];
+  const BookmarkNode* node = [self nodeFromMenuItem:sender];
+  if (node)
+    [self openURL:node->GetURL() disposition:NEW_WINDOW];
 }
 
 - (IBAction)openBookmarkInIncognitoWindow:(id)sender {
-  BookmarkNode* node = [self nodeFromMenuItem:sender];
-  [self openURL:node->GetURL() disposition:OFF_THE_RECORD];
+  const BookmarkNode* node = [self nodeFromMenuItem:sender];
+  if (node)
+    [self openURL:node->GetURL() disposition:OFF_THE_RECORD];
 }
 
 - (IBAction)editBookmark:(id)sender {
-  BookmarkNode* node = [self nodeFromMenuItem:sender];
+  const BookmarkNode* node = [self nodeFromMenuItem:sender];
+  if (!node)
+    return;
 
   if (node->is_folder()) {
     BookmarkNameFolderController* controller =
@@ -1226,26 +1233,31 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
 }
 
 - (IBAction)copyBookmark:(id)sender {
-  BookmarkNode* node = [self nodeFromMenuItem:sender];
-  NSPasteboard* pboard = [NSPasteboard generalPasteboard];
-  [self copyBookmarkNode:node
-            toPasteboard:pboard];
+  const BookmarkNode* node = [self nodeFromMenuItem:sender];
+  if (node) {
+    NSPasteboard* pboard = [NSPasteboard generalPasteboard];
+    [self copyBookmarkNode:node
+              toPasteboard:pboard];
+  }
 }
 
 - (IBAction)deleteBookmark:(id)sender {
-  BookmarkNode* node = [self nodeFromMenuItem:sender];
-  bookmarkModel_->Remove(node->GetParent(),
-                         node->GetParent()->IndexOfChild(node));
-  // TODO(jrg): don't close; rebuild.
-  // http://crbug.com/36614
-  [self closeAllBookmarkFolders];
+  const BookmarkNode* node = [self nodeFromMenuItem:sender];
+  if (node) {
+    bookmarkModel_->Remove(node->GetParent(),
+                           node->GetParent()->IndexOfChild(node));
+    // TODO(jrg): don't close; rebuild.
+    // http://crbug.com/36614
+    [self closeAllBookmarkFolders];
+  }
 }
 
 // An ObjC version of bookmark_utils::OpenAllImpl().
-- (void)openBookmarkNodesRecursive:(BookmarkNode*)node
+- (void)openBookmarkNodesRecursive:(const BookmarkNode*)node
                        disposition:(WindowOpenDisposition)disposition {
+  DCHECK(node);
   for (int i = 0; i < node->GetChildCount(); i++) {
-    BookmarkNode* child = node->GetChild(i);
+    const BookmarkNode* child = node->GetChild(i);
     if (child->is_url()) {
       [self openURL:child->GetURL() disposition:disposition];
       // We revert to a basic disposition in case the initial
@@ -1257,30 +1269,43 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
   }
 }
 
-// Return the BookmarkNode associated with the given NSMenuItem.
-- (BookmarkNode*)nodeFromMenuItem:(id)sender {
+// Return the BookmarkNode associated with the given NSMenuItem.  Can
+// return NULL which means "do nothing".  One case where it would
+// return NULL is if the bookmark model gets modified while you have a
+// context menu open.
+- (const BookmarkNode*)nodeFromMenuItem:(id)sender {
+  const BookmarkNode* node = NULL;
   BookmarkMenu* menu = (BookmarkMenu*)[sender menu];
-  if ([menu isKindOfClass:[BookmarkMenu class]])
-    return const_cast<BookmarkNode*>([menu node]);
-  return NULL;
+  if ([menu isKindOfClass:[BookmarkMenu class]]) {
+    int64 id = [menu id];
+    node = bookmarkModel_->GetNodeByID(id);
+  }
+  return node;
 }
 
 - (IBAction)openAllBookmarks:(id)sender {
-  BookmarkNode* node = [self nodeFromMenuItem:sender];
-  [self openBookmarkNodesRecursive:node disposition:NEW_FOREGROUND_TAB];
-  UserMetrics::RecordAction("OpenAllBookmarks", browser_->profile());
+  const BookmarkNode* node = [self nodeFromMenuItem:sender];
+  if (node) {
+    [self openBookmarkNodesRecursive:node disposition:NEW_FOREGROUND_TAB];
+    UserMetrics::RecordAction("OpenAllBookmarks", browser_->profile());
+  }
 }
 
 - (IBAction)openAllBookmarksNewWindow:(id)sender {
-  BookmarkNode* node = [self nodeFromMenuItem:sender];
-  [self openBookmarkNodesRecursive:node disposition:NEW_WINDOW];
-  UserMetrics::RecordAction("OpenAllBookmarksNewWindow", browser_->profile());
+  const BookmarkNode* node = [self nodeFromMenuItem:sender];
+  if (node) {
+    [self openBookmarkNodesRecursive:node disposition:NEW_WINDOW];
+    UserMetrics::RecordAction("OpenAllBookmarksNewWindow", browser_->profile());
+  }
 }
 
 - (IBAction)openAllBookmarksIncognitoWindow:(id)sender {
-  BookmarkNode* node = [self nodeFromMenuItem:sender];
-  [self openBookmarkNodesRecursive:node disposition:OFF_THE_RECORD];
-  UserMetrics::RecordAction("OpenAllBookmarksIncognitoWindow", browser_->profile());
+  const BookmarkNode* node = [self nodeFromMenuItem:sender];
+  if (node) {
+    [self openBookmarkNodesRecursive:node disposition:OFF_THE_RECORD];
+    // Must be on 1 line due to UMA scripts
+    UserMetrics::RecordAction("OpenAllBookmarksIncognitoWindow", browser_->profile());
+  }
 }
 
 // May be called from the bar or from a folder button.
@@ -1547,7 +1572,7 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
   DCHECK(model == bookmarkModel_);
   if (!model->IsLoaded())
     return;
-  // Else brute force nuke and build.
+  // Brute force nuke and build.
   savedFrameWidth_ = NSWidth([[self view] frame]);
   const BookmarkNode* node = model->GetBookmarkBarNode();
   [self clearBookmarkBar];
@@ -1566,7 +1591,10 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
 - (void)setNodeForBarMenu {
   const BookmarkNode* node = bookmarkModel_->GetBookmarkBarNode();
   BookmarkMenu* menu = static_cast<BookmarkMenu*>([[self view] menu]);
-  [menu setRepresentedObject:[NSValue valueWithPointer:node]];
+
+  // Make sure types are compatible
+  DCHECK(sizeof(long long) == sizeof(int64));
+  [menu setRepresentedObject:[NSNumber numberWithLongLong:node->id()]];
 }
 
 - (void)beingDeleted:(BookmarkModel*)model {
diff --git a/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm b/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm
index d0ce683..05d8649 100644
--- a/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm
@@ -188,16 +188,19 @@ class BookmarkBarControllerTest : public CocoaTest {
                   withAnimation:NO];
   }
 
-  // Return a menu item that points to the right URL.
+  // Return a menu item that points to the given URL.
   NSMenuItem* ItemForBookmarkBarMenu(GURL& gurl) {
-    node_.reset(new BookmarkNode(gurl));
-    [menu_ setRepresentedObject:[NSValue valueWithPointer:node_.get()]];
+    BookmarkModel* model = helper_.profile()->GetBookmarkModel();
+    const BookmarkNode* parent = model->GetBookmarkBarNode();
+    const BookmarkNode* node = model->AddURL(parent, parent->GetChildCount(),
+                                             L"A title", gurl);
+    [menu_ setRepresentedObject:[NSNumber numberWithLongLong:node->id()]];
     return menu_item_;
   }
 
   // Does NOT take ownership of node.
   NSMenuItem* ItemForBookmarkBarMenu(const BookmarkNode* node) {
-    [menu_ setRepresentedObject:[NSValue valueWithPointer:node]];
+    [menu_ setRepresentedObject:[NSNumber numberWithLongLong:node->id()]];
     return menu_item_;
   }
 
@@ -208,7 +211,6 @@ class BookmarkBarControllerTest : public CocoaTest {
   scoped_nsobject<BookmarkMenu> menu_;
   scoped_nsobject<NSMenuItem> menu_item_;
   scoped_nsobject<NSButtonCell> cell_;
-  scoped_ptr<BookmarkNode> node_;
 };
 
 TEST_F(BookmarkBarControllerTest, ShowWhenShowBookmarkBarTrue) {
@@ -908,7 +910,7 @@ TEST_F(BookmarkBarControllerTest, TestButtonOrBar) {
   EXPECT_TRUE(first && second);
 
   NSMenuItem* menuItem = [[[first cell] menu] itemAtIndex:0];
-  BookmarkNode* node = [bar_ nodeFromMenuItem:menuItem];
+  const BookmarkNode* node = [bar_ nodeFromMenuItem:menuItem];
   EXPECT_TRUE(node);
   EXPECT_EQ(node, model->GetBookmarkBarNode()->GetChild(0));
 
@@ -936,7 +938,7 @@ TEST_F(BookmarkBarControllerTest, TestMenuNodeAndDisable) {
   BookmarkMenu* menu = static_cast<BookmarkMenu*>([[button cell] menu]);
   EXPECT_TRUE(menu);
   EXPECT_TRUE([menu isKindOfClass:[BookmarkMenu class]]);
-  EXPECT_EQ(folder, [menu node]);
+  EXPECT_EQ(folder->id(), [menu id]);
 
   // Make sure "Open All" is disabled (nothing to open -- no children!)
   // (Assumes "Open All" is the 1st item)
@@ -1191,6 +1193,45 @@ TEST_F(BookmarkBarControllerTest, DropDestination) {
   }
 }
 
+TEST_F(BookmarkBarControllerTest, NodeDeletedWhileMenuIsOpen) {
+  BookmarkModel* model = helper_.profile()->GetBookmarkModel();
+  [bar_ loaded:model];
+
+  const BookmarkNode* parent = model->GetBookmarkBarNode();
+  const BookmarkNode* initialNode = model->AddURL(
+      parent, parent->GetChildCount(),
+      L"initial",
+      GURL("http://www.google.com"));
+
+  NSMenuItem* item = ItemForBookmarkBarMenu(initialNode);
+  EXPECT_EQ(0U, bar_.get()->urls_.size());
+
+  // Basic check of the menu item and an IBOutlet it can call.
+  EXPECT_EQ(initialNode, [bar_ nodeFromMenuItem:item]);
+  [bar_ openBookmarkInNewWindow:item];
+  EXPECT_EQ(1U, bar_.get()->urls_.size());
+  [bar_ clear];
+
+  // Now delete the node and make sure things are happy (no crash,
+  // NULL node caught).
+  model->Remove(parent, parent->IndexOfChild(initialNode));
+  EXPECT_EQ(nil, [bar_ nodeFromMenuItem:item]);
+  // Should not crash by referencing a deleted node.
+  [bar_ openBookmarkInNewWindow:item];
+  // Confirm the above did nothing in case it somehow didn't crash.
+  EXPECT_EQ(0U, bar_.get()->urls_.size());
+
+  // Confirm some more non-crashes.
+  [bar_ openBookmarkInNewForegroundTab:item];
+  [bar_ openBookmarkInIncognitoWindow:item];
+  [bar_ editBookmark:item];
+  [bar_ copyBookmark:item];
+  [bar_ deleteBookmark:item];
+  [bar_ openAllBookmarks:item];
+  [bar_ openAllBookmarksNewWindow:item];
+  [bar_ openAllBookmarksIncognitoWindow:item];
+}
+
 TEST_F(BookmarkBarControllerTest, CloseFolderOnAnimate) {
   BookmarkModel* model = helper_.profile()->GetBookmarkModel();
   const BookmarkNode* parent = model->GetBookmarkBarNode();
@@ -1224,6 +1265,7 @@ TEST_F(BookmarkBarControllerTest, CloseFolderOnAnimate) {
   EXPECT_FALSE([bar_ folderController]);
 }
 
+
 class BookmarkBarControllerNotificationTest : public CocoaTest {
  public:
   BookmarkBarControllerNotificationTest() {
diff --git a/chrome/browser/cocoa/bookmark_button_cell.mm b/chrome/browser/cocoa/bookmark_button_cell.mm
index 6ee2e8c..e7f7656 100644
--- a/chrome/browser/cocoa/bookmark_button_cell.mm
+++ b/chrome/browser/cocoa/bookmark_button_cell.mm
@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "base/logging.h"
+#import "chrome/browser/bookmarks/bookmark_model.h"
 #import "chrome/browser/cocoa/bookmark_button_cell.h"
 #import "chrome/browser/cocoa/bookmark_menu.h"
 
@@ -78,12 +79,14 @@
 
 // We share the context menu among all bookmark buttons.  To allow us
 // to disambiguate when needed (e.g. "open bookmark"), we set the
-// menu's associated node to be our represented object.
+// menu's associated bookmark node ID to be our represented object.
 - (NSMenu*)menu {
   if (empty_)
     return nil;
   BookmarkMenu* menu = (BookmarkMenu*)[super menu];
-  [menu setRepresentedObject:[self representedObject]];
+  const BookmarkNode* node =
+      static_cast<const BookmarkNode*>([[self representedObject] pointerValue]);
+  [menu setRepresentedObject:[NSNumber numberWithLongLong:node->id()]];
   return menu;
 }
 
diff --git a/chrome/browser/cocoa/bookmark_button_cell_unittest.mm b/chrome/browser/cocoa/bookmark_button_cell_unittest.mm
index 07f9984..cb467f5 100644
--- a/chrome/browser/cocoa/bookmark_button_cell_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_button_cell_unittest.mm
@@ -33,6 +33,8 @@
 namespace {
 
 class BookmarkButtonCellTest : public CocoaTest {
+  public:
+    BrowserTestHelper helper_;
 };
 
 // Make sure it's not totally bogus
@@ -55,6 +57,11 @@ TEST_F(BookmarkButtonCellTest, MouseEnterStuff) {
   scoped_nsobject<BookmarkButtonCell> cell(
       [[BookmarkButtonCell alloc] initTextCell:@"Testing"]);
   [cell setMenu:[[[BookmarkMenu alloc] initWithTitle:@"foo"] autorelease]];
+
+  BookmarkModel* model = helper_.profile()->GetBookmarkModel();
+  const BookmarkNode* node = model->GetBookmarkBarNode();
+  [cell setBookmarkNode:node];
+
   EXPECT_TRUE([cell.get() showsBorderOnlyWhileMouseInside]);
   EXPECT_TRUE([cell menu]);
 
@@ -64,7 +71,6 @@ TEST_F(BookmarkButtonCellTest, MouseEnterStuff) {
 }
 
 TEST_F(BookmarkButtonCellTest, BookmarkNode) {
-  BrowserTestHelper helper_;
   BookmarkModel& model(*(helper_.profile()->GetBookmarkModel()));
   scoped_nsobject<BookmarkButtonCell> cell(
       [[BookmarkButtonCell alloc] initTextCell:@"Testing"]);
diff --git a/chrome/browser/cocoa/bookmark_menu.h b/chrome/browser/cocoa/bookmark_menu.h
index 7a5ff25..5d39edf 100644
--- a/chrome/browser/cocoa/bookmark_menu.h
+++ b/chrome/browser/cocoa/bookmark_menu.h
@@ -3,16 +3,18 @@
 // found in the LICENSE file.
 
 #import <Cocoa/Cocoa.h>
-struct BookmarkNode;
+
+#include "base/basictypes.h"
+
 
 // The context menu for bookmark buttons needs to know which
 // BookmarkNode it is talking about.  For example, "Open All" is
 // disabled if the bookmark node is a folder and has no children.
 @interface BookmarkMenu : NSMenu {
  @private
-  const BookmarkNode* node_;
+  int64 id_;  // id of the bookmark node we represent.
 }
 - (void)setRepresentedObject:(id)object;
-@property const BookmarkNode* node;
+@property int64 id;
 @end
 
diff --git a/chrome/browser/cocoa/bookmark_menu.mm b/chrome/browser/cocoa/bookmark_menu.mm
index cbfdc38..514aaf1 100644
--- a/chrome/browser/cocoa/bookmark_menu.mm
+++ b/chrome/browser/cocoa/bookmark_menu.mm
@@ -7,15 +7,15 @@
 
 @implementation BookmarkMenu
 
-@synthesize node = node_;
+@synthesize id = id_;
 
 // Convention in the bookmark bar controller: the bookmark button
 // cells have a BookmarkNode as their represented object.  This object
 // is placed in a BookmarkMenu at the time a cell is asked for its
 // menu.
 - (void)setRepresentedObject:(id)object {
-  if ([object isKindOfClass:[NSValue class]]) {
-    node_ = static_cast<const BookmarkNode*>([object pointerValue]);
+  if ([object isKindOfClass:[NSNumber class]]) {
+    id_ = static_cast<int64>([object longLongValue]);
   }
 }
 
diff --git a/chrome/browser/cocoa/bookmark_menu_unittest.mm b/chrome/browser/cocoa/bookmark_menu_unittest.mm
index 4063b17..d7797b2 100644
--- a/chrome/browser/cocoa/bookmark_menu_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_menu_unittest.mm
@@ -20,9 +20,10 @@ TEST_F(BookmarkMenuTest, Basics) {
                                                               action:NULL
                                                        keyEquivalent:@""]);
   [menu addItem:item];
-  NSValue* value = [NSValue valueWithPointer:menu.get()];
-  [menu setRepresentedObject:value];
-  EXPECT_EQ((void*)menu.get(), (void*)[menu node]);
+  long long l = 103849459459598948LL;  // arbitrary
+  NSNumber* number = [NSNumber numberWithLongLong:l];
+  [menu setRepresentedObject:number];
+  EXPECT_EQ(l, [menu id]);
 }
 
 }  // namespace