Memory leak in PageInfoBubble with --secondary-ui-md |
||
Issue descriptionChrome Version : 60.0.3112.113 OS Version: OS X 10.12.6 Tweaking a unit test in https://chromium-review.googlesource.com/c/chromium/src/+/748861 and enabling SecondaryUiMd on the bots exposes: https://logs.chromium.org/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux%2Flinux_chromium_asan_rel_ng%2F480976%2F%2B%2Frecipes%2Fsteps%2Funit_tests__with_patch_%2F0%2Flogs%2FPageInfoBubbleViewTest.SetPermissionInfo%2F0 SUMMARY: AddressSanitizer: 46048 byte(s) leaked in 462 allocation(s). <truncated (726177 bytes)> 8 0x1407cb0e in base::(anonymous namespace)::LaunchUnitTestsInternal(base::RepeatingCallback<int ()> const&, unsigned long, int, bool, base::RepeatingCallback<void ()> const&) base/test/launcher/unit_test_launcher.cc:216 #29 0x1407c5e6 in base::LaunchUnitTests(int, char**, base::RepeatingCallback<int ()> const&) base/test/launcher/unit_test_launcher.cc:475:10 #30 0x140579b0 in main chrome/test/base/run_all_unittests.cc:30:10 #31 0x7f6c71b04f44 in __libc_start_main /build/eglibc-SvCtMH/eglibc-2.19/csu/libc-start.c:287 Indirect leak of 16 byte(s) in 1 object(s) allocated from: #0 0x7951c12 in operator new(unsigned long) (/b/s/w/ir/out/Release/unit_tests+0x7951c12) #1 0x165c6e5f in __allocate buildtools/third_party/libc++/trunk/include/new:226:10 #2 0x165c6e5f in allocate buildtools/third_party/libc++/trunk/include/memory:1786 #3 0x165c6e5f in allocate buildtools/third_party/libc++/trunk/include/memory:1541 #4 0x165c6e5f in __split_buffer buildtools/third_party/libc++/trunk/include/__split_buffer:309 #5 0x165c6e5f in std::__1::vector<views::View*, std::__1::allocator<views::View*> >::insert(std::__1::__wrap_iter<views::View* const*>, views::View* const&) buildtools/third_party/libc++/trunk/include/vector:1753 #6 0x165c44ac in views::View::AddChildViewAt(views::View*, int) ui/views/view.cc:213:13 #7 0x164d6bce in views::Combobox::Combobox(ui::ComboboxModel*, views::Combobox::Style) ui/views/controls/combobox/combobox.cc:454:3 #8 0x1df9eaf0 in internal::PermissionCombobox::PermissionCombobox(internal::ComboboxModelAdapter*, bool, bool) chrome/browser/ui/views/page_info/permission_selector_row.cc:236:7 #9 0x1dfa0491 in PermissionSelectorRow::InitializeComboboxView(views::GridLayout*, PageInfoUI::PermissionInfo const&) chrome/browser/ui/views/page_info/permission_selector_row.cc:388:19 #10 0x1df9f823 in PermissionSelectorRow::PermissionSelectorRow(Profile*, GURL const&, PageInfoUI::PermissionInfo const&, views::GridLayout*) chrome/browser/ui/views/page_info/permission_selector_row.cc:308:5 #11 0x1df8d3a1 in make_unique<PermissionSelectorRow, Profile *&, const GURL &, const PageInfoUI::PermissionInfo &, views::GridLayout *&> buildtools/third_party/libc++/trunk/include/memory:3065:32 #12 0x1df8d3a1 in MakeUnique<PermissionSelectorRow, Profile *&, const GURL &, const PageInfoUI::PermissionInfo &, views::GridLayout *&> base/memory/ptr_util.h:25 #13 0x1df8d3a1 in PageInfoBubbleView::SetPermissionInfo(std::__1::vector<PageInfoUI::PermissionInfo, std::__1::allocator<PageInfoUI::PermissionInfo> > const&, std::__1::vector<std::__1::unique_ptr<PageInfoUI::ChosenObjectInfo, std::__1::default_delete<PageInfoUI::ChosenObjectInfo> >, std::__1::allocator<std::__1::unique_ptr<PageInfoUI::ChosenObjectInfo, std::__1::default_delete<PageInfoUI::ChosenObjectInfo> > > >) chrome/browser/ui/views/page_info/page_info_bubble_view.cc:764 #14 0x1d9c0aa9 in PageInfo::PresentSitePermissions() chrome/browser/ui/page_info/page_info.cc:849:8 #15 0x1d9c3920 in PageInfo::OnSitePermissionChanged(ContentSettingsType, ContentSetting) chrome/browser/ui/page_info/page_info.cc:437:3 #16 0xc8f5343 in SetPermissionInfo chrome/browser/ui/views/page_info/page_info_bubble_view_unittest.cc:112:26 I think the problem is this: PermissionSelectorRow::~PermissionSelectorRow() { // Gross. On paper the Combobox and the ComboboxModelAdapter are both owned by // this class, but actually, the Combobox is owned by View and will be // destroyed in ~View(), which runs *after* ~PermissionSelectorRow() is done, // which means the Combobox gets destroyed after its ComboboxModel, which // causes an explosion when the Combobox attempts to stop observing the // ComboboxModel. This hack ensures the Combobox is deleted before its // ComboboxModel. // // Technically, the MenuButton has the same problem, but MenuButton doesn't // use its model in its destructor. if (combobox_) { combobox_->parent()->RemoveChildView(combobox_); } } View::RemoveChildView doesn't delete the view it's given when removing it from the view hierarchy. Usually to remove views simply `delete combobox_;` does the trick, since View::~View() also removes the view from the hierarchy. Trying the fix in https://chromium-review.googlesource.com/c/chromium/src/+/748861
,
Nov 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d34d8303ef116967f7a50a2734bc43338c11deb1 commit d34d8303ef116967f7a50a2734bc43338c11deb1 Author: Trent Apted <tapted@chromium.org> Date: Wed Nov 01 23:15:16 2017 Fix PageInfoBubbleViewTest.SetPermissionInfo with SecondaryUiMd Under MD, the changed setting is always read from the combobox selected index when changed in the UI. PermissionSelectorRow::PermissionChanged() ignores the argument, except to detect whether it is the default. Also PermissionMenuModel gets strings using PageInfoUI:: PermissionActionToUIString(..) rather than directly from the ResourceBundle. Ensure the unit test acconts for these. Test exposed a memory leak with SecondaryUiMd, which enables views:: Combobox for the selectors. Combobox needs to be deleted before its model is destroyed. View::RemoveChildView doesn't delete its argument, but simply calling delete view does end up calling RemoveChildView(). Bug: 713030 , 780408 Change-Id: I373cf0d40316bb2fc42e76bb470d1977c0678e2b Reviewed-on: https://chromium-review.googlesource.com/748861 Reviewed-by: Lucas Garron <lgarron@chromium.org> Commit-Queue: Trent Apted <tapted@chromium.org> Cr-Commit-Position: refs/heads/master@{#513315} [modify] https://crrev.com/d34d8303ef116967f7a50a2734bc43338c11deb1/chrome/browser/ui/views/page_info/page_info_bubble_view_unittest.cc [modify] https://crrev.com/d34d8303ef116967f7a50a2734bc43338c11deb1/chrome/browser/ui/views/page_info/permission_selector_row.cc
,
Nov 7 2017
|
||
►
Sign in to add a comment |
||
Comment 1 by tapted@chromium.org
, Nov 1 2017