MacViews: Check failed: !view || !compositor_widget_ via LabelButton::AddInkDropLayer under [window close] |
|||||
Issue description
Chrome Version : 53.0.2761.2
OS Version: OS X 10.11.5
In a MacViewsBrowser build, close the browser by closing the last tab.
It looks like this code triggers:
void InkDropHostView::VisibilityChanged(View* starting_from, bool is_visible) {
View::VisibilityChanged(starting_from, is_visible);
if (GetWidget() && !is_visible) {
ink_drop()->AnimateToState(InkDropState::HIDDEN);
ink_drop()->SetHovered(false);
}
}
The View is becoming invisible, because the Widget is (because it is closing). That seems like a bad time to start up animations.
Perhaps we would want GetWidget() to return null if it is closing, but I think that will break too much stuff.
Instead, we could make -[ViewsNSWindowDelegate onWindowOrderWillChange:] suppress *Widget* visibility changes during close. It's possible these are only happening because of code in NativeWidgetMac:
void NativeWidgetMac::Close() {
...
// Clear the view early to suppress repaints.
bridge_->SetRootView(NULL);
// Widget::Close() ensures [Non]ClientView::CanClose() returns true, so there
// is no need to call the NSWindow or its delegate's -windowShouldClose:
// implementation in the manner of -[NSWindow performClose:]. But,
// like -performClose:, first remove the window from AppKit's display
// list to avoid crashes like http://crbug.com/156101 .
[window orderOut:nil];
[window performSelector:@selector(close) withObject:nil afterDelay:0];
}
i.e. that orderOut.
But just suppressing repaints due to this orderOut isn't going to be enough -- too much other stuff might happen before that -[NSWindow close] is triggered.
Looking at http://crbug.com/156101 , I don't think that will affect MacViews. So I think we should just remove that orderOut in NativeWidgetMac::Close().
Then also suppress visibility changes if Widget::Close() has been called, but this will diverge from what Windows and Linux do.
[14626:1295:0614/110730:FATAL:bridged_native_widget.mm(521)] Check failed: !view || !compositor_widget_.
0 libbase.dylib 0x000000011674d9ce _ZN4base5debug10StackTraceC2Ev + 30
1 libbase.dylib 0x000000011674da35 _ZN4base5debug10StackTraceC1Ev + 21
2 libbase.dylib 0x00000001167d13d0 _ZN7logging10LogMessageD2Ev + 80
3 libbase.dylib 0x00000001167cefe5 _ZN7logging10LogMessageD1Ev + 21
4 libviews.dylib 0x000000012cbcf80d _ZN5views19BridgedNativeWidget11SetRootViewEPNS_4ViewE + 413
5 libviews.dylib 0x000000012cd1dbd5 _ZN5views15NativeWidgetMac18ReorderNativeViewsEv + 133
6 libviews.dylib 0x000000012cd31afd _ZN5views6Widget18ReorderNativeViewsEv + 29
7 libviews.dylib 0x000000012cd0d6dd _ZN5views4View13ReorderLayersEv + 285
8 libviews.dylib 0x000000012cd04133 _ZN5views4View11CreateLayerEv + 467
9 libviews.dylib 0x000000012cd0420b _ZN5views4View15SetPaintToLayerEb + 123
10 libviews.dylib 0x000000012cbf9cdd _ZN5views11LabelButton15AddInkDropLayerEPN2ui5LayerE + 45
11 libviews.dylib 0x000000012cbabf73 _ZN5views11InkDropImpl26AddRootLayerToHostIfNeededEv + 435
12 libviews.dylib 0x000000012cbab90d _ZN5views11InkDropImpl19CreateInkDropRippleEv + 957
13 libviews.dylib 0x000000012cbab42f _ZN5views11InkDropImpl14AnimateToStateENS_12InkDropStateE + 79
14 libviews.dylib 0x000000012cba956f _ZN5views15InkDropHostView17VisibilityChangedEPNS_4ViewEb + 127
15 libviews.dylib 0x000000012cbf3e26 _ZN5views12CustomButton17VisibilityChangedEPNS_4ViewEb + 54
16 libviews.dylib 0x000000012cd0f381 _ZN5views4View21VisibilityChangedImplEPS0_b + 65
17 libviews.dylib 0x000000012cd03b81 _ZN5views4View32PropagateVisibilityNotificationsEPS0_b + 129
18 libviews.dylib 0x000000012cd03b5e _ZN5views4View32PropagateVisibilityNotificationsEPS0_b + 94
19 libviews.dylib 0x000000012cd03b5e _ZN5views4View32PropagateVisibilityNotificationsEPS0_b + 94
20 libviews.dylib 0x000000012cd03b5e _ZN5views4View32PropagateVisibilityNotificationsEPS0_b + 94
21 libviews.dylib 0x000000012cd03b5e _ZN5views4View32PropagateVisibilityNotificationsEPS0_b + 94
22 libviews.dylib 0x000000012cd03b5e _ZN5views4View32PropagateVisibilityNotificationsEPS0_b + 94
23 libviews.dylib 0x000000012cd323b6 _ZN5views6Widget31OnNativeWidgetVisibilityChangedEb + 86
24 libviews.dylib 0x000000012cbd4768 _ZN5views19BridgedNativeWidget21OnVisibilityChangedToEb + 824
25 libviews.dylib 0x000000012cbe93f5 -[ViewsNSWindowDelegate onWindowOrderWillChange:] + 53
26 libviews.dylib 0x000000012cbe7f7c -[NativeWidgetMacNSWindow orderWindow:relativeTo:] + 76
27 libviews.dylib 0x000000012cd1f365 _ZN5views15NativeWidgetMac5CloseEv + 277
28 libviews.dylib 0x000000012cd3036a _ZN5views6Widget5CloseEv + 362
29 libchrome_main_dll.dylib 0x000000010a51fbee _ZN11BrowserView5CloseEv + 46
30 libchrome_main_dll.dylib 0x000000010a72f7db _ZN7Browser10CloseFrameEv + 43
31 libchrome_main_dll.dylib 0x000000010a73ace0 _ZN4base8internal15RunnableAdapterIM7BrowserFvvEE3RunIPS2_JEEEvOT_DpOT0_ + 112
32 libchrome_main_dll.dylib 0x000000010a73cc92 _ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRNS0_15RunnableAdapterIM7BrowserFvvEEENS_7WeakPtrIS5_EEJEEEvOT_T0_DpOT1_ + 98
,
Aug 11 2016
,
Aug 11 2016
CL for this is here: https://codereview.chromium.org/2061693003/
,
Aug 11 2016
,
Oct 13 2016
Issue 654156 has been merged into this issue.
,
Oct 18 2016
So I poked around on Windows and there's currently a spew of layers being added during layout. e.g. when LocationBarView::Layout() does selected_keyword_view_->SetVisible(false); location_icon_view_->SetVisible(false); keyword_hint_view_->SetVisible(false); https://cs.chromium.org/chromium/src/chrome/browser/ui/views/location_bar/location_bar_view.cc?sq=package:chromium&rcl=1476731755&l=546 The logic in InkDropHostView::VisibilityChanged() is causing layers to be added for these hidden -> hidden transitions, which is costly and unnecessary. Fix for this: https://codereview.chromium.org/2431493002 (the approach in #c3 only affects mac).
,
Oct 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2cb97f14a013d26b5139d2144537d591181a7f4e commit 2cb97f14a013d26b5139d2144537d591181a7f4e Author: tapted <tapted@chromium.org> Date: Thu Oct 20 00:09:34 2016 MD Buttons: Don't add layers for hidden -> hidden transitions. InkDropHostView::VisibilityChanged() is invoked when never-transitioned (or even never-shown) views are set hidden. This can happen with calls to View::SetVisibility(false), Widget::SetVisible(false) or Widget::Close*(). These Views may never be shown, so don't add a layer to animate ink drops in these cases. BUG= 619798 Review-Url: https://chromiumcodereview.appspot.com/2431493002 Cr-Commit-Position: refs/heads/master@{#426345} [modify] https://crrev.com/2cb97f14a013d26b5139d2144537d591181a7f4e/ui/views/animation/ink_drop_impl.cc [modify] https://crrev.com/2cb97f14a013d26b5139d2144537d591181a7f4e/ui/views/animation/ink_drop_impl_unittest.cc [modify] https://crrev.com/2cb97f14a013d26b5139d2144537d591181a7f4e/ui/views/animation/test/test_ink_drop_host.cc [modify] https://crrev.com/2cb97f14a013d26b5139d2144537d591181a7f4e/ui/views/animation/test/test_ink_drop_host.h [modify] https://crrev.com/2cb97f14a013d26b5139d2144537d591181a7f4e/ui/views/controls/button/custom_button_unittest.cc
,
Oct 20 2016
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by tapted@chromium.org
, Jun 14 2016