New issue
Advanced search Search tips

Issue 663418 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug



Sign in to add a comment

NativeWidgetMac has different ownership semantics for controls

Project Member Reported by sky@chromium.org, Nov 8 2016

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.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 8 2016

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

commit e7886de9e860bac867bf6e029d8c6f2a0431a7b7
Author: sky <sky@chromium.org>
Date: Tue Nov 08 18:23:43 2016

Fix leak in FocusManager tests

They create a widget and never destroy it.

BUG= 660994 ,  663418 
TEST=test only change
R=ben@chromium.org

Committed: https://crrev.com/402f62030eda12d30ad6341e2774d9af2208bee1
Review-Url: https://codereview.chromium.org/2483083002
Cr-Original-Commit-Position: refs/heads/master@{#430457}
Cr-Commit-Position: refs/heads/master@{#430657}

[modify] https://crrev.com/e7886de9e860bac867bf6e029d8c6f2a0431a7b7/ui/views/focus/focus_traversal_unittest.cc

Owner: patricia...@chromium.org
Status: Assigned (was: Untriaged)
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.
Project Member

Comment 3 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)

Sign in to add a comment