Fixes crash in menu code that would happen if you attempted to show a

context menu while a context menu was already shown. The code would
end up attempting to access an object that was since deleted (the
MenuItemView). In fixing the crash I realized the logic wasn't quite right
for this case either, so fixed that too and added a test.

BUG=80392
TEST=see bug
R=ben@chromium.org

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

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

6 files changed, 124 insertions, 33 deletions

diff --git a/chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc b/chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc
index 44fd7d6..997327f 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc
@@ -44,6 +44,9 @@
 // See http://crbug.com/47089 for details.
 #define MAYBE_CloseMenuAfterClosingContextMenu \
         DISABLED_CloseMenuAfterClosingContextMenu
+
+// See bug http://crbug.com/60444 for details.
+#define MAYBE_ScrollButtonScrolls DISABLED_ScrollButtonScrolls
 #else
 
 #define MAYBE_DND DND
@@ -53,14 +56,8 @@
 #define MAYBE_KeyEvents KeyEvents
 #define MAYBE_CloseWithModalDialog CloseWithModalDialog
 #define MAYBE_CloseMenuAfterClosingContextMenu CloseMenuAfterClosingContextMenu
-
-#endif
-
-#if defined(OS_LINUX)
-// See bug http://crbug.com/60444 for details.
-#define MAYBE_ScrollButtonScrolls DISABLED_ScrollButtonScrolls
-#else
 #define MAYBE_ScrollButtonScrolls ScrollButtonScrolls
