Fixes browser crash that could occur if the user closed a tab with an

unload handler, then right clicked on the tab. The fix is to close the
menu if the tab closes.

BUG=4846
TEST=see bug

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

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

2 files changed, 52 insertions, 12 deletions

diff --git a/chrome/browser/views/tabs/tab.cc b/chrome/browser/views/tabs/tab.cc
index 8a23f20..5591869 100644
--- a/chrome/browser/views/tabs/tab.cc
+++ b/chrome/browser/views/tabs/tab.cc
@@ -20,9 +20,9 @@ static const SkScalar kTabCapWidth = 15;
 static const SkScalar kTabTopCurveWidth = 4;
 static const SkScalar kTabBottomCurveWidth = 3;
 
-class TabContextMenuController : public views::MenuDelegate {
+class Tab::ContextMenuController : public views::MenuDelegate {
  public:
-  explicit TabContextMenuController(Tab* tab)
+  explicit ContextMenuController(Tab* tab)
       : tab_(tab),
         last_command_(TabStripModel::CommandFirst) {
     menu_.reset(new views::MenuItemView(this));
@@ -48,16 +48,24 @@ class TabContextMenuController : public views::MenuDelegate {
         TabStripModel::CommandCloseTabsOpenedBy,
         l10n_util::GetString(IDS_TAB_CXMENU_CLOSETABSOPENEDBY));
   }
-  virtual ~TabContextMenuController() {
-    tab_->delegate()->StopAllHighlighting();
-  }
 
   void RunMenuAt(int x, int y) {
     menu_->RunMenuAt(tab_->GetWidget()->GetHWND(), gfx::Rect(x, y, 0, 0),
                      views::MenuItemView::TOPLEFT, true);
+    if (tab_)
+      tab_->ContextMenuClosed();
+    delete this;
+  }
+
+  void Cancel() {
+    tab_ = NULL;
+    menu_->Cancel();
   }
 
  private:
+  virtual ~ContextMenuController() {
+  }
+
   // views::MenuDelegate implementation:
   virtual bool IsCommandEnabled(int id) const {
     // The MenuItemView used to contain the contents of the Context Menu itself
@@ -65,7 +73,7 @@ class TabContextMenuController : public views::MenuDelegate {
     // some reason during its construction. The TabStripModel can't handle
     // command indices it doesn't know about, so we need to filter this out
     // here.
-    if (id == 0)
+    if (id == 0 || !tab_)
       return false;
     return tab_->delegate()->IsCommandEnabledForTab(
         static_cast<TabStripModel::ContextMenuCommand>(id),
@@ -73,12 +81,18 @@ class TabContextMenuController : public views::MenuDelegate {
   }
 
   virtual void ExecuteCommand(int id) {
+    if (!tab_)
+      return;
+
     tab_->delegate()->ExecuteCommandForTab(
         static_cast<TabStripModel::ContextMenuCommand>(id),
         tab_);
   }
 
   virtual void SelectionChanged(views::MenuItemView* menu) {
+    if (!tab_)
+      return;
+
     TabStripModel::ContextMenuCommand command =
         static_cast<TabStripModel::ContextMenuCommand>(menu->GetCommand());
     tab_->delegate()->StopHighlightTabsForCommand(last_command_, tab_);
@@ -90,14 +104,15 @@ class TabContextMenuController : public views::MenuDelegate {
   // The context menu.
   scoped_ptr<views::MenuItemView> menu_;
 
-  // The Tab the context menu was brought up for.
+  // The Tab the context menu was brought up for. Set to NULL when the menu
+  // is canceled.
   Tab* tab_;
 
   // The last command that was selected, so that we can start/stop highlighting
   // appropriately as the user moves through the menu.
   TabStripModel::ContextMenuCommand last_command_;
 
-  DISALLOW_EVIL_CONSTRUCTORS(TabContextMenuController);
+  DISALLOW_COPY_AND_ASSIGN(ContextMenuController);
 };
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -106,7 +121,8 @@ class TabContextMenuController : public views::MenuDelegate {
 Tab::Tab(TabDelegate* delegate)
     : TabRenderer(),
       delegate_(delegate),
-      closing_(false) {
+      closing_(false),
+      menu_controller_(NULL) {
   close_button()->SetListener(this, 0);
   close_button()->SetAccessibleName(l10n_util::GetString(IDS_ACCNAME_CLOSE));
   close_button()->SetAnimationDuration(0);
@@ -114,6 +130,13 @@ Tab::Tab(TabDelegate* delegate)
 }
 
 Tab::~Tab() {
+  if (menu_controller_) {
+    // The menu is showing. Close the menu.
+    menu_controller_->Cancel();
+
+    // Invoke this so that we hide the highlight.
+    ContextMenuClosed();
+  }
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -202,11 +225,13 @@ bool Tab::GetAccessibleName(std::wstring* name) {
 
 void Tab::ShowContextMenu(views::View* source, int x, int y,
                           bool is_mouse_gesture) {
-  TabContextMenuController controller(this);
-  controller.RunMenuAt(x, y);
+  if (menu_controller_)
+    return;
+  menu_controller_ = new ContextMenuController(this);
+  menu_controller_->RunMenuAt(x, y);
+  // ContextMenuController takes care of deleting itself.
 }
 
-
 ///////////////////////////////////////////////////////////////////////////////
 // views::BaseButton::ButtonListener implementation:
 
@@ -243,3 +268,8 @@ void Tab::MakePathForTab(gfx::Path* path) const {
   path->lineTo(0, h);
   path->close();
 }
+
+void Tab::ContextMenuClosed() {
+  delegate()->StopAllHighlighting();
+  menu_controller_ = NULL;
+}
diff --git a/chrome/browser/views/tabs/tab.h b/chrome/browser/views/tabs/tab.h
index 5534e82..17134e1 100644
--- a/chrome/browser/views/tabs/tab.h
+++ b/chrome/browser/views/tabs/tab.h
@@ -84,6 +84,10 @@ class Tab : public TabRenderer,
   virtual bool IsSelected() const;
 
  private:
+  class ContextMenuController;
+
+  friend class ContextMenuController;
+
   // views::View overrides:
   virtual bool HasHitTestMask() const;
   virtual void GetHitTestMask(gfx::Path* mask) const;
@@ -110,6 +114,9 @@ class Tab : public TabRenderer,
   // representation. Used by GetViewForPoint for hit-testing.
   void MakePathForTab(gfx::Path* path) const;
 
+  // Invoked when the context menu closes.
+  void ContextMenuClosed();
+
   // An instance of a delegate object that can perform various actions based on
   // user gestures.
   TabDelegate* delegate_;
@@ -117,6 +124,9 @@ class Tab : public TabRenderer,
   // True if the tab is being animated closed.
   bool closing_;
 
+  // If non-null it means we're showing a menu for the tab.
+  ContextMenuController* menu_controller_;
+
   DISALLOW_COPY_AND_ASSIGN(Tab);
 };