Make infobars ignore button clicks when closing.

This also fixes an unfiled bug where clicking the "deny" button on the Mac before translate infobar would double-count the denial.

BUG=54253,90985
TEST=none
Review URL: http://codereview.chromium.org/7981045

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

14 files changed, 161 insertions, 125 deletions

diff --git a/chrome/browser/ui/cocoa/extensions/extension_infobar_controller.mm b/chrome/browser/ui/cocoa/extensions/extension_infobar_controller.mm
index c62d2ea..f00862e 100644
--- a/chrome/browser/ui/cocoa/extensions/extension_infobar_controller.mm
+++ b/chrome/browser/ui/cocoa/extensions/extension_infobar_controller.mm
@@ -228,6 +228,11 @@ class InfobarBridge : public ExtensionInfoBarDelegate::DelegateObserver,
            object:window_];
 }
 
+- (void)infobarWillClose {
+  [self disablePopUpMenu:contextMenu_.get()];
+  [super infobarWillClose];
+}
+
 - (void)extensionViewFrameChanged {
   [self adjustExtensionViewSize];
 
diff --git a/chrome/browser/ui/cocoa/infobars/infobar_controller.h b/chrome/browser/ui/cocoa/infobars/infobar_controller.h
index b5d080d..32e5633 100644
--- a/chrome/browser/ui/cocoa/infobars/infobar_controller.h
+++ b/chrome/browser/ui/cocoa/infobars/infobar_controller.h
@@ -48,15 +48,27 @@ class TabContentsWrapper;
 - (id)initWithDelegate:(InfoBarDelegate*)delegate
                  owner:(TabContentsWrapper*)owner;
 
+// Returns YES if the infobar is owned.  If this is NO, it is not safe to call
+// any delegate functions, since they might attempt to access the owner.  Code
+// should generally just do nothing at all in this case (once we're closing, all
+// controls can safely just go dead).
+- (BOOL)isOwned;
+
 // Called when someone clicks on the OK or Cancel buttons.  Subclasses
 // must override if they do not hide the buttons.
 - (void)ok:(id)sender;
 - (void)cancel:(id)sender;
 
-// Called when someone clicks on the close button.  Dismisses the
-// infobar without taking any action.
+// Called when someone clicks on the close button.  Dismisses the infobar
+// without taking any action.
+// NOTE: Subclasses should not call this to close the infobar as it will lead to
+// errors in stat counting.  Call -removeSelf instead.
 - (IBAction)dismiss:(id)sender;
 
+// Asks the container controller to remove the infobar for this delegate.  This
+// call will trigger a notification that starts the infobar animating closed.
+- (void)removeSelf;
+
 // Returns a pointer to this controller's view, cast as an AnimatableView.
 - (AnimatableView*)animatableView;
 
@@ -86,6 +98,12 @@ class TabContentsWrapper;
 
 @end
 
+@interface InfoBarController (Protected)
+// Closes and disables the provided menu.  Subclasses should call this for each
+// popup menu in -infobarWillClose.
+- (void)disablePopUpMenu:(NSMenu*)menu;
+@end
+
 /////////////////////////////////////////////////////////////////////////
 // InfoBarController subclasses, one for each InfoBarDelegate
 // subclass.  Each of these subclasses overrides addAdditionalControls to
diff --git a/chrome/browser/ui/cocoa/infobars/infobar_controller.mm b/chrome/browser/ui/cocoa/infobars/infobar_controller.mm
index 6e9e79f..bc7889d 100644
--- a/chrome/browser/ui/cocoa/infobars/infobar_controller.mm
+++ b/chrome/browser/ui/cocoa/infobars/infobar_controller.mm
@@ -34,10 +34,6 @@ const float kAnimateCloseDuration = 0.12;
 // Sets |label_| based on |labelPlaceholder_|, sets |labelPlaceholder_| to nil.
 - (void)initializeLabel;
 
-// Asks the container controller to remove the infobar for this delegate.  This
-// call will trigger a notification that starts the infobar animating closed.
-- (void)removeSelf;
-
 // Performs final cleanup after an animation is finished or stopped, including
 // notifying the InfoBarDelegate that the infobar was closed and removing the
 // infobar from its container, if necessary.
@@ -105,6 +101,10 @@ const float kAnimateCloseDuration = 0.12;
   return YES;
 }
 
