New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 780408 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug
Team-Security-UX

Blocking:
issue 713030



Sign in to add a comment

Memory leak in PageInfoBubble with --secondary-ui-md

Project Member Reported by tapted@chromium.org, Nov 1 2017

Issue description

Chrome 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
 
Blocking: 713030
Project Member

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

Status: Fixed (was: Started)

Sign in to add a comment