D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-02-04 Thread Fabian Vogt 
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 triggered solid's `onMtabChanged` excessively.
  If that's also the case for the reporter of that bug, this should've made a 
big difference, otherwise only a small one.
  
  The output of heaptrack would tell for sure.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27002

To: fvogt, #frameworks, davidedmundson, meven
Cc: ngraham, anthonyfieroni, meven, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-02-04 Thread Nathaniel Graham 
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, LeGast00n, GB_2, 
michaelh, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-02-04 Thread Méven Car 
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 user meant 160 MB compared to 
200MB or similar prior to patch.
  >
  >
  > Updated - should be clearer now :-)
  
  
  Great to see the impact, this was a serious leak.

REPOSITORY
  R241 KIO

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

2020-02-04 Thread Fabian Vogt 
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

REVISION DETAIL
  https://phabricator.kde.org/D27002

AFFECTED FILES
  src/filewidgets/kurlnavigatorplacesselector.cpp

To: fvogt, #frameworks, davidedmundson, meven
Cc: anthonyfieroni, meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-02-04 Thread Fabian Vogt 
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 clearer now :-)

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

2020-02-04 Thread Fabian Vogt 
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

2020-02-04 Thread Méven Car 
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

To: fvogt, #frameworks, davidedmundson, meven
Cc: anthonyfieroni, meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-02-04 Thread David Edmundson 
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, 
ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-02-04 Thread Fabian Vogt 
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

2020-01-30 Thread Fabian Vogt 
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

2020-01-30 Thread Fabian Vogt 
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

2020-01-30 Thread Méven Car 
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, #frameworks
Cc: anthonyfieroni, meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-01-30 Thread Fabian Vogt 
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, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-01-30 Thread Anthony Fieroni 
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
  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

2020-01-30 Thread Fabian Vogt 
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, #frameworks
Cc: meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-01-30 Thread Méven Car 
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 before the call to `m_placesMenu->clear();`

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27002

To: fvogt, #frameworks
Cc: meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-01-30 Thread Fabian Vogt 
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 DETAIL
  https://phabricator.kde.org/D27002

AFFECTED FILES
  src/filewidgets/kurlnavigatorplacesselector.cpp

To: fvogt, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-01-29 Thread Fabian Vogt 
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 and their actions
  were not deallocated properly.

TEST PLAN
  Builds, waiting for user feedback.

REPOSITORY
  R241 KIO

BRANCH
  noleak

REVISION DETAIL
  https://phabricator.kde.org/D27002

AFFECTED FILES
  src/filewidgets/kurlnavigatorplacesselector.cpp

To: fvogt, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns