Merge 63576 - Menu crash fix for domui/gtk menu that is caused by multiple context menu requests.

Renderer may issue multiple ContextMenu request even if the menu is open.(this happenes when renderer receives mouse events before context menu is actually opened by previous request.)
 This is causing a crash in menu2 (both domiu/gtk impl) because menu2 has nested message loop, and the menu object gets deleted by 2nd request while it's handling message loop.

This CL addresses this issue by creating separate object to handle
the nested loop. Use only variables on stack and do not touch
instance variable if the menu is already deleted.

BUG=chromium-os:7929, chromium-os:7228
TEST=manual. on a page, continously click mouse to open/close context menu.
   It used to crash at some point, but shouldn't crash with this patch.

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

TBR=oshima@chromium.org
Review URL: http://codereview.chromium.org/4081002

git-svn-id: svn://svn.chromium.org/chrome/branches/552/src@63594 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 156 insertions, 17 deletions

diff --git a/chrome/browser/chromeos/views/native_menu_domui.cc b/chrome/browser/chromeos/views/native_menu_domui.cc
index d421f5f..d1a0458 100644
--- a/chrome/browser/chromeos/views/native_menu_domui.cc
+++ b/chrome/browser/chromeos/views/native_menu_domui.cc
@@ -20,9 +20,13 @@
 #include "gfx/rect.h"
 #include "views/controls/menu/menu_2.h"
 #include "views/controls/menu/native_menu_gtk.h"
