Turn Bookmark Menus Async

chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.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 "chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.h"
 
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/bookmarks/bookmark_stats.h"
 #include "chrome/browser/ui/views/bookmarks/bookmark_bar_view.h"
@@ skipping 21 matching lines @@
                                                int start_child_index,
                                                bool for_drop)
     : menu_delegate_(new BookmarkMenuDelegate(browser, page_navigator, parent)),
       node_(node),
       observer_(NULL),
       for_drop_(for_drop),
       bookmark_bar_(NULL) {
   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) {
   bookmark_bar_ = bookmark_bar;
   views::MenuButton* menu_button = bookmark_bar_->GetMenuButtonForNode(node_);
   DCHECK(menu_button);
   views::MenuAnchorPosition anchor;
   bookmark_bar_->GetAnchorPositionForButton(menu_button, &anchor);
   gfx::Point screen_loc;
   views::View::ConvertPointToScreen(menu_button, &screen_loc);
@@ skipping 127 matching lines @@
 
 int BookmarkMenuController::GetMaxWidthForMenu(MenuItemView* view) {
   return menu_delegate_->GetMaxWidthForMenu(view);
 }
 
 void BookmarkMenuController::WillShowMenu(MenuItemView* menu) {
   menu_delegate_->WillShowMenu(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() {
   menu_delegate_->GetBookmarkModel()->RemoveObserver(this);
   if (observer_)
     observer_->BookmarkMenuControllerDeleted(this);
 }