Update closing of nested asynchronous menus

ui/views/controls/menu/menu_controller.cc

 // Copyright (c) 2012 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 "ui/views/controls/menu/menu_controller.h"
 
 #include "base/i18n/case_conversion.h"
 #include "base/i18n/rtl.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
@@ skipping 398 matching lines @@
   if (!blocking_run_) {
     // If we didn't block the caller we need to notify the menu, which
     // triggers deleting us.
     DCHECK(selected);
     showing_ = false;
     delegate_->OnMenuClosed(internal::MenuControllerDelegate::NOTIFY_DELEGATE,
                             selected->GetRootMenuItem(), accept_event_flags_);
     // 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(
     internal::MenuControllerDelegate* delegate) {
   delegate_stack_.push_back(std::make_pair(delegate, async_run_));
   delegate_ = delegate;
 }
 
 void MenuController::SetAsyncRun(bool is_async) {
   delegate_stack_.back().second = is_async;
@@ skipping 374 matching lines @@
     drop_target = drop_target->GetParentMenuItem();
 
   if (!IsBlockingRun()) {
     delegate_->OnMenuClosed(
         internal::MenuControllerDelegate::DONT_NOTIFY_DELEGATE,
         item->GetRootMenuItem(), accept_event_flags_);
   }
 
   // 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);
 }
 
 void MenuController::OnDragEnteredScrollButton(SubmenuView* source,
                                                bool is_up) {
   MenuPart part;
   part.type = is_up ? MenuPart::SCROLL_UP : MenuPart::SCROLL_DOWN;
   part.submenu = source;
   UpdateScrolling(part);
@@ skipping 18 matching lines @@
 void MenuController::OnDragComplete(bool should_close) {
   DCHECK(drag_in_progress_);
   drag_in_progress_ = false;
   // During a drag, mouse events are processed directly by the widget, and not
   // sent to the MenuController. At drag completion, reset pressed state and
   // the event target.
   current_mouse_pressed_state_ = 0;
   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);
+    }
   }
 }
 
 ui::PostDispatchAction MenuController::OnWillDispatchKeyEvent(
     base::char16 character,
     ui::KeyboardCode key_code) {
   if (exit_type() == MenuController::EXIT_ALL ||
       exit_type() == MenuController::EXIT_DESTROYED) {
     TerminateNestedMessageLoop();
     return ui::POST_DISPATCH_PERFORM_DEFAULT;
@@ skipping 322 matching lines @@
       active_mouse_view_id_(ViewStorage::GetInstance()->CreateStorageID()),
       delegate_(delegate),
       message_loop_depth_(0),
       closing_event_time_(base::TimeDelta()),
       menu_start_time_(base::TimeTicks()),
       async_run_(false),
       is_combobox_(false),
       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;
 }
 
 MenuController::~MenuController() {
   DCHECK(!showing_);
   if (owner_)
     owner_->RemoveObserver(this);
   if (active_instance_ == this)
     active_instance_ = NULL;
@@ skipping 56 matching lines @@
 void MenuController::Accept(MenuItemView* item, int event_flags) {
   DCHECK(IsBlockingRun());
   result_ = item;
   if (item && !menu_stack_.empty() &&
       !item->GetDelegate()->ShouldCloseAllMenusOnExecute(item->GetCommand())) {
     SetExitType(EXIT_OUTERMOST);
   } else {
     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,
                                      const gfx::Point& mouse_location) {
   if (!menu_stack_.empty() || !pressed_lock_.get())
     return false;
 
   View* source_view = source->GetScrollViewContainer();
   if (mouse_location.x() >= 0 &&
       mouse_location.x() < source_view->width() &&
@@ skipping 1117 matching lines @@
   // the current loop.
   bool quit_now = exit_type_ != EXIT_NONE && message_loop_depth_;
   if (quit_now)
     TerminateNestedMessageLoop();
 }
 
 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.
   if (async_run_ && ViewsDelegate::GetInstance())
     ViewsDelegate::GetInstance()->ReleaseRef();
 
   // Close any open menus.
   SetSelection(nullptr, SELECTION_UPDATE_IMMEDIATELY | SELECTION_EXIT);
 
 #if defined(OS_WIN)
@@ skipping 100 matching lines @@
   }
 }
 
 gfx::Screen* MenuController::GetScreen() {
   Widget* root = owner_ ? owner_->GetTopLevelWidget() : NULL;
   return root ? gfx::Screen::GetScreenFor(root->GetNativeView())
               : gfx::Screen::GetNativeScreen();
 }
 
 }  // namespace views