Ubuntu Online Accounts API

memory leaks from OnlineAccounts::Manager

Bug #1617180 reported by James Henstridge
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Ubuntu Online Accounts API
Confirmed
Medium
Alberto Mardegan
webapps-sprint
New
Undecided
Unassigned
storage-framework (Ubuntu)
Fix Released
Undecided
Unassigned

Bug Description

When running valgrind over the the storage-framework test suite, we noticed a leak that seemed to originate from online-accounts-api.

Running valgrind over online-accounts-api's functional_tests test program shows leaks like:

    2,579 (112 direct, 2,467 indirect) bytes in 1 blocks are definitely lost in loss record 365 of 380
       at 0x4C2E0EF: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
       by 0x40B7811: QDBusConnectionPrivate::sendWithReplyAsync(QDBusMessage const&, QObject*, char const*, char const*, int) (in /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5.5.1)
       by 0x40A5A67: QDBusConnection::asyncCall(QDBusMessage const&, int) const (in /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5.5.1)
       by 0x40C19C8: QDBusAbstractInterface::asyncCallWithArgumentList(QString const&, QList<QVariant> const&) (in /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5.5.1)
       by 0x40C39FC: QDBusAbstractInterface::asyncCall(QString const&, QVariant const&, QVariant const&, QVariant const&, QVariant const&, QVariant const&, QVariant const&, QVariant const&, QVariant const&) (in /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5.5.1)
       by 0x4E4D666: OnlineAccounts::DBusInterface::requestAccess(QString const&, QMap<QString, QVariant> const&) (dbus_interface.cpp:70)
       by 0x4E4F76F: OnlineAccounts::ManagerPrivate::requestAccess(QString const&, QMap<QString, QVariant> const&) (manager.cpp:78)
       by 0x4E4F8A8: OnlineAccounts::Manager::requestAccess(QString const&, OnlineAccounts::AuthenticationData const&) (manager.cpp:252)

And:

    2,701 (112 direct, 2,589 indirect) bytes in 1 blocks are definitely lost in loss record 366 of 380
       at 0x4C2E0EF: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
       by 0x40B7811: QDBusConnectionPrivate::sendWithReplyAsync(QDBusMessage const&, QObject*, char const*, char const*, int) (in /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5.5.1)
       by 0x40A5A67: QDBusConnection::asyncCall(QDBusMessage const&, int) const (in /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5.5.1)
       by 0x40C19C8: QDBusAbstractInterface::asyncCallWithArgumentList(QString const&, QList<QVariant> const&) (in /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5.5.1)
       by 0x40C39FC: QDBusAbstractInterface::asyncCall(QString const&, QVariant const&, QVariant const&, QVariant const&, QVariant const&, QVariant const&, QVariant const&, QVariant const&, QVariant const&) (in /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5.5.1)
       by 0x4E4D047: OnlineAccounts::DBusInterface::getAccounts(QMap<QString, QVariant> const&) (dbus_interface.cpp:54)
       by 0x4E4FD53: OnlineAccounts::ManagerPrivate::retrieveAccounts() (manager.cpp:114)
       by 0x4E52F30: ManagerPrivate (manager.cpp:51)
       by 0x4E52F30: OnlineAccounts::Manager::Manager(QString const&, QDBusConnection const&, QObject*) (manager.cpp:173)
       by 0x4E531AE: OnlineAccounts::Manager::Manager(QString const&, QObject*) (manager.cpp:166)

Based on the size of the leaks, it is a QDBusPendingCallPrivate instance that is getting leaked each time. It's not obvious why it's being leaked though: the code doesn't look that out of the ordinary.

Related branches

lp://staging/~michihenning/online-accounts-api/leak-fix
Alberto Mardegan (community): Approve
Diff: 42 lines (+4/-2)
3 files modified
src/lib/OnlineAccounts/pending_call.h (+2/-1)
src/lib/OnlineAccountsDaemon/i18n.cpp (+1/-1)
src/lib/Ubuntu/OnlineAccounts.2/account.cpp (+1/-0)
lp://staging/~mardy/online-accounts-api/test-leaks-1617180
Michi Henning (community): Approve
Online Accounts: Pending requested
Diff: 53 lines (+18/-0)
2 files modified
tests/lib/OnlineAccounts/functional_tests/functional_tests.cpp (+9/-0)
tests/lib/qml_module/tst_qml_module.cpp (+9/-0)
Revision history for this message
James Henstridge (jamesh) wrote :
James Henstridge (jamesh)
summary: - memory leaks from OnlineAccounts::Manager constructor
+ memory leaks from OnlineAccounts::Manager
Revision history for this message
James Henstridge (jamesh) wrote :

