Fixes bug in wrench menu where closing with menu open could trigger

crash.

BUG=56842
TEST=see bug

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

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

6 files changed, 61 insertions, 7 deletions

diff --git a/chrome/browser/browser_browsertest.cc b/chrome/browser/browser_browsertest.cc
index e63eb84..572355a 100644
--- a/chrome/browser/browser_browsertest.cc
+++ b/chrome/browser/browser_browsertest.cc
@@ -98,6 +98,38 @@ class MockTabStripModelObserver : public TabStripModelObserver {
   DISALLOW_COPY_AND_ASSIGN(MockTabStripModelObserver);
 };
 
+// Used by CloseWithAppMenuOpen. Invokes CloseWindow on the supplied browser.
+class CloseWindowTask : public Task {
+ public:
+  explicit CloseWindowTask(Browser* browser) : browser_(browser) {}
+
+  virtual void Run() {
+    browser_->CloseWindow();
+  }
+
+ private:
+  Browser* browser_;
+
+  DISALLOW_COPY_AND_ASSIGN(CloseWindowTask);
+};
+
+// Used by CloseWithAppMenuOpen. Posts a CloseWindowTask and shows the app menu.
+class RunCloseWithAppMenuTask : public Task {
+ public:
+  explicit RunCloseWithAppMenuTask(Browser* browser) : browser_(browser) {}
+
+  virtual void Run() {
+    // ShowAppMenu is modal under views. Schedule a task that closes the window.
+    MessageLoop::current()->PostTask(FROM_HERE, new CloseWindowTask(browser_));
+    browser_->ShowAppMenu();
+  }
+
+ private:
+  Browser* browser_;
+
+  DISALLOW_COPY_AND_ASSIGN(RunCloseWithAppMenuTask);
+};
+
 }  // namespace
 
 class BrowserTest : public ExtensionBrowserTest {
@@ -532,6 +564,16 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, RestorePinnedTabs) {
 }
 #endif  // !defined(OS_CHROMEOS)
 
+// This test verifies we don't crash when closing the last window and the app
+// menu is showing.
+IN_PROC_BROWSER_TEST_F(BrowserTest, CloseWithAppMenuOpen) {
+  if (browser_defaults::kBrowserAliveWithNoWindows)
+    return;
+
+  // We need a message loop running for menus on windows.
+  MessageLoop::current()->PostTask(FROM_HERE,
+                                   new RunCloseWithAppMenuTask(browser()));
+}
 
 // TODO(ben): this test was never enabled. It has bit-rotted since being added.
 // It originally lived in browser_unittest.cc, but has been moved here to make
