New issue
Advanced search Search tips

Issue 837767 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

macviews: crash quitting with menu open via system menubar

Project Member Reported by ellyjo...@chromium.org, Apr 27 2018

Issue description

1) Open a menu (omnibox context menu works well)
2) File -> Quit
3) Crash

The stack trace reveals that:

BrowserCloseManager::CloseBrowsers() -> BridgedNativeWidget::SetVisibilityState() -> ~CocoaMouseCapture() -> MenuHost::OnMouseCaptureLost() -> MenuController::Cancel(this == nullptr)

so perhaps the menu is already destructed or partly so when we arrive here.
 
Labels: Sprint-2
Status: Started (was: Assigned)
The issue is a MenuModel being destroyed while still referenced by MenuModelAdapter. Specifically, the Textfield's MenuModel is destroyed like so:

0   libbase.dylib                       0x000000010cc4032e base::debug::StackTrace::StackTrace(unsigned long) + 174
1   libbase.dylib                       0x000000010cc403ed base::debug::StackTrace::StackTrace(unsigned long) + 29
2   libbase.dylib                       0x000000010c83dd1c base::debug::StackTrace::StackTrace() + 28
3   libui_base.dylib                    0x000000010d1d1bdb ui::MenuModel::~MenuModel() + 123
4   libui_base.dylib                    0x000000010d1d2fdd ui::SimpleMenuModel::~SimpleMenuModel() + 93
5   libui_base.dylib                    0x000000010d1d3045 ui::SimpleMenuModel::~SimpleMenuModel() + 21
6   libui_base.dylib                    0x000000010d1d3069 ui::SimpleMenuModel::~SimpleMenuModel() + 25
7   libviews.dylib                      0x0000000140d48654 views::Textfield::OnCaretBoundsChanged() + 372
8   libviews.dylib                      0x0000000140d48cc1 views::Textfield::UpdateAfterChange(bool, bool) + 193
9   libviews.dylib                      0x0000000140d49869 views::Textfield::SelectRange(gfx::Range const&) + 89
10  libchrome_dll.dylib                 0x0000000121569fb9 OmniboxViewViews::OnBlur() + 3033
11  libviews.dylib                      0x0000000140dcd6b9 views::View::Blur() + 25
12  libviews.dylib                      0x0000000140d78d0f views::FocusManager::SetFocusedViewWithReason(views::View*, views::FocusManager::FocusChangeReason) + 511
13  libviews.dylib                      0x0000000140bef542 views::FocusManager::SetFocusedView(views::View*) + 34
14  libviews.dylib                      0x0000000140d7a10a views::FocusManager::ClearFocus() + 42
15  libviews.dylib                      0x0000000140d7a31a views::FocusManager::StoreFocusedView(bool) + 106
16  libviews.dylib                      0x0000000140c5e755 views::BridgedNativeWidget::OnWindowKeyStatusChangedTo(bool) + 293
17  libviews.dylib                      0x0000000140c74cfc -[ViewsNSWindowDelegate windowDidResignKey:] + 44
18  CoreFoundation                      0x00007fff32ca861c __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 12
19  CoreFoundation                      0x00007fff32ca84ea _CFXRegistrationPost + 458
20  CoreFoundation                      0x00007fff32ca8221 ___CFXNotificationPost_block_invoke + 225
21  CoreFoundation                      0x00007fff32c66d72 -[_CFXNotificationRegistrar find:object:observer:enumerator:] + 1826
22  CoreFoundation                      0x00007fff32c65e03 _CFXNotificationPost + 659
23  Foundation                          0x00007fff34e308c7 -[NSNotificationCenter postNotificationName:object:userInfo:] + 66
24  AppKit                              0x00007fff303fe0e1 -[NSWindow resignKeyWindow] + 867
25  AppKit                              0x00007fff303bf8b8 -[NSWindow _orderOutAndCalcKeyWithCounter:stillVisible:docWindow:] + 253
26  AppKit                              0x00007fff30347c0c NSPerformVisuallyAtomicChange + 146
27  AppKit                              0x00007fff30af5833 -[NSWindow _doWindowOrderOutWithWithKeyCalc:forCounter:orderingDone:docWindow:] + 85
28  AppKit                              0x00007fff30af6d26 -[NSWindow _reallyDoOrderWindowOutRelativeTo:findKey:forCounter:force:isModal:] + 446
29  AppKit                              0x00007fff3034f8ec -[NSWindow _reallyDoOrderWindow:relativeTo:findKey:forCounter:force:isModal:] + 172
30  AppKit                              0x00007fff3034e765 -[NSWindow _doOrderWindow:relativeTo:findKey:forCounter:force:isModal:] + 287
31  AppKit                              0x00007fff3034e5cb -[NSWindow orderWindow:relativeTo:] + 169
32  libviews.dylib                      0x0000000140c733a1 -[NativeWidgetMacNSWindow orderWindow:relativeTo:] + 81
33  libchrome_dll.dylib                 0x00000001212fc281 -[BrowserNativeWidgetWindow orderWindow:relativeTo:] + 81
34  libviews.dylib                      0x0000000140c5a410 views::BridgedNativeWidget::SetVisibilityState(views::BridgedNativeWidget::WindowVisibilityState) + 352
35  libviews.dylib                      0x0000000140de38ec views::NativeWidgetMac::Hide() + 108
36  libviews.dylib                      0x0000000140df6d7d views::Widget::Hide() + 29
37  libchrome_dll.dylib                 0x0000000121500440 BrowserView::CanClose() + 256
38  libviews.dylib                      0x0000000140e17a10 views::NonClientView::CanClose() + 32
39  libviews.dylib                      0x0000000140df62c2 views::Widget::Close() + 98
40  libchrome_dll.dylib                 0x00000001214f3cbe BrowserView::Close() + 46
41  libchrome_dll.dylib                 0x000000011bd70e7c BrowserCloseManager::CloseBrowsers() + 1996

