avoid memory leak in slave jobs
ClosedPublic

Authored by jtamate on Jul 20 2018, 6:24 PM.

Details

Summary

Making ~SlaveInterface virtual, the connection created in Slave constructor is now deleted. (thanks @dfaure).
Changed to new connect syntax.

BUG: 396651

Test Plan

The leaks of the bug report are gone.

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.
jtamate created this revision.Jul 20 2018, 6:24 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 20 2018, 6:24 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
jtamate requested review of this revision.Jul 20 2018, 6:24 PM
apol added a subscriber: apol.Jul 21 2018, 1:03 AM
apol added inline comments.
src/core/slave.cpp
148

Good catch, maybe it should be a QScopedPointer?

dfaure requested changes to this revision.Jul 21 2018, 8:35 AM

You can remove the "hopefully" from the commit log. setConnection is not used anywhere in KIO, and couldn't be used from outside KIO, since Connection isn't an exported class. I just marked setConnection/connection as deprecated and to be removed in KF6.

"~SlaveInterfacePrivate should be virtual" sounds like the actual fix for this bug indeed.

This revision now requires changes to proceed.Jul 21 2018, 8:35 AM
jtamate updated this revision to Diff 38163.Jul 21 2018, 9:51 AM
jtamate retitled this revision from avoid memory leak over sftp to avoid memory leak in slave jobs.
jtamate edited the summary of this revision. (Show Details)

I still don't know how to avoid this leak, but I guess this will be fixed with new syntax for QMetaObject::invokeMethod and similar.

==26857== 22,638 (216 direct, 22,422 indirect) bytes in 3 blocks are definitely lost in loss record 2,175 of 2,182
==26857==    at 0x4C2EB4F: operator new(unsigned long) (vg_replace_malloc.c:334)
==26857==    by 0xD7E74CE: QObjectPrivate::connectImpl(QObject const*, int, QObject const*, void**, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int const*, QMet
aObject const*) (qobject.cpp:4834)
==26857==    by 0xD7E78AC: QObject::connectImpl(QObject const*, void**, QObject const*, void**, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int const*, QMetaObj
ect const*) (qobject.cpp:4789)
==26857==    by 0x7B9B21B: QMetaObject::Connection QObject::connect<void (KIO::Connection::*)(), void (KIO::Slave::*)()>(QtPrivate::FunctionPointer<void (KIO::Connec
tion::*)()>::Object const*, void (KIO::Connection::*)(), QtPrivate::FunctionPointer<void (KIO::Slave::*)()>::Object const*, void (KIO::Slave::*)(), Qt::ConnectionTyp
e) (qobject.h:254)
==26857==    by 0x7B98376: KIO::Slave::accept() (slave.cpp:174)
==26857==    by 0x7B9C212: QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (KIO::Slave::*)()>::call(void (KIO::Slave::*)(), KIO::Slave
*, void**) (qobjectdefs_impl.h:134)
==26857==    by 0x7B9C0F0: void QtPrivate::FunctionPointer<void (KIO::Slave::*)()>::call<QtPrivate::List<>, void>(void (KIO::Slave::*)(), KIO::Slave*, void**) (qobje
ctdefs_impl.h:167)
==26857==    by 0x7B9BD84: QtPrivate::QSlotObject<void (KIO::Slave::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (
qobjectdefs_impl.h:396)
==26857==    by 0xD7E3BAF: QMetaObject::activate(QObject*, int, int, void**) (qobjectdefs_impl.h:376)
==26857==    by 0x7C3E176: KIO::ConnectionServer::newConnection() (moc_connectionserver.cpp:127)
==26857==    by 0x7C3DF93: KIO::ConnectionServer::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) (moc_connectionserver.cpp:72)
==26857==    by 0xD7E3A52: QMetaObject::activate(QObject*, int, int, void**) (qobject.cpp:3771)
dfaure accepted this revision.Jul 21 2018, 10:00 AM

The old connect syntax did not leak in itself. If you see a connection leaked, it means one of the two objects (sender or receiver) isn't deleted.

This revision is now accepted and ready to land.Jul 21 2018, 10:00 AM
jtamate updated this revision to Diff 38166.Jul 21 2018, 11:20 AM
jtamate edited the summary of this revision. (Show Details)

Using --num-callers=50, the leak shows as:

==3437== 54,952 (32 direct, 54,920 indirect) bytes in 1 blocks are definitely lost in loss record 2,284 of 2,284
==3437==    at 0x4C2EB4F: operator new(unsigned long) (vg_replace_malloc.c:334)
==3437==    by 0xD7E54A9: QObjectPrivate::addConnection(int, QObjectPrivate::Connection*) (qobject.cpp:390)
==3437==    by 0xD7E64AC: QMetaObjectPrivate::connect(QObject const*, int, QMetaObject const*, QObject const*, int, QMetaObject const*, int, int*) (qobject.cpp:3319)
==3437==    by 0xD7E8D95: QObject::connect(QObject const*, char const*, QObject const*, char const*, Qt::ConnectionType) (qobject.cpp:2777)
==3437==    by 0x7B34899: QObject::connect(QObject const*, char const*, char const*, Qt::ConnectionType) const (qobject.h:465)
==3437==    by 0x7B94057: KIO::SlaveInterface::SlaveInterface(KIO::SlaveInterfacePrivate&, QObject*) (slaveinterface.cpp:48)
==3437==    by 0x7B985BC: KIO::Slave::Slave(QString const&, QObject*) (slave.cpp:216)
==3437==    by 0x7B99401: KIO::Slave::createSlave(QString const&, QUrl const&, int&, QString&) (slave.cpp:474)
==3437==    by 0x7BC321A: KIO::ProtoQueue::createSlave(QString const&, KIO::SimpleJob*, QUrl const&) (scheduler.cpp:542)
==3437==    by 0x7BC37CA: KIO::ProtoQueue::startAJob() (scheduler.cpp:629)
==3437==    by 0x7BC6FF5: KIO::ProtoQueue::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) (moc_scheduler_p.cpp:252)

The slaves registered are killed and deleted, therefore register the created slave.
This leak is gone.

jtamate updated this revision to Diff 38172.Jul 21 2018, 11:42 AM
jtamate edited the summary of this revision. (Show Details)

As I do not know how to discard new incorrect updates... The previous incorrect one creates a crash.
Return to the good one and land.

This revision was automatically updated to reflect the committed changes.