Fix some valgrind failures.

BUG=http://crbug.com/18158, http://crbug.com/30381, http://crbug.com/30371, http://crbug.com/30373
TEST=valgrind bots green.  Bookmark bubble still works.

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

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

7 files changed, 72 insertions, 27 deletions

diff --git a/chrome/browser/cocoa/bookmark_bubble_controller.mm b/chrome/browser/cocoa/bookmark_bubble_controller.mm
index b8ae9c5..43f996b 100644
--- a/chrome/browser/cocoa/bookmark_bubble_controller.mm
+++ b/chrome/browser/cocoa/bookmark_bubble_controller.mm
@@ -17,9 +17,10 @@
 @implementation ChooseAnotherFolder
 @end
 
-@interface BookmarkBubbleController ()
+@interface BookmarkBubbleController (PrivateAPI)
 - (void)updateBookmarkNode;
 - (void)fillInFolderList;
+- (void)parentWindowWillClose:(NSNotification*)notification;
 @end
 
 @implementation BookmarkBubbleController
@@ -54,7 +55,7 @@
     // Watch to see if the parent window closes, and if so, close this one.
     NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
     [center addObserver:self
-               selector:@selector(windowWillClose:)
+               selector:@selector(parentWindowWillClose:)
                    name:NSWindowWillCloseNotification
                  object:parentWindow_];
   }
@@ -66,7 +67,13 @@
   [super dealloc];
 }
 
