Cache and delete old ConversationsDbusInterfaces to avoid memory leak
ClosedPublic

Authored by sredman on Oct 31 2018, 8:00 PM.

Details

Summary

After using the ConversationsDbusInterface for a little while, there can be significant (MBs) memory usage of cached messages. The QDBusAbstractAdaptor does not like to be manually deleted, but it looks like it is safe to do so after constructing a new one

This contradicts the comment in the BatteryPlugin and the BatteryDbusInterface, which says deletelater() is not safe. Has Qt been updated since then?

Test Plan
  • Run daemon
  • Hopefully experience no crashes after many phone reconnects

Diff Detail

Repository
R224 KDE Connect
Branch
ConversationsDbusMemoryLeak
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7100
Build 7118: arc lint + arc unit
sredman created this revision.Oct 31 2018, 8:00 PM
Restricted Application added a project: KDE Connect. · View Herald TranscriptOct 31 2018, 8:00 PM
Restricted Application added a subscriber: kdeconnect. · View Herald Transcript
sredman requested review of this revision.Oct 31 2018, 8:00 PM
sredman updated this revision to Diff 44570.Oct 31 2018, 8:16 PM
  • Remove comment about app crash, since the idea is that it won't
apol requested changes to this revision.Nov 2 2018, 11:25 PM
apol added a subscriber: apol.
apol added inline comments.
plugins/sms/smsplugin.cpp
49

Use take, so we don't have to look it up twice?

50

use !=

plugins/sms/smsplugin.h
138

Why static?

This revision now requires changes to proceed.Nov 2 2018, 11:25 PM
sredman updated this revision to Diff 44880.Nov 5 2018, 3:11 AM
sredman marked an inline comment as done.
  • Use !=
sredman added inline comments.Nov 5 2018, 3:12 AM
plugins/sms/smsplugin.cpp
49

I don't think take works in this case: If the map does not contain the key, it will give a default-constructed value. We could then immediately delete it, but that would probably cause even more problems
Does passing an iterator to erase cause a second lookup? That seems silly

plugins/sms/smsplugin.h
138

Once the device disconnects, the plugin is destroyed. When it is re-created, we need access to the old pointer, thus a static variable

I have been running this patch for about a week and it seems to be working safely (zero crashes) but I'm still concerned about the initial reasoning for not deleting the interface!

apol added a comment.Nov 12 2018, 3:50 PM

This sounds weird, who caches the data? I don't think Qt does any caching, and if it's kde connect, nuking the dbus interface sounds like we're barking up the wrong tree.

In D16553#358421, @apol wrote:

This sounds weird, who caches the data? I don't think Qt does any caching, and if it's kde connect, nuking the dbus interface sounds like we're barking up the wrong tree.

I'm not sure I understand the question. This plugin (so KDE Connect) is caching the pointer in the static QMap

Ideally we shouldn't have to touch the dbus interface ourselves, and the documentation says it gets taken care of automatically, but that is not happening. You can run the plugin and cause it to load messages to see the unbounded-increasing memory usage (disconnect and reconnect the phone to keep using ever-more memory) or put some debugging in ~ConversationsDbusInterface (and observe that the method is never called without this patch)

sredman updated this revision to Diff 47652.Dec 16 2018, 5:05 AM

Rebase onto master to avoid merge conflict

albertvaka accepted this revision.Jan 7 2019, 10:56 AM
sredman updated this revision to Diff 49484.Jan 14 2019, 8:42 PM
  • Clairify pointer-saving comment
apol accepted this revision.Jan 16 2019, 2:02 PM

I don't understand why it happens but I also don't have an alternative.

This revision is now accepted and ready to land.Jan 16 2019, 2:02 PM
sredman closed this revision.Jan 21 2019, 7:01 PM