Cache and delete old ConversationsDbusInterfaces to avoid memory leak

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

Reviewers: #kde_connect, apol, albertvaka

Reviewed By: #kde_connect, apol, albertvaka

Subscribers: apol, kdeconnect

Tags: #kde_connect

Differential Revision: https://phabricator.kde.org/D16553
This commit is contained in:
Simon Redman 2019-01-14 13:44:53 -07:00
parent e77c1c87ac
commit 74ba660cad
2 changed files with 22 additions and 0 deletions

View file

@ -37,15 +37,30 @@ K_PLUGIN_FACTORY_WITH_JSON( KdeConnectPluginFactory, "kdeconnect_sms.json", regi
Q_LOGGING_CATEGORY(KDECONNECT_PLUGIN_SMS, "kdeconnect.plugin.sms") Q_LOGGING_CATEGORY(KDECONNECT_PLUGIN_SMS, "kdeconnect.plugin.sms")
QMap<QString, ConversationsDbusInterface*> SmsPlugin::conversationInterfaces;
SmsPlugin::SmsPlugin(QObject* parent, const QVariantList& args) SmsPlugin::SmsPlugin(QObject* parent, const QVariantList& args)
: KdeConnectPlugin(parent, args) : KdeConnectPlugin(parent, args)
, m_telepathyInterface(QStringLiteral("org.freedesktop.Telepathy.ConnectionManager.kdeconnect"), QStringLiteral("/kdeconnect")) , m_telepathyInterface(QStringLiteral("org.freedesktop.Telepathy.ConnectionManager.kdeconnect"), QStringLiteral("/kdeconnect"))
, m_conversationInterface(new ConversationsDbusInterface(this)) , m_conversationInterface(new ConversationsDbusInterface(this))
{ {
// Once the new ConversationsDbusInterface has been fully set up, the old one can safely be
// deleted
const auto& oldInterfaceItr = SmsPlugin::conversationInterfaces.find(device()->id());
if (oldInterfaceItr != SmsPlugin::conversationInterfaces.end()) {
ConversationsDbusInterface* oldInterface = oldInterfaceItr.value();
oldInterface->deleteLater();
SmsPlugin::conversationInterfaces.erase(oldInterfaceItr);
}
} }
SmsPlugin::~SmsPlugin() SmsPlugin::~SmsPlugin()
{ {
// Because of how Qt's Dbus is designed, we are unable to immediately delete m_conversationInterface,
// See the comment in ~NotificationsPlugin() for more information
// We save the old pointer for now; the internal data can be used as a cache.
// The next time we construct an SmsPlugin, we access the old pointer and delete it
SmsPlugin::conversationInterfaces[device()->id()] = m_conversationInterface;
} }
bool SmsPlugin::receivePacket(const NetworkPacket& np) bool SmsPlugin::receivePacket(const NetworkPacket& np)

View file

@ -131,6 +131,13 @@ private:
QDBusInterface m_telepathyInterface; QDBusInterface m_telepathyInterface;
ConversationsDbusInterface* m_conversationInterface; ConversationsDbusInterface* m_conversationInterface;
/*
* Keep a map of all interfaces ever constructed
* See comments in ~SmsPlugin() for the reason
*/
static QMap<QString, ConversationsDbusInterface*> conversationInterfaces;
}; };
#endif #endif