Attempt at fixing crash in menus shown from infobars. Here's what the

current code does when closing an infobar:
. The animation ends, resulting in a delayed deletion of InfoBarView
  (InfoBarContainerView::PlatformSpecificRemoveInfoBar).
. The InfoBarDelegate deletes itself (InfoBarDelegate::InfoBarClosed).
. Eventually the InfoBarView is deleted.
This leaves a window of time between which the view is alive, but the
delegate has been deleted. The view doesn't directly reference the
delegate anymore, but the menu models created by the infobarviews
do. This means if a paint comes in to the menu it's going to query the
deleted delegate and we crash.

I made CancelMenu pure virtual in hopes of avoiding this in the future
by making subclasses think about it.

BUG=93314
TEST=make sure menus on infobars still work.
R=pkasting@chromium.org

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

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

11 files changed, 97 insertions, 66 deletions

diff --git a/chrome/browser/ui/views/infobars/after_translate_infobar.cc b/chrome/browser/ui/views/infobars/after_translate_infobar.cc
index 43803af..a661079 100644
--- a/chrome/browser/ui/views/infobars/after_translate_infobar.cc
+++ b/chrome/browser/ui/views/infobars/after_translate_infobar.cc
@@ -11,9 +11,6 @@
 #include "views/controls/button/menu_button.h"
 #include "views/controls/label.h"
 #include "views/controls/menu/menu_item_view.h"