So looking at the Qt code, the QDBusPendingCallPrivate class is ref counted, where most references will be owned by a QDBusPendingCall instance (or a subclass). It has one additional reference that is managed manually in the QDBusConnectionPrivate code.

I suspect it is this last reference that is getting lost somewhere, but I've got no idea why I haven't noticed something similar in other pieces of code invoking asynchronous methods with QtDBus.

Revision history for this message
James Henstridge (jamesh) wrote :

Here's a log captured on vivid/amd64 (Qt 5.6.1). I can see the same QDBusPendingCallPrivate leaks.

Revision history for this message
James Henstridge (jamesh) wrote :

Oops. That should read yakkety/amd64. I'll add a vivid trace shortly.

Revision history for this message
Michi Henning (michihenning) wrote :

I'm seeing loads of complaints from valgrind when running the online-accounts-api tests.

For tst_daemon: http://pastebin.ubuntu.com/23657421/

For tst_qml_module: http://pastebin.ubuntu.com/23657424/

For functional_tests: http://pastebin.ubuntu.com/23657435/

Alberto Mardegan (mardy)
Changed in online-accounts-api:
status: New → Confirmed
importance: Undecided → Medium
assignee: nobody → Alberto Mardegan (mardy)
Revision history for this message
Alberto Mardegan (mardy) wrote :

Thanks for the logs.

I quickly investigated the issue, and the leaks from the functional_tests (that is, from the client library) are happening because the tests don't iterate the main loop, and therefore some cleanup methods which were queued in the main loop are never executed. I'll fix that when convenient (the bug is in the tests, so your app should not be affected).

When running the tst_daemon, I get the same leaks you reported, plus this error, near the beginning of the tests:

==6662== Syscall param pselect6(sig) points to unaddressable byte(s)
==6662== at 0x6011FCC: pselect (pselect.c:69)
==6662== by 0x532CBD9: qt_safe_select(int, fd_set*, fd_set*, fd_set*, timespec const*) (qcore_unix.cpp:86)
==6662== by 0x532CD87: qt_select_msecs(int, fd_set*, fd_set*, int) (qcore_unix.cpp:113)
==6662== by 0x526F254: QProcessPrivate::waitForFinished(int) (qprocess_unix.cpp:899)
==6662== by 0x520E3DA: QProcess::waitForFinished(int) (qprocess.cpp:1876)
==6662== by 0x5771255: QtDBusTest::SuicidalProcess::~SuicidalProcess() (in /usr/lib/x86_64-linux-gnu/libqtdbustest.so.1.0.0)
==6662== by 0x576EF68: QtDBusTest::QProcessDBusService::~QProcessDBusService() (in /usr/lib/x86_64-linux-gnu/libqtdbustest.so.1.0.0)
==6662== by 0x576EFC8: QtDBusTest::QProcessDBusService::~QProcessDBusService() (in /usr/lib/x86_64-linux-gnu/libqtdbustest.so.1.0.0)
==6662== by 0x576D0F8: ??? (in /usr/lib/x86_64-linux-gnu/libqtdbustest.so.1.0.0)
==6662== by 0x576E2B4: QtDBusTest::DBusTestRunner::~DBusTestRunner() (in /usr/lib/x86_64-linux-gnu/libqtdbustest.so.1.0.0)
==6662== by 0x12F523: ~DBusService (functional_tests.cpp:176)
==6662== by 0x12F523: DBusService::~DBusService() (functional_tests.cpp:176)
==6662== by 0x52E638D: QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const (qmetaobject.cpp:2211)
==6662== Address 0x802001670 is 0 bytes inside a block of size 32 in arena "core"
==6662==

Did you get it as well? Anyway, the leaks reported in this test are not that important, given that they do not refer to the daemon library (which is executed within another process) but to the tests themselves.

Revision history for this message
Michi Henning (michihenning) wrote :

I'm seeing the pselect error in the SF tests suite. From memory, it's non-deterministic though.

I'm concerned about the sendWithReplyAsync() leak. That's a big leak, and we are seeing this all over the place in storage framework. That one really needs fixing.

Revision history for this message
Alberto Mardegan (mardy) wrote :

Which one exactly? I don't see a leak with that function name in the stack, in the logs you posted.

Revision history for this message
Michi Henning (michihenning) wrote :

I'll try to reproduce tomorrow. As I said, it's non-deterministic.

Michi Henning (michihenning)
Changed in storage-framework (Ubuntu):
status: New → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.
Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.