Fixes possible crash if the window hosting a menu was closed while the

menu was showing. When this happens the window the menu creates is
implicitly destroyed (because the parent is going away).

BUG=25563
TEST=see bug

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

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

chrome/browser/views/bookmark_bar_view.cc

@@ -412,6 +412,12 @@ BookmarkBarView::~BookmarkBarView() {
   NotifyModelChanged();
   if (model_)
     model_->RemoveObserver(this);
+
+  // It's possible for the menu to outlive us, reset the observer to make sure
+  // it doesn't have a reference to us.
+  if (bookmark_menu_)
+    bookmark_menu_->set_observer(NULL);
+
   StopShowFolderDropMenuTimer();
 
   if (sync_service_)

chrome/browser/views/bookmark_bar_view_test.cc

@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -22,6 +22,7 @@
 #include "views/controls/menu/menu_controller.h"
 #include "views/controls/menu/menu_item_view.h"
 #include "views/controls/menu/submenu_view.h"
+#include "views/views_delegate.h"
 #include "views/window/window.h"
 
 #if defined(OS_LINUX)
@@ -45,6 +46,37 @@
 
 namespace {
 
+class ViewsDelegateImpl : public views::ViewsDelegate {
+ public:
+  ViewsDelegateImpl() {}
+  virtual Clipboard* GetClipboard() const { return NULL; }
+  virtual void SaveWindowPlacement(const std::wstring& window_name,
+                                   const gfx::Rect& bounds,
+                                   bool maximized) {}
+  virtual bool GetSavedWindowBounds(const std::wstring& window_name,
+                                    gfx::Rect* bounds) const {
+    return false;
+  }
+  virtual bool GetSavedMaximizedState(const std::wstring& window_name,
+                                      bool* maximized) const {
+    return false;
+  }
+
+#if defined(OS_WIN)
+  virtual HICON GetDefaultWindowIcon() const { return 0; }
+#endif
+
+  virtual void AddRef() {
+  }
+
+  virtual void ReleaseRef() {
+    MessageLoopForUI::current()->Quit();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ViewsDelegateImpl);
+};
+
 // PageNavigator implementation that records the URL.
 class TestingPageNavigator : public PageNavigator {
  public:
@@ -148,9 +180,14 @@ class BookmarkBarViewEventTestBase : public ViewEventTestBase {
     views::MenuItemView::allow_task_nesting_during_run_ = false;
     ViewEventTestBase::TearDown();
     BookmarkBarView::testing_ = false;
+    views::ViewsDelegate::views_delegate = NULL;
   }
 
  protected:
+  void InstallViewsDelegate() {
+    views::ViewsDelegate::views_delegate = &views_delegate_;
+  }
+
   virtual views::View* CreateContentsView() {
     return bb_view_;
   }
@@ -200,6 +237,7 @@ class BookmarkBarViewEventTestBase : public ViewEventTestBase {
   scoped_ptr<TestingProfile> profile_;
   ChromeThread ui_thread_;
   ChromeThread file_thread_;
+  ViewsDelegateImpl views_delegate_;
 };
 
 // Clicks on first menu, makes sure button is depressed. Moves mouse to first
@@ -369,7 +407,7 @@ class BookmarkBarViewTest3 : public BookmarkBarViewEventTestBase {
     EXPECT_EQ(GURL(), navigator_.url_);
 
     // Hide menu.
-    menu->GetMenuController()->Cancel(true);
+    menu->GetMenuController()->CancelAll();
 
     Done();
   }
@@ -784,7 +822,7 @@ class BookmarkBarViewTest9 : public BookmarkBarViewEventTestBase {
     ASSERT_NE(start_y_, menu_loc.y());
 
     // Hide menu.
-    bb_view_->GetMenu()->GetMenuController()->Cancel(true);
+    bb_view_->GetMenu()->GetMenuController()->CancelAll();
 
     // On linux, Cancelling menu will call Quit on the message loop,
     // which can interfere with Done. We need to run Done in the
@@ -1255,7 +1293,7 @@ class BookmarkBarViewTest15 : public BookmarkBarViewEventTestBase {
     // And the deleted_menu_id_ should have been removed.
     ASSERT_TRUE(menu->GetMenuItemByID(deleted_menu_id_) == NULL);
 
-    bb_view_->GetMenu()->GetMenuController()->Cancel(true);
+    bb_view_->GetMenu()->GetMenuController()->CancelAll();
 
     Done();
   }
@@ -1265,3 +1303,36 @@ class BookmarkBarViewTest15 : public BookmarkBarViewEventTestBase {
 };
 
 VIEW_TEST(BookmarkBarViewTest15, MenuStaysVisibleAfterDelete)
+
+// Tests that we don't crash or get stuck if the parent of a menu is closed.
+class BookmarkBarViewTest16 : public BookmarkBarViewEventTestBase {
+ protected:
+  virtual void DoTestOnMessageLoop() {
+    InstallViewsDelegate();
+
+    // Move the mouse to the first folder on the bookmark bar and press the
+    // mouse.
+    views::TextButton* button = bb_view_->GetBookmarkButton(0);
+    ui_controls::MoveMouseToCenterAndPress(button, ui_controls::LEFT,
+        ui_controls::DOWN | ui_controls::UP,
+        CreateEventTask(this, &BookmarkBarViewTest16::Step2));
+  }
+
+ private:
+  void Step2() {
+    // Menu should be showing.
+    views::MenuItemView* menu = bb_view_->GetMenu();
+    ASSERT_TRUE(menu != NULL);
+    ASSERT_TRUE(menu->GetSubmenu()->IsShowing());
+
+    // Button should be depressed.
+    views::TextButton* button = bb_view_->GetBookmarkButton(0);
+    ASSERT_TRUE(button->state() == views::CustomButton::BS_PUSHED);
+
+    // Close the window.
+    window_->Close();
+    window_ = NULL;
+  }
+};
+
+VIEW_TEST(BookmarkBarViewTest16, DeleteMenu)

chrome/browser/views/chrome_views_delegate.cc

@@ -81,3 +81,11 @@ HICON ChromeViewsDelegate::GetDefaultWindowIcon() const {
                   MAKEINTRESOURCE(IDR_MAINFRAME));
 }
 #endif
+
+void ChromeViewsDelegate::AddRef() {
+  g_browser_process->AddRefModule();
+}
+
+void ChromeViewsDelegate::ReleaseRef() {
+  g_browser_process->ReleaseModule();
+}

chrome/browser/views/chrome_views_delegate.h

@@ -25,6 +25,8 @@ class ChromeViewsDelegate : public views::ViewsDelegate {
 #if defined(OS_WIN)
   virtual HICON GetDefaultWindowIcon() const;
 #endif
+  virtual void AddRef();
+  virtual void ReleaseRef();
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ChromeViewsDelegate);

views/controls/button/menu_button.cc

@@ -49,11 +49,14 @@ MenuButton::MenuButton(ButtonListener* listener,
       menu_delegate_(menu_delegate),
       show_menu_marker_(show_menu_marker),
       menu_marker_(ResourceBundle::GetSharedInstance().GetBitmapNamed(
-          IDR_MENU_DROPARROW)) {
+                       IDR_MENU_DROPARROW)),
+      destroyed_flag_(NULL) {
   set_alignment(TextButton::ALIGN_LEFT);
 }
 
 MenuButton::~MenuButton() {
+  if (destroyed_flag_)
+    *destroyed_flag_ = true;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -145,7 +148,17 @@ bool MenuButton::Activate() {
     GetRootView()->SetMouseHandler(NULL);
 
     menu_visible_ = true;
+
+    bool destroyed = false;
+    destroyed_flag_ = &destroyed;
+
     menu_delegate_->RunMenu(this, menu_position);
+
+    if (destroyed) {
+      // The menu was deleted while showing. Don't attempt any processing.
+      return false;
+    }
+
     menu_visible_ = false;
     menu_closed_time_ = Time::Now();
 

views/controls/button/menu_button.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -97,6 +97,10 @@ class MenuButton : public TextButton {
   // text buttons.
   const SkBitmap* menu_marker_;
 
+  // If non-null the destuctor sets this to true. This is set while the menu is
+  // showing and used to detect if the menu was deleted while running.
+  bool* destroyed_flag_;
+
   DISALLOW_COPY_AND_ASSIGN(MenuButton);
 };
 

views/controls/menu/menu_controller.cc

@@ -16,8 +16,10 @@
 #include "views/drag_utils.h"
 #include "views/screen.h"
 #include "views/view_constants.h"
+#include "views/views_delegate.h"
 #include "views/widget/root_view.h"
 #include "views/widget/widget.h"
+
 #if defined(OS_LINUX)
 #include "base/keyboard_code_conversion_gtk.h"
 #endif
@@ -191,6 +193,10 @@ MenuItemView* MenuController::Run(gfx::NativeWindow parent,
   DLOG(INFO) << " entering nested loop, depth=" << nested_depth;
 #endif
 
+  // Make sure Chrome doesn't attempt to shut down while the menu is showing.
+  if (ViewsDelegate::views_delegate)
+    ViewsDelegate::views_delegate->AddRef();
+
   MessageLoopForUI* loop = MessageLoopForUI::current();
   if (MenuItemView::allow_task_nesting_during_run_) {
     bool did_allow_task_nesting = loop->NestableTasksAllowed();
@@ -201,6 +207,9 @@ MenuItemView* MenuController::Run(gfx::NativeWindow parent,
     loop->Run(this);
   }
 
+  if (ViewsDelegate::views_delegate)
+    ViewsDelegate::views_delegate->ReleaseRef();
+
 #ifdef DEBUG_MENU
   nested_depth--;
   DLOG(INFO) << " exiting nested loop,  depth=" << nested_depth;
@@ -238,11 +247,14 @@ MenuItemView* MenuController::Run(gfx::NativeWindow parent,
       CloseAllNestedMenus();
 
       // Set exit_all_, which makes sure all nested loops exit immediately.
-      exit_type_ = EXIT_ALL;
+      if (exit_type_ != EXIT_DESTROYED)
+        exit_type_ = EXIT_ALL;
     }
   }
 
-  if (menu_button_) {
+  // If we stopped running because one of the menus was destroyed chances are
+  // the button was also destroyed.
+  if (exit_type_ != EXIT_DESTROYED && menu_button_) {
     menu_button_->SetState(CustomButton::BS_NORMAL);
     menu_button_->SchedulePaint();
   }
@@ -286,7 +298,7 @@ void MenuController::SetSelection(MenuItemView* menu_item,
     StartShowTimer();
 }
 
-void MenuController::Cancel(bool all) {
+void MenuController::Cancel(ExitType type) {
   if (!showing_) {
     // This occurs if we're in the process of notifying the delegate for a drop
     // and the delegate cancels us.
@@ -294,7 +306,7 @@ void MenuController::Cancel(bool all) {
   }
 
   MenuItemView* selected = state_.item;
-  exit_type_ = all ? EXIT_ALL : EXIT_OUTERMOST;
+  exit_type_ = type;
 
   // Hide windows immediately.
   SetSelection(NULL, false, true);
@@ -336,7 +348,7 @@ void MenuController::OnMousePressed(SubmenuView* source,
 #endif
 
     // And close.
-    Cancel(true);
+    Cancel(EXIT_ALL);
     return;
   }
 
@@ -394,7 +406,7 @@ void MenuController::OnMouseDragged(SubmenuView* source,
         if (showing_) {
           // We're still showing, close all menus.
           CloseAllNestedMenus();
-          Cancel(true);
+          Cancel(EXIT_ALL);
         }  // else case, drop was on us.
       }  // else case, someone canceled us, don't do anything
     }
@@ -637,7 +649,7 @@ void MenuController::SetActiveInstance(MenuController* controller) {
 bool MenuController::Dispatch(const MSG& msg) {
   DCHECK(blocking_run_);
 
-  if (exit_type_ == EXIT_ALL) {
+  if (exit_type_ == EXIT_ALL || exit_type_ == EXIT_DESTROYED) {
     // We must translate/dispatch the message here, otherwise we would drop
     // the message on the floor.
     TranslateMessage(&msg);
@@ -678,7 +690,7 @@ bool MenuController::Dispatch(const MSG& msg) {
     case WM_CANCELMODE:
     case WM_SYSKEYDOWN:
       // Exit immediately on system keys.
-      Cancel(true);
+      Cancel(EXIT_ALL);
       return false;
 
     default:
@@ -693,7 +705,7 @@ bool MenuController::Dispatch(const MSG& msg) {
 bool MenuController::Dispatch(GdkEvent* event) {
   gtk_main_do_event(event);
 
-  if (exit_type_ == EXIT_ALL)
+  if (exit_type_ == EXIT_ALL || exit_type_ == EXIT_DESTROYED)
     return false;
 
   switch (event->type) {
@@ -766,7 +778,7 @@ bool MenuController::OnKeyDown(int key_code
            (!state_.item->HasSubmenu() ||
             !state_.item->GetSubmenu()->IsShowing()))) {
         // User pressed escape and only one menu is shown, cancel it.
-        Cancel(false);
+        Cancel(EXIT_OUTERMOST);
         return false;
       } else {
         CloseSubmenu();

views/controls/menu/menu_controller.h

@@ -38,6 +38,22 @@ class MenuController : public MessageLoopForUI::Dispatcher {
   friend class MenuHostRootView;
   friend class MenuItemView;
 
+  // Enumeration of how the menu should exit.
+  enum ExitType {
+    // Don't exit.
+    EXIT_NONE,
+
+    // All menus, including nested, should be exited.
+    EXIT_ALL,
+
+    // Only the outermost menu should be exited.
+    EXIT_OUTERMOST,
+
+    // This is set if the menu is being closed as the result of one of the menus
+    // being destroyed.
+    EXIT_DESTROYED
+  };
+
   // If a menu is currently active, this returns the controller for it.
   static MenuController* GetActiveInstance();
 
@@ -66,12 +82,12 @@ class MenuController : public MessageLoopForUI::Dispatcher {
                     bool open_submenu,
                     bool update_immediately);
 
-  // Cancels the current Run. If all is true, any nested loops are canceled
-  // as well. This immediately hides all menus.
-  void Cancel(bool all);
+  // Cancels the current Run. See ExitType for a description of what happens
+  // with the various parameters.
+  void Cancel(ExitType type);
 
-  // An alternative to Cancel(true) that can be used with a OneShotTimer.
-  void CancelAll() { return Cancel(true); }
+  // An alternative to Cancel(EXIT_ALL) that can be used with a OneShotTimer.
+  void CancelAll() { Cancel(EXIT_ALL); }
 
   // Various events, forwarded from the submenu.
   //
@@ -98,18 +114,6 @@ class MenuController : public MessageLoopForUI::Dispatcher {
   void OnDragExitedScrollButton(SubmenuView* source);
 
  private:
-  // Enumeration of how the menu should exit.
-  enum ExitType {
-    // Don't exit.
-    EXIT_NONE,
-
-    // All menus, including nested, should be exited.
-    EXIT_ALL,
-
-    // Only the outermost menu should be exited.
-    EXIT_OUTERMOST
-  };
-
   class MenuScrollTask;
 
   // Tracks selection information.

views/controls/menu/menu_host.h

@@ -0,0 +1,64 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef VIEWS_CONTROLS_MENU_MENU_HOST_H_
+#define VIEWS_CONTROLS_MENU_MENU_HOST_H_
+
+#include "gfx/native_widget_types.h"
+#include "gfx/rect.h"
+
+namespace views {
+
+class SubmenuView;
+class View;
+
+// SubmenuView uses a MenuHost to house the SubmenuView. MenuHost typically
+// extends the native Widget type, but is defined here for clarity of what
+// methods SubmenuView uses.
+//
+// SubmenuView owns the MenuHost. When SubmenuView is done with the MenuHost
+// |DestroyMenuHost| is invoked. The one exception to this is if the native
+// OS destroys the widget out from under us, in which case |MenuHostDestroyed|
+// is invoked back on the SubmenuView and the SubmenuView then drops references
+// to the MenuHost.
+class MenuHost {
+ public:
+  // Creates the platform specific MenuHost. Ownership passes to the caller.
+  static MenuHost* Create(SubmenuView* submenu_view);
+
+  // Initializes and shows the MenuHost.
+  virtual void Init(gfx::NativeWindow parent,
+                    const gfx::Rect& bounds,
+                    View* contents_view,
+                    bool do_capture) = 0;
+
+  // Returns true if the menu host is visible.
+  virtual bool IsMenuHostVisible() = 0;
+
+  // Shows the menu host. If |do_capture| is true the menu host should do a
+  // mouse grab.
+  virtual void ShowMenuHost(bool do_capture) = 0;
+
+  // Hides the menu host.
+  virtual void HideMenuHost() = 0;
+
+  // Destroys and deletes the menu host.
+  virtual void DestroyMenuHost() = 0;
+
+  // Sets the bounds of the menu host.
+  virtual void SetMenuHostBounds(const gfx::Rect& bounds) = 0;
+
+  // Releases a mouse grab installed by |ShowMenuHost|.
+  virtual void ReleaseMenuHostCapture() = 0;
+
+  // Returns the native window of the MenuHost.
+  virtual gfx::NativeWindow GetMenuHostWindow() = 0;
+
+ protected:
+  virtual ~MenuHost() {}
+};
+
+}  // namespace views
+
+#endif  // VIEWS_CONTROLS_MENU_MENU_HOST_H_

views/controls/menu/menu_host_gtk.cc

@@ -1,8 +1,7 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-
 #include "views/controls/menu/menu_host_gtk.h"
 
 #include <gdk/gdk.h>
@@ -14,9 +13,14 @@
 
 namespace views {
 
-MenuHost::MenuHost(SubmenuView* submenu)
+// static
+MenuHost* MenuHost::Create(SubmenuView* submenu_view) {
+  return new MenuHostGtk(submenu_view);
+}
+
+MenuHostGtk::MenuHostGtk(SubmenuView* submenu)
     : WidgetGtk(WidgetGtk::TYPE_POPUP),
-      closed_(false),
+      destroying_(false),
       submenu_(submenu),
       did_pointer_grab_(false) {
   GdkEvent* event = gtk_get_current_event();
@@ -29,50 +33,92 @@ MenuHost::MenuHost(SubmenuView* submenu)
   }
 }
 
-void MenuHost::Init(gfx::NativeWindow parent,
+MenuHostGtk::~MenuHostGtk() {
+}
+
+void MenuHostGtk::Init(gfx::NativeWindow parent,
                     const gfx::Rect& bounds,
                     View* contents_view,
                     bool do_capture) {
+  make_transient_to_parent();
   WidgetGtk::Init(GTK_WIDGET(parent), bounds);
+  // Make sure we get destroyed when the parent is destroyed.
+  gtk_window_set_destroy_with_parent(GTK_WINDOW(GetNativeView()), TRUE);
   gtk_window_set_type_hint(GTK_WINDOW(GetNativeView()),
                            GDK_WINDOW_TYPE_HINT_MENU);
   SetContentsView(contents_view);
-  Show();
+  ShowMenuHost(do_capture);
+}
+
+bool MenuHostGtk::IsMenuHostVisible() {
+  return IsVisible();
+}
+
+void MenuHostGtk::ShowMenuHost(bool do_capture) {
+  WidgetGtk::Show();
   if (do_capture)
     DoCapture();
 }
 
-gfx::NativeWindow MenuHost::GetNativeWindow() {
+void MenuHostGtk::HideMenuHost() {
+  // Make sure we release capture before hiding.
+  ReleaseMenuHostCapture();
+
+  WidgetGtk::Hide();
+}
+
+void MenuHostGtk::DestroyMenuHost() {
+  HideMenuHost();
+  destroying_ = true;
+  CloseNow();
+}
+
+void MenuHostGtk::SetMenuHostBounds(const gfx::Rect& bounds) {
+  SetBounds(bounds);
+}
+
+void MenuHostGtk::ReleaseMenuHostCapture() {
+  ReleaseGrab();
+}
+
+gfx::NativeWindow MenuHostGtk::GetMenuHostWindow() {
   return GTK_WINDOW(GetNativeView());
 }
 
-void MenuHost::Show() {
-  WidgetGtk::Show();
+RootView* MenuHostGtk::CreateRootView() {
+  return new MenuHostRootView(this, submenu_);
 }
 
-void MenuHost::Hide() {
-  if (closed_) {
-    // We're already closed, nothing to do.
-    // This is invoked twice if the first time just hid us, and the second
-    // time deleted Closed (deleted) us.
-    return;
+bool MenuHostGtk::ReleaseCaptureOnMouseReleased() {
+  return false;
+}
+
+void MenuHostGtk::ReleaseGrab() {
+  WidgetGtk::ReleaseGrab();
+  if (did_pointer_grab_) {
+    did_pointer_grab_ = false;
+    gdk_pointer_ungrab(GDK_CURRENT_TIME);
   }
-  // The menus are freed separately, and possibly before the window is closed,
-  // remove them so that View doesn't try to access deleted objects.
-  static_cast<MenuHostRootView*>(GetRootView())->suspend_events();
-  GetRootView()->RemoveAllChildViews(false);
-  ReleaseGrab();
-  closed_ = true;
-  WidgetGtk::Hide();
 }
 
-void MenuHost::HideWindow() {
-  // Make sure we release capture before hiding.
-  ReleaseGrab();
-  WidgetGtk::Hide();
+void MenuHostGtk::OnDestroy(GtkWidget* object) {
+  if (!destroying_) {
+    // We weren't explicitly told to destroy ourselves, which means the menu was
+    // deleted out from under us (the window we're parented to was closed). Tell
+    // the SubmenuView to drop references to us.
+    submenu_->MenuHostDestroyed();
+  }
+  WidgetGtk::OnDestroy(object);
 }
 
-void MenuHost::DoCapture() {
+gboolean MenuHostGtk::OnGrabBrokeEvent(GtkWidget* widget, GdkEvent* event) {
+  // Grab breaking only happens when drag and drop starts. So, we don't try
+  // and ungrab or cancel the menu.
+  did_pointer_grab_ = false;
+  return WidgetGtk::OnGrabBrokeEvent(widget, event);
+}
+
+void MenuHostGtk::DoCapture() {
   // Release the current grab.
   GtkWidget* current_grab_window = gtk_grab_get_current();
   if (current_grab_window)
@@ -93,38 +139,6 @@ void MenuHost::DoCapture() {
   did_pointer_grab_ = (grab_status == GDK_GRAB_SUCCESS);
   DCHECK(did_pointer_grab_);
   // need keyboard grab.
-#ifdef DEBUG_MENU
-  DLOG(INFO) << "Doing capture";
-#endif
-}
-
-void MenuHost::ReleaseCapture() {
-  ReleaseGrab();
-}
-
-RootView* MenuHost::CreateRootView() {
-  return new MenuHostRootView(this, submenu_);
-}
-
-gboolean MenuHost::OnGrabBrokeEvent(GtkWidget* widget, GdkEvent* event) {
-  // Grab breaking only happens when drag and drop starts. So, we don't try
-  // and ungrab or cancel the menu.
-  did_pointer_grab_ = false;
-  return WidgetGtk::OnGrabBrokeEvent(widget, event);
-}
-
-// Overriden to return false, we do NOT want to release capture on mouse
-// release.
-bool MenuHost::ReleaseCaptureOnMouseReleased() {
-  return false;
-}
-
-void MenuHost::ReleaseGrab() {
-  WidgetGtk::ReleaseGrab();
-  if (did_pointer_grab_) {
-    did_pointer_grab_ = false;
-    gdk_pointer_ungrab(GDK_CURRENT_TIME);
-  }
 }
 
 }  // namespace views

views/controls/menu/menu_host_gtk.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -6,6 +6,7 @@
 #ifndef VIEWS_CONTROLS_MENU_MENU_HOST_GTK_H_
 #define VIEWS_CONTROLS_MENU_MENU_HOST_GTK_H_
 
+#include "views/controls/menu/menu_host.h"
 #include "views/widget/widget_gtk.h"
 
 namespace views {
@@ -13,28 +14,27 @@ namespace views {
 class SubmenuView;
 
 // MenuHost implementation for Gtk.
-class MenuHost : public WidgetGtk {
+class MenuHostGtk : public WidgetGtk, public MenuHost {
  public:
-  explicit MenuHost(SubmenuView* submenu);
+  explicit MenuHostGtk(SubmenuView* submenu);
+  virtual ~MenuHostGtk();
 
+  // MenuHost overrides.
   void Init(gfx::NativeWindow parent,
             const gfx::Rect& bounds,
             View* contents_view,
             bool do_capture);
-
-  gfx::NativeWindow GetNativeWindow();
-
-  void Show();
-  virtual void Hide();
-  virtual void HideWindow();
-  void DoCapture();
-  void ReleaseCapture();
+  virtual bool IsMenuHostVisible();
+  virtual void ShowMenuHost(bool do_capture);
+  virtual void HideMenuHost();
+  virtual void DestroyMenuHost();
+  virtual void SetMenuHostBounds(const gfx::Rect& bounds);
+  virtual void ReleaseMenuHostCapture();
+  virtual gfx::NativeWindow GetMenuHostWindow();
 
  protected:
   virtual RootView* CreateRootView();
 
-  virtual gboolean OnGrabBrokeEvent(GtkWidget* widget, GdkEvent* event);
-
   // Overriden to return false, we do NOT want to release capture on mouse
   // release.
   virtual bool ReleaseCaptureOnMouseReleased();
@@ -42,9 +42,14 @@ class MenuHost : public WidgetGtk {
   // Overriden to also release pointer grab.
   virtual void ReleaseGrab();
 
+  virtual void OnDestroy(GtkWidget* object);
+  virtual gboolean OnGrabBrokeEvent(GtkWidget* widget, GdkEvent* event);
+
  private:
-  // If true, we've been closed.
-  bool closed_;
+  void DoCapture();
+
+  // If true, DestroyMenuHost has been invoked.
+  bool destroying_;
 
   // The view we contain.
   SubmenuView* submenu_;
@@ -52,7 +57,7 @@ class MenuHost : public WidgetGtk {
   // Have we done a pointer grab?
   bool did_pointer_grab_;
 
-  DISALLOW_COPY_AND_ASSIGN(MenuHost);
+  DISALLOW_COPY_AND_ASSIGN(MenuHostGtk);
 };
 
 }  // namespace views

views/controls/menu/menu_host_root_view.cc

@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -56,7 +56,7 @@ void MenuHostRootView::OnMouseReleased(const MouseEvent& event,
   if (forward_drag_to_menu_controller_) {
     forward_drag_to_menu_controller_ = false;
     if (canceled) {
-      GetMenuController()->Cancel(true);
+      GetMenuController()->Cancel(MenuController::EXIT_ALL);
     } else {
       GetMenuController()->OnMouseReleased(submenu_, event);
     }

views/controls/menu/menu_host_win.cc

@@ -12,8 +12,13 @@
 
 namespace views {
 
-MenuHost::MenuHost(SubmenuView* submenu)
-    : closed_(false),
+// static
+MenuHost* MenuHost::Create(SubmenuView* submenu_view) {
+  return new MenuHostWin(submenu_view);
+}
+
+MenuHostWin::MenuHostWin(SubmenuView* submenu)
+    : destroying_(false),
       submenu_(submenu),
       owns_capture_(false) {
   set_window_style(WS_POPUP);
@@ -30,45 +35,69 @@ MenuHost::MenuHost(SubmenuView* submenu)
   set_window_ex_style(WS_EX_TOPMOST | WS_EX_NOACTIVATE);
 }
 
-void MenuHost::Init(HWND parent,
-                    const gfx::Rect& bounds,
-                    View* contents_view,
-                    bool do_capture) {
+MenuHostWin::~MenuHostWin() {
+}
+
+void MenuHostWin::Init(HWND parent,
+                       const gfx::Rect& bounds,
+                       View* contents_view,
+                       bool do_capture) {
   WidgetWin::Init(parent, bounds);
   SetContentsView(contents_view);
-  Show();
+  ShowMenuHost(do_capture);
+}
+
+bool MenuHostWin::IsMenuHostVisible() {
+  return IsVisible();
+}
+
+void MenuHostWin::ShowMenuHost(bool do_capture) {
+  // We don't want to take focus away from the hosting window.
+  ShowWindow(SW_SHOWNA);
+
   if (do_capture)
     DoCapture();
 }
 
-void MenuHost::Show() {
-  // We don't want to take focus away from the hosting window.
-  ShowWindow(SW_SHOWNA);
-}
-
-void MenuHost::Hide() {
-  if (closed_) {
-    // We're already closed, nothing to do.
-    // This is invoked twice if the first time just hid us, and the second
-    // time deleted Closed (deleted) us.
-    return;
-  }
-  // The menus are freed separately, and possibly before the window is closed,
-  // remove them so that View doesn't try to access deleted objects.
-  static_cast<MenuHostRootView*>(GetRootView())->suspend_events();
-  GetRootView()->RemoveAllChildViews(false);
-  closed_ = true;
-  ReleaseCapture();
-  WidgetWin::Hide();
-}
-
-void MenuHost::HideWindow() {
+void MenuHostWin::HideMenuHost() {
   // Make sure we release capture before hiding.
-  ReleaseCapture();
+  ReleaseMenuHostCapture();
+
   WidgetWin::Hide();
 }
 
-void MenuHost::OnCaptureChanged(HWND hwnd) {
+void MenuHostWin::DestroyMenuHost() {
+  HideMenuHost();
+  destroying_ = true;
+  CloseNow();
+}
+
+void MenuHostWin::SetMenuHostBounds(const gfx::Rect& bounds) {
+  SetBounds(bounds);
+}
+
+void MenuHostWin::ReleaseMenuHostCapture() {
+  if (owns_capture_) {
+    owns_capture_ = false;
+    ::ReleaseCapture();
+  }
+}
+
+gfx::NativeWindow MenuHostWin::GetMenuHostWindow() {
+  return GetNativeView();
+}
+
+void MenuHostWin::OnDestroy() {
+  if (!destroying_) {
+    // We weren't explicitly told to destroy ourselves, which means the menu was
+    // deleted out from under us (the window we're parented to was closed). Tell
+    // the SubmenuView to drop references to us.
+    submenu_->MenuHostDestroyed();
+  }
+  WidgetWin::OnDestroy();
+}
+
+void MenuHostWin::OnCaptureChanged(HWND hwnd) {
   WidgetWin::OnCaptureChanged(hwnd);
   owns_capture_ = false;
 #ifdef DEBUG_MENU
@@ -76,7 +105,20 @@ void MenuHost::OnCaptureChanged(HWND hwnd) {
 #endif
 }
 
-void MenuHost::DoCapture() {
+void MenuHostWin::OnCancelMode() {
+  submenu_->GetMenuItem()->GetMenuController()->Cancel(
+      MenuController::EXIT_ALL);
+}
+
+RootView* MenuHostWin::CreateRootView() {
+  return new MenuHostRootView(this, submenu_);
+}
+
+bool MenuHostWin::ReleaseCaptureOnMouseReleased() {
+  return false;
+}
+
+void MenuHostWin::DoCapture() {
   owns_capture_ = true;
   SetCapture();
   has_capture_ = true;
@@ -85,33 +127,4 @@ void MenuHost::DoCapture() {
 #endif
 }
 
-void MenuHost::ReleaseCapture() {
-  if (owns_capture_) {
-#ifdef DEBUG_MENU
-    DLOG(INFO) << "released capture";
-#endif
-    owns_capture_ = false;
-    ::ReleaseCapture();
-  }
-}
-
-RootView* MenuHost::CreateRootView() {
-  return new MenuHostRootView(this, submenu_);
-}
-
-void MenuHost::OnCancelMode() {
-  if (!closed_) {
-#ifdef DEBUG_MENU
-    DLOG(INFO) << "OnCanceMode, closing menu";
-#endif
-    submenu_->GetMenuItem()->GetMenuController()->Cancel(true);
-  }
-}
-
-// Overriden to return false, we do NOT want to release capture on mouse
-// release.
-bool MenuHost::ReleaseCaptureOnMouseReleased() {
-  return false;
-}
-
 }  // namespace views

views/controls/menu/menu_host_win.h

@@ -6,6 +6,7 @@
 #ifndef VIEWS_CONTROLS_MENU_MENU_HOST_WIN_H_
 #define VIEWS_CONTROLS_MENU_MENU_HOST_WIN_H_
 
+#include "views/controls/menu/menu_host.h"
 #include "views/widget/widget_win.h"
 
 namespace views {
@@ -13,36 +14,41 @@ namespace views {
 class SubmenuView;
 
 // MenuHost implementation for windows.
-class MenuHost : public WidgetWin {
+class MenuHostWin : public WidgetWin, public MenuHost {
  public:
-  explicit MenuHost(SubmenuView* submenu);
+  explicit MenuHostWin(SubmenuView* submenu);
+  virtual ~MenuHostWin();
 
-  void Init(HWND parent,
-            const gfx::Rect& bounds,
-            View* contents_view,
-            bool do_capture);
+  // MenuHost overrides:
+  virtual void Init(HWND parent,
+                    const gfx::Rect& bounds,
+                    View* contents_view,
+                    bool do_capture);
+  virtual bool IsMenuHostVisible();
+  virtual void ShowMenuHost(bool do_capture);
+  virtual void HideMenuHost();
+  virtual void DestroyMenuHost();
+  virtual void SetMenuHostBounds(const gfx::Rect& bounds);
+  virtual void ReleaseMenuHostCapture();
+  virtual gfx::NativeWindow GetMenuHostWindow();
 
-  gfx::NativeWindow GetNativeWindow() { return GetNativeView(); }
-
-  void Show();
-  virtual void Hide();
-  virtual void HideWindow();
+  // WidgetWin overrides:
+  virtual void OnDestroy();
   virtual void OnCaptureChanged(HWND hwnd);
-  void DoCapture();
-  void ReleaseCapture();
+  virtual void OnCancelMode();
 
  protected:
   virtual RootView* CreateRootView();
 
-  virtual void OnCancelMode();
-
   // Overriden to return false, we do NOT want to release capture on mouse
   // release.
   virtual bool ReleaseCaptureOnMouseReleased();
 
  private:
-  // If true, we've been closed.
-  bool closed_;
+  void DoCapture();
+
+  // If true, DestroyMenuHost has been invoked.
+  bool destroying_;
 
   // If true, we own the capture and need to release it.
   bool owns_capture_;
@@ -50,7 +56,7 @@ class MenuHost : public WidgetWin {
   // The view we contain.
   SubmenuView* submenu_;
 
-  DISALLOW_COPY_AND_ASSIGN(MenuHost);
+  DISALLOW_COPY_AND_ASSIGN(MenuHostWin);
 };
 
 }  // namespace views

views/controls/menu/menu_item_view.cc

@@ -98,7 +98,7 @@ void MenuItemView::RunMenuAt(gfx::NativeWindow parent,
     // A menu is already showing, but it isn't a blocking menu. Cancel it.
     // We can get here during drag and drop if the user right clicks on the
     // menu quickly after the drop.
-    controller->Cancel(true);
+    controller->Cancel(MenuController::EXIT_ALL);
     controller = NULL;
   }
   bool owns_controller = false;
@@ -145,7 +145,7 @@ void MenuItemView::RunMenuForDropAt(gfx::NativeWindow parent,
   // If there is a menu, hide it so that only one menu is shown during dnd.
   MenuController* current_controller = MenuController::GetActiveInstance();
   if (current_controller) {
-    current_controller->Cancel(true);
+    current_controller->Cancel(MenuController::EXIT_ALL);
   }
 
   // Always create a new controller for non-blocking.
@@ -160,7 +160,7 @@ void MenuItemView::RunMenuForDropAt(gfx::NativeWindow parent,
 void MenuItemView::Cancel() {
   if (controller_ && !canceled_) {
     canceled_ = true;
-    controller_->Cancel(true);
+    controller_->Cancel(MenuController::EXIT_ALL);
   }
 }
 

views/controls/menu/submenu_view.cc

@@ -6,15 +6,10 @@
 
 #include "gfx/canvas.h"
 #include "views/controls/menu/menu_controller.h"
+#include "views/controls/menu/menu_host.h"
 #include "views/controls/menu/menu_scroll_view_container.h"
 #include "views/widget/root_view.h"
 
-#if defined(OS_WIN)
-#include "views/controls/menu/menu_host_win.h"
-#elif defined(OS_LINUX)
-#include "views/controls/menu/menu_host_gtk.h"
-#endif
-
 // Height of the drop indicator. This should be an even number.
 static const int kDropIndicatorHeight = 2;
 
@@ -208,20 +203,18 @@ bool SubmenuView::OnMouseWheel(const MouseWheelEvent& e) {
 }
 
 bool SubmenuView::IsShowing() {
-  return host_ && host_->IsVisible();
+  return host_ && host_->IsMenuHostVisible();
 }
 
 void SubmenuView::ShowAt(gfx::NativeWindow parent,
                          const gfx::Rect& bounds,
                          bool do_capture) {
   if (host_) {
-    host_->Show();
-    if (do_capture)
-      host_->DoCapture();
+    host_->ShowMenuHost(do_capture);
     return;
   }
 
-  host_ = new MenuHost(this);
+  host_ = MenuHost::Create(this);
   // Force construction of the scroll view container.
   GetScrollViewContainer();
   // Make sure the first row is visible.
@@ -230,23 +223,25 @@ void SubmenuView::ShowAt(gfx::NativeWindow parent,
 }
 
 void SubmenuView::Reposition(const gfx::Rect& bounds) {
-  host_->SetBounds(bounds);
+  if (host_)
+    host_->SetMenuHostBounds(bounds);
 }
 
 void SubmenuView::Close() {
   if (host_) {
-    host_->Close();
+    host_->DestroyMenuHost();
     host_ = NULL;
   }
 }
 
 void SubmenuView::Hide() {
   if (host_)
-    host_->HideWindow();
+    host_->HideMenuHost();
 }
 
 void SubmenuView::ReleaseCapture() {
-  host_->ReleaseCapture();
+  if (host_)
+    host_->ReleaseMenuHostCapture();
 }
 
 bool SubmenuView::SkipDefaultKeyEventProcessing(const views::KeyEvent& e) {
@@ -282,7 +277,12 @@ MenuScrollViewContainer* SubmenuView::GetScrollViewContainer() {
 }
 
 gfx::NativeWindow SubmenuView::native_window() const {
-  return host_ ? host_->GetNativeWindow() : NULL;
+  return host_ ? host_->GetMenuHostWindow() : NULL;
+}
+
+void SubmenuView::MenuHostDestroyed() {
+  host_ = NULL;
+  GetMenuItem()->GetMenuController()->Cancel(MenuController::EXIT_DESTROYED);
 }
 
 void SubmenuView::PaintDropIndicator(gfx::Canvas* canvas,

views/controls/menu/submenu_view.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -118,6 +118,11 @@ class SubmenuView : public View {
   // Returns the NativeWindow host of the menu, or NULL if not showing.
   gfx::NativeWindow native_window() const;
 
+  // Invoked if the menu is prematurely destroyed. This can happen if the window
+  // closes while the menu is shown. If invoked the SubmenuView must drop all
+  // references to the MenuHost as the MenuHost is about to be deleted.
+  void MenuHostDestroyed();
+
   // Padding around the edges of the submenu.
   static const int kSubmenuBorderSize;
 
@@ -138,7 +143,8 @@ class SubmenuView : public View {
   // Parent menu item.
   MenuItemView* parent_menu_item_;
 
-  // Widget subclass used to show the children.
+  // Widget subclass used to show the children. This is deleted when we invoke
+  // |DestroyMenuHost|, or |MenuHostDestroyed| is invoked back on us.
   MenuHost* host_;
 
   // If non-null, indicates a drop is in progress and drop_item is the item

views/view_unittest.cc

@@ -687,6 +687,8 @@ class TestViewsDelegate : public views::ViewsDelegate {
   virtual HICON GetDefaultWindowIcon() const {
     return NULL;
   }
+  virtual void AddRef() {}
+  virtual void ReleaseRef() {}
 
  private:
   mutable scoped_ptr<Clipboard> clipboard_;

views/views.gyp

@@ -111,6 +111,7 @@
         'controls/menu/menu_delegate.h',
         'controls/menu/menu_gtk.cc',
         'controls/menu/menu_gtk.h',
+        'controls/menu/menu_host.h',
         'controls/menu/menu_host_root_view.cc',
         'controls/menu/menu_host_root_view.h',
         'controls/menu/menu_host_win.cc',

views/views_delegate.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved. Use of this
+// Copyright (c) 2010 The Chromium Authors. All rights reserved. Use of this
 // source code is governed by a BSD-style license that can be found in the
 // LICENSE file.
 
@@ -49,6 +49,11 @@ class ViewsDelegate {
   virtual HICON GetDefaultWindowIcon() const = 0;
 #endif
 
+  // AddRef/ReleaseRef are invoked while a menu is visible. They are used to
+  // ensure we don't attempt to exit while a menu is showing.
+  virtual void AddRef() = 0;
+  virtual void ReleaseRef() = 0;
+
   // The active ViewsDelegate used by the views system.
   static ViewsDelegate* views_delegate;
 };

views/widget/widget_gtk.cc

@@ -989,6 +989,9 @@ gboolean WidgetGtk::OnGrabBrokeEvent(GtkWidget* widget, GdkEvent* event) {
 }
 
 void WidgetGtk::OnGrabNotify(GtkWidget* widget, gboolean was_grabbed) {
+  if (!window_contents_)
+    return;  // Grab broke after window destroyed, don't try processing it.
+
   gtk_grab_remove(window_contents_);
   HandleGrabBroke();
 }