Update closing of nested asynchronous menus

ui/views/controls/menu/menu_controller.cc

Index: ui/views/controls/menu/menu_controller.cc
diff --git a/ui/views/controls/menu/menu_controller.cc b/ui/views/controls/menu/menu_controller.cc
index bc718eff70ce3a0ed357e28858ec1ca18f9dcf2f..f764cf735dd581d4a33edb992c9060cb0ff99fdf 100644
--- a/ui/views/controls/menu/menu_controller.cc
+++ b/ui/views/controls/menu/menu_controller.cc
@@ -416,12 +416,7 @@ void MenuController::Cancel(ExitType type) {
     // WARNING: the call to MenuClosed deletes us.
     return;
   }
-  if (async_run_) {
-    internal::MenuControllerDelegate* delegate = delegate_;
-    MenuItemView* result = ExitMenuRun();
-    delegate->OnMenuClosed(internal::MenuControllerDelegate::NOTIFY_DELEGATE,
-                           result, accept_event_flags_);
-  }
+  ExitAsyncRun();
 }
 
 void MenuController::AddNestedDelegate(
@@ -816,6 +811,11 @@ int MenuController::OnPerformDrop(SubmenuView* source,
 
   // WARNING: the call to MenuClosed deletes us.
 
+  // ExitAsyncRun can delete us, however CloseAllNestedMenus enqueues work which

[sky, 2015/12/11 18:37:46]

I'm nervous of posting the task. What work is causing problems? Can we move the
deletion later on so as to avoid the delayed delete?

[jonross, 2016/01/06 21:18:31]

The shutdown enqueues the closing the current widget. Which ends up running
within the message loop that ash's drag/drop controller started.

The subsequent deletion of the MenuController leads to a corrupted buffer that
is attempted to be released a second time. During a subsequent frame render.

When MenuController was nesting message loops itself the deletion would occur
after the drag/drop loop was gone, as well as after its own loop is gone.
(SetExitType terminates the loop)

Without MenuController starting up a loop is receives no further notification
from drag/drop after OnPerformDrop to trigger the deletion from.

That's why I chose to post a task here. Any suggestions on a better way to delay
the delete?

[sky, 2016/01/06 22:20:56]

What do you mean by 'corrupted buffer that is released a second time'?
Shouldn't it close itself before calling to the delegate (as IsBlockingRun())
does above?

[jonross, 2016/01/08 20:52:01]

I'm seeing a few memory errors:
   - Found a corrupted memory buffer in MallocBlock
   - Memory was written to after being freed
Which is being triggered in cc/layers/picture_layer.cc
PictureLayer::PushPropertiesTo

This occurs after DragDropController::StartDragAndDrop has returned from its
message loop. During a subsequent paint.

As for closing itself before calling the delegate, the IsBlockingRun is not hit
in the case of the crash as that is tied to opening the menu for drag/drop. In
this case:
   - Wrench menu is created as async
   - The Bookmarks submenu is opened
   - Perform a drag/drop in the bookmarks submenu.

However you are correct, MenuController should close itself before signalling
the delegate. ExitAsyncMenu does to this.

+  // our deletion interferes with. Perform exit afterwards.
+  base::MessageLoopForUI::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&MenuController::ExitAsyncRun, weak_factory_.GetWeakPtr()));
   return drop_target->GetDelegate()->OnPerformDrop(
       drop_target, drop_position, event);
 }
@@ -854,7 +854,13 @@ void MenuController::OnDragComplete(bool should_close) {
   current_mouse_event_target_ = nullptr;
   if (showing_ && should_close && GetActiveInstance() == this) {
     CloseAllNestedMenus();
-    Cancel(EXIT_ALL);
+    if (async_run_) {
+      base::MessageLoopForUI::current()->PostTask(
+          FROM_HERE,
+          base::Bind(&MenuController::CancelAll, weak_factory_.GetWeakPtr()));
+    } else {
+      Cancel(EXIT_ALL);
+    }
   }
 }
 
@@ -1197,7 +1203,8 @@ MenuController::MenuController(bool blocking,
       item_selected_by_touch_(false),
       current_mouse_event_target_(nullptr),
       current_mouse_pressed_state_(0),
-      message_loop_(MenuMessageLoop::Create()) {
+      message_loop_(MenuMessageLoop::Create()),
+      weak_factory_(this) {
   delegate_stack_.push_back(std::make_pair(delegate_, async_run_));
   active_instance_ = this;
 }
@@ -1274,12 +1281,7 @@ void MenuController::Accept(MenuItemView* item, int event_flags) {
     SetExitType(EXIT_ALL);
   }
   accept_event_flags_ = event_flags;
-  if (async_run_) {
-    internal::MenuControllerDelegate* delegate = delegate_;
-    MenuItemView* result = ExitMenuRun();
-    delegate->OnMenuClosed(internal::MenuControllerDelegate::NOTIFY_DELEGATE,
-                           result, accept_event_flags_);
-  }
+  ExitAsyncRun();
 }
 
 bool MenuController::ShowSiblingMenu(SubmenuView* source,
@@ -2417,6 +2419,19 @@ void MenuController::TerminateNestedMessageLoop() {
   message_loop_->QuitNow();
 }
 
+void MenuController::ExitAsyncRun() {
+  if (!async_run_)
+    return;
+  bool nested = delegate_stack_.size() > 1;
+  // ExitMenuRun unwinds nested delegates
+  internal::MenuControllerDelegate* delegate = delegate_;
+  MenuItemView* result = ExitMenuRun();
+  delegate->OnMenuClosed(internal::MenuControllerDelegate::NOTIFY_DELEGATE,
+                         result, accept_event_flags_);
+  if (nested && exit_type_ == EXIT_ALL)
+    ExitAsyncRun();
+}
+
 MenuItemView* MenuController::ExitMenuRun() {
   // Release the lock which prevents Chrome from shutting down while the menu is
   // showing.