Attempt at fixing menu crashes. I'm trying to fix three things:

. Its possible the hook function may end up getting invoked when
  open_native_menu_win_ is NULL.
. If ~NativeMenuWin runs before the menu has closed, we crash.
. Delays notifying model until using a task after the nested message
  loop returns. This better matches what WM_MENUCOMMAND does.

BUG=79810
TEST=none, make sure menus on windows haven't regressed.
R=ben@chromium.org

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

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

2 files changed, 44 insertions, 8 deletions

diff --git a/views/controls/menu/native_menu_win.cc b/views/controls/menu/native_menu_win.cc
index f9c1cf7..9d7369b 100644
--- a/views/controls/menu/native_menu_win.cc
+++ b/views/controls/menu/native_menu_win.cc
@@ -7,6 +7,7 @@
 #include <Windowsx.h>
 
 #include "base/logging.h"
+#include "base/message_loop.h"
 #include "base/stl_util-inl.h"
 #include "base/string_util.h"
 #include "base/win/wrapped_window_proc.h"
@@ -339,10 +340,14 @@ NativeMenuWin::NativeMenuWin(ui::MenuModel* model, HWND system_menu_for)
       menu_action_(MENU_ACTION_NONE),
       menu_to_select_(NULL),
       position_to_select_(-1),
-      parent_(NULL) {
+      ALLOW_THIS_IN_INITIALIZER_LIST(menu_to_select_factory_(this)),
+      parent_(NULL),
+      destroyed_flag_(NULL) {
 }
 
 NativeMenuWin::~NativeMenuWin() {
+  if (destroyed_flag_)
+    *destroyed_flag_ = true;
   STLDeleteContainerPointers(items_.begin(), items_.end());
   DestroyMenu(menu_);
 }
@@ -374,15 +379,27 @@ void NativeMenuWin::RunMenuAt(const gfx::Point& point, int alignment) {
   HWND hwnd = host_window_->hwnd();
   menu_to_select_ = NULL;
   position_to_select_ = -1;
+  menu_to_select_factory_.RevokeAll();
+  bool destroyed = false;
+  destroyed_flag_ = &destroyed;
   TrackPopupMenu(menu_, flags, point.x(), point.y(), 0, host_window_->hwnd(),
-                   NULL);
-
+                 NULL);
   UnhookWindowsHookEx(hhook);
   open_native_menu_win_ = NULL;
-
+  if (destroyed)
+    return;
+  destroyed_flag_ = NULL;
   if (menu_to_select_) {
+    // Folks aren't too happy if we notify immediately. In particular, notifying
+    // the delegate can cause destruction leaving the stack in a weird
+    // state. Instead post a task, then notify. This mirrors what WM_MENUCOMMAND
+    // does.
+    menu_to_select_factory_.RevokeAll();
+    MessageLoop::current()->PostTask(
+        FROM_HERE,
+        menu_to_select_factory_.NewRunnableMethod(
+            &NativeMenuWin::DelayedSelect));
     menu_action_ = MENU_ACTION_SELECTED;
-    menu_to_select_->model_->ActivatedAt(position_to_select_);
   }
 }
 
@@ -458,6 +475,11 @@ void NativeMenuWin::SetMinimumWidth(int width) {
 // static
 NativeMenuWin* NativeMenuWin::open_native_menu_win_ = NULL;
 
+void NativeMenuWin::DelayedSelect() {
+  if (menu_to_select_)
+    menu_to_select_->model_->ActivatedAt(position_to_select_);
+}
+
 // static
 bool NativeMenuWin::GetHighlightedMenuItemInfo(
     HMENU menu,
@@ -487,6 +509,9 @@ LRESULT CALLBACK NativeMenuWin::MenuMessageHook(
   LRESULT result = CallNextHookEx(NULL, n_code, w_param, l_param);
 
   NativeMenuWin* this_ptr = open_native_menu_win_;
+  if (!this_ptr)
+    return result;
+
   // The first time this hook is called, that means the menu has successfully
   // opened, so call the callback function on all of our listeners.
   if (!this_ptr->listeners_called_) {
@@ -506,8 +531,9 @@ LRESULT CALLBACK NativeMenuWin::MenuMessageHook(
       // items trigger painting of the tabstrip on mouse over) we have this
       // workaround. When the mouse is released on a menu item we remember the
       // menu item and end the menu. When the nested message loop returns we
-      // notify the model. It's still possible to get a WM_MENUCOMMAND, so we
-      // have to be careful that we don't notify the model twice.
+      // schedule a task to notify the model. It's still possible to get a
+      // WM_MENUCOMMAND, so we have to be careful that we don't notify the model
+      // twice.
       this_ptr->menu_to_select_ = info.menu;
       this_ptr->position_to_select_ = info.position;
       EndMenu();
diff --git a/views/controls/menu/native_menu_win.h b/views/controls/menu/native_menu_win.h
index 0ac9618..3d1ea7d 100644
--- a/views/controls/menu/native_menu_win.h
+++ b/views/controls/menu/native_menu_win.h
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/memory/scoped_ptr.h"
+#include "base/task.h"
 #include "ui/base/models/simple_menu_model.h"
 #include "views/controls/menu/menu_wrapper.h"
 
@@ -87,6 +88,9 @@ class NativeMenuWin : public MenuWrapper {
   // Creates the host window that receives notifications from the menu.
   void CreateHostWindow();
 
+  // Callback from task to notify menu it was selected.
+  void DelayedSelect();
+
   // Given a menu that's currently popped-up, find the currently highlighted
   // item. Returns true if a highlighted item was found.
   static bool GetHighlightedMenuItemInfo(HMENU menu,
@@ -132,13 +136,19 @@ class NativeMenuWin : public MenuWrapper {
   // once.
   bool listeners_called_;
 
-  // See comment in MenuMessageHook for details on this.
+  // See comment in MenuMessageHook for details on these.
   NativeMenuWin* menu_to_select_;
   int position_to_select_;
+  ScopedRunnableMethodFactory<NativeMenuWin> menu_to_select_factory_;
 
   // If we're a submenu, this is our parent.
   NativeMenuWin* parent_;
 
+  // If non-null the destructor sets this to true. This is set to non-null while
+  // the menu is showing. It is used to detect if the menu was deleted while
+  // running.
+  bool* destroyed_flag_;
+
   // Ugly: a static pointer to the instance of this class that currently
   // has a menu open, because our hook function that receives keyboard
   // events doesn't have a mechanism to get a user data pointer.