diff --git a/chrome/browser/views/toolbar_view.cc b/chrome/browser/views/toolbar_view.cc
index 277a7ef..d2cc229 100644
--- a/chrome/browser/views/toolbar_view.cc
+++ b/chrome/browser/views/toolbar_view.cc
@@ -251,7 +251,7 @@ void ToolbarView::RunMenu(views::View* source, const gfx::Point& /*pt*/) {
 
   bool destroyed_flag = false;
   destroyed_flag_ = &destroyed_flag;
-  wrench_menu_.reset(new WrenchMenu(browser_));
+  wrench_menu_ = new WrenchMenu(browser_);
   wrench_menu_->Init(wrench_menu_model_.get());
 
   for (size_t i = 0; i < menu_listeners_.size(); ++i)
diff --git a/chrome/browser/views/toolbar_view.h b/chrome/browser/views/toolbar_view.h
index 4031b4a..84ad2b0 100644
--- a/chrome/browser/views/toolbar_view.h
+++ b/chrome/browser/views/toolbar_view.h
@@ -10,6 +10,7 @@
 
 #include "app/menus/accelerator.h"
 #include "app/slide_animation.h"
+#include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
 #include "chrome/browser/back_forward_menu_model.h"
 #include "chrome/browser/command_updater.h"
@@ -189,7 +190,7 @@ class ToolbarView : public AccessibleToolbarView,
   scoped_ptr<menus::SimpleMenuModel> wrench_menu_model_;
 
   // Wrench menu.
-  scoped_ptr<WrenchMenu> wrench_menu_;
+  scoped_refptr<WrenchMenu> wrench_menu_;
 
   // Vector of listeners to receive callbacks when the menu opens.
   std::vector<views::MenuListener*> menu_listeners_;
diff --git a/chrome/browser/views/wrench_menu.cc b/chrome/browser/views/wrench_menu.cc
index 8236492..0c51c68 100644
--- a/chrome/browser/views/wrench_menu.cc
+++ b/chrome/browser/views/wrench_menu.cc
@@ -565,9 +565,6 @@ WrenchMenu::WrenchMenu(Browser* browser)
       selected_index_(0) {
 }
 
-WrenchMenu::~WrenchMenu() {
-}
-
 void WrenchMenu::Init(menus::MenuModel* model) {
   DCHECK(!root_.get());
   root_.reset(new MenuItemView(this));
@@ -579,6 +576,11 @@ void WrenchMenu::Init(menus::MenuModel* model) {
 }
 
 void WrenchMenu::RunMenu(views::MenuButton* host) {
+  // Up the ref count while the menu is displaying. This way if the window is
+  // deleted while we're running we won't prematurely delete the menu.
+  // TODO(sky): fix this, the menu should really take ownership of the menu
+  // (57890).
+  scoped_refptr<WrenchMenu> dont_delete_while_running(this);
   gfx::Point screen_loc;
   views::View::ConvertPointToScreen(host, &screen_loc);
   gfx::Rect bounds(screen_loc, host->size());
@@ -638,6 +640,9 @@ bool WrenchMenu::GetAccelerator(int id, views::Accelerator* accelerator) {
   return true;
 }
 
+WrenchMenu::~WrenchMenu() {
+}
+
 void WrenchMenu::PopulateMenu(MenuItemView* parent,
                               MenuModel* model,
                               int* next_id) {
diff --git a/chrome/browser/views/wrench_menu.h b/chrome/browser/views/wrench_menu.h
index 73653cc..e9aa2fb 100644
--- a/chrome/browser/views/wrench_menu.h
+++ b/chrome/browser/views/wrench_menu.h
@@ -10,6 +10,7 @@
 #include <utility>
 
 #include "app/menus/menu_model.h"
+#include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
 #include "views/controls/menu/menu_delegate.h"
 
@@ -22,10 +23,10 @@ class View;
 }  // namespace views
 
 // WrenchMenu adapts the WrenchMenuModel to view's menu related classes.
-class WrenchMenu : public views::MenuDelegate {
+class WrenchMenu : public base::RefCounted<WrenchMenu>,
+                   public views::MenuDelegate {
  public:
   explicit WrenchMenu(Browser* browser);
-  ~WrenchMenu();
 
   void Init(menus::MenuModel* model);
 
@@ -39,12 +40,16 @@ class WrenchMenu : public views::MenuDelegate {
   virtual bool GetAccelerator(int id, views::Accelerator* accelerator);
 
  private:
+  friend class base::RefCounted<WrenchMenu>;
+
   class CutCopyPasteView;
   class ZoomView;
 
   typedef std::pair<menus::MenuModel*,int> Entry;
   typedef std::map<int,Entry> IDToEntry;
 
+  ~WrenchMenu();
+
   // Populates |parent| with all the child menus in |model|. Recursively invokes
   // |PopulateMenu| for any submenu. |next_id| is incremented for every menu
   // that is created.
diff --git a/views/controls/menu/menu_item_view.cc b/views/controls/menu/menu_item_view.cc
index bd1fbaf..cdc0421 100644
--- a/views/controls/menu/menu_item_view.cc
+++ b/views/controls/menu/menu_item_view.cc
@@ -91,6 +91,7 @@ MenuItemView::~MenuItemView() {
   // loop is running deletion can't be done, otherwise the stack gets
   // thoroughly screwed. The destructor should be made private, and
   // MenuController should be the only place handling deletion of the menu.
+  // (57890).
   delete submenu_;
 }