Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu
ClosedPublic

Authored by fvogt on Wed, Jan 29, 6:55 PM.

Details

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. User feedback: "so far so good, 160 MB Memory usage". It was ~7GiB before this patch.

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
fvogt created this revision.Wed, Jan 29, 6:55 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptWed, Jan 29, 6:55 PM
fvogt requested review of this revision.Wed, Jan 29, 6:55 PM
fvogt updated this revision to Diff 74645.Thu, Jan 30, 8:31 AM

Make a copy, QObject::children returns const & for some reason, so gets modified during iteration.

meven added a subscriber: meven.Thu, Jan 30, 8:39 AM
meven added inline comments.
src/filewidgets/kurlnavigatorplacesselector.cpp
75

Shouldn't it be done before the call to m_placesMenu->clear();

fvogt added inline comments.Thu, Jan 30, 8:43 AM
src/filewidgets/kurlnavigatorplacesselector.cpp
75

How would that make a difference?

anthonyfieroni added inline comments.
src/filewidgets/kurlnavigatorplacesselector.cpp
76

Why cast?

fvogt added inline comments.Thu, Jan 30, 8:47 AM
src/filewidgets/kurlnavigatorplacesselector.cpp
76

To only delete submenus, not anything else.

meven added inline comments.Thu, Jan 30, 12:38 PM
src/filewidgets/kurlnavigatorplacesselector.cpp
75

Misconception on my part, .children() is not affected by .clear()

fvogt edited the test plan for this revision. (Show Details)Thu, Jan 30, 12:41 PM
fvogt marked 3 inline comments as done.
fvogt added a comment.Tue, Feb 4, 10:33 AM

I'll land tomorrow if no objections.

davidedmundson accepted this revision.Tue, Feb 4, 10:39 AM
This revision is now accepted and ready to land.Tue, Feb 4, 10:39 AM
meven accepted this revision.EditedTue, Feb 4, 10:39 AM

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.

fvogt edited the test plan for this revision. (Show Details)Tue, Feb 4, 10:41 AM

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 :-)

This revision was automatically updated to reflect the committed changes.
meven added a comment.Tue, Feb 4, 10:47 AM

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.

ngraham added a subscriber: ngraham.Tue, Feb 4, 6:22 PM

Could this be the fix for https://bugs.kde.org/show_bug.cgi?id=398908, or part of it?

fvogt added a comment.Tue, Feb 4, 6:38 PM

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.

asensi added a subscriber: asensi.Mon, Feb 10, 11:11 PM