D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

fvogt added a comment. In D27002#605999 , @ngraham wrote: > Could this be the fix for https://bugs.kde.org/show_bug.cgi?id=398908, or part of it? This leak presented itself by steadily growing memory use while something still unknown

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

ngraham added a comment. Could this be the fix for https://bugs.kde.org/show_bug.cgi?id=398908, or part of it? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27002 To: fvogt, #frameworks, davidedmundson, meven Cc: ngraham, anthonyfieroni, meven, kde-frameworks-devel,

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

meven added a comment. In D27002#605864 , @fvogt wrote: > In D27002#605860 , @meven wrote: > > > User feedback: "so far so good, 160 MB Memory usage" > > Does not sound reassuring, I guess the

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

This revision was automatically updated to reflect the committed changes. Closed by commit R241:213ed50634c0: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu (authored by fvogt). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27002?vs=74645=74989

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

fvogt added a comment. In D27002#605860 , @meven wrote: > User feedback: "so far so good, 160 MB Memory usage" > Does not sound reassuring, I guess the user meant 160 MB compared to 200MB or similar prior to patch. Updated - should be

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

fvogt edited the test plan for this revision. REPOSITORY R241 KIO BRANCH noleak REVISION DETAIL https://phabricator.kde.org/D27002 To: fvogt, #frameworks, davidedmundson, meven Cc: anthonyfieroni, meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

meven accepted this revision. meven added a comment. User feedback: "so far so good, 160 MB Memory usage" Does not sound reassuring, I guess the use meant 160 MB compared to 200MB prior to patch. REPOSITORY R241 KIO BRANCH noleak REVISION DETAIL https://phabricator.kde.org/D27002

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

davidedmundson accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH noleak REVISION DETAIL https://phabricator.kde.org/D27002 To: fvogt, #frameworks, davidedmundson Cc: anthonyfieroni, meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh,

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

fvogt added a comment. I'll land tomorrow if no objections. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27002 To: fvogt, #frameworks Cc: anthonyfieroni, meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

fvogt marked 3 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27002 To: fvogt, #frameworks Cc: anthonyfieroni, meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

fvogt edited the test plan for this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27002 To: fvogt, #frameworks Cc: anthonyfieroni, meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

meven added inline comments. INLINE COMMENTS > fvogt wrote in kurlnavigatorplacesselector.cpp:75 > How would that make a difference? Misconception on my part, .children() is not affected by .clear() REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27002 To: fvogt,

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

fvogt added inline comments. INLINE COMMENTS > anthonyfieroni wrote in kurlnavigatorplacesselector.cpp:76 > Why cast? To only delete submenus, not anything else. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27002 To: fvogt, #frameworks Cc: anthonyfieroni, meven,

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

anthonyfieroni added inline comments. INLINE COMMENTS > kurlnavigatorplacesselector.cpp:76 > +for(QObject *obj : QObjectList(m_placesMenu->children())) { > +delete qobject_cast(obj); // Noop for nullptr > +} Why cast? REPOSITORY R241 KIO REVISION DETAIL

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

fvogt added inline comments. INLINE COMMENTS > meven wrote in kurlnavigatorplacesselector.cpp:75 > Shouldn't it be done before the call to `m_placesMenu->clear();` How would that make a difference? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27002 To: fvogt,

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

meven added inline comments. INLINE COMMENTS > kurlnavigatorplacesselector.cpp:75 > +// Submenus have to be deleted explicitly (QTBUG-11070) > +for(QObject *obj : QObjectList(m_placesMenu->children())) { > +delete qobject_cast(obj); // Noop for nullptr Shouldn't it be done

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

fvogt updated this revision to Diff 74645. fvogt added a comment. Make a copy, QObject::children returns const & for some reason, so gets modified during iteration. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27002?vs=74611=74645 BRANCH noleak REVISION

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

fvogt created this revision. fvogt added a reviewer: Frameworks. Herald added a project: Frameworks. fvogt requested review of this revision. REVISION SUMMARY This method gets called each time solid notices a change, which can in some setups be very frequent. It leaked memory as the submenus