but then it is used via: MenuHost::OnMouseCaptureLost() -> MenuController::Cancel() -> MenuController::SetSelection() -> MenuModelAdapter::WillHideMenu(), which tries to use the now-dead model :(


In turn, the issue there is that MenuModelAdapter requires that:

  // The caller retains ownership of the ui::MenuModel instance and
  // must ensure it exists for the lifetime of the adapter.

and (implicitly) MenuRunner has the same contract. Textfield context menus do:

  std::unique_ptr<ui::SimpleMenuModel> context_menu_contents_;
  std::unique_ptr<ViewsTextServicesContextMenu> text_services_context_menu_;
  std::unique_ptr<views::MenuRunner> context_menu_runner_;

which ought to be fine, except that they also do:

#if defined(OS_MACOSX)
  // On Mac, the context menu contains a look up item which displays the
  // selected text. As such, the menu needs to be updated if the selection has
  // changed.
  context_menu_contents_.reset();
#endif

which breaks the contract of the context menu runner :(.
Project Member

Comment 5 by bugdroid1@chromium.org, May 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b3aa2b657d09c332d88ed29183c3f1710ae7111c

commit b3aa2b657d09c332d88ed29183c3f1710ae7111c
Author: Elly Fong-Jones <ellyjones@chromium.org>
Date: Wed May 02 12:57:57 2018

macviews: fix textfield context menu closure crash

When closing the window on Mac via the system menus, Textfields can end up
with their caret bounds changed (through a roundabout route) while they
have a context menu open. This causes a reset of the context menu's model
with the runner still referencing the old model, which leads to chaos. To
fix that, reset the runner as well when updating the bounds.

Bug:  837767 
Change-Id: Id955d84c525f8d71121414f2784f216b4d9e6a65
Reviewed-on: https://chromium-review.googlesource.com/1037404
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555364}
[modify] https://crrev.com/b3aa2b657d09c332d88ed29183c3f1710ae7111c/ui/views/controls/textfield/textfield.cc

Status: Fixed (was: Started)
Labels: TE-Verified-M68 TE-Verified-68.0.3419.0
Able to reproduce the issue on mac 10.13.3 using the build without fix.

Verified the fix on Mac 10.13.3 using Chrome version #68.0.3419.0 as per the comment #0.
Attaching screen shot for reference.
Observed that quitting chrome with menu open via system menubar did not produce any crash.
Hence, the fix is working as expected. 
Adding the verified labels.

Thanks...!!
837767.mp4
1015 KB View Download

Sign in to add a comment