NativeWidgetMac has different ownership semantics for controls |
|||
Issue description
I encountered this in trying to fix a leak in FocusTraversalTest. This test creates a Widget of TYPE_CONTROL, with a parent specified in the InitParams and attaches it using NativeViewHost. For NativeWidgetAura widgets created like this are *not* owned by the parent, or rather whether they are owned by the parent depends upon if they are still in the aura Window hierarchy at the time the parent Window is destroyed. Because this test uses NativeViewHost the Window associated with the control Widget is removed from the hierarchy during destruction and so not destroyed when the parent Widget is destroyed.
AFAICT that isn't the case on the mac side. Specifically it seems like control widget ends up in BridgedNativeWidget::child_windows_ so that when the parent is destroyed all the children are destroyed, regardless of whether the Widget is actually parented.
I'm not sure if the bug is in NativeWidgetMac or NativeViewHostMac, probably a combination of the two.
The specific code is in focus_traversal_unittest. BorderView is the class creating the Widget. FocusTraversalTest.NormalTraversal is one of the tests impacted.
Here's the trace from where the child widget gets destroyed on Mac:
0x61200007f888 is located 72 bytes inside of 312-byte region [0x61200007f840,0x61200007f978)
freed by thread T0 here:
#0 0x10e26c846 in 0x00055846 (in libclang_rt.asan_osx_dynamic.dylib) + 198
#1 0x10b1c003c in views::NativeWidgetMac::~NativeWidgetMac() (in views_unittests) + 204
#2 0x10b1c018d in views::NativeWidgetMac::~NativeWidgetMac() (in views_unittests) + 13
#3 0x10b1c05c1 in views::NativeWidgetMac::OnWindowDestroyed() (in views_unittests) + 561
#4 0x10b0678e6 in -[ViewsNSWindowDelegate windowWillClose:] (in views_unittests) + 422
#5 0x7fff87db1e0b in __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ (in CoreFoundation) + 11
#6 0x7fff87ca582c in _CFXNotificationPost (in CoreFoundation) + 2892
#7 0x7fff8fe7bdd9 in -[NSNotificationCenter postNotificationName:object:userInfo:] (in Foundation) + 67
#8 0x7fff89a451ec in __18-[NSWindow _close]_block_invoke (in AppKit) + 178
#9 0x7fff89a450ff in -[NSWindow _close] (in AppKit) + 369
#10 0x10b04d242 in views::BridgedNativeWidget::~BridgedNativeWidget() (in views_unittests) + 786
#11 0x10b04e86d in views::BridgedNativeWidget::~BridgedNativeWidget() (in views_unittests) + 13
#12 0x10b1c0529 in views::NativeWidgetMac::OnWindowDestroyed() (in views_unittests) + 409
#13 0x10b0678e6 in -[ViewsNSWindowDelegate windowWillClose:] (in views_unittests) + 422
#14 0x7fff87db1e0b in __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ (in CoreFoundation) + 11
#15 0x7fff87ca582c in _CFXNotificationPost (in CoreFoundation) + 2892
#16 0x7fff8fe7bdd9 in -[NSNotificationCenter postNotificationName:object:userInfo:] (in Foundation) + 67
#17 0x7fff89a451ec in __18-[NSWindow _close]_block_invoke (in AppKit) + 178
#18 0x7fff89a450ff in -[NSWindow _close] (in AppKit) + 369
#19 0x10b1c866c in base::internal::Invoker\u003Cbase::internal::BindState\u003Cvoid (*)(base::mac::ScopedBlock\u003Cvoid () block_pointer>), base::mac::ScopedBlock\u003Cvoid () block_pointer> >, void ()>::Run(base::internal::BindStateBase*) (in views_unittests) + 284
#20 0x10b211f10 in base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) (in views_unittests) + 784
#21 0x10b25c93f in base::MessageLoop::RunTask(base::PendingTask*) (in views_unittests) + 1599
#22 0x10b25d71c in base::MessageLoop::DeferOrRunPendingTask(base::PendingTask) (in views_unittests) + 124
#23 0x10b25e8be in base::MessageLoop::DoWork() (in views_unittests) + 1150
#24 0x10b26bc68 in base::MessagePumpCFRunLoopBase::RunWork() (in views_unittests) + 344
#25 0x10b248919 in base::mac::CallWithEHFrame(void () block_pointer) (in views_unittests) + 9
#26 0x10b26a110 in base::MessagePumpCFRunLoopBase::RunWorkSource(void*) (in views_unittests) + 368
#27 0x7fff87d145b0 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ (in CoreFoundation) + 16
#28 0x7fff87d05c61 in __CFRunLoopDoSources0 (in CoreFoundation) + 241
#29 0x7fff87d053ee in __CFRunLoopRun (in CoreFoundation) + 830
This does *not* happen on other platforms, BorderView needs to explicitly delete the widget on other platforms. I'm going to ifdef the destruction for now.
,
Nov 8 2016
Thanks for filing this! Patti has an alternative fix for this in https://codereview.chromium.org/2345183002/diff/710001/ui/views/focus/focus_traversal_unittest.cc In a way, the issue is special to that particular test. (Unlike, e.g., views::Webview) BorderView uses a Widget as a convenient way to get a NativeWidget that it Attach()s to the NativeViewHost. NativeViewHostAura::AddClippingWindow() does : Widget::ReparentNativeView(host_->native_view(), &clipping_window_); This moves the aura::Window ownership from the BorderView::widget_'s *parent* aura::Window to NativeViewHostAura::clipping_window_. However it doesn't change the ownership that BorderView::widget_ still has over the same aura::Window. Because BorderView::widget_ still knows about the NativeWidget it created, and (on Mac only) that NativeWidget is closed when BorderView::widget_'s *parent* is closed - BorderView::widget_ is also destroyed because the widget was created with NATIVE_WIDGET_OWNS_WIDGET. The fix in https://codereview.chromium.org/2345183002/diff/710001/ui/views/focus/focus_traversal_unittest.cc is just to use WIDGET_OWNS_NATIVE_WIDGET and stop using a raw pointer for BorderView::widget_. The problem then arises on Mac because child *window* accounting is managed by NativeWidgetMac (NSWindow can't do it, but child *view* accounting with NSView still happens natively in NativeViewHostMac). On Aura, these are the same, so the call to ReparentNativeView is able to break the cascading closure. However, it doesn't address BorderView::widget_'s loose ownership of the NativeWidget it created. That is, ReparentNativeView is affecting the ownership at a different layer to Widget. So, in Aura, when BorderView calls Attach(), the NativeWidget is really owned by two things as well: the Widget and the NativeViewHost's clipping_window_ (just not the parent aura::Window). An alternative to https://codereview.chromium.org/2345183002/diff/710001/ui/views/focus/focus_traversal_unittest.cc might be to perform an explicit detachment of NativeViews that are owned by Widgets in NativeViewHostMac::AttachNativeView(). But, outside of tests, that NativeView isn't usually owned by a Widget so in release code it should do be a no-op.
,
Nov 10 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b22818b5f930aa590f9c5021194904e49b742bda commit b22818b5f930aa590f9c5021194904e49b742bda Author: patricialor <patricialor@chromium.org> Date: Thu Nov 10 05:28:52 2016 views_unittests: Fix FocusTraversalTest's BorderView::widget_ deletion. FocusTraversalTest's BorderView uses a views::Widget as a convenient way to create a NativeWindow, but it sets another Widget's NativeWindow as the parent while holding a weak pointer BorderView::widget_. To safely clean up, make sure |widget_|'s NativeWidget is only owned by |widget_| and make it a unique_ptr instead. This avoids problems when the parent NativeWindow cleans up its native children. BUG= 663418 Review-Url: https://codereview.chromium.org/2482193003 Cr-Commit-Position: refs/heads/master@{#431189} [modify] https://crrev.com/b22818b5f930aa590f9c5021194904e49b742bda/ui/views/focus/focus_traversal_unittest.cc
,
Nov 25 2016
|
|||
►
Sign in to add a comment |
|||
Comment 1 by bugdroid1@chromium.org
, Nov 8 2016