Segfault after changing contact alias

[tpikonen]

Brief Description

OS: Linux Debian buster
qTox version: git master
Commit hash: 55ef1e0
toxcore: 0.2.8
Qt: 5.11.3
Hardware: Core2 duo

Reproducible: Almost Always

Steps to reproduce

1. Right-click on contact in contact list
2. Click on "Set alias"
3. Change the alias of the contact and press enter

Observed Behavior

Contact alias is changed and saved in config. qTox segfaults.

Expected Behavior

Contact alias is changed and saved in config.

Additional Info

Backtrace:

(gdb) bt
#0  0xb40751ce in QMetaObject::activate(QObject*, int, int, void**) () at /usr/lib/i386-linux-gnu/sse2/libQt5Core.so.5
#1  0xb4075dad in QMetaObject::activate(QObject*, QMetaObject const*, int, void**) () at /usr/lib/i386-linux-gnu/sse2/libQt5Core.so.5
#2  0x00664f75 in FriendWidget::friendWidgetRenamed(FriendWidget*) (this=0x1ca3a60, _t1=0x1ca3a60)
   at /home/tpikonen/tmp/qtox/git-qTox/qtox_static_autogen/WFD7YQQOTJ/moc_friendwidget.cpp:300
#3  0x007b2a9b in FriendWidget::<lambda(QString)>::operator()(QString) const (__closure=0x15228a8)
   at /home/tpikonen/tmp/qtox/git-qTox/src/widget/friendwidget.cpp:74
#4  0x007b6629 in QtPrivate::FunctorCall<QtPrivate::IndexesList<0>, QtPrivate::List<const QString&>, void, FriendWidget::FriendWidget(std::shared_ptr<FriendChatroom>, bool)::<lambda(QString)> >::call(FriendWidget::<lambda(QString)> &, void **) (f=..., arg=0xbfffd4f4)
   at /usr/include/i386-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:128
#5  0x007b6469 in QtPrivate::Functor<FriendWidget::FriendWidget(std::shared_ptr<FriendChatroom>, bool)::<lambda(QString)>, 1>::call<QtPrivate::List<QString const&>, void>(FriendWidget::<lambda(QString)> &, void *, void **) (f=..., arg=0xbfffd4f4) at /usr/include/i386-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:238
#6  0x007b60d9 in QtPrivate::QFunctorSlotObject<FriendWidget::FriendWidget(std::shared_ptr<FriendChatroom>, bool)::<lambda(QString)>, 1, QtPrivate::List<const QString&>, void>::impl(int, QtPrivate::QSlotObjectBase *, QObject *, void **, bool *) (which=1, this_=0x15228a0, r=0x189f330, a=0xbfffd4f4, ret=0x0)
   at /usr/include/i386-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:421
#7  0xb4075914 in QMetaObject::activate(QObject*, int, int, void**) () at /usr/lib/i386-linux-gnu/sse2/libQt5Core.so.5
#8  0xb4075dad in QMetaObject::activate(QObject*, QMetaObject const*, int, void**) () at /usr/lib/i386-linux-gnu/sse2/libQt5Core.so.5
#9  0x0065f1af in Contact::displayedNameChanged(QString const&) (this=0x189f330, _t1=...)
   at /home/tpikonen/tmp/qtox/git-qTox/qtox_static_autogen/VNQA4LF4BW/moc_contact.cpp:127
#10 0x006cdbbc in Friend::setAlias(QString const&) (this=0x189f330, alias=...) at /home/tpikonen/tmp/qtox/git-qTox/src/model/friend.cpp:85
#11 0x007b8f71 in QtPrivate::FunctorCall<QtPrivate::IndexesList<0>, QtPrivate::List<QString const&>, void, void (Friend::*)(QString const&)>::call(void (Friend::*)(QString const&), Friend*, void**) (f=
   (void (Friend::*)(Friend * const, const QString &)) 0x6cdace <Friend::setAlias(QString const&)>, o=0x189f330, arg=0xbfffd734) 
   at /usr/include/i386-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:134