+- (BOOL)isOwned {
+  return !!owner_;
+}
+
 // Called when someone clicks on the ok button.
 - (void)ok:(id)sender {
   // Subclasses must override this method if they do not hide the ok button.
@@ -119,12 +119,17 @@ const float kAnimateCloseDuration = 0.12;
 
 // Called when someone clicks on the close button.
 - (void)dismiss:(id)sender {
-  if (delegate_)
-    delegate_->InfoBarDismissed();
-
+  if (![self isOwned])
+    return;
+  delegate_->InfoBarDismissed();
   [self removeSelf];
 }
 
+- (void)removeSelf {
+  DCHECK(owner_);
+  owner_->infobar_tab_helper()->RemoveInfoBar(delegate_);
+}
+
 - (AnimatableView*)animatableView {
   return static_cast<AnimatableView*>([self view]);
 }
@@ -165,10 +170,6 @@ const float kAnimateCloseDuration = 0.12;
   // currently-running animations, so do not set |infoBarClosing_| until after
   // starting the animation.
   infoBarClosing_ = YES;
-
-  // The owner called this method to close the infobar, so there will
-  // be no need to forward future remove events to the owner.
-  owner_ = NULL;
 }
 
 - (void)addAdditionalControls {
@@ -176,7 +177,7 @@ const float kAnimateCloseDuration = 0.12;
 }
 
 - (void)infobarWillClose {
-  // Default implementation does nothing.
+  owner_ = NULL;
 }
 
 - (void)removeButtons {
@@ -190,6 +191,20 @@ const float kAnimateCloseDuration = 0.12;
   [label_.get() setFrame:labelFrame];
 }
 
+- (void)disablePopUpMenu:(NSMenu*)menu {
+  // Remove the menu if visible.
+  [menu cancelTracking];
+
+  // If the menu is re-opened, prevent queries to update items.
+  [menu setDelegate:nil];
+
+  // Prevent target/action messages to the controller.
+  for (NSMenuItem* item in [menu itemArray]) {
+    [item setEnabled:NO];
+    [item setTarget:nil];
+  }
+}
+
 @end
 
 @implementation InfoBarController (PrivateMethods)
@@ -208,16 +223,6 @@ const float kAnimateCloseDuration = 0.12;
   [label_.get() setDelegate:self];
 }
 
