Fix crashes with infobar closure.

I forgot that not all InfoBarView subclasses (e.g. LinkInfoBar) need to override ButtonPressed().  This means the base class has to manually check for an owner.

Also, the theme infobar was erroneously returning true from Cancel() even though it was already closing.  This led to RemoveSelf() being called twice.  I fixed this and also put a defensive check in RemoveSelf() with a long comment about why it's there.

BUG=98691
TEST=none
Review URL: http://codereview.chromium.org/8093008

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

6 files changed, 40 insertions, 24 deletions

diff --git a/chrome/browser/extensions/theme_installed_infobar_delegate.cc b/chrome/browser/extensions/theme_installed_infobar_delegate.cc
index fa3b884..9254d1f 100644
--- a/chrome/browser/extensions/theme_installed_infobar_delegate.cc
+++ b/chrome/browser/extensions/theme_installed_infobar_delegate.cc
@@ -55,16 +55,15 @@ bool ThemeInstalledInfoBarDelegate::Cancel() {
         extension_service_->GetExtensionById(previous_theme_id_, true);
     if (previous_theme) {
       theme_service_->SetTheme(previous_theme);
-      return true;
+        return false;  // The theme change will close us.
     }
   }
 
-  if (previous_using_native_theme_) {
+  if (previous_using_native_theme_)
     theme_service_->SetNativeTheme();
-  } else {
+  else
     theme_service_->UseDefaultTheme();
-  }
-  return true;
+  return false;  // The theme change will close us.
 }
 
 gfx::Image* ThemeInstalledInfoBarDelegate::GetIcon() const {
diff --git a/chrome/browser/infobars/infobar.cc b/chrome/browser/infobars/infobar.cc
index f909d23..9fb30f5 100644
--- a/chrome/browser/infobars/infobar.cc
+++ b/chrome/browser/infobars/infobar.cc
@@ -102,8 +102,16 @@ void InfoBar::AnimationProgressed(const ui::Animation* animation) {
 }
 
 void InfoBar::RemoveSelf() {
-  DCHECK(owner_);
-  owner_->RemoveInfoBar(delegate_);
+  // |owner_| should never be NULL here.  If it is, then someone violated what
+  // they were supposed to do -- e.g. a ConfirmInfoBarDelegate subclass returned
+  // true from Accept() or Cancel() even though the infobar was already closing.
+  // In the worst case, if we also switched tabs during that process, then
+  // |this| has already been destroyed.  But if that's the case, then we're
+  // going to deref a garbage |this| pointer here whether we check |owner_| or
+  // not, and in other cases (where we're still closing and |this| is valid),
+  // checking |owner_| here will avoid a NULL deref.
+  if (owner_)
+    owner_->RemoveInfoBar(delegate_);
 }
 
 void InfoBar::SetBarTargetHeight(int height) {
diff --git a/chrome/browser/tab_contents/confirm_infobar_delegate.h b/chrome/browser/tab_contents/confirm_infobar_delegate.h
index c39f1e1..593fe3d 100644
--- a/chrome/browser/tab_contents/confirm_infobar_delegate.h
+++ b/chrome/browser/tab_contents/confirm_infobar_delegate.h
@@ -34,25 +34,25 @@ class ConfirmInfoBarDelegate : public InfoBarDelegate {
   // Return whether or not the specified button needs elevation.
   virtual bool NeedElevation(InfoBarButton button) const;
 
-  // Called when the OK button is pressed. If the function returns true, the
-  // InfoBarDelegate should be removed from the associated TabContentsWrapper.
+  // Called when the OK button is pressed. If this function returns true, the
+  // infobar is then immediately closed. Subclasses MUST NOT return true if in
+  // handling this call something triggers the infobar to begin closing.
   virtual bool Accept();
 
-  // Called when the Cancel button is pressed.  If the function returns true,
-  // the InfoBarDelegate should be removed from the associated
-  // TabContentsWrapper.
+  // Called when the Cancel button is pressed. If this function returns true,
+  // the infobar is then immediately closed. Subclasses MUST NOT return true if
+  // in handling this call something triggers the infobar to begin closing.
   virtual bool Cancel();
 
   // Returns the text of the link to be displayed, if any. Otherwise returns
   // and empty string.
   virtual string16 GetLinkText() const;
 
-  // Called when the Link is clicked. The |disposition| specifies how the
-  // resulting document should be loaded (based on the event flags present when
-  // the link was clicked). This function returns true if the InfoBar should be
-  // closed now or false if it should remain until the user explicitly closes
-  // it.
-  // Will only be called if GetLinkText() returns non-empty string.
+  // Called when the Link (if any) is clicked. The |disposition| specifies how
+  // the resulting document should be loaded (based on the event flags present
+  // when the link was clicked). If this function returns true, the infobar is
+  // then immediately closed. Subclasses MUST NOT return true if in handling
+  // this call something triggers the infobar to begin closing.
   virtual bool LinkClicked(WindowOpenDisposition disposition);
 
  protected:
diff --git a/chrome/browser/tab_contents/link_infobar_delegate.h b/chrome/browser/tab_contents/link_infobar_delegate.h
index 0a20f13..e4d7d33 100644
--- a/chrome/browser/tab_contents/link_infobar_delegate.h
+++ b/chrome/browser/tab_contents/link_infobar_delegate.h
@@ -26,9 +26,9 @@ class LinkInfoBarDelegate : public InfoBarDelegate {
 
   // Called when the Link is clicked. The |disposition| specifies how the
   // resulting document should be loaded (based on the event flags present when
-  // the link was clicked). This function returns true if the InfoBar should be
-  // closed now or false if it should remain until the user explicitly closes
-  // it.
+  // the link was clicked). If this function returns true, the infobar is then
+  // immediately closed. Subclasses MUST NOT return true if in handling this
+  // call something triggers the infobar to begin closing.
   virtual bool LinkClicked(WindowOpenDisposition disposition);
 
  protected:
diff --git a/chrome/browser/ui/cocoa/infobars/infobar_controller.mm b/chrome/browser/ui/cocoa/infobars/infobar_controller.mm
index d7e8177..c81a363 100644
--- a/chrome/browser/ui/cocoa/infobars/infobar_controller.mm
+++ b/chrome/browser/ui/cocoa/infobars/infobar_controller.mm
@@ -126,8 +126,16 @@ const float kAnimateCloseDuration = 0.12;
 }
 
 - (void)removeSelf {
-  DCHECK(owner_);
-  owner_->RemoveInfoBar(delegate_);
+  // |owner_| should never be NULL here.  If it is, then someone violated what
+  // they were supposed to do -- e.g. a ConfirmInfoBarDelegate subclass returned
+  // true from Accept() or Cancel() even though the infobar was already closing.
+  // In the worst case, if we also switched tabs during that process, then
+  // |this| has already been destroyed.  But if that's the case, then we're
+  // going to deref a garbage |this| pointer here whether we check |owner_| or
+  // not, and in other cases (where we're still closing and |this| is valid),
+  // checking |owner_| here will avoid a NULL deref.
+  if (owner_)
+    owner_->RemoveInfoBar(delegate_);
 }
 
 - (AnimatableView*)animatableView {
diff --git a/chrome/browser/ui/views/infobars/infobar_view.cc b/chrome/browser/ui/views/infobars/infobar_view.cc
index 2d911e2..6d8209c 100644
--- a/chrome/browser/ui/views/infobars/infobar_view.cc
+++ b/chrome/browser/ui/views/infobars/infobar_view.cc
@@ -263,7 +263,8 @@ 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 (!owned())
+    return;  // We're closing; don't call anything, it might access the owner.
   if (sender == close_button_) {
     delegate()->InfoBarDismissed();
     RemoveSelf();