Prevent nested Menu Cancelling

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/macros.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 497 matching lines @@
 
 void MenuController::Cancel(ExitType type) {
   // If the menu has already been destroyed, no further cancellation is
   // needed.  We especially don't want to set the |exit_type_| to a lesser
   // value.
   if (exit_type_ == EXIT_DESTROYED || exit_type_ == type)
     return;
 
   if (!showing_) {
     // This occurs if we're in the process of notifying the delegate for a drop
-    // and the delegate cancels us.
+    // and the delegate cancels us. Or if the releasing of ViewsDelegate causes
+    // an immediate shutdown.
     return;
   }
 
   MenuItemView* selected = state_.item;
   SetExitType(type);
 
   SendMouseCaptureLostToActiveView();
 
   // Hide windows immediately.
   SetSelection(NULL, SELECTION_UPDATE_IMMEDIATELY | SELECTION_EXIT);
 
   if (!blocking_run_) {
     // TODO(jonross): remove after tracking down the cause of
     // (crbug.com/683087).
     bool nested = delegate_stack_.size() > 1;
     CHECK(!nested);
     base::WeakPtr<MenuController> this_ref = AsWeakPtr();
     // 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.
     CHECK(!this_ref);
     return;
   }
 
+  // If |type| is EXIT_ALL we update the state of the menu to not showing. For
+  // dragging this ensures that the correct visual state is reported until the
+  // drag operation completes. For non-dragging cases it is possible that the
+  // release of ViewsDelegate leads immediately to shutdown, which can trigger
+  // nested calls to Cancel. We want to reject these to prevent attempting a
+  // nested tear down of this and |delegate_|.
+  if (type == EXIT_ALL)
+    showing_ = false;
+
   // On Windows and Linux the destruction of this menu's Widget leads to the
   // teardown of the platform specific drag-and-drop Widget. Do not shutdown
   // while dragging, leave the Widget hidden until drag-and-drop has completed,
   // at which point all menus will be destroyed.
-  //
-  // If |type| is EXIT_ALL we update the state of the menu to not showing. So
-  // that during the completion of a drag we are not incorrectly reporting the
-  // visual state.
   if (!drag_in_progress_)
     ExitAsyncRun();
-  else if (type == EXIT_ALL)
-    showing_ = false;
 }
 
 void MenuController::AddNestedDelegate(
     internal::MenuControllerDelegate* delegate) {
   // TODO(jonross): remove after tracking down the cause of (crbug.com/683087).
   for (auto delegates : delegate_stack_) {
     // Having the same delegate in the stack could cause deletion order issues.
     CHECK_NE(delegates.first, delegate);
   }
 
@@ skipping 2056 matching lines @@
   if (this_ref && nested && exit_type_ == EXIT_ALL)
     ExitAsyncRun();
 }
 
 MenuItemView* MenuController::ExitMenuRun() {
   base::WeakPtr<MenuController> this_ref = AsWeakPtr();
 
   // Release the lock which prevents Chrome from shutting down while the menu is
   // showing.
   if (async_run_ && ViewsDelegate::GetInstance())
     ViewsDelegate::GetInstance()->ReleaseRef();

[sky, 2017/02/07 19:23:51]

Is your worry that this line is causing reentrancy that we don't expect? Is the
comment on 2620 wrong? That the delegate has been destroyed too?

[jonross, 2017/02/07 20:07:21]

So I only seem to have a partial stack trace from the fuzzing. 
However the related issue 685462 shows that this line can cause reentrancy of
the MenuController::Cancel.

Specifically:
MenuRunnerImpl::Release()
MenuControllerDelegate::OnMenuClosed()
MenuController::ExitAsyncRun()
MenuController::Cancel(EXIT_DESTROYED)
... a bunch of widget closing calls....
ChromeViewsDelegate::ReleaseRef()
MenuController::ExitAsyncRun()
MenuController::Cancel(EXIT_ALL)
... something that hides the menu, either timer or clicking outside.

The comment on 2620 is not comprehensive. There are three cases:
  1) Releasing ViewsDelegate does nothing, MenuController and
MenuControllerDelegate are both alive after ExitMenuRun
  2) Releasing ViewsDelegate leads to MenuController being destroyed, however
the MenuControllerDelegate is not tied directly to a Widget, and awaits
OnMenuClosed before destroying itself
  3) Releasing ViewsDelegate leads to Widget destruction, which deletes both the
MenuController and the MenuDelegate.

1) is the most most common, no risk.
2) occurs on each shutdown, as the BookmarksBar is built this way, and performs
some related shutdown cleanup here.
3) is what this bug seems to be.

 
   // Releasing the lock can result in Chrome shutting down, deleting this.
   if (!this_ref)
     return nullptr;
 
   // Close any open menus.
   SetSelection(nullptr, SELECTION_UPDATE_IMMEDIATELY | SELECTION_EXIT);
 
 #if defined(OS_WIN)
   // On Windows, if we select the menu item by touch and if the window at the
@@ skipping 130 matching lines @@
   if (hot_button_)
     hot_button_->SetHotTracked(false);
   hot_button_ = hot_button;
   if (hot_button) {
     hot_button->SetHotTracked(true);
     hot_button->NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION, true);
   }
 }
 
 }  // namespace views