-- (void)removeSelf {
-  // TODO(rohitrao): This method can be called even if the infobar has already
-  // been removed and |delegate_| is NULL.  Is there a way to rewrite the code
-  // so that inner event loops don't cause us to try and remove the infobar
-  // twice?  http://crbug.com/54253
-  if (owner_)
-    owner_->infobar_tab_helper()->RemoveInfoBar(delegate_);
-  owner_ = NULL;
-}
-
 - (void)cleanUpAfterAnimation:(BOOL)finished {
   // Don't need to do any cleanup if the bar was animating open.
   if (!infoBarClosing_)
@@ -298,20 +303,12 @@ const float kAnimateCloseDuration = 0.12;
 // is called by the InfobarTextField on its delegate (the
 // LinkInfoBarController).
 - (void)linkClicked {
+  if (![self isOwned])
+    return;
   WindowOpenDisposition disposition =
       event_utils::WindowOpenDispositionFromNSEvent([NSApp currentEvent]);
-  if (delegate_ &&
-      delegate_->AsLinkInfoBarDelegate()->LinkClicked(disposition)) {
-    // Call |-removeSelf| on the outermost runloop to force a delay. As shown in
-    // <http://crbug.com/87201>, the second click event can be delivered after
-    // the animation finishes (and this gets released and deallocated), which
-    // leads to zombie messaging. Unfortunately, the order between the animation
-    // finishing and the click event being delivered is nondeterministic, so
-    // this hack is the best that can be done.
-    [self performSelector:@selector(removeSelf)
-               withObject:nil
-               afterDelay:0.0];
-  }
+  if (delegate_->AsLinkInfoBarDelegate()->LinkClicked(disposition))
+    [self removeSelf];
 }
 
 @end
@@ -324,13 +321,17 @@ const float kAnimateCloseDuration = 0.12;
 
 // Called when someone clicks on the "OK" button.
 - (IBAction)ok:(id)sender {
-  if (delegate_ && delegate_->AsConfirmInfoBarDelegate()->Accept())
+  if (![self isOwned])
+    return;
+  if (delegate_->AsConfirmInfoBarDelegate()->Accept())
     [self removeSelf];
 }
 
 // Called when someone clicks on the "Cancel" button.
 - (IBAction)cancel:(id)sender {
-  if (delegate_ && delegate_->AsConfirmInfoBarDelegate()->Cancel())
+  if (![self isOwned])
+    return;
+  if (delegate_->AsConfirmInfoBarDelegate()->Cancel())
     [self removeSelf];
 }
 
@@ -429,10 +430,11 @@ const float kAnimateCloseDuration = 0.12;
 // is called by the InfobarTextField on its delegate (the
 // LinkInfoBarController).
 - (void)linkClicked {
+  if (![self isOwned])
+    return;
   WindowOpenDisposition disposition =
       event_utils::WindowOpenDispositionFromNSEvent([NSApp currentEvent]);
-  if (delegate_ &&
-      delegate_->AsConfirmInfoBarDelegate()->LinkClicked(disposition))
+  if (delegate_->AsConfirmInfoBarDelegate()->LinkClicked(disposition))
     [self removeSelf];
 }
 
diff --git a/chrome/browser/ui/cocoa/infobars/infobar_controller_unittest.mm b/chrome/browser/ui/cocoa/infobars/infobar_controller_unittest.mm
index 460345d..d4b24c1 100644
--- a/chrome/browser/ui/cocoa/infobars/infobar_controller_unittest.mm
+++ b/chrome/browser/ui/cocoa/infobars/infobar_controller_unittest.mm
@@ -7,13 +7,16 @@
 #include "base/memory/scoped_nsobject.h"
 #include "base/string_util.h"
 #include "base/sys_string_conversions.h"
+#include "chrome/browser/infobars/infobar_tab_helper.h"
 #include "chrome/browser/tab_contents/confirm_infobar_delegate.h"
-#import "chrome/browser/ui/cocoa/cocoa_test_helper.h"
+#include "chrome/browser/ui/cocoa/cocoa_profile_test.h"
 #import "chrome/browser/ui/cocoa/infobars/infobar_container_controller.h"
 #import "chrome/browser/ui/cocoa/infobars/infobar_controller.h"
 #include "chrome/browser/ui/cocoa/infobars/mock_confirm_infobar_delegate.h"
 #include "chrome/browser/ui/cocoa/infobars/mock_link_infobar_delegate.h"
 #include "chrome/browser/ui/cocoa/run_loop_testing.h"
+#include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
+#import "content/browser/tab_contents/tab_contents.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
 
@@ -89,16 +92,19 @@ namespace {
 ///////////////////////////////////////////////////////////////////////////
 // Test fixtures
 
-class LinkInfoBarControllerTest : public CocoaTest,
+class LinkInfoBarControllerTest : public CocoaProfileTest,
                                   public MockLinkInfoBarDelegate::Owner {
  public:
   virtual void SetUp() {
-    CocoaTest::SetUp();
+    CocoaProfileTest::SetUp();
+    tab_contents_.reset(new TabContentsWrapper(new TabContents(profile(), NULL,
+        MSG_ROUTING_NONE, NULL, NULL)));
+    tab_contents_->infobar_tab_helper()->set_infobars_enabled(false);
 
     delegate_ = new MockLinkInfoBarDelegate(this);
-    controller_.reset(
-        [[TestLinkInfoBarController alloc] initWithDelegate:delegate_
-                                                      owner:NULL]);
+    controller_.reset([[TestLinkInfoBarController alloc]
+        initWithDelegate:delegate_
+                   owner:tab_contents_.get()]);
     container_.reset(
         [[InfoBarContainerTest alloc] initWithController:controller_]);
     [controller_ setContainerController:container_];
@@ -109,7 +115,7 @@ class LinkInfoBarControllerTest : public CocoaTest,
   virtual void TearDown() {
     if (delegate_)
       delete delegate_;
-    CocoaTest::TearDown();
+    CocoaProfileTest::TearDown();
   }
 
  protected:
@@ -126,18 +132,23 @@ class LinkInfoBarControllerTest : public CocoaTest,
     closed_delegate_link_clicked_ = delegate_->link_clicked();
     delegate_ = NULL;
   }
+
+  scoped_ptr<TabContentsWrapper> tab_contents_;
 };
 
-class ConfirmInfoBarControllerTest : public CocoaTest,
+class ConfirmInfoBarControllerTest : public CocoaProfileTest,
                                      public MockConfirmInfoBarDelegate::Owner {
  public:
   virtual void SetUp() {
-    CocoaTest::SetUp();
+    CocoaProfileTest::SetUp();
+    tab_contents_.reset(new TabContentsWrapper(new TabContents(profile(), NULL,
+        MSG_ROUTING_NONE, NULL, NULL)));
+    tab_contents_->infobar_tab_helper()->set_infobars_enabled(false);
 
     delegate_ = new MockConfirmInfoBarDelegate(this);
-    controller_.reset(
-        [[TestConfirmInfoBarController alloc] initWithDelegate:delegate_
-                                                         owner:NULL]);
+    controller_.reset([[TestConfirmInfoBarController alloc]
+        initWithDelegate:delegate_
+                   owner:tab_contents_.get()]);
     container_.reset(
         [[InfoBarContainerTest alloc] initWithController:controller_]);
     [controller_ setContainerController:container_];
@@ -150,7 +161,7 @@ class ConfirmInfoBarControllerTest : public CocoaTest,
   virtual void TearDown() {
     if (delegate_)
       delete delegate_;
-    CocoaTest::TearDown();
+    CocoaProfileTest::TearDown();
   }
 
  protected:
@@ -171,6 +182,8 @@ class ConfirmInfoBarControllerTest : public CocoaTest,
     closed_delegate_link_clicked_ = delegate_->link_clicked();
     delegate_ = NULL;
   }
+
+  scoped_ptr<TabContentsWrapper> tab_contents_;
 };
 
 
@@ -186,7 +199,7 @@ TEST_F(LinkInfoBarControllerTest, ShowAndDismiss) {
   EXPECT_TRUE(delegate_->icon_accessed());
 
   // Check that dismissing the infobar deletes the delegate.
-  [controller_ dismiss:nil];
+  [controller_ removeSelf];
   ASSERT_TRUE(delegate_closed());
   EXPECT_FALSE(closed_delegate_link_clicked_);
 }
@@ -233,7 +246,7 @@ TEST_F(ConfirmInfoBarControllerTest, ShowAndDismiss) {
             base::SysNSStringToUTF8([controller_.get() labelString]));
 
   // Check that dismissing the infobar deletes the delegate.
-  [controller_ dismiss:nil];
+  [controller_ removeSelf];
   ASSERT_TRUE(delegate_closed());
   EXPECT_FALSE(closed_delegate_ok_clicked_);
   EXPECT_FALSE(closed_delegate_cancel_clicked_);
diff --git a/chrome/browser/ui/cocoa/translate/before_translate_infobar_controller.mm b/chrome/browser/ui/cocoa/translate/before_translate_infobar_controller.mm
index 2f1a205..5f42ede 100644
--- a/chrome/browser/ui/cocoa/translate/before_translate_infobar_controller.mm
+++ b/chrome/browser/ui/cocoa/translate/before_translate_infobar_controller.mm
@@ -106,16 +106,16 @@ NSButton* CreateNSButtonWithResourceIDAndParameter(
 
 // This is called when the "Never Translate [language]" button is pressed.
 - (void)neverTranslate:(id)sender {
-  TranslateInfoBarDelegate* delegate = [self delegate];
-  if (delegate)
-    delegate->NeverTranslatePageLanguage();
+  if (![self isOwned])
+    return;
+  [self delegate]->NeverTranslatePageLanguage();
 }
 
 // This is called when the "Always Translate [language]" button is pressed.
 - (void)alwaysTranslate:(id)sender {
-  TranslateInfoBarDelegate* delegate = [self delegate];
-  if (delegate)
-    delegate->AlwaysTranslatePageLanguage();
+  if (![self isOwned])
+    return;
+  [self delegate]->AlwaysTranslatePageLanguage();
 }
 
 - (bool)verifyLayout {
diff --git a/chrome/browser/ui/cocoa/translate/translate_infobar_base.mm b/chrome/browser/ui/cocoa/translate/translate_infobar_base.mm
index 08655e7..291fb01 100644
--- a/chrome/browser/ui/cocoa/translate/translate_infobar_base.mm
+++ b/chrome/browser/ui/cocoa/translate/translate_infobar_base.mm
@@ -108,26 +108,6 @@ void AddMenuItem(NSMenu *menu, id target, SEL selector, NSString* title,
 
 }  // namespace TranslateInfoBarUtilities
 
-namespace {
-
-// Helper to close and disable popup menus when the infobar closes.
-// Disabling the popup button would cause a distracting visual change.
-void DisablePopUpMenu(NSMenu *menu) {
-  // Remove the menu if visible.
-  [menu cancelTracking];
-
-  // If the menu is re-opened, prevent queries to update items.
-  [menu setDelegate:nil];
-
-  // Prevent target/action messages to the controller.
-  for (NSMenuItem* item in [menu itemArray]) {
-    [item setEnabled:NO];
-    [item setTarget:nil];
-  }
-}
-
-}  // namespace
-
 // TranslateInfoBarDelegate views specific method:
 InfoBar* TranslateInfoBarDelegate::CreateInfoBar(TabContentsWrapper* owner) {
   TranslateInfoBarControllerBase* infobar_controller = NULL;
@@ -492,9 +472,9 @@ InfoBar* TranslateInfoBarDelegate::CreateInfoBar(TabContentsWrapper* owner) {
 }
 
 - (void)infobarWillClose {
-  DisablePopUpMenu([fromLanguagePopUp_ menu]);
-  DisablePopUpMenu([toLanguagePopUp_ menu]);
-  DisablePopUpMenu([optionsPopUp_ menu]);
+  [self disablePopUpMenu:[fromLanguagePopUp_ menu]];
+  [self disablePopUpMenu:[toLanguagePopUp_ menu]];
+  [self disablePopUpMenu:[optionsPopUp_ menu]];
   [super infobarWillClose];
 }
 
@@ -520,47 +500,42 @@ InfoBar* TranslateInfoBarDelegate::CreateInfoBar(TabContentsWrapper* owner) {
 
 // Called when "Translate" button is clicked.
 - (IBAction)ok:(id)sender {
-  // The delegate may be NULL if the infobar was closed.
+  if (![self isOwned])
+    return;
   TranslateInfoBarDelegate* delegate = [self delegate];
-  if (delegate) {
-    TranslateInfoBarDelegate::Type state = delegate->type();
-    DCHECK(state == TranslateInfoBarDelegate::BEFORE_TRANSLATE ||
-           state == TranslateInfoBarDelegate::TRANSLATION_ERROR);
-    delegate->Translate();
-  }
+  TranslateInfoBarDelegate::Type state = delegate->type();
+  DCHECK(state == TranslateInfoBarDelegate::BEFORE_TRANSLATE ||
+         state == TranslateInfoBarDelegate::TRANSLATION_ERROR);
+  delegate->Translate();
   UMA_HISTOGRAM_COUNTS("Translate.Translate", 1);
 }
 
 // Called when someone clicks on the "Nope" button.
 - (IBAction)cancel:(id)sender {
-  // The delegate may be NULL if the infobar was closed.
+  if (![self isOwned])
+    return;
   TranslateInfoBarDelegate* delegate = [self delegate];
-  if (delegate) {
-    DCHECK(delegate->type() == TranslateInfoBarDelegate::BEFORE_TRANSLATE);
-    delegate->TranslationDeclined();
-    UMA_HISTOGRAM_COUNTS("Translate.DeclineTranslate", 1);
-  }
-  [super dismiss:nil];
+  DCHECK(delegate->type() == TranslateInfoBarDelegate::BEFORE_TRANSLATE);
+  delegate->TranslationDeclined();
+  UMA_HISTOGRAM_COUNTS("Translate.DeclineTranslate", 1);
+  [super removeSelf];
 }
 
 - (void)messageButtonPressed:(id)sender {
-  // The delegate may be NULL if the infobar was closed.
-  TranslateInfoBarDelegate* delegate = [self delegate];
-  if (delegate)
-    delegate->MessageInfoBarButtonPressed();
+  if (![self isOwned])
+    return;
+  [self delegate]->MessageInfoBarButtonPressed();
 }
 
 - (IBAction)showOriginal:(id)sender {
-  // The delegate may be NULL if the infobar was closed.
-  TranslateInfoBarDelegate* delegate = [self delegate];
-  if (delegate)
-    delegate->RevertTranslation();
+  if (![self isOwned])
+    return;
+  [self delegate]->RevertTranslation();
 }
 
 // Called when any of the language drop down menus are changed.
 - (void)languageMenuChanged:(id)item {
-  // The delegate may be NULL if the infobar was closed.
-  if (![self delegate])
+  if (![self isOwned])
     return;
   if ([item respondsToSelector:@selector(tag)]) {
     int cmd = [item tag];
@@ -579,6 +554,8 @@ InfoBar* TranslateInfoBarDelegate::CreateInfoBar(TabContentsWrapper* owner) {
 
 // Called when the options menu is changed.
 - (void)optionsMenuChanged:(id)item {
+  if (![self isOwned])
+    return;
   if ([item respondsToSelector:@selector(tag)]) {
     int cmd = [item tag];
     // Danger Will Robinson! : This call can release the infobar (e.g. invoking
diff --git a/chrome/browser/ui/views/infobars/after_translate_infobar.cc b/chrome/browser/ui/views/infobars/after_translate_infobar.cc
index a661079..e6879da 100644
--- a/chrome/browser/ui/views/infobars/after_translate_infobar.cc
+++ b/chrome/browser/ui/views/infobars/after_translate_infobar.cc
@@ -126,6 +126,8 @@ void AfterTranslateInfoBar::ViewHierarchyChanged(bool is_add,
 
 void AfterTranslateInfoBar::ButtonPressed(views::Button* sender,
                                           const views::Event& event) {
+  if (!owned())
+    return;  // We're closing; don't call anything, it might access the owner.
   if (sender == revert_button_)
     GetDelegate()->RevertTranslation();
   else
@@ -163,6 +165,8 @@ void AfterTranslateInfoBar::TargetLanguageChanged() {
 }
 
 void AfterTranslateInfoBar::RunMenu(View* source, const gfx::Point& pt) {
+  if (!owned())
+    return;  // We're closing; don't call anything, it might access the owner.
   ui::MenuModel* menu_model = NULL;
   views::MenuButton* button = NULL;
   views::MenuItemView::AnchorPosition anchor = views::MenuItemView::TOPLEFT;
@@ -179,5 +183,4 @@ void AfterTranslateInfoBar::RunMenu(View* source, const gfx::Point& pt) {
     anchor = views::MenuItemView::TOPRIGHT;
   }
   RunMenuAt(menu_model, button, anchor);
-  // TODO(pkasting): this may be deleted after rewrite.
 }
diff --git a/chrome/browser/ui/views/infobars/before_translate_infobar.cc b/chrome/browser/ui/views/infobars/before_translate_infobar.cc
index d8d557b..1a3c831 100644
--- a/chrome/browser/ui/views/infobars/before_translate_infobar.cc
+++ b/chrome/browser/ui/views/infobars/before_translate_infobar.cc
@@ -161,6 +161,8 @@ int BeforeTranslateInfoBar::ContentMinimumWidth() const {
 
 void BeforeTranslateInfoBar::ButtonPressed(views::Button* sender,
                                            const views::Event& event) {
+  if (!owned())
+    return;  // We're closing; don't call anything, it might access the owner.
   TranslateInfoBarDelegate* delegate = GetDelegate();
   if (sender == accept_button_) {
     delegate->Translate();
@@ -186,6 +188,8 @@ void BeforeTranslateInfoBar::OriginalLanguageChanged() {
 }
 
 void BeforeTranslateInfoBar::RunMenu(View* source, const gfx::Point& pt) {
+  if (!owned())
+    return;  // We're closing; don't call anything, it might access the owner.
   ui::MenuModel* menu_model = NULL;
   views::MenuButton* button = NULL;
   views::MenuItemView::AnchorPosition anchor = views::MenuItemView::TOPLEFT;
@@ -199,5 +203,4 @@ void BeforeTranslateInfoBar::RunMenu(View* source, const gfx::Point& pt) {
     anchor = views::MenuItemView::TOPRIGHT;
   }
   RunMenuAt(menu_model, button, anchor);
-  // TODO(pkasting): this may be deleted after rewrite.
 }
diff --git a/chrome/browser/ui/views/infobars/confirm_infobar.cc b/chrome/browser/ui/views/infobars/confirm_infobar.cc
index d1decd0..20b5dde 100644
--- a/chrome/browser/ui/views/infobars/confirm_infobar.cc
+++ b/chrome/browser/ui/views/infobars/confirm_infobar.cc
@@ -98,6 +98,8 @@ void ConfirmInfoBar::ViewHierarchyChanged(bool is_add,
 
 void ConfirmInfoBar::ButtonPressed(views::Button* sender,
                                    const views::Event& event) {
+  if (!owned())
+    return;  // We're closing; don't call anything, it might access the owner.
   ConfirmInfoBarDelegate* delegate = GetDelegate();
   if ((ok_button_ != NULL) && sender == ok_button_) {
     if (delegate->Accept())
@@ -122,6 +124,8 @@ int ConfirmInfoBar::ContentMinimumWidth() const {
 }
 
 void ConfirmInfoBar::LinkClicked(views::Link* source, int event_flags) {
+  if (!owned())
+    return;  // We're closing; don't call anything, it might access the owner.
   DCHECK(link_ != NULL);
   DCHECK_EQ(link_, source);
   if (GetDelegate()->LinkClicked(
diff --git a/chrome/browser/ui/views/infobars/extension_infobar.cc b/chrome/browser/ui/views/infobars/extension_infobar.cc
index f9e5fd1..467a8dc 100644
--- a/chrome/browser/ui/views/infobars/extension_infobar.cc
+++ b/chrome/browser/ui/views/infobars/extension_infobar.cc
@@ -137,6 +137,8 @@ void ExtensionInfoBar::OnDelegateDeleted() {
 }
 
 void ExtensionInfoBar::RunMenu(View* source, const gfx::Point& pt) {
+  if (!owned())
+    return;  // We're closing; don't call anything, it might access the owner.
   const Extension* extension = GetDelegate()->extension_host()->extension();
   if (!extension->ShowConfigureContextMenus())
     return;
@@ -148,7 +150,6 @@ void ExtensionInfoBar::RunMenu(View* source, const gfx::Point& pt) {
       new ExtensionContextMenuModel(extension, browser, NULL);
   DCHECK_EQ(source, menu_);
   RunMenuAt(options_menu_contents.get(), menu_, views::MenuItemView::TOPLEFT);
-  // TODO(pkasting): this may be deleted after rewrite.
 }
 
 ExtensionInfoBarDelegate* ExtensionInfoBar::GetDelegate() {
diff --git a/chrome/browser/ui/views/infobars/infobar_view.cc b/chrome/browser/ui/views/infobars/infobar_view.cc
index 87eb191..d1124d7 100644
--- a/chrome/browser/ui/views/infobars/infobar_view.cc
+++ b/chrome/browser/ui/views/infobars/infobar_view.cc
@@ -68,6 +68,10 @@ InfoBarView::InfoBarView(TabContentsWrapper* owner, InfoBarDelegate* delegate)
 }
 
 InfoBarView::~InfoBarView() {
+  // We should have closed any open menus in PlatformSpecificHide(), then
+  // subclasses' RunMenu() functions should have prevented opening any new ones
+  // once we became unowned.
+  DCHECK(!menu_runner_.get());
 }
 
 // static
@@ -259,12 +263,9 @@ void InfoBarView::PaintChildren(gfx::Canvas* canvas) {
 
 void InfoBarView::ButtonPressed(views::Button* sender,
                                 const views::Event& event) {
+  DCHECK(owned());  // Subclasses should never call us while we're closing.
   if (sender == close_button_) {
-    // If we're not owned, we're already closing, so don't call
-    // InfoBarDismissed(), since this can lead to us double-recording
-    // dismissals.
-    if (delegate() && owned())
-      delegate()->InfoBarDismissed();
+    delegate()->InfoBarDismissed();
     RemoveSelf();
   }
 }
@@ -294,16 +295,15 @@ const InfoBarContainer::Delegate* InfoBarView::container_delegate() const {
 void InfoBarView::RunMenuAt(ui::MenuModel* menu_model,
                             views::MenuButton* button,
                             views::MenuItemView::AnchorPosition anchor) {
+  DCHECK(owned());  // We'd better not open any menus while we're closing.
   views::MenuModelAdapter adapter(menu_model);
   gfx::Point screen_point;
   views::View::ConvertPointToScreen(button, &screen_point);
   menu_runner_.reset(new views::MenuRunner(adapter.CreateMenu()));
-  // Ignore the result as we know we can only get here after the menu has
-  // closed.
+  // Ignore the result since we don't need to handle a deleted menu specially.
   ignore_result(menu_runner_->RunMenuAt(
       GetWidget(), button, gfx::Rect(screen_point, button->size()), anchor,
       views::MenuRunner::HAS_MNEMONICS));
-  // TODO(pkasting): this may be deleted after rewrite.
 }
 
 void InfoBarView::PlatformSpecificShow(bool animate) {
@@ -327,10 +327,10 @@ void InfoBarView::PlatformSpecificShow(bool animate) {
 }
 
 void InfoBarView::PlatformSpecificHide(bool animate) {
-  // We're being removed. Cancel any menus we may have open.  Because we are
-  // deleted after a delay and after our delegate is deleted we have to
-  // explicitly cancel the menu rather than relying on the destructor to cancel
-  // the menu.
+  // Cancel any menus we may have open.  It doesn't make sense to leave them
+  // open while we're hidden, and if we're going to become unowned, we can't
+  // allow the user to choose any options and potentially call functions that
+  // try to access the owner.
   menu_runner_.reset();
 
   // It's possible to be called twice (once with |animate| true and once with it
diff --git a/chrome/browser/ui/views/infobars/infobar_view.h b/chrome/browser/ui/views/infobars/infobar_view.h
index 1917881..bb83c10 100644
--- a/chrome/browser/ui/views/infobars/infobar_view.h
+++ b/chrome/browser/ui/views/infobars/infobar_view.h
@@ -51,16 +51,19 @@ class InfoBarView : public InfoBar,
   static views::Label* CreateLabel(const string16& text);
 
   // Creates a link with the appropriate font and color for an infobar.
+  // NOTE: Subclasses must ignore link clicks if we're unowned.
   static views::Link* CreateLink(const string16& text,
                                  views::LinkListener* listener,
                                  const SkColor& background_color);
 
   // Creates a menu button with an infobar-specific appearance.
+  // NOTE: Subclasses must ignore button presses if we're unowned.
   static views::MenuButton* CreateMenuButton(
       const string16& text,
       views::ViewMenuDelegate* menu_delegate);
 
   // Creates a text button with an infobar-specific appearance.
+  // NOTE: Subclasses must ignore button presses if we're unowned.
   static views::TextButton* CreateTextButton(views::ButtonListener* listener,
                                              const string16& text,
                                              bool needs_elevation);
@@ -72,6 +75,8 @@ class InfoBarView : public InfoBar,
                                     View* child) OVERRIDE;
 
   // views::ButtonListener:
+  // NOTE: This must not be called if we're unowned.  (Subclasses should ignore
+  // calls to ButtonPressed() in this case.)
   virtual void ButtonPressed(views::Button* sender,
                              const views::Event& event) OVERRIDE;
 
@@ -88,8 +93,9 @@ class InfoBarView : public InfoBar,
   // Convenience getter.
   const InfoBarContainer::Delegate* container_delegate() const;
 
-  // Show a menu at the specified position. By invoking this InfobarView ensures
-  // the menu is destroyed at the appropriate time.
+  // Shows a menu at the specified position.
+  // NOTE: This must not be called if we're unowned.  (Subclasses should ignore
+  // calls to RunMenu() in this case.)
   void RunMenuAt(ui::MenuModel* menu_model,
                  views::MenuButton* button,
                  views::MenuItemView::AnchorPosition anchor);
diff --git a/chrome/browser/ui/views/infobars/link_infobar.cc b/chrome/browser/ui/views/infobars/link_infobar.cc
index 3844722..4496b0a 100644
--- a/chrome/browser/ui/views/infobars/link_infobar.cc
+++ b/chrome/browser/ui/views/infobars/link_infobar.cc
@@ -73,6 +73,8 @@ void LinkInfoBar::ViewHierarchyChanged(bool is_add, View* parent, View* child) {
 }
 
 void LinkInfoBar::LinkClicked(views::Link* source, int event_flags) {
+  if (!owned())
+    return;  // We're closing; don't call anything, it might access the owner.
   DCHECK(link_ != NULL);
   DCHECK_EQ(link_, source);
   if (GetDelegate()->LinkClicked(
diff --git a/chrome/browser/ui/views/infobars/translate_message_infobar.cc b/chrome/browser/ui/views/infobars/translate_message_infobar.cc
index 7a70485..d08fcf3 100644
--- a/chrome/browser/ui/views/infobars/translate_message_infobar.cc
+++ b/chrome/browser/ui/views/infobars/translate_message_infobar.cc
@@ -57,6 +57,8 @@ void TranslateMessageInfoBar::ViewHierarchyChanged(bool is_add,
 
 void TranslateMessageInfoBar::ButtonPressed(views::Button* sender,
                                             const views::Event& event) {
+  if (!owned())
+    return;  // We're closing; don't call anything, it might access the owner.
   if (sender == button_)
     GetDelegate()->MessageInfoBarButtonPressed();
   else