-#include "views/controls/menu/menu_model_adapter.h"
-#include "views/controls/menu/menu_runner.h"
-#include "views/widget/widget.h"
 
 AfterTranslateInfoBar::AfterTranslateInfoBar(
     TabContentsWrapper* owner,
@@ -167,20 +164,20 @@ void AfterTranslateInfoBar::TargetLanguageChanged() {
 
 void AfterTranslateInfoBar::RunMenu(View* source, const gfx::Point& pt) {
   ui::MenuModel* menu_model = NULL;
+  views::MenuButton* button = NULL;
+  views::MenuItemView::AnchorPosition anchor = views::MenuItemView::TOPLEFT;
   if (source == original_language_menu_button_) {
     menu_model = &original_language_menu_model_;
+    button = original_language_menu_button_;
   } else if (source == target_language_menu_button_) {
     menu_model = &target_language_menu_model_;
+    button = target_language_menu_button_;
   } else {
     DCHECK_EQ(options_menu_button_, source);
     menu_model = &options_menu_model_;
+    button = options_menu_button_;
+    anchor = views::MenuItemView::TOPRIGHT;
   }
-
-  views::MenuModelAdapter menu_model_adapter(menu_model);
-  menu_runner_.reset(new views::MenuRunner(menu_model_adapter.CreateMenu()));
-  if (menu_runner_->RunMenuAt(
-          source->GetWidget(), NULL, gfx::Rect(pt, gfx::Size()),
-          views::MenuItemView::TOPRIGHT, views::MenuRunner::HAS_MNEMONICS) ==
-      views::MenuRunner::MENU_DELETED)
-    return;
+  RunMenuAt(menu_model, button, anchor);
+  // TODO(pkasting): this may be deleted after rewrite.
 }
diff --git a/chrome/browser/ui/views/infobars/after_translate_infobar.h b/chrome/browser/ui/views/infobars/after_translate_infobar.h
index 443e60e..bf26881 100644
--- a/chrome/browser/ui/views/infobars/after_translate_infobar.h
+++ b/chrome/browser/ui/views/infobars/after_translate_infobar.h
@@ -14,7 +14,6 @@
 class TranslateInfoBarDelegate;
 namespace views {
 class MenuButton;
-class MenuRunner;
 }
 
 class AfterTranslateInfoBar : public TranslateInfoBarBase,
@@ -27,15 +26,18 @@ class AfterTranslateInfoBar : public TranslateInfoBarBase,
   virtual ~AfterTranslateInfoBar();
 
   // TranslateInfoBarBase:
-  virtual void Layout();
-  virtual void ViewHierarchyChanged(bool is_add, View* parent, View* child);
-  virtual void ButtonPressed(views::Button* sender, const views::Event& event);
-  virtual int ContentMinimumWidth() const;
-  virtual void OriginalLanguageChanged();
-  virtual void TargetLanguageChanged();
+  virtual void Layout() OVERRIDE;
+  virtual void ViewHierarchyChanged(bool is_add,
+                                    View* parent,
+                                    View* child) OVERRIDE;
+  virtual void ButtonPressed(views::Button* sender,
+                             const views::Event& event) OVERRIDE;
+  virtual int ContentMinimumWidth() const OVERRIDE;
+  virtual void OriginalLanguageChanged() OVERRIDE;
+  virtual void TargetLanguageChanged() OVERRIDE;
 
   // ViewMenuDelegate:
-  virtual void RunMenu(View* source, const gfx::Point& pt);
+  virtual void RunMenu(View* source, const gfx::Point& pt) OVERRIDE;
 
   // The text displayed in the infobar is something like:
   // "Translated from <lang1> to <lang2> [more text in some languages]"
@@ -54,8 +56,6 @@ class AfterTranslateInfoBar : public TranslateInfoBarBase,
   LanguagesMenuModel target_language_menu_model_;
   OptionsMenuModel options_menu_model_;
 
-  scoped_ptr<views::MenuRunner> menu_runner_;
-
   // True if the target language comes before the original one.
   bool swapped_language_buttons_;
 
diff --git a/chrome/browser/ui/views/infobars/before_translate_infobar.cc b/chrome/browser/ui/views/infobars/before_translate_infobar.cc
index cadd177..d8d557b 100644
--- a/chrome/browser/ui/views/infobars/before_translate_infobar.cc
+++ b/chrome/browser/ui/views/infobars/before_translate_infobar.cc
@@ -11,9 +11,6 @@
 #include "views/controls/button/menu_button.h"
 #include "views/controls/label.h"
 #include "views/controls/menu/menu_item_view.h"
-#include "views/controls/menu/menu_model_adapter.h"
-#include "views/controls/menu/menu_runner.h"
-#include "views/widget/widget.h"
 
 BeforeTranslateInfoBar::BeforeTranslateInfoBar(
     TabContentsWrapper* owner,
@@ -190,17 +187,17 @@ void BeforeTranslateInfoBar::OriginalLanguageChanged() {
 
 void BeforeTranslateInfoBar::RunMenu(View* source, const gfx::Point& pt) {
   ui::MenuModel* menu_model = NULL;
+  views::MenuButton* button = NULL;
+  views::MenuItemView::AnchorPosition anchor = views::MenuItemView::TOPLEFT;
   if (source == language_menu_button_) {
     menu_model = &languages_menu_model_;
+    button = language_menu_button_;
   } else {
     DCHECK_EQ(options_menu_button_, source);
     menu_model = &options_menu_model_;
+    button = options_menu_button_;
+    anchor = views::MenuItemView::TOPRIGHT;
   }
-
-  views::MenuModelAdapter menu_model_adapter(menu_model);
-  menu_runner_.reset(new views::MenuRunner(menu_model_adapter.CreateMenu()));
-  if (menu_runner_->RunMenuAt(source->GetWidget(), NULL,
-          gfx::Rect(pt, gfx::Size()), views::MenuItemView::TOPRIGHT,
-          views::MenuRunner::HAS_MNEMONICS) == views::MenuRunner::MENU_DELETED)
-    return;
+  RunMenuAt(menu_model, button, anchor);
+  // TODO(pkasting): this may be deleted after rewrite.
 }
diff --git a/chrome/browser/ui/views/infobars/before_translate_infobar.h b/chrome/browser/ui/views/infobars/before_translate_infobar.h
index f7e0ad0..18d2125 100644
--- a/chrome/browser/ui/views/infobars/before_translate_infobar.h
+++ b/chrome/browser/ui/views/infobars/before_translate_infobar.h
@@ -14,7 +14,6 @@
 class TranslateInfoBarDelegate;
 namespace views {
 class MenuButton;
-class MenuRunner;
 }
 
 class BeforeTranslateInfoBar : public TranslateInfoBarBase,
@@ -27,14 +26,17 @@ class BeforeTranslateInfoBar : public TranslateInfoBarBase,
   virtual ~BeforeTranslateInfoBar();
 
   // TranslateInfoBarBase:
-  virtual void Layout();
-  virtual void ButtonPressed(views::Button* sender, const views::Event& event);
-  virtual void ViewHierarchyChanged(bool is_add, View* parent, View* child);
-  virtual int ContentMinimumWidth() const;
-  virtual void OriginalLanguageChanged();
+  virtual void Layout() OVERRIDE;
+  virtual void ButtonPressed(views::Button* sender,
+                             const views::Event& event) OVERRIDE;
+  virtual void ViewHierarchyChanged(bool is_add,
+                                    View* parent,
+                                    View* child) OVERRIDE;
+  virtual int ContentMinimumWidth() const OVERRIDE;
+  virtual void OriginalLanguageChanged() OVERRIDE;
 
   // views::ViewMenuDelegate:
-  virtual void RunMenu(View* source, const gfx::Point& pt);
+  virtual void RunMenu(View* source, const gfx::Point& pt) OVERRIDE;
 
   // The text displayed in the infobar is something like:
   // "The page is in <lang>. Would you like to translate it?"
@@ -53,8 +55,6 @@ class BeforeTranslateInfoBar : public TranslateInfoBarBase,
   LanguagesMenuModel languages_menu_model_;
   OptionsMenuModel options_menu_model_;
 
-  scoped_ptr<views::MenuRunner> menu_runner_;
-
   DISALLOW_COPY_AND_ASSIGN(BeforeTranslateInfoBar);
 };
 
diff --git a/chrome/browser/ui/views/infobars/confirm_infobar.h b/chrome/browser/ui/views/infobars/confirm_infobar.h
index 52dff90..a93d27b 100644
--- a/chrome/browser/ui/views/infobars/confirm_infobar.h
+++ b/chrome/browser/ui/views/infobars/confirm_infobar.h
@@ -30,10 +30,13 @@ class ConfirmInfoBar : public InfoBarView,
   virtual ~ConfirmInfoBar();
 
   // InfoBarView:
-  virtual void Layout();
-  virtual void ViewHierarchyChanged(bool is_add, View* parent, View* child);
-  virtual void ButtonPressed(views::Button* sender, const views::Event& event);
-  virtual int ContentMinimumWidth() const;
+  virtual void Layout() OVERRIDE;
+  virtual void ViewHierarchyChanged(bool is_add,
+                                    View* parent,
+                                    View* child) OVERRIDE;
+  virtual void ButtonPressed(views::Button* sender,
+                             const views::Event& event) OVERRIDE;
+  virtual int ContentMinimumWidth() const OVERRIDE;
 
   // views::LinkListener:
   virtual void LinkClicked(views::Link* source, int event_flags) OVERRIDE;
diff --git a/chrome/browser/ui/views/infobars/extension_infobar.cc b/chrome/browser/ui/views/infobars/extension_infobar.cc
index 49f02ab..f9e5fd1 100644
--- a/chrome/browser/ui/views/infobars/extension_infobar.cc
+++ b/chrome/browser/ui/views/infobars/extension_infobar.cc
@@ -18,8 +18,6 @@
 #include "ui/gfx/canvas_skia.h"
 #include "views/controls/button/menu_button.h"
 #include "views/controls/menu/menu_item_view.h"
-#include "views/controls/menu/menu_model_adapter.h"
-#include "views/controls/menu/menu_runner.h"
 #include "views/widget/widget.h"
 
 // ExtensionInfoBarDelegate ----------------------------------------------------
@@ -148,14 +146,9 @@ void ExtensionInfoBar::RunMenu(View* source, const gfx::Point& pt) {
       browser();
   scoped_refptr<ExtensionContextMenuModel> options_menu_contents =
       new ExtensionContextMenuModel(extension, browser, NULL);
-  views::MenuModelAdapter options_menu_delegate(options_menu_contents.get());
-  menu_runner_.reset(new views::MenuRunner(options_menu_delegate.CreateMenu()));
-  gfx::Point screen_point;
-  views::View::ConvertPointToScreen(menu_, &screen_point);
-  if (menu_runner_->RunMenuAt(GetWidget(), menu_,
-          gfx::Rect(screen_point, menu_->size()), views::MenuItemView::TOPLEFT,
-          views::MenuRunner::HAS_MNEMONICS) == views::MenuRunner::MENU_DELETED)
-    return;
+  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/extension_infobar.h b/chrome/browser/ui/views/infobars/extension_infobar.h
index 2d42e98..0eff801 100644
--- a/chrome/browser/ui/views/infobars/extension_infobar.h
+++ b/chrome/browser/ui/views/infobars/extension_infobar.h
@@ -14,7 +14,6 @@
 class TabContentsWrapper;
 namespace views {
 class MenuButton;
-class MenuRunner;
 }
 
 class ExtensionInfoBar : public InfoBarView,
@@ -29,9 +28,11 @@ class ExtensionInfoBar : public InfoBarView,
   virtual ~ExtensionInfoBar();
 
   // InfoBarView:
-  virtual void Layout();
-  virtual void ViewHierarchyChanged(bool is_add, View* parent, View* child);
-  virtual int ContentMinimumWidth() const;
+  virtual void Layout() OVERRIDE;
+  virtual void ViewHierarchyChanged(bool is_add,
+                                    View* parent,
+                                    View* child) OVERRIDE;
+  virtual int ContentMinimumWidth() const OVERRIDE;
 
   // ImageLoadingTracker::Observer:
   virtual void OnImageLoaded(SkBitmap* image,
@@ -57,8 +58,6 @@ class ExtensionInfoBar : public InfoBarView,
   // Keeps track of images being loaded on the File thread.
   ImageLoadingTracker tracker_;
 
-  scoped_ptr<views::MenuRunner> menu_runner_;
-
   DISALLOW_COPY_AND_ASSIGN(ExtensionInfoBar);
 };
 
diff --git a/chrome/browser/ui/views/infobars/infobar_view.cc b/chrome/browser/ui/views/infobars/infobar_view.cc
index 8faa905..87eb191 100644
--- a/chrome/browser/ui/views/infobars/infobar_view.cc
+++ b/chrome/browser/ui/views/infobars/infobar_view.cc
@@ -26,6 +26,8 @@
 #include "views/controls/image_view.h"
 #include "views/controls/label.h"
 #include "views/controls/link.h"
+#include "views/controls/menu/menu_model_adapter.h"
+#include "views/controls/menu/menu_runner.h"
 #include "views/focus/external_focus_tracker.h"
 #include "views/widget/widget.h"
 #include "views/window/non_client_view.h"
@@ -289,6 +291,21 @@ const InfoBarContainer::Delegate* InfoBarView::container_delegate() const {
   return infobar_container ? infobar_container->delegate() : NULL;
 }
 
+void InfoBarView::RunMenuAt(ui::MenuModel* menu_model,
+                            views::MenuButton* button,
+                            views::MenuItemView::AnchorPosition anchor) {
+  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_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) {
   views::Widget* widget = GetWidget();
   views::FocusManager* focus_manager = GetFocusManager();
@@ -310,6 +327,12 @@ 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.
+  menu_runner_.reset();
+
   // It's possible to be called twice (once with |animate| true and once with it
   // false); in this case the second RemoveFocusChangeListener() call will
   // silently no-op.
diff --git a/chrome/browser/ui/views/infobars/infobar_view.h b/chrome/browser/ui/views/infobars/infobar_view.h
index 75d8760..1917881 100644
--- a/chrome/browser/ui/views/infobars/infobar_view.h
+++ b/chrome/browser/ui/views/infobars/infobar_view.h
@@ -12,8 +12,12 @@
 #include "chrome/browser/tab_contents/infobar_container.h"
 #include "third_party/skia/include/core/SkPath.h"
 #include "views/controls/button/button.h"
+#include "views/controls/menu/menu_item_view.h"
 #include "views/focus/focus_manager.h"
 
+namespace ui {
+class MenuModel;
+}
 namespace views {
 class ExternalFocusTracker;
 class ImageButton;
@@ -22,6 +26,7 @@ class Label;
 class Link;
 class LinkListener;
 class MenuButton;
+class MenuRunner;
 class TextButton;
 class ViewMenuDelegate;
 }
@@ -83,6 +88,12 @@ 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.
+  void RunMenuAt(ui::MenuModel* menu_model,
+                 views::MenuButton* button,
+                 views::MenuItemView::AnchorPosition anchor);
+
  private:
   static const int kHorizontalPadding;
 
@@ -115,6 +126,9 @@ class InfoBarView : public InfoBar,
   SkPath fill_path_;
   SkPath stroke_path_;
 
+  // Used to run the menu.
+  scoped_ptr<views::MenuRunner> menu_runner_;
+
   DISALLOW_COPY_AND_ASSIGN(InfoBarView);
 };
 
diff --git a/chrome/browser/ui/views/infobars/link_infobar.h b/chrome/browser/ui/views/infobars/link_infobar.h
index 18e49f8..168b095 100644
--- a/chrome/browser/ui/views/infobars/link_infobar.h
+++ b/chrome/browser/ui/views/infobars/link_infobar.h
@@ -23,8 +23,10 @@ class LinkInfoBar : public InfoBarView,
   virtual ~LinkInfoBar();
 
   // InfoBarView:
-  virtual void Layout();
-  virtual void ViewHierarchyChanged(bool is_add, View* parent, View* child);
+  virtual void Layout() OVERRIDE;
+  virtual void ViewHierarchyChanged(bool is_add,
+                                    View* parent,
+                                    View* child) OVERRIDE;
 
   // views::LinkListener:
   virtual void LinkClicked(views::Link* source, int event_flags) OVERRIDE;
diff --git a/chrome/browser/ui/views/infobars/translate_message_infobar.h b/chrome/browser/ui/views/infobars/translate_message_infobar.h
index a70fd8d..d59ba01 100644
--- a/chrome/browser/ui/views/infobars/translate_message_infobar.h
+++ b/chrome/browser/ui/views/infobars/translate_message_infobar.h
@@ -17,10 +17,13 @@ class TranslateMessageInfoBar : public TranslateInfoBarBase {
   virtual ~TranslateMessageInfoBar();
 
   // TranslateInfoBarBase:
-  virtual void Layout();
-  virtual void ViewHierarchyChanged(bool is_add, View* parent, View* child);
-  virtual void ButtonPressed(views::Button* sender, const views::Event& event);
-  virtual int ContentMinimumWidth() const;
+  virtual void Layout() OVERRIDE;
+  virtual void ViewHierarchyChanged(bool is_add,
+                                    View* parent,
+                                    View* child) OVERRIDE;
+  virtual void ButtonPressed(views::Button* sender,
+                             const views::Event& event) OVERRIDE;
+  virtual int ContentMinimumWidth() const OVERRIDE;
 
   views::Label* label_;
   views::TextButton* button_;