Merge 47632 - Speculative bookmark crash fix.

The crash in http://crbug.com/43881 might be a reference to a dangling pointer. 

One theory for this crash is:
 use bookmark sync
 1st machine: open a context menu on a bookmark button
 2nd machine: delete that bookmark (or it's parent), or rearrange things somehow
 1st machine: select Delete on the menu

Fix is to close context menus when we see a model change.

BUG=43881

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

TBR=jrg@chromium.org
Review URL: http://codereview.chromium.org/2095029

git-svn-id: svn://svn.chromium.org/chrome/branches/375/src@48085 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 59 insertions, 4 deletions

diff --git a/chrome/browser/cocoa/bookmark_bar_controller.h b/chrome/browser/cocoa/bookmark_bar_controller.h
index 29952b8..6b9e428 100644
--- a/chrome/browser/cocoa/bookmark_bar_controller.h
+++ b/chrome/browser/cocoa/bookmark_bar_controller.h
@@ -334,7 +334,8 @@ willAnimateFromState:(bookmarks::VisualState)oldState
 - (BOOL)isEventAnExitEvent:(NSEvent*)event;
 - (id)folderTarget;
 - (int)displayedButtonCount;
-
+- (NSMenu*)buttonContextMenu;
+- (void)setButtonContextMenu:(id)menu;
 @end
 
 // The (internal) |NSPasteboard| type string for bookmark button drags, used for
diff --git a/chrome/browser/cocoa/bookmark_bar_controller.mm b/chrome/browser/cocoa/bookmark_bar_controller.mm
index ca8110c..bbb43d0 100644
--- a/chrome/browser/cocoa/bookmark_bar_controller.mm
+++ b/chrome/browser/cocoa/bookmark_bar_controller.mm
@@ -549,6 +549,16 @@ const NSTimeInterval kBookmarkBarAnimationDuration = 0.12;
   return displayedButtonCount_;
 }
 
+- (NSMenu*)buttonContextMenu {
+  return buttonContextMenu_;
+}
+
+// Intentionally ignores ownership issues; used for testing and we try
+// to minimize touching the object passed in (likely a mock).
+- (void)setButtonContextMenu:(id)menu {
+  buttonContextMenu_ = menu;
+}
+
 // Keep the "no items" label centered in response to a frame size change.
 - (void)centerNoItemsLabel {
   // Note that this computation is done in the parent's coordinate system,
@@ -1727,8 +1737,19 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
   [self configureOffTheSideButtonContentsAndVisibility];
 }
 
+// To avoid problems with sync, changes that may impact the current
+// bookmark (e.g. deletion) make sure context menus are closed.  This
+// prevents deleting a node which no longer exists.
+- (void)cancelMenuTracking {
+  [buttonContextMenu_ cancelTracking];
+  [buttonFolderContextMenu_ cancelTracking];
+}
+
 - (void)nodeAdded:(BookmarkModel*)model
            parent:(const BookmarkNode*)newParent index:(int)newIndex {
+  // If a context menu is open, close it.
+  [self cancelMenuTracking];
+
   const BookmarkNode* newNode = newParent->GetChild(newIndex);
   id<BookmarkButtonControllerProtocol> newController =
       [self controllerForNode:newParent];
@@ -1740,6 +1761,9 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
 
 - (void)nodeRemoved:(BookmarkModel*)model
              parent:(const BookmarkNode*)oldParent index:(int)index {
+  // If a context menu is open, close it.
+  [self cancelMenuTracking];
+
   // Locate the parent node. The parent may not be showing, in which case
   // we do nothing.
   id<BookmarkButtonControllerProtocol> parentController =
diff --git a/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm b/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm
index a80b7e5..bb4e8b4 100644
--- a/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm
@@ -1329,6 +1329,32 @@ TEST_F(BookmarkBarControllerTest, NodeDeletedWhileMenuIsOpen) {
   [bar_ openAllBookmarksIncognitoWindow:item];
 }
 
+TEST_F(BookmarkBarControllerTest, NodeDeletedWhileContextMenuIsOpen) {
+  BookmarkModel* model = helper_.profile()->GetBookmarkModel();
+  [bar_ loaded:model];
+
+  const BookmarkNode* parent = model->GetBookmarkBarNode();
+  const BookmarkNode* folder = model->AddGroup(parent,
+                                               parent->GetChildCount(),
+                                               L"group");
+  const BookmarkNode* framma = model->AddURL(folder, folder->GetChildCount(),
+                                             L"f1",
+                                             GURL("http://framma-lamma.com"));
+
+  // Mock in a menu
+  id origMenu = [bar_ buttonContextMenu];
+  id fakeMenu = [OCMockObject partialMockForObject:origMenu];
+  [[fakeMenu expect] cancelTracking];
+  [bar_ setButtonContextMenu:fakeMenu];
+
+  // Force a delete which should cancelTracking on the menu.
+  model->Remove(framma->GetParent(), framma->GetParent()->IndexOfChild(framma));
+
+  // Restore, then confirm cancelTracking was called.
+  [bar_ setButtonContextMenu:origMenu];
+  [fakeMenu verify];
+}
+
 TEST_F(BookmarkBarControllerTest, CloseFolderOnAnimate) {
   BookmarkModel* model = helper_.profile()->GetBookmarkModel();
   const BookmarkNode* parent = model->GetBookmarkBarNode();
diff --git a/chrome/browser/cocoa/bookmark_model_observer_for_cocoa.h b/chrome/browser/cocoa/bookmark_model_observer_for_cocoa.h
index 398b2b8..b5ba908 100644
--- a/chrome/browser/cocoa/bookmark_model_observer_for_cocoa.h
+++ b/chrome/browser/cocoa/bookmark_model_observer_for_cocoa.h
@@ -33,6 +33,10 @@ class BookmarkModelObserverForCocoa : public BookmarkModelObserver {
   // IBOutlet.  The arg passed is nil.
   // Many notifications happen independently of node
   // (e.g. BeingDeleted), so |node| can be nil.
+  //
+  // |object| is NOT retained, since the expected use case is for
+  // ||object| to own the BookmarkModelObserverForCocoa and we don't
+  // want a retain cycle.
   BookmarkModelObserverForCocoa(const BookmarkNode* node,
                                 BookmarkModel* model,
                                 NSObject* object,
@@ -40,7 +44,7 @@ class BookmarkModelObserverForCocoa : public BookmarkModelObserver {
     DCHECK(model);
     node_ = node;
     model_ = model;
-    object_.reset([object retain]);
+    object_ = object;
     selector_ = selector;
     model_->AddObserver(this);
   }
@@ -69,7 +73,7 @@ class BookmarkModelObserverForCocoa : public BookmarkModelObserver {
   }
   virtual void BookmarkNodeChanged(BookmarkModel* model,
                                    const BookmarkNode* node) {
-    if (node_ == node)
+    if ((node_ == node) || (!node_))
       Notify();
   }
   virtual void BookmarkImportBeginning(BookmarkModel* model) {
@@ -98,7 +102,7 @@ class BookmarkModelObserverForCocoa : public BookmarkModelObserver {
  private:
   const BookmarkNode* node_;  // Weak; owned by a BookmarkModel.
   BookmarkModel* model_;  // Weak; it is owned by a Profile.
-  scoped_nsobject<NSObject> object_;
+  NSObject* object_; // Weak, like a delegate.
   SEL selector_;
 
   void Notify() {