+
 #endif
 
 namespace {
@@ -463,6 +460,9 @@ class ContextMenuNotificationObserver : public NotificationObserver {
     MessageLoop::current()->PostTask(FROM_HERE, task_);
   }
 
+  // Sets the task that is posted when the context menu is shown.
+  void set_task(Task* task) { task_ = task; }
+
  private:
   NotificationRegistrar registrar_;
   Task* task_;
@@ -1216,7 +1216,7 @@ class BookmarkBarViewTest13 : public BookmarkBarViewEventTestBase {
 
 VIEW_TEST(BookmarkBarViewTest13, ClickOnContextMenuSeparator)
 
-// Makes sure right cliking on a folder on the bookmark bar doesn't result in
+// Makes sure right clicking on a folder on the bookmark bar doesn't result in
 // both a context menu and showing the menu.
 class BookmarkBarViewTest14 : public BookmarkBarViewEventTestBase {
  public:
@@ -1368,3 +1368,77 @@ class BookmarkBarViewTest16 : public BookmarkBarViewEventTestBase {
 
 // Disabled, http://crbug.com/64303.
 VIEW_TEST(BookmarkBarViewTest16, DISABLED_DeleteMenu)
+
+// Makes sure right clicking on an item while a context menu is already showing
+// doesn't crash and works.
+class BookmarkBarViewTest17 : public BookmarkBarViewEventTestBase {
+ public:
+  BookmarkBarViewTest17()
+      : ALLOW_THIS_IN_INITIALIZER_LIST(
+          observer_(CreateEventTask(this, &BookmarkBarViewTest17::Step3))) {
+  }
+
+ protected:
+  virtual void DoTestOnMessageLoop() {
+    // Move the mouse to the other folder on the bookmark bar and press the
+    // left mouse button.
+    views::TextButton* button = bb_view_->other_bookmarked_button();
+    ui_controls::MoveMouseToCenterAndPress(button, ui_controls::LEFT,
+        ui_controls::DOWN | ui_controls::UP,
+        CreateEventTask(this, &BookmarkBarViewTest17::Step2));
+  }
+
+ private:
+  void Step2() {
+    // Menu should be showing.
+    views::MenuItemView* menu = bb_view_->GetMenu();
+    ASSERT_TRUE(menu != NULL);
+    ASSERT_TRUE(menu->GetSubmenu()->IsShowing());
+
+    // Right click on the second item to show its context menu.
+    views::MenuItemView* child_menu = menu->GetSubmenu()->GetMenuItemAt(2);
+    ASSERT_TRUE(child_menu != NULL);
+    ui_controls::MoveMouseToCenterAndPress(child_menu, ui_controls::RIGHT,
+        ui_controls::DOWN | ui_controls::UP, NULL);
+    // Step3 will be invoked by ContextMenuNotificationObserver.
+  }
+
+  void Step3() {
+    // Make sure the context menu is showing.
+    views::MenuItemView* context_menu = bb_view_->GetContextMenu();
+    ASSERT_TRUE(context_menu != NULL);
+    ASSERT_TRUE(context_menu->GetSubmenu());
+    ASSERT_TRUE(context_menu->GetSubmenu()->IsShowing());
+
+    // Right click on the first menu item to trigger its context menu.
+    views::MenuItemView* menu = bb_view_->GetMenu();
+    ASSERT_TRUE(menu != NULL);
+    ASSERT_TRUE(menu->GetSubmenu()->IsShowing());
+    views::MenuItemView* child_menu = menu->GetSubmenu()->GetMenuItemAt(1);
+    ASSERT_TRUE(child_menu != NULL);
+
+    observer_.set_task(CreateEventTask(this, &BookmarkBarViewTest17::Step4));
+    ui_controls::MoveMouseToCenterAndPress(child_menu, ui_controls::RIGHT,
+        ui_controls::DOWN | ui_controls::UP, NULL);
+    // Step4 will be invoked by ContextMenuNotificationObserver.
+  }
+
+  void Step4() {
+    // The context menu should still be showing.
+    views::MenuItemView* context_menu = bb_view_->GetContextMenu();
+    ASSERT_TRUE(context_menu != NULL);
+
+    // And the menu should be showing.
+    views::MenuItemView* menu = bb_view_->GetMenu();
+    ASSERT_TRUE(menu != NULL);
+    ASSERT_TRUE(menu->GetSubmenu()->IsShowing());
+
+    bb_view_->GetMenu()->GetMenuController()->CancelAll();
+
+    Done();
+  }
+
+  ContextMenuNotificationObserver observer_;
+};
+
+VIEW_TEST(BookmarkBarViewTest17, ContextMenus3)
diff --git a/views/controls/menu/menu_controller.cc b/views/controls/menu/menu_controller.cc
index 2533cef..5703d69 100644
--- a/views/controls/menu/menu_controller.cc
+++ b/views/controls/menu/menu_controller.cc
@@ -400,7 +400,7 @@ void MenuController::OnMousePressed(SubmenuView* source,
 
   DCHECK(!active_mouse_view_);
 
-  MenuPart part = GetMenuPartByScreenCoordinate(source, event.x(), event.y());
+  MenuPart part = GetMenuPart(source, event.location());
   if (part.is_scroll())
     return;  // Ignore presses on scroll buttons.
 
@@ -418,7 +418,18 @@ void MenuController::OnMousePressed(SubmenuView* source,
 #endif
 
     // And close.
-    Cancel(EXIT_ALL);
+    ExitType exit_type = EXIT_ALL;
+    if (!menu_stack_.empty()) {
+      // We're running nested menus. Only exit all if the mouse wasn't over one
+      // of the menus from the last run.
+      gfx::Point screen_loc(event.location());
+      View::ConvertPointToScreen(source->GetScrollViewContainer(), &screen_loc);
+      MenuPart last_part = GetMenuPartByScreenCoordinateUsingMenu(
+          menu_stack_.back().item, screen_loc);
+      if (last_part.type != MenuPart::NONE)
+        exit_type = EXIT_OUTERMOST;
+    }
+    Cancel(exit_type);
     return;
   }
 
@@ -441,7 +452,7 @@ void MenuController::OnMousePressed(SubmenuView* source,
 
 void MenuController::OnMouseDragged(SubmenuView* source,
                                     const MouseEvent& event) {
-  MenuPart part = GetMenuPartByScreenCoordinate(source, event.x(), event.y());
+  MenuPart part = GetMenuPart(source, event.location());
   UpdateScrolling(part);
 
   if (!blocking_run_)
@@ -503,7 +514,7 @@ void MenuController::OnMouseReleased(SubmenuView* source,
   DCHECK(state_.item);
   possible_drag_ = false;
   DCHECK(blocking_run_);
-  MenuPart part = GetMenuPartByScreenCoordinate(source, event.x(), event.y());
+  MenuPart part = GetMenuPart(source, event.location());
   if (event.IsRightMouseButton() && (part.type == MenuPart::MENU_ITEM &&
                                      part.menu)) {
     // Set the selection immediately, making sure the submenu is only open
@@ -550,7 +561,7 @@ void MenuController::OnMouseMoved(SubmenuView* source,
   if (showing_submenu_)
     return;
 
-  MenuPart part = GetMenuPartByScreenCoordinate(source, event.x(), event.y());
+  MenuPart part = GetMenuPart(source, event.location());
 
   UpdateScrolling(part);
 
@@ -582,7 +593,7 @@ void MenuController::OnMouseEntered(SubmenuView* source,
 #if defined(OS_LINUX)
 bool MenuController::OnMouseWheel(SubmenuView* source,
                                   const MouseWheelEvent& event) {
-  MenuPart part = GetMenuPartByScreenCoordinate(source, event.x(), event.y());
+  MenuPart part = GetMenuPart(source, event.location());
   return part.submenu && part.submenu->OnMouseWheel(event);
 }
 #endif
@@ -1151,25 +1162,26 @@ bool MenuController::IsScrollButtonAt(SubmenuView* source,
   return false;
 }
 
-MenuController::MenuPart MenuController::GetMenuPartByScreenCoordinate(
+MenuController::MenuPart MenuController::GetMenuPart(
     SubmenuView* source,
-    int source_x,
-    int source_y) {
-  MenuPart part;
-
-  gfx::Point screen_loc(source_x, source_y);
+    const gfx::Point& source_loc) {
+  gfx::Point screen_loc(source_loc);
   View::ConvertPointToScreen(source->GetScrollViewContainer(), &screen_loc);
+  return GetMenuPartByScreenCoordinateUsingMenu(state_.item, screen_loc);
+}
 
-  MenuItemView* item = state_.item;
+MenuController::MenuPart MenuController::GetMenuPartByScreenCoordinateUsingMenu(
+    MenuItemView* item,
+    const gfx::Point& screen_loc) {
+  MenuPart part;
   while (item) {
     if (item->HasSubmenu() && item->GetSubmenu()->IsShowing() &&
-        GetMenuPartByScreenCoordinateImpl(item->GetSubmenu(), screen_loc,
-                                          &part)) {
+        GetMenuPartByScreenCoordinateImpl(
+            item->GetSubmenu(), screen_loc, &part)) {
       return part;
     }
     item = item->GetParentMenuItem();
   }
-
   return part;
 }
 
diff --git a/views/controls/menu/menu_controller.h b/views/controls/menu/menu_controller.h
index 177a460..17a5dc0 100644
--- a/views/controls/menu/menu_controller.h
+++ b/views/controls/menu/menu_controller.h
@@ -157,8 +157,7 @@ class MenuController : public MessageLoopForUI::Dispatcher {
     gfx::Rect monitor_bounds;
   };
 
-  // Used by GetMenuPartByScreenCoordinate to indicate the menu part at a
-  // particular location.
+  // Used by GetMenuPart to indicate the menu part at a particular location.
   struct MenuPart {
     // Type of part.
     enum Type {
@@ -264,10 +263,14 @@ class MenuController : public MessageLoopForUI::Dispatcher {
                         int y,
                         MenuPart::Type* part);
 
-  // Returns the target for the mouse event.
-  MenuPart GetMenuPartByScreenCoordinate(SubmenuView* source,
-                                         int source_x,
-                                         int source_y);
+  // Returns the target for the mouse event. The coordinates are in terms of
+  // source's scroll view container.
+  MenuPart GetMenuPart(SubmenuView* source, const gfx::Point& source_loc);
+
+  // Returns the target for mouse events. The search is done through |item| and
+  // all its parents.
+  MenuPart GetMenuPartByScreenCoordinateUsingMenu(MenuItemView* item,
+                                                  const gfx::Point& screen_loc);
 
   // Implementation of GetMenuPartByScreenCoordinate for a single menu. Returns
   // true if the supplied SubmenuView contains the location in terms of the
diff --git a/views/controls/menu/menu_host.cc b/views/controls/menu/menu_host.cc
index dd4db60..1b0f3a6 100644
--- a/views/controls/menu/menu_host.cc
+++ b/views/controls/menu/menu_host.cc
@@ -63,6 +63,7 @@ void MenuHost::HideMenuHost() {
 void MenuHost::DestroyMenuHost() {
   HideMenuHost();
   destroying_ = true;
+  static_cast<MenuHostRootView*>(GetWidget()->GetRootView())->ClearSubmenu();
   GetWidget()->Close();
 }
 
@@ -100,8 +101,7 @@ void MenuHost::OnNativeMenuHostCancelCapture() {
     return;
   MenuController* menu_controller =
       submenu_->GetMenuItem()->GetMenuController();
-  if (menu_controller &&
-      !menu_controller->drag_in_progress())
+  if (menu_controller && !menu_controller->drag_in_progress())
     menu_controller->CancelAll();
 }
 
diff --git a/views/controls/menu/menu_host_root_view.cc b/views/controls/menu/menu_host_root_view.cc
index 405eaf8..061d682 100644
--- a/views/controls/menu/menu_host_root_view.cc
+++ b/views/controls/menu/menu_host_root_view.cc
@@ -73,7 +73,7 @@ bool MenuHostRootView::OnMouseWheel(const MouseWheelEvent& event) {
 }
 
 MenuController* MenuHostRootView::GetMenuController() {
-  return submenu_->GetMenuItem()->GetMenuController();
+  return submenu_ ? submenu_->GetMenuItem()->GetMenuController() : NULL;
 }
 
 }  // namespace views
diff --git a/views/controls/menu/menu_host_root_view.h b/views/controls/menu/menu_host_root_view.h
index 6ff0c16..12e7cc9 100644
--- a/views/controls/menu/menu_host_root_view.h
+++ b/views/controls/menu/menu_host_root_view.h
@@ -24,6 +24,8 @@ class MenuHostRootView : public RootView {
   MenuHostRootView(Widget* widget, SubmenuView* submenu);
   ~MenuHostRootView();
 
+  void ClearSubmenu() { submenu_ = NULL; }
+
   // Overridden from View:
   virtual bool OnMousePressed(const MouseEvent& event) OVERRIDE;
   virtual bool OnMouseDragged(const MouseEvent& event) OVERRIDE;