#12 0x007b8b5a in QtPrivate::FunctionPointer<void (Friend::*)(QString const&)>::call<QtPrivate::List<QString const&>, void>(void (Friend::*)(QString const&), Friend*, void**) (f=(void (Friend::*)(Friend * const, const QString &)) 0x6cdace <Friend::setAlias(QString const&)>, o=0x189f330, arg=0xbfffd734)
   at /usr/include/i386-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:167
#13 0x007b80a6 in QtPrivate::QSlotObject<void (Friend::*)(QString const&), QtPrivate::List<QString const&>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (which=1, this_=0x197d200, r=0x189f330, a=0xbfffd734, ret=0x0) at /usr/include/i386-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:396
#14 0xb4075914 in QMetaObject::activate(QObject*, int, int, void**) () at /usr/lib/i386-linux-gnu/sse2/libQt5Core.so.5
#15 0xb4075dad in QMetaObject::activate(QObject*, QMetaObject const*, int, void**) () at /usr/lib/i386-linux-gnu/sse2/libQt5Core.so.5
#16 0x0066102f in CroppingLabel::editFinished(QString const&) (this=0x19731f0, _t1=...)

This patch seems to fix the problem, but someone with a better knowledge of Qt and C++ lambdas needs to explain why.

diff --git a/src/widget/friendwidget.cpp b/src/widget/friendwidget.cpp
index 4adfea3c..8a9c083b 100644
--- a/src/widget/friendwidget.cpp
+++ b/src/widget/friendwidget.cpp
@@ -71,7 +71,7 @@ FriendWidget::FriendWidget(std::shared_ptr<FriendChatroom> chatroom, bool compac
     connect(nameLabel, &CroppingLabel::editFinished, frnd, &Friend::setAlias);
     // update on changes of the displayed name
     connect(frnd, &Friend::displayedNameChanged, nameLabel, &CroppingLabel::setText);
-    connect(frnd, &Friend::displayedNameChanged, [this](const QString /* &newName */){emit friendWidgetRenamed(this);});
+    connect(frnd, &Friend::displayedNameChanged, this, [this](const QString /* &newName */){emit friendWidgetRenamed(this);});
     connect(chatroom.get(), &FriendChatroom::activeChanged, this, &FriendWidget::setActive);
     statusMessageLabel->setTextFormat(Qt::PlainText);
 }

[anthonybilinski]

https://wiki.qt.io/New_Signal_Slot_Syntax#New:_connecting_to_simple_function discusses connecting lambdas to slots, and how without the third "this" argument in your diff above, that the functor will not be disconnected, but with the argument, that the connection can be broken when the "this" object is destroyed:

There is no automatic disconnection when the 'receiver' is destroyed because it's a functor with no QObject. However, since 5.2 there is an overload which adds a "context object". When that object is destroyed, the connection is broken (the context is also used for the thread affinity: the lambda will be called in the thread of the event loop of the object used as context).

Similarly in this stackoverflow post, there's a more thorough explanation: https://stackoverflow.com/a/19721153

I'm not really sure how this FriendWidget is invalid if you just changed the name through the FriendWidget, but this seems like a generally unsafe thing to do and I can't reproduce locally to look into the call path more in detail, so I think just going with the additional "context object" of this is a good fix.

[anthonybilinski]

For a bit more explanation, "this" (the FriendWidget) is used in the lambda, which the lambda holds a pointer to. The FriendWidget is being deleted, but the lambda still exists and is holding an invalid pointer to the now deleted FriendWidget, and Qt only knows that this Friend causes that lambda to be called, so has no idea that the lambda is now invalid. Adding this as the third argument lets Qt know that our lambda depends on this still being alive, so Qt can do its normal behaviour of breaking connections when either the sender or receiver is destroyed, preventing the lambda from ever being called after this is destroyed.