Differential D27650

ItemContainer: disconnect signals in destructor
ClosedPublic
Actions

Authored by alnikiforov on Feb 25 2020, 2:21 PM.

Details

Reviewers
ngraham
davidedmundson
mart
Commits
R120:8f0da90f18ff: ItemContainer: disconnect signals in destructor
Summary

Otherwise, setLayout function might be called for
already destructed instance of ItemContainer,
leading to double reference counter decrement of m_layout QPointer,
eventually invalidating such pointers prematurely.

BUG: 417603

Test Plan
  1. Unlock widgets via command: qdbus org.kde.plasmashell /PlasmaShell evaluateScript "lockCorona(false)"
  2. On desktop push right mouse button and select menu item 'Add Widgets...'
  3. Add various widgets to desktop using drag'n'drop on desktop. I've added at least following widgets on same desktop screen: Audio Volume, Battery and Brightness, Binary Clock, Clipboard, Color Picker, Grouping Plasmoid, Quick Chat
  4. Remove just added widgets in random order
  5. If necessary, repeat steps 3 and 4 a few times If widgets aren't appearing on desktop despite adding them via drag'n'drop, it's bugged and ready to crash. But it's not a requirement for crash.
  6. lock widgets via command: qdbus org.kde.plasmashell /PlasmaShell evaluateScript "lockCorona(true)"
  7. repeat steps 1-6 multiple times
  8. plasmashell shouldn't crash

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
alnikiforov created this revision.Feb 25 2020, 2:21 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 25 2020, 2:21 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
alnikiforov requested review of this revision.Feb 25 2020, 2:21 PM
davidedmundson requested changes to this revision.EditedFeb 25 2020, 2:43 PM

This function takes a plain pointer and wraps it into weak shared pointer.

That's QWeakPointer.

QPointer is something else that doesn't have an equivalent in stdlib.
It takes a QObject, then QObject has a special hook to unset watching QPointers to nullptr in QObject::~QObject()

I don't understand how this could fix anything.

If this is null before, then after this patch m_layout would just be dangling which is worse.

This revision now requires changes to proceed.Feb 25 2020, 2:43 PM
alnikiforov added a comment.Feb 26 2020, 8:19 AM
In D27650#617550, @davidedmundson wrote:

This function takes a plain pointer and wraps it into weak shared pointer.

That's QWeakPointer.

QPointer is something else that doesn't have an equivalent in stdlib.
It takes a QObject, then QObject has a special hook to unset watching QPointers to nullptr in QObject::~QObject()

QPointer actually uses QWeakPointer to work:

https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/kernel/qpointer.h#n59
https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/qsharedpointer_impl.h#n309
https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/qsharedpointer_impl.h#n450
https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/qsharedpointer_impl.h#n569

And this used QWeakPointer leads to premature destruction of object m_layout points to.

I don't understand how this could fix anything.

If this is null before, then after this patch m_layout would just be dangling which is worse.

I've debugged this issue a bit more. Here's what I found.

Initial setup from gdb:

(gdb) break ItemContainer::setLayout
Breakpoint 1 at 0x7fffba5e2ea0: file /usr/src/debug/plasma-workspace-5.18.1/components/containmentlayoutmanager/itemcontainer.cpp, line 177.
(gdb) run --replace
Thread 1 "plasmashell" hit Breakpoint 1, ItemContainer::setLayout (this=this@entry=0xc01f80, layout=0x0) at /usr/src/debug/plasma-workspace-5.18.1/components/containmentlayoutmanager/itemcontainer.cpp:177
177     {
(gdb) c
Continuing.

Thread 1 "plasmashell" hit Breakpoint 1, ItemContainer::setLayout (this=0xc01f80, layout=0xbe0a60) at /usr/src/debug/plasma-workspace-5.18.1/components/containmentlayoutmanager/itemcontainer.cpp:177
177     {
(gdb) n
178         if (m_layout == layout) {
(gdb) n
198         if (parentItem() != layout) {
(gdb) n
210         });
(gdb) print m_layout
$1 = {wp = {d = 0xc01bc0, value = 0xbe0a60}}
(gdb) print m_layout.wp.d
$2 = (QWeakPointer<QObject>::Data *) 0xc01bc0
(gdb) print *m_layout.wp.d
$3 = {weakref = {_q_value = {<std::__atomic_base<int>> = {static _S_alignment = 4, _M_i = 4}, <No data fields>}}, strongref = {_q_value = {<std::__atomic_base<int>> = {static _S_alignment = 4, _M_i = -1}, <No data fields>}}, 
  destroyer = 0x7ffff57ada40 <main_arena+96>}
(gdb) watch ((QWeakPointer<QObject>::Data *) 0xc01bc0)->weakref._q_value._M_i
Hardware watchpoint 2: ((QWeakPointer<QObject>::Data *) 0xc01bc0)->weakref._q_value._M_i
(gdb) watch ((QWeakPointer<QObject>::Data *) 0xc01bc0)->strongref._q_value._M_i
Hardware watchpoint 3: ((QWeakPointer<QObject>::Data *) 0xc01bc0)->strongref._q_value._M_i
(gdb) break QObject::~QObject if (this == 0xbe0a60)
Breakpoint 4 at 0x41c3e0 (75 locations)
(gdb) c
Continuing.

I've added some widgets like described in test plan. Now I'm removing one of widgets:

Thread 1 "plasmashell" hit Hardware watchpoint 2: ((QWeakPointer<QObject>::Data *) 0xc01bc0)->weakref._q_value._M_i

Old value = 5
New value = 4
0x00007fffba5e4445 in QWeakPointer<QObject>::~QWeakPointer (this=0x10c2fc0, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/atomic_base.h:326
326           operator--() noexcept
(gdb) bt
#0  0x00007fffba5e4445 in QWeakPointer<QObject>::~QWeakPointer (this=0x10c2fc0, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/atomic_base.h:326
#1  QPointer<AppletsLayout>::~QPointer (this=0x10c2fc0, __in_chrg=<optimized out>) at /usr/include/qt5/QtCore/qpointer.h:53
#2  ItemContainer::~ItemContainer (this=0x10c2f60, __in_chrg=<optimized out>) at /usr/src/debug/plasma-workspace-5.18.1/components/containmentlayoutmanager/itemcontainer.cpp:64
#3  0x00007fffba5d30a5 in QQmlPrivate::QQmlElement<AppletContainer>::~QQmlElement (this=0x10c2f60, __in_chrg=<optimized out>) at /usr/include/qt5/QtQml/qqmlprivate.h:106
#4  QQmlPrivate::QQmlElement<AppletContainer>::~QQmlElement (this=0x10c2f60, __in_chrg=<optimized out>) at /usr/include/qt5/QtQml/qqmlprivate.h:108
#5  0x00007ffff5ce2380 in QObject::event (this=this@entry=0x10c2f60, e=e@entry=0x2b76cc0) at kernel/qobject.cpp:1252
#6  0x00007ffff77c2ae3 in QQuickItem::event (this=0x10c2f60, ev=0x2b76cc0) at items/qquickitem.cpp:8105
#7  0x00007ffff6711c42 in QApplicationPrivate::notify_helper (this=this@entry=0x4c14c0, receiver=receiver@entry=0x10c2f60, e=e@entry=0x2b76cc0) at kernel/qapplication.cpp:3700
#8  0x00007ffff671b1c0 in QApplication::notify (this=0x7fffffffd730, receiver=0x10c2f60, e=0x2b76cc0) at kernel/qapplication.cpp:3446
#9  0x00007ffff5cb7212 in QCoreApplication::notifyInternal2 (receiver=0x10c2f60, event=0x2b76cc0) at ../../src/corelib/kernel/qobject.h:142
#10 0x00007ffff5cb9e08 in QCoreApplicationPrivate::sendPostedEvents (receiver=0x0, event_type=0, data=0x4b6330) at kernel/qcoreapplication.cpp:1825
#11 0x00007ffff5d0d963 in postEventSourceDispatch (s=0x51a7c0) at kernel/qeventdispatcher_glib.cpp:276
#12 0x00007ffff3b86c6d in g_main_dispatch (context=0x7fffe8005010) at ../glib/gmain.c:3179
#13 g_main_context_dispatch (context=context@entry=0x7fffe8005010) at ../glib/gmain.c:3844
#14 0x00007ffff3b86ef0 in g_main_context_iterate (context=context@entry=0x7fffe8005010, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../glib/gmain.c:3917
#15 0x00007ffff3b86f7f in g_main_context_iteration (context=0x7fffe8005010, may_block=may_block@entry=1) at ../glib/gmain.c:3978
#16 0x00007ffff5d0cfa1 in QEventDispatcherGlib::processEvents (this=0x51b110, flags=...) at kernel/qeventdispatcher_glib.cpp:422
#17 0x00007ffff5cb5e9b in QEventLoop::exec (this=this@entry=0x7fffffffd5f0, flags=..., flags@entry=...) at ../../src/corelib/global/qflags.h:140
#18 0x00007ffff5cbd942 in QCoreApplication::exec () at ../../src/corelib/global/qflags.h:120
#19 0x00007ffff608ecbc in QGuiApplication::exec () at kernel/qguiapplication.cpp:1784
#20 0x00007ffff6711bb5 in QApplication::exec () at kernel/qapplication.cpp:2856
#21 0x000000000041f4f1 in main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/plasma-workspace-5.18.1/shell/main.cpp:228
(gdb) c
Continuing.

Thread 1 "plasmashell" hit Breakpoint 1, ItemContainer::setLayout (this=0x10c2f60, layout=0x0) at /usr/src/debug/plasma-workspace-5.18.1/components/containmentlayoutmanager/itemcontainer.cpp:177
177     {
(gdb) bt
#0  ItemContainer::setLayout (this=0x10c2f60, layout=0x0) at /usr/src/debug/plasma-workspace-5.18.1/components/containmentlayoutmanager/itemcontainer.cpp:177
#1  0x00007ffff5ce1c98 in QtPrivate::QSlotObjectBase::call (a=0x7fffffffcee0, r=0x10c2f60, this=0x2dcf4b0) at ../../src/corelib/kernel/qobjectdefs_impl.h:394
#2  QMetaObject::activate (sender=0x10c2f60, signalOffset=<optimized out>, local_signal_index=<optimized out>, argv=<optimized out>) at kernel/qobject.cpp:3784
#3  0x00007ffff77aedb2 in QQuickItem::parentChanged (this=this@entry=0x10c2f60, _t1=<optimized out>) at .moc/moc_qquickitem.cpp:1100
#4  0x00007ffff77c3fdd in QQuickItem::setParentItem (this=this@entry=0x10c2f60, parentItem=parentItem@entry=0x0) at items/qquickitem.cpp:2808
#5  0x00007ffff77c4c38 in QQuickItem::~QQuickItem (this=0x10c2f60, __in_chrg=<optimized out>) at items/qquickitem.cpp:2390
#6  0x00007fffba5d30a5 in QQmlPrivate::QQmlElement<AppletContainer>::~QQmlElement (this=0x10c2f60, __in_chrg=<optimized out>) at /usr/include/qt5/QtQml/qqmlprivate.h:106
#7  QQmlPrivate::QQmlElement<AppletContainer>::~QQmlElement (this=0x10c2f60, __in_chrg=<optimized out>) at /usr/include/qt5/QtQml/qqmlprivate.h:108
#8  0x00007ffff5ce2380 in QObject::event (this=this@entry=0x10c2f60, e=e@entry=0x2b76cc0) at kernel/qobject.cpp:1252
#9  0x00007ffff77c2ae3 in QQuickItem::event (this=0x10c2f60, ev=0x2b76cc0) at items/qquickitem.cpp:8105
#10 0x00007ffff6711c42 in QApplicationPrivate::notify_helper (this=this@entry=0x4c14c0, receiver=receiver@entry=0x10c2f60, e=e@entry=0x2b76cc0) at kernel/qapplication.cpp:3700
#11 0x00007ffff671b1c0 in QApplication::notify (this=0x7fffffffd730, receiver=0x10c2f60, e=0x2b76cc0) at kernel/qapplication.cpp:3446
#12 0x00007ffff5cb7212 in QCoreApplication::notifyInternal2 (receiver=0x10c2f60, event=0x2b76cc0) at ../../src/corelib/kernel/qobject.h:142
#13 0x00007ffff5cb9e08 in QCoreApplicationPrivate::sendPostedEvents (receiver=0x0, event_type=0, data=0x4b6330) at kernel/qcoreapplication.cpp:1825
#14 0x00007ffff5d0d963 in postEventSourceDispatch (s=0x51a7c0) at kernel/qeventdispatcher_glib.cpp:276
#15 0x00007ffff3b86c6d in g_main_dispatch (context=0x7fffe8005010) at ../glib/gmain.c:3179
#16 g_main_context_dispatch (context=context@entry=0x7fffe8005010) at ../glib/gmain.c:3844
#17 0x00007ffff3b86ef0 in g_main_context_iterate (context=context@entry=0x7fffe8005010, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../glib/gmain.c:3917
#18 0x00007ffff3b86f7f in g_main_context_iteration (context=0x7fffe8005010, may_block=may_block@entry=1) at ../glib/gmain.c:3978
#19 0x00007ffff5d0cfa1 in QEventDispatcherGlib::processEvents (this=0x51b110, flags=...) at kernel/qeventdispatcher_glib.cpp:422
#20 0x00007ffff5cb5e9b in QEventLoop::exec (this=this@entry=0x7fffffffd5f0, flags=..., flags@entry=...) at ../../src/corelib/global/qflags.h:140
#21 0x00007ffff5cbd942 in QCoreApplication::exec () at ../../src/corelib/global/qflags.h:120
#22 0x00007ffff608ecbc in QGuiApplication::exec () at kernel/qguiapplication.cpp:1784
#23 0x00007ffff6711bb5 in QApplication::exec () at kernel/qapplication.cpp:2856
#24 0x000000000041f4f1 in main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/plasma-workspace-5.18.1/shell/main.cpp:228
(gdb) c
Continuing.

Thread 1 "plasmashell" hit Hardware watchpoint 2: ((QWeakPointer<QObject>::Data *) 0xc01bc0)->weakref._q_value._M_i

Old value = 4
New value = 3
0x00007fffba5e2fd7 in QWeakPointer<QObject>::~QWeakPointer (this=<synthetic pointer>, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/atomic_base.h:326
326           operator--() noexcept
(gdb) bt
#0  0x00007fffba5e2fd7 in QWeakPointer<QObject>::~QWeakPointer (this=<synthetic pointer>, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/atomic_base.h:326
#1  QWeakPointer<QObject>::operator= (other=..., other=..., this=0x10c2fc0) at /usr/include/qt5/QtCore/qsharedpointer_impl.h:599
#2  QWeakPointer<QObject>::assign<QObject> (ptr=0x0, this=0x10c2fc0) at /usr/include/qt5/QtCore/qsharedpointer_impl.h:684
#3  QPointer<AppletsLayout>::operator= (p=0x0, this=0x10c2fc0) at /usr/include/qt5/QtCore/qpointer.h:83
#4  ItemContainer::setLayout (this=0x10c2f60, layout=0x0) at /usr/src/debug/plasma-workspace-5.18.1/components/containmentlayoutmanager/itemcontainer.cpp:191
#5  0x00007ffff5ce1c98 in QtPrivate::QSlotObjectBase::call (a=0x7fffffffcee0, r=0x10c2f60, this=0x2dcf4b0) at ../../src/corelib/kernel/qobjectdefs_impl.h:394
#6  QMetaObject::activate (sender=0x10c2f60, signalOffset=<optimized out>, local_signal_index=<optimized out>, argv=<optimized out>) at kernel/qobject.cpp:3784
#7  0x00007ffff77aedb2 in QQuickItem::parentChanged (this=this@entry=0x10c2f60, _t1=<optimized out>) at .moc/moc_qquickitem.cpp:1100
#8  0x00007ffff77c3fdd in QQuickItem::setParentItem (this=this@entry=0x10c2f60, parentItem=parentItem@entry=0x0) at items/qquickitem.cpp:2808
#9  0x00007ffff77c4c38 in QQuickItem::~QQuickItem (this=0x10c2f60, __in_chrg=<optimized out>) at items/qquickitem.cpp:2390
#10 0x00007fffba5d30a5 in QQmlPrivate::QQmlElement<AppletContainer>::~QQmlElement (this=0x10c2f60, __in_chrg=<optimized out>) at /usr/include/qt5/QtQml/qqmlprivate.h:106
#11 QQmlPrivate::QQmlElement<AppletContainer>::~QQmlElement (this=0x10c2f60, __in_chrg=<optimized out>) at /usr/include/qt5/QtQml/qqmlprivate.h:108
#12 0x00007ffff5ce2380 in QObject::event (this=this@entry=0x10c2f60, e=e@entry=0x2b76cc0) at kernel/qobject.cpp:1252
#13 0x00007ffff77c2ae3 in QQuickItem::event (this=0x10c2f60, ev=0x2b76cc0) at items/qquickitem.cpp:8105
#14 0x00007ffff6711c42 in QApplicationPrivate::notify_helper (this=this@entry=0x4c14c0, receiver=receiver@entry=0x10c2f60, e=e@entry=0x2b76cc0) at kernel/qapplication.cpp:3700
#15 0x00007ffff671b1c0 in QApplication::notify (this=0x7fffffffd730, receiver=0x10c2f60, e=0x2b76cc0) at kernel/qapplication.cpp:3446
#16 0x00007ffff5cb7212 in QCoreApplication::notifyInternal2 (receiver=0x10c2f60, event=0x2b76cc0) at ../../src/corelib/kernel/qobject.h:142
#17 0x00007ffff5cb9e08 in QCoreApplicationPrivate::sendPostedEvents (receiver=0x0, event_type=0, data=0x4b6330) at kernel/qcoreapplication.cpp:1825
#18 0x00007ffff5d0d963 in postEventSourceDispatch (s=0x51a7c0) at kernel/qeventdispatcher_glib.cpp:276
#19 0x00007ffff3b86c6d in g_main_dispatch (context=0x7fffe8005010) at ../glib/gmain.c:3179
#20 g_main_context_dispatch (context=context@entry=0x7fffe8005010) at ../glib/gmain.c:3844
#21 0x00007ffff3b86ef0 in g_main_context_iterate (context=context@entry=0x7fffe8005010, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../glib/gmain.c:3917
#22 0x00007ffff3b86f7f in g_main_context_iteration (context=0x7fffe8005010, may_block=may_block@entry=1) at ../glib/gmain.c:3978
#23 0x00007ffff5d0cfa1 in QEventDispatcherGlib::processEvents (this=0x51b110, flags=...) at kernel/qeventdispatcher_glib.cpp:422
#24 0x00007ffff5cb5e9b in QEventLoop::exec (this=this@entry=0x7fffffffd5f0, flags=..., flags@entry=...) at ../../src/corelib/global/qflags.h:140
#25 0x00007ffff5cbd942 in QCoreApplication::exec () at ../../src/corelib/global/qflags.h:120
#26 0x00007ffff608ecbc in QGuiApplication::exec () at kernel/qguiapplication.cpp:1784
#27 0x00007ffff6711bb5 in QApplication::exec () at kernel/qapplication.cpp:2856
#28 0x000000000041f4f1 in main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/plasma-workspace-5.18.1/shell/main.cpp:228
(gdb) c

As you may see, due to m_layout being a QPointer, which contains QWeakPointer, reference counter is decreased twice. If there is no QPointer, there is no double decrement and no issue. And m_layout doesn't become dangling pointer since object m_layout points to is created in qml and is not destroyed until desktop view is destroyed.

Thus, while this fix is not perfect, it definitely fixes current issue.
But looking into it closely after additional investigation, I think it might be fixed using another approach: signals have to be disconnected so that signal to already deconstructed object wouldn't be called from it's parent class. I'll upload updated version.

alnikiforov updated this revision to Diff 76439.Feb 26 2020, 8:21 AM
alnikiforov retitled this revision from Don't use guarded pointers for AppletsLayout to ItemContainer: disconnect signals in destructor.
alnikiforov edited the summary of this revision. (Show Details)

Uploaded updated change

alnikiforov added a comment.Feb 26 2020, 8:27 AM

Fix consists of following line:

disconnect(this, &QQuickItem::parentChanged, this, nullptr);

Two other disconnects are just in case of similar issues arising and for code consistency.

anthonyfieroni added a subscriber: anthonyfieroni.Feb 26 2020, 8:39 AM
anthonyfieroni added inline comments.
components/containmentlayoutmanager/itemcontainer.cpp
66

You can translate this calls to
m_sizeHintAdjustTimer->disconnect(this)

davidedmundson added a comment.Feb 26 2020, 11:10 AM

And this used QWeakPointer leads to premature destruction of object m_layout points to.

Do you have commit access?

components/containmentlayoutmanager/itemcontainer.cpp
68

Right, this one line makes a lot of sense.

I can imagine QQuickItem::~QuickItem changes parent, and calling ItemContainer::setLayout after we've run our ItemContainers destructor means all bets are completely off. I can believe an already destroyed QPointer behaves weirdly.

Lets not change the other two. There's no chance of an event loop processing a timer event after ItemContainer:::~ItemContainer before we're auto disconnected.

alnikiforov added a comment.Feb 26 2020, 11:26 AM
In D27650#618207, @davidedmundson wrote:

And this used QWeakPointer leads to premature destruction of object m_layout points to.

Do you have commit access?

Yep, original description was not entirely correct.

No, AFAIK I don't have commit access.

components/containmentlayoutmanager/itemcontainer.cpp
68

Should I remove these 2 disconnects or leave them as they are?

davidedmundson added a comment.Feb 26 2020, 12:58 PM

No, AFAIK I don't have commit access.

Ok, well I hope this is the first of many patches. I like working with people who can use gdb!

Good stuff.

components/containmentlayoutmanager/itemcontainer.cpp
68

Lets get rid of them. Then it helps self-document that this the one left is doing something explicitly for a good reason.

alnikiforov updated this revision to Diff 76462.Feb 26 2020, 1:03 PM
alnikiforov edited the summary of this revision. (Show Details)

Got rid of two disconnects and updated summary formatting.

davidedmundson added a comment.Feb 26 2020, 5:49 PM

No, AFAIK I don't have commit access.

Can I have your full name + email address please.

davidedmundson accepted this revision.Feb 26 2020, 5:54 PM
This revision is now accepted and ready to land.Feb 26 2020, 5:54 PM
alnikiforov added a comment.Feb 27 2020, 11:53 AM
In D27650#618492, @davidedmundson wrote:

Can I have your full name + email address please.

Aleksei Nikiforov <darktemplar@basealt.ru>

cfeck added a subscriber: cfeck.Mar 6 2020, 11:36 AM

If this is still needed, please commit and clarify the status of the referenced bug.

davidedmundson added a comment.Mar 6 2020, 11:43 AM

Urgh, sorry.

Thanks cfeck

Closed by commit R120:8f0da90f18ff: ItemContainer: disconnect signals in destructor (authored by alnikiforov, committed by davidedmundson). · Explain WhyMar 6 2020, 11:44 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 77089

View Options

components/containmentlayoutmanager/itemcontainer.cpp

Loading...