Turn Bookmark Menus Async

chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc

Index: chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc b/chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc
index a5f4eab6d2e88f80a0040a6a21e2fadd336af085..85b7fdd3287c0b0bdf533dc68308cc59057c2c36 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc
@@ -39,8 +39,10 @@ BookmarkMenuController::BookmarkMenuController(Browser* browser,
   menu_delegate_->Init(this, NULL, node, start_child_index,
                        BookmarkMenuDelegate::HIDE_PERMANENT_FOLDERS,
                        BOOKMARK_LAUNCH_LOCATION_BAR_SUBFOLDER);
-  menu_runner_.reset(new views::MenuRunner(
-      menu_delegate_->menu(), for_drop ? views::MenuRunner::FOR_DROP : 0));
+  int run_type = views::MenuRunner::ASYNC;
+  if (for_drop)
+    run_type |= views::MenuRunner::FOR_DROP;
+  menu_runner_.reset(new views::MenuRunner(menu_delegate_->menu(), run_type));
 }
 
 void BookmarkMenuController::RunMenuAt(BookmarkBarView* bookmark_bar) {
@@ -188,8 +190,12 @@ void BookmarkMenuController::WillShowMenu(MenuItemView* menu) {
 }
 
 void BookmarkMenuController::BookmarkModelChanged() {
-  if (!menu_delegate_->is_mutating_model())
-    menu()->Cancel();
+  // Do not manipulate if menu is shutdown, or if currently changing the model.
+  if (!menu_runner_ || !menu_runner_->IsRunning() ||

[sky, 2016/07/20 16:47:10]

Why do you need the !IsRunning() check? Shouldn't that be a no-op if !running?

[jonross, 2016/07/20 17:43:19]

It is a no opt as long as there has not been any destruction of the classes.

I found that this method can be called after its associated menu/menucontroller
have shut. Menu holds onto the MenuController*, which can lead to calls on
destructed objects.

I'd considered updating MenuController to have other silent exits, but thought
it might be better to detect where calling code was causing crashes, and to fix
there.

[sky, 2016/07/20 19:41:53]

I would expect a function named Cancel() to be callable at any time, and it's
easy to miss that you should only call it under certain circumstances. I think
it better to have MenuRunner cleanup internal state so that Cancel() can be
called at anytime.

[jonross, 2016/07/21 14:02:58]

Done.

+      menu_delegate_->is_mutating_model()) {
+    return;
+  }
+  menu()->Cancel();
 }
 
 BookmarkMenuController::~BookmarkMenuController() {