+#include "views/controls/menu/nested_dispatcher_gtk.h"
 
 namespace {
 
+using chromeos::NativeMenuDOMUI;
+using chromeos::DOMUIMenuWidget;
+
 // Returns true if the menu item type specified can be executed as a command.
 bool MenuTypeCanExecute(menus::MenuModel::ItemType type) {
   return type == menus::MenuModel::TYPE_COMMAND ||
@@ -31,9 +35,11 @@ bool MenuTypeCanExecute(menus::MenuModel::ItemType type) {
 }
 
 gboolean Destroy(GtkWidget* widget, gpointer data) {
-  chromeos::NativeMenuDOMUI* domui_menu =
-      static_cast<chromeos::NativeMenuDOMUI*>(data);
-  domui_menu->Hide();
+  DOMUIMenuWidget* menu_widget = static_cast<DOMUIMenuWidget*>(data);
+  NativeMenuDOMUI* domui_menu = menu_widget->domui_menu();
+  // domui_menu can be NULL if widget is destroyed by signal.
+  if (domui_menu)
+    domui_menu->Hide();
   return true;
 }
 
@@ -51,7 +57,7 @@ gfx::NativeWindow FindActiveToplevelWindow() {
 }
 
 // Currently opened menu. See RunMenuAt for reason why we need this.
-chromeos::NativeMenuDOMUI* current_ = NULL;
+NativeMenuDOMUI* current_ = NULL;
 
 }  // namespace
 
@@ -83,7 +89,8 @@ NativeMenuDOMUI::NativeMenuDOMUI(menus::MenuModel* menu_model, bool root)
       activated_index_(-1),
       menu_action_(MENU_ACTION_NONE),
       menu_url_(StringPrintf("chrome://%s", chrome::kChromeUIMenu)),
-      on_menu_opened_called_(false) {
+      on_menu_opened_called_(false),
+      nested_dispatcher_(NULL) {
   menu_widget_ = new DOMUIMenuWidget(this, root);
   // Set the initial location off the screen not to show small
   // window with dropshadow.
@@ -91,7 +98,12 @@ NativeMenuDOMUI::NativeMenuDOMUI(menus::MenuModel* menu_model, bool root)
 }
 
 NativeMenuDOMUI::~NativeMenuDOMUI() {
-  DCHECK(!menu_shown_) << "Deleting while the menu is shown";
+  if (nested_dispatcher_) {
+    // Menu is destroyed while its in message loop.
+    // Let nested dispatcher know the creator is deleted.
+    nested_dispatcher_->CreatorDestroyed();
+    Hide();
+  }
   if (menu_widget_) {
     menu_widget_->Close();
     menu_widget_ = NULL;
@@ -141,15 +153,20 @@ void NativeMenuDOMUI::RunMenuAt(const gfx::Point& point, int alignment) {
   if (parent) {
     handle = g_signal_connect(G_OBJECT(parent), "destroy",
                               G_CALLBACK(&Destroy),
-                              this);
+                              menu_widget_);
   }
-
   // We need to turn on nestable tasks as a renderer uses tasks internally.
   // Without this, renderer cannnot finish loading page.
-  bool nestable = MessageLoopForUI::current()->NestableTasksAllowed();
-  MessageLoopForUI::current()->SetNestableTasksAllowed(true);
-  MessageLoopForUI::current()->Run(this);
-  MessageLoopForUI::current()->SetNestableTasksAllowed(nestable);
+  nested_dispatcher_ =
+      new views::NestedDispatcherGtk(this, true /* allow nested */);
+  bool deleted = nested_dispatcher_->RunAndSelfDestruct();
+  current_ = NULL;  // this is static and safe to access.
+  if (deleted) {
+    // The menu was destryed while menu is shown, so return immediately.
+    // Don't touch the instance which is already deleted.
+    return;
+  }
+  nested_dispatcher_ = NULL;
   if (menu_shown_) {
     // If this happens it means we haven't yet gotten the hide signal and
     // someone else quit the message loop on us.
@@ -162,7 +179,6 @@ void NativeMenuDOMUI::RunMenuAt(const gfx::Point& point, int alignment) {
   menu_widget_->Hide();
   // Close All submenus.
   submenu_.reset();
-  current_ = NULL;
   ProcessActivate();
 }
 
@@ -359,7 +375,8 @@ void NativeMenuDOMUI::ShowAt(MenuLocator* locator) {
 NativeMenuDOMUI* NativeMenuDOMUI::FindMenuAt(const gfx::Point& point) {
   if (submenu_.get()) {
     NativeMenuDOMUI* found = submenu_->FindMenuAt(point);
-    if (found) return found;
+    if (found)
+      return found;
   }
   gfx::Rect bounds;
   menu_widget_->GetBounds(&bounds, false);
diff --git a/chrome/browser/chromeos/views/native_menu_domui.h b/chrome/browser/chromeos/views/native_menu_domui.h
index 2c5e216a..fb03604e 100644
--- a/chrome/browser/chromeos/views/native_menu_domui.h
+++ b/chrome/browser/chromeos/views/native_menu_domui.h
@@ -10,6 +10,7 @@
 
 #include "base/message_loop.h"
 #include "base/observer_list.h"
+#include "base/scoped_ptr.h"
 #include "chrome/browser/chromeos/dom_ui/domui_menu_control.h"
 #include "googleurl/src/gurl.h"
 #include "views/controls/menu/menu_wrapper.h"
@@ -22,6 +23,10 @@ namespace menus {
 class MenuModel;
 }  // namespace menus
 
+namespace views {
+class NestedDispatcherGtk;
+}  // namespace views;
+
 namespace chromeos {
 
 class MenuLocator;
@@ -30,7 +35,7 @@ class DOMUIMenuWidget;
 // A DOMUI implementation of MenuWrapper.
 class NativeMenuDOMUI : public views::MenuWrapper,
                         public DOMUIMenuControl,
-                        public MessageLoopForUI::Dispatcher {
+                        public MessageLoop::Dispatcher {
  public:
   NativeMenuDOMUI(menus::MenuModel* menu_model, bool root);
   virtual ~NativeMenuDOMUI();
@@ -140,6 +145,11 @@ class NativeMenuDOMUI : public views::MenuWrapper,
   // A guard flag to avoid calling MenuListener::OnMenuOpened twice.
   bool on_menu_opened_called_;
 
+  // Nested dispatcher object that can outlive this object.
+  // This is to deal with the menu being deleted while the nested
+  // message loop is handled. see http://crosbug.com/7929 .
+  views::NestedDispatcherGtk* nested_dispatcher_;
+
   DISALLOW_COPY_AND_ASSIGN(NativeMenuDOMUI);
 };
 
diff --git a/views/controls/menu/native_menu_gtk.cc b/views/controls/menu/native_menu_gtk.cc
index 14c1f6d..31a8c3f 100644
--- a/views/controls/menu/native_menu_gtk.cc
+++ b/views/controls/menu/native_menu_gtk.cc
@@ -21,6 +21,7 @@
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "views/accelerator.h"
 #include "views/controls/menu/menu_2.h"
+#include "views/controls/menu/nested_dispatcher_gtk.h"
 
 namespace {
 
@@ -63,10 +64,16 @@ NativeMenuGtk::NativeMenuGtk(Menu2* menu)
       activated_index_(-1),
       activate_factory_(this),
       host_menu_(menu),
-      menu_action_(MENU_ACTION_NONE) {
+      menu_action_(MENU_ACTION_NONE),
+      nested_dispatcher_(NULL) {
 }
 
 NativeMenuGtk::~NativeMenuGtk() {
+  if (nested_dispatcher_) {
+    // Menu is destroyed while its in message loop.
+    // Let nested dispatcher know the creator is deleted.
+    nested_dispatcher_->CreatorDestroyed();
+  }
   if (menu_) {
     // Don't call MenuDestroyed because menu2 has already been destroyed.
     g_signal_handler_disconnect(menu_, destroy_handler_id_);
@@ -104,7 +111,14 @@ void NativeMenuGtk::RunMenuAt(const gfx::Point& point, int alignment) {
                        G_CALLBACK(OnMenuMoveCurrentThunk), this);
 
   // Block until menu is no longer shown by running a nested message loop.
-  MessageLoopForUI::current()->Run(this);
+  nested_dispatcher_ = new NestedDispatcherGtk(this, false);
+  bool deleted = nested_dispatcher_->RunAndSelfDestruct();
+  if (deleted) {
+    // The menu was destryed while menu is shown, so return immediately.
+    // Don't touch the instance which is already deleted.
+    return;
+  }
+  nested_dispatcher_ = NULL;
   if (!menu_hidden_) {
     // If this happens it means we haven't yet gotten the hide signal and
     // someone else quit the message loop on us.
diff --git a/views/controls/menu/native_menu_gtk.h b/views/controls/menu/native_menu_gtk.h
index 79a6060..6145925 100644
--- a/views/controls/menu/native_menu_gtk.h
+++ b/views/controls/menu/native_menu_gtk.h
@@ -21,6 +21,8 @@ class MenuModel;
 
 namespace views {
 
+class NestedDispatcherGtk;
+
 // A Gtk implementation of MenuWrapper.
 //
 // NOTE: On windows the activate message is not sent immediately when an item
@@ -136,6 +138,11 @@ class NativeMenuGtk : public MenuWrapper,
   // Vector of listeners to receive callbacks when the menu opens.
   std::vector<MenuListener*> listeners_;
 
+  // Nested dispatcher object that can outlive this object.
+  // This is to deal with the menu being deleted while the nested
+  // message loop is handled. see http://crosbug.com/7228 .
+  NestedDispatcherGtk* nested_dispatcher_;
+
   DISALLOW_COPY_AND_ASSIGN(NativeMenuGtk);
 };
 
diff --git a/views/controls/menu/nested_dispatcher_gtk.cc b/views/controls/menu/nested_dispatcher_gtk.cc
new file mode 100644
index 0000000..856b0a8
--- /dev/null
+++ b/views/controls/menu/nested_dispatcher_gtk.cc
@@ -0,0 +1,39 @@
+// 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/nested_dispatcher_gtk.h"
+
+namespace views {
+
+NestedDispatcherGtk::NestedDispatcherGtk(MessageLoopForUI::Dispatcher* creator,
+                                         bool allow_nested_task)
+  : creator_(creator),
+    allow_nested_task_(allow_nested_task) {
+}
+
+bool NestedDispatcherGtk::RunAndSelfDestruct() {
+  bool nestable = MessageLoopForUI::current()->NestableTasksAllowed();
+  if (allow_nested_task_)
+    MessageLoopForUI::current()->SetNestableTasksAllowed(true);
+  MessageLoopForUI::current()->Run(this);
+  if (allow_nested_task_)
+    MessageLoopForUI::current()->SetNestableTasksAllowed(nestable);
+  bool creator_is_deleted = creator_ == NULL;
+  delete this;
+  return creator_is_deleted;
+}
+
+void NestedDispatcherGtk::CreatorDestroyed() {
+  creator_ = NULL;
+}
+
+bool NestedDispatcherGtk::Dispatch(GdkEvent* event) {
+  if (creator_ != NULL) {
+    return creator_->Dispatch(event);
+  } else {
+    return false;
+  }
+}
+
+}  // namespace views
diff --git a/views/controls/menu/nested_dispatcher_gtk.h b/views/controls/menu/nested_dispatcher_gtk.h
new file mode 100644
index 0000000..06f54fa
--- /dev/null
+++ b/views/controls/menu/nested_dispatcher_gtk.h
@@ -0,0 +1,50 @@
+// 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_NESTED_DISPATCHER_GTK_H_
+#define VIEWS_CONTROLS_MENU_NATIVE_DISPATCHER_GTK_H_
+#pragma once
+
+#include "base/message_loop.h"
+
+namespace views {
+
+// A nested dispatcher that can out-live the creator of this
+// dispatcher. This is to deal with the scenario where a menu object
+// is deleted while it's handling the message loop.  Note that
+// |RunAndSelfDestruct| method always delete itself regardless of
+// whether or not the menu object is deleted, so a menu object should
+// create a new instance for each open request.
+// http://crosbug.com/7228, http://crosbug.com/7929
+class NestedDispatcherGtk : public MessageLoopForUI::Dispatcher {
+ public:
+  NestedDispatcherGtk(MessageLoopForUI::Dispatcher* creator,
+                      bool allow_nested_task);
+
+  // Run the messsage loop and returns if the menu has been
+  // deleted in the nested loop. Returns true if the menu is
+  // deleted, or false otherwise.
+  bool RunAndSelfDestruct();
+
+  // Tells the nested dispatcher that creator has been destroyed.
+  void CreatorDestroyed();
+
+ private:
+  virtual ~NestedDispatcherGtk() {}
+
+  // Overriden from MessageLoopForUI::Dispatcher:
+  virtual bool Dispatch(GdkEvent* event);
+
+  // Creator of the nested loop.
+  MessageLoopForUI::Dispatcher* creator_;
+
+  // True to allow nested task in the message loop.
+  bool allow_nested_task_;
+
+  DISALLOW_COPY_AND_ASSIGN(NestedDispatcherGtk);
+};
+
+}  // namespace views
+
+#endif  // VIEWS_CONTROLS_MENU_NESTED_DISPATCHER_GTK_H_
diff --git a/views/views.gyp b/views/views.gyp
index af6096d..f1d7fdd 100644
--- a/views/views.gyp
+++ b/views/views.gyp
@@ -146,6 +146,8 @@
         'controls/menu/native_menu_gtk.h',
         'controls/menu/native_menu_win.cc',
         'controls/menu/native_menu_win.h',
+        'controls/menu/nested_dispatcher_gtk.cc',
+        'controls/menu/nested_dispatcher_gtk.h',
         'controls/menu/radio_button_image_gtk.cc',
         'controls/menu/radio_button_image_gtk.h',
         'controls/menu/submenu_view.cc',