-- (void)windowWillClose:(NSNotification *)notification {
+- (void)parentWindowWillClose:(NSNotification*)notification {
+  [self close];
+}
+
+- (void)windowWillClose:(NSNotification*)notification {
+  // We caught a close so we don't need to watch for the parent closing.
+  [[NSNotificationCenter defaultCenter] removeObserver:self];
   [self autorelease];
 }
 
diff --git a/chrome/browser/cocoa/bookmark_bubble_controller_unittest.mm b/chrome/browser/cocoa/bookmark_bubble_controller_unittest.mm
index a053140..c98b93d 100644
--- a/chrome/browser/cocoa/bookmark_bubble_controller_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_bubble_controller_unittest.mm
@@ -27,14 +27,17 @@ class BookmarkBubbleControllerTest : public CocoaTest {
 
   virtual void TearDown() {
     [controller_ close];
+    controller_ = nil;
     CocoaTest::TearDown();
   }
 
   // Returns a controller but ownership not transferred.
   // Only one of these will be valid at a time.
   BookmarkBubbleController* ControllerForNode(const BookmarkNode* node) {
-    if (controller_)
+    if (controller_ && !IsWindowClosing()) {
       [controller_ close];
+      controller_ = nil;
+    }
     controller_ = [[BookmarkBubbleController alloc]
                       initWithParentWindow:test_window()
                           topLeftForBubble:TopLeftForBubble()
@@ -42,6 +45,8 @@ class BookmarkBubbleControllerTest : public CocoaTest {
                                       node:node
                          alreadyBookmarked:YES];
     EXPECT_TRUE([controller_ window]);
+    // The window must be gone or we'll fail a unit test with windows left open.
+    [static_cast<InfoBubbleWindow*>([controller_ window]) setDelayOnClose:NO];
     [controller_ showWindow:nil];
     return controller_;
   }
diff --git a/chrome/browser/cocoa/bookmark_editor_controller_unittest.mm b/chrome/browser/cocoa/bookmark_editor_controller_unittest.mm
index f505585..5e9f947 100644
--- a/chrome/browser/cocoa/bookmark_editor_controller_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_editor_controller_unittest.mm
@@ -177,6 +177,7 @@ TEST_F(BookmarkEditorControllerYesNodeTest, YesNodeShowTree) {
 }
 
 class BookmarkEditorControllerTreeTest : public CocoaTest {
+
  public:
   BrowserTestHelper browser_helper_;
   BookmarkEditorController* controller_;
@@ -185,6 +186,8 @@ class BookmarkEditorControllerTreeTest : public CocoaTest {
   const BookmarkNode* group_bb_;
   const BookmarkNode* group_c_;
   const BookmarkNode* bookmark_bb_3_;
+  GURL bb3_url_1_;
+  GURL bb3_url_2_;
 
   BookmarkEditorControllerTreeTest() {
     // Set up a small bookmark hierarchy, which will look as follows:
@@ -210,8 +213,13 @@ class BookmarkEditorControllerTreeTest : public CocoaTest {
     model.AddURL(group_bb_, 0, L"bb-0", GURL("http://bb-0.com"));
     model.AddURL(group_bb_, 1, L"bb-1", GURL("http://bb-1.com"));
     model.AddURL(group_bb_, 2, L"bb-2", GURL("http://bb-2.com"));
-    bookmark_bb_3_ =
-    model.AddURL(group_bb_, 3, L"bb-3", GURL("http://bb-3.com"));
+
+    // To find it later, this bookmark name must always have a URL
+    // of http://bb-3.com or https://bb-3.com
+    bb3_url_1_ = GURL("http://bb-3.com");
+    bb3_url_2_ = GURL("https://bb-3.com");
+    bookmark_bb_3_ = model.AddURL(group_bb_, 3, L"bb-3", bb3_url_1_);
+
     model.AddURL(group_bb_, 4, L"bb-4", GURL("http://bb-4.com"));
     model.AddURL(group_b_, 2, L"b-1", GURL("http://b-2.com"));
     model.AddURL(group_b_, 3, L"b-2", GURL("http://b-3.com"));
@@ -244,6 +252,22 @@ class BookmarkEditorControllerTreeTest : public CocoaTest {
     controller_ = NULL;
     CocoaTest::TearDown();
   }
+
+  // After changing a node, pointers to the node may be invalid.  This
+  // is because the node itself may not be updated; it may removed and
+  // a new one is added in that location.  (Implementation detail of
+  // BookmarkEditorController).  This method updates the class's
+  // bookmark_bb_3_ so that it points to the new node for testing.
+  void UpdateBB3() {
+    std::vector<const BookmarkNode*> nodes;
+    BookmarkModel* model = browser_helper_.profile()->GetBookmarkModel();
+    model->GetNodesByURL(bb3_url_1_, &nodes);
+    if (nodes.size() == 0)
+      model->GetNodesByURL(bb3_url_2_, &nodes);
+    DCHECK(nodes.size());
+    bookmark_bb_3_ = nodes[0];
+  }
+
 };
 
 TEST_F(BookmarkEditorControllerTreeTest, VerifyBookmarkTestModel) {
@@ -301,6 +325,7 @@ TEST_F(BookmarkEditorControllerTreeTest, RenameBookmarkInPlace) {
   const BookmarkNode* oldParent = bookmark_bb_3_->GetParent();
   [controller_ setDisplayName:@"NEW NAME"];
   [controller_ ok:nil];
+  UpdateBB3();
   const BookmarkNode* newParent = bookmark_bb_3_->GetParent();
   ASSERT_EQ(newParent, oldParent);
   int childIndex = newParent->IndexOfChild(bookmark_bb_3_);
@@ -309,8 +334,9 @@ TEST_F(BookmarkEditorControllerTreeTest, RenameBookmarkInPlace) {
 
 TEST_F(BookmarkEditorControllerTreeTest, ChangeBookmarkURLInPlace) {
   const BookmarkNode* oldParent = bookmark_bb_3_->GetParent();
-  [controller_ setDisplayURL:@"http://NEWURL.com"];
+  [controller_ setDisplayURL:@"https://bb-3.com"];
   [controller_ ok:nil];
+  UpdateBB3();
   const BookmarkNode* newParent = bookmark_bb_3_->GetParent();
   ASSERT_EQ(newParent, oldParent);
   int childIndex = newParent->IndexOfChild(bookmark_bb_3_);
@@ -320,6 +346,7 @@ TEST_F(BookmarkEditorControllerTreeTest, ChangeBookmarkURLInPlace) {
 TEST_F(BookmarkEditorControllerTreeTest, ChangeBookmarkGroup) {
   [controller_ selectTestNodeInBrowser:group_c_];
   [controller_ ok:nil];
+  UpdateBB3();
   const BookmarkNode* parent = bookmark_bb_3_->GetParent();
   ASSERT_EQ(parent, group_c_);
   int childIndex = parent->IndexOfChild(bookmark_bb_3_);
@@ -330,6 +357,7 @@ TEST_F(BookmarkEditorControllerTreeTest, ChangeNameAndBookmarkGroup) {
   [controller_ setDisplayName:@"NEW NAME"];
   [controller_ selectTestNodeInBrowser:group_c_];
   [controller_ ok:nil];
+  UpdateBB3();
   const BookmarkNode* parent = bookmark_bb_3_->GetParent();
   ASSERT_EQ(parent, group_c_);
   int childIndex = parent->IndexOfChild(bookmark_bb_3_);
diff --git a/chrome/browser/cocoa/bookmark_menu_unittest.mm b/chrome/browser/cocoa/bookmark_menu_unittest.mm
index 2afb16c..4063b17 100644
--- a/chrome/browser/cocoa/bookmark_menu_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_menu_unittest.mm
@@ -11,15 +11,15 @@
 namespace {
 
 class BookmarkMenuTest : public CocoaTest {
- public:
 };
 
 TEST_F(BookmarkMenuTest, Basics) {
-  scoped_nsobject<BookmarkMenu> menu;
-  menu.reset([[BookmarkMenu alloc] initWithTitle:@"title"]);
-  [menu addItem:[[NSMenuItem alloc] initWithTitle:@"item"
-                                           action:NULL
-                                    keyEquivalent:@""]];
+  scoped_nsobject<BookmarkMenu> menu([[BookmarkMenu alloc]
+                                       initWithTitle:@"title"]);
+  scoped_nsobject<NSMenuItem> item([[NSMenuItem alloc] initWithTitle:@"item"
+                                                              action:NULL
+                                                       keyEquivalent:@""]);
+  [menu addItem:item];
   NSValue* value = [NSValue valueWithPointer:menu.get()];
   [menu setRepresentedObject:value];
   EXPECT_EQ((void*)menu.get(), (void*)[menu node]);
diff --git a/chrome/browser/cocoa/info_bubble_window.h b/chrome/browser/cocoa/info_bubble_window.h
index 925ee98..ab23b10 100644
--- a/chrome/browser/cocoa/info_bubble_window.h
+++ b/chrome/browser/cocoa/info_bubble_window.h
@@ -10,8 +10,13 @@
  @private
   // Is self in the process of closing.
   BOOL closing_;
+  // If NO the window will close immediately instead of fading out.
+  // Default YES.
+  BOOL delayOnClose_;
 }
 
+@property BOOL delayOnClose;
+
 @end
 
 // Methods to only be used by unittests.
diff --git a/chrome/browser/cocoa/info_bubble_window.mm b/chrome/browser/cocoa/info_bubble_window.mm
index b415eb2..47fe38d 100644
--- a/chrome/browser/cocoa/info_bubble_window.mm
+++ b/chrome/browser/cocoa/info_bubble_window.mm
@@ -51,6 +51,8 @@ const NSTimeInterval kOrderOutAnimationDuration = 0.15;
 
 @implementation InfoBubbleWindow
 
+@synthesize delayOnClose = delayOnClose_;
+
 - (id)initWithContentRect:(NSRect)contentRect
                 styleMask:(NSUInteger)aStyle
                   backing:(NSBackingStoreType)bufferingType
@@ -63,6 +65,7 @@ const NSTimeInterval kOrderOutAnimationDuration = 0.15;
     [self setExcludedFromWindowsMenu:YES];
     [self setOpaque:NO];
     [self setHasShadow:YES];
+    delayOnClose_ = YES;
 
     // Start invisible. Will be made visible when ordered front.
     [self setAlphaValue:0.0];
@@ -96,12 +99,17 @@ const NSTimeInterval kOrderOutAnimationDuration = 0.15;
 - (void)close {
   // Block the window from receiving events while it fades out.
   closing_ = YES;
-  // Apply animations to hide self.
-  [NSAnimationContext beginGrouping];
-  [[NSAnimationContext currentContext]
+
+  if (!delayOnClose_) {
+    [self finishCloseAfterAnimation];
+  } else {
+    // Apply animations to hide self.
+    [NSAnimationContext beginGrouping];
+    [[NSAnimationContext currentContext]
       gtm_setDuration:kOrderOutAnimationDuration];
-  [[self animator] setAlphaValue:0.0];
-  [NSAnimationContext endGrouping];
+    [[self animator] setAlphaValue:0.0];
+    [NSAnimationContext endGrouping];
+  }
 }
 
 // Called by InfoBubbleWindowCloser when the window is to be really closed
@@ -151,4 +159,5 @@ const NSTimeInterval kOrderOutAnimationDuration = 0.15;
 - (BOOL)isClosing {
   return closing_;
 }
+
 @end
diff --git a/chrome/test/data/valgrind/unit_tests.gtest_mac.txt b/chrome/test/data/valgrind/unit_tests.gtest_mac.txt
index 30c4d03..a013c37 100644
--- a/chrome/test/data/valgrind/unit_tests.gtest_mac.txt
+++ b/chrome/test/data/valgrind/unit_tests.gtest_mac.txt
@@ -9,13 +9,6 @@ WebDropTargetTest.Data
 # Following tests fail on valgrind.
 # See http://crbug.com/30370.
 AutocompleteTextFieldEditorTest.CutCopyTest
-# See http://crbug.com/30371.
-BookmarkBubbleControllerTest.TestNewParentSameName
-# See http://crbug.com/30373.
-BookmarkEditorControllerTreeTest.RenameBookmarkInPlace
-BookmarkEditorControllerTreeTest.ChangeBookmarkURLInPlace
-BookmarkEditorControllerTreeTest.ChangeBookmarkGroup
-BookmarkEditorControllerTreeTest.ChangeNameAndBookmarkGroup
 # See http://crbug.com/30375.
 DownloadUtilTest.AddFileToPasteboardTest
 # See http://crbug.com/30378.
@@ -25,8 +18,6 @@ FindPasteboardTest.SendsNotificationWhenTextChanges
 RenderViewTest.MacTestCmdUp
 
 # Folowing tests do not pass memcheck test.
-# See http://crbug.com/30381.
-BookmarkMenuTest.Basics
 # See http://crbug.com/30384.
 ChromeBrowserWindowTest.DoesHideTitle
 # See http://crbug.com/30386.