D27650: Don't use guarded pointers for AppletsLayout

[Aleksei Nikiforov]

alnikiforov added a comment.


  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::Data *) 0xc01bc0
(gdb) print *m_layout.wp.d
$3 = {weakref = {_q_value = {> = {static 
_S_alignment = 4, _M_i = 4}, }}, strongref = {_q_value = 
{> = {static _S_alignment = 4, _M_i = -1}, }}, 
  destroyer = 0x757ada40 }
(gdb) watch ((QWeakPointer::Data *) 
0xc01bc0)->weakref._q_value._M_i
Hardware watchpoint 2: ((QWeakPointer::Data *) 
0xc01bc0)->weakref._q_value._M_i
(gdb) watch ((QWeakPointer::Data *) 
0xc01bc0)->strongref._q_value._M_i
Hardware watchpoint 3: ((QWeakPointer::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::Data *) 0xc01bc0)->weakref._q_value._M_i

Old value = 5
New value = 4
0x7fffba5e4445 in QWeakPointer::~QWeakPointer (this=0x10c2fc0, 
__in_chrg=) at /usr/include/c++/9/bits/atomic_base.h:326
326   operator--() noexcept
(gdb) bt
#0  0x7fffba5e4445 in QWeakPointer::~QWeakPointer 
(this=0x10c2fc0, __in_chrg=) at 
/usr/include/c++/9/bits/atomic_base.h:326
#1  QPointer::~QPointer (this=0x10c2fc0, 
__in_chrg=) at /usr/include/qt5/QtCore/qpointer.h:53
#2  ItemContainer::~ItemContainer (this=0x10c2f60, __in_chrg=) at 
/usr/src/debug/plasma-workspace-5.18.1/components/containmentlayoutmanager/itemcontainer.cpp:64
#3  0x7fffba5d30a5 in 
QQmlPrivate::QQmlElement::~QQmlElement (this=0x10c2f60, 
__in_chrg=) at /usr/include/qt5/QtQml/qqmlprivate.h:106
#4  QQmlPrivate::QQmlElement::~QQmlElement 
(this=0x10c2f60, __in_chrg=) at 
/usr/include/qt5/QtQml/qqmlprivate.h:108
#5  0x75ce2380 in QObject::event (this=this@entry=0x10c2f60, 
e=e@entry=0x2b76cc0) at kernel/qobject.cpp:1252
#6  0x777c2ae3 in QQuickItem::event (this=0x10c2f60, ev=0x2b76cc0) 
at items/qquickitem.cpp:8105
#7  0x76711c42 in QApplicationPrivate::notify_helper 
(this=this@entry=0x4c14c0, receiver=receiver@entry=0x10c2f60, 
e=e@entry=0x2b76cc0) at kernel/qapplication.cpp:3700
#8  0x7671b1c0 in QApplication::notify (this=0x7fffd730, 
receiver=0x10c2f60, e=0x2b76cc0) at kernel/qapplication.cpp:3446
#9  0x75cb7212 in QCoreApplication::notifyInternal2 
(receiver=0x10c2f60, event=0x2b76cc0) at ../../src/corelib/kernel/qobject.h:142
#10 0x75cb9e08 in QCoreApplicationPrivate::sendPostedEvents 
(receiver=0x0, event_type=0, data=0x4b6330) at kernel/qcoreapplication.cpp:1825
#11 0x75d0d963 in postEventSourceDispatch (s=0x51a7c0) at 
kernel/qeventdispatcher_glib.cpp:276
#12 0x73b86c6d in g_main_dispatch (context=0x7fffe8005010) at 
../glib/gmain.c:3179
#13 

D27650: Don't use guarded pointers for AppletsLayout

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added a comment.
This revision now requires changes to proceed.


  > 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, then m_layout is dangling which is worse.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27650

To: alnikiforov, ngraham, davidedmundson, mart
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27650: Don't use guarded pointers for AppletsLayout

[Aleksei Nikiforov]

alnikiforov created this revision.
alnikiforov added reviewers: ngraham, davidedmundson, mart.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
alnikiforov requested review of this revision.

REVISION SUMMARY
  Using these pointer types incorrectly leads to premature destruction
  of AppletsLayout object and crash
  
  AppletsLayout m_layout is set only in function ItemContainer::setLayout.
  This function takes a plain pointer and wraps it into weak shared pointer.
  Eventually weak shared pointer's internal counter reaches zero and object 
instance m_layout points to is destroyed,
  although it was created via qml in plasma-desktop. This object destruction 
leads to a lot of issues, including eventual crash.
  
  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

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27650

AFFECTED FILES
  components/containmentlayoutmanager/itemcontainer.h

To: alnikiforov, ngraham, davidedmundson, mart
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart