Bug 343976

Folks, could you please check whether the frameworkintegration you're using already contains this patch:

http://quickgit.kde.org/?p=frameworkintegration.git&a=commitdiff&h=032154385627f79740d5a0e90028aa8ae8ed65c3

Created attachment 91033 [details]
attachment-1672-0.html

Yes, patch already included in my environment. 

On February 11, 2015 3:30:43 PM CET, =?UTF-8?Q?Jan=20Kundr=C3=A1t=20? <jkt@kde.org> wrote:
>
>https://bugs.kde.org/show_bug.cgi?id=343976
>
>--- Comment #1 from Jan Kundrát <jkt@kde.org> ---
>Folks, could you please check whether the frameworkintegration you're
>using
>already contains this patch:
>
>http://quickgit.kde.org/?p=frameworkintegration.git&a=commitdiff&h=032154385627f79740d5a0e90028aa8ae8ed65c3
>
>-- 
>You are receiving this mail because:
>You are on the CC list for the bug.

for those being able to reproduce: could you please provide a gdb backtrace?

Created attachment 91392 [details]
GDB Backtrace of Trojita crash

Full backtrace attached.

Thanks. Looks rather obvious: SystemTrayMenu::m_menu gets deleted and then deleted again due to being a QScopedPointer.

Not tested patch:
diff --git a/src/platformtheme/kdeplatformsystemtrayicon.cpp b/src/platformtheme/kdeplatformsystemtrayicon.cpp
index ce3d3de..b84012e 100644
--- a/src/platformtheme/kdeplatformsystemtrayicon.cpp
+++ b/src/platformtheme/kdeplatformsystemtrayicon.cpp
@@ -34,6 +34,7 @@ SystemTrayMenu::SystemTrayMenu()
 {
     connect(m_menu.data(), &QMenu::aboutToShow, this, &QPlatformMenu::aboutToShow);
     connect(m_menu.data(), &QMenu::aboutToHide, this, &QPlatformMenu::aboutToHide);
+    connect(m_menu.data(), &QObject::deleted, this, [this] { m_menu.take(); });
 }
 
 SystemTrayMenu::~SystemTrayMenu()

Created attachment 91407 [details]
Backtrace of Trojita crash with patch applied to 'frameworkintegration'

Still crashes with this patch.
Backtrace attached.

thanks for testing. Well it helped it's a different crash now.

Why the scoped pointer instead of an object child itfp?

I'd say the problem is that the scopedpointer is deleted with the leaf destructor, but the object is still alive and handling events.

-> favorable solution?
QMenu *m_menu;
...
, m_menu(new QMenu(this));

I just tried to remember why it's a scoped pointer and not an object child: QMenu takes a QWidget* parent and we don't have that.

What might work is

m_menu = new QMenu;
m_menu->setParent(this);

Created attachment 91429 [details]
Possible patch removing QScopedPointer

I only compile tested. Please test whether this solves the problem.

could just crash as well (at least i've seen widgets force-parented an object segfault)
deleteLater in the deconstructor may be the safer choice.

(In reply to Thomas Lübking from comment #12)
> could just crash as well (at least i've seen widgets force-parented an
> object segfault)
> deleteLater in the deconstructor may be the safer choice.

Given the original crash that would probably also crash as the QMenu might be destroyed before going into the dtor. So if the force-parented variant doesn't work we probably need to guard with a QPointer.

+ connect(m_menu.data(), &QObject::deleted, this, [this] { m_menu = nullptr });

~() {
if (m_menu)
   m_menu->deleteLater();
}

I however wonder what should possibly have deleted this private and unparented member?
I mean, you don't walk along and delete stuff that belongs to somebody totally random else.

-> Something (the systray icon?) occupies this thing as it passes by?
In that case we'd have to watch QEvent::ParentAboutToChange or QEvent::ParentChanged and stop to care about it?

The reparenting might happen at:

void SystemTrayMenuItem::setMenu(QPlatformMenu *menu)
{
    if (SystemTrayMenu *ourMenu = qobject_cast<SystemTrayMenu *>(menu)) {
        m_action->setMenu(ourMenu->menu());
    }
}

Nope, QAction does not take ownership of the menu and setMenu isn't virtual.
(Also, QAction is no QWidget)

I can confirm that https://bugsfiles.kde.org/attachment.cgi?id=91429 fixes the problem on Plasma 5.2.1. Thanks!

Would you mind if I commit and push this patch?

Done on MG's behalf:

commit 27dac81ae82f79b872c044848fed8bdcf03e17f7
Author: Martin Gräßlin <mgraesslin@kde.org>
Date:   Sun Mar 8 18:36:45 2015 +0100

    Use explicit memory management, no QScopedPointer
    
    BUG: 343976

(In reply to Jan Kundrát from comment #17)
> I can confirm that https://bugsfiles.kde.org/attachment.cgi?id=91429 fixes
> the problem on Plasma 5.2.1. Thanks!

I have now an auto-test for the issue and cannot confirm that the patch fixes it, it just creates another problem:
Program received signal SIGABRT, Aborted.
0x00007ffff258e107 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
56      ../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  0x00007ffff258e107 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x00007ffff258f4e8 in __GI_abort () at abort.c:89
#2  0x00007ffff31c72f1 in qt_message_fatal (context=..., message=...) at global/qlogging.cpp:1422
#3  0x00007ffff31c3b12 in QMessageLogger::fatal (this=0x7fffffffccf0, msg=0x7ffff352c0c8 "ASSERT: \"%s\" in file %s, line %d") at global/qlogging.cpp:643
#4  0x00007ffff31bda92 in qt_assert (assertion=0x7ffff35f85f8 "!d->isWidget", file=0x7ffff35f8007 "kernel/qobject.cpp", line=1936) at global/qglobal.cpp:2868
#5  0x00007ffff349031d in QObject::setParent (this=0x684f90, parent=0x67f8f0) at kernel/qobject.cpp:1936
#6  0x00007fffe3613e83 in SystemTrayMenu::SystemTrayMenu (this=0x67f8f0) at /home/martin/src/kf5/frameworks/frameworkintegration/src/platformtheme/kdeplatformsystemtrayicon.cpp:36
#7  0x00007fffe3614bee in KDEPlatformSystemTrayIcon::createMenu (this=0x669d90) at /home/martin/src/kf5/frameworks/frameworkintegration/src/platformtheme/kdeplatformsystemtrayicon.cpp:309
#8  0x00007ffff48ebe6c in QSystemTrayIconPrivate::addPlatformMenu (this=0x669ce0, menu=0x682110) at util/qsystemtrayicon.cpp:757
#9  0x00007ffff48ebb91 in QSystemTrayIconPrivate::updateMenu_sys_qpa (this=0x669ce0) at util/qsystemtrayicon.cpp:708
#10 0x00007ffff490cdbe in QSystemTrayIconPrivate::updateMenu_sys (this=0x669ce0) at util/qsystemtrayicon_x11.cpp:305
#11 0x00007ffff48e9b49 in QSystemTrayIcon::setContextMenu (this=0x67f480, menu=0x682110) at util/qsystemtrayicon.cpp:167
#12 0x0000000000409fb9 in KSniUnitTest::testHideDontCrash (this=0x7fffffffddb0) at /home/martin/src/kf5/frameworks/frameworkintegration/autotests/ksni_unittest.cpp:41
#13 0x000000000040a171 in KSniUnitTest::qt_static_metacall (_o=0x7fffffffddb0, _c=QMetaObject::InvokeMetaMethod, _id=0, _a=0x7fffffffd070) at /opt/build/kf5/frameworks/frameworkintegration/autotests/ksni_unittest.moc:67
#14 0x00007ffff345e541 in QMetaMethod::invoke (this=0x7fffffffd530, object=0x7fffffffddb0, connectionType=Qt::DirectConnection, returnValue=..., val0=..., val1=..., val2=..., val3=..., val4=..., val5=..., val6=..., val7=..., 
    val8=..., val9=...) at kernel/qmetaobject.cpp:2183
#15 0x00007ffff345da13 in QMetaObject::invokeMethod (obj=0x7fffffffddb0, member=0x67f460 "testHideDontCrash", type=Qt::DirectConnection, ret=..., val0=..., val1=..., val2=..., val3=..., val4=..., val5=..., val6=..., val7=..., 
    val8=..., val9=...) at kernel/qmetaobject.cpp:1478
#16 0x00007ffff7fcd97b in QMetaObject::invokeMethod (obj=0x7fffffffddb0, member=0x67f460 "testHideDontCrash", type=Qt::DirectConnection, val0=..., val1=..., val2=..., val3=..., val4=..., val5=..., val6=..., val7=..., val8=..., 
    val9=...) at ../../include/QtCore/../../src/corelib/kernel/qobjectdefs.h:391
#17 0x00007ffff7fc9db3 in QTest::qInvokeTestMethodDataEntry (slot=0x67f460 "testHideDontCrash") at qtestcase.cpp:1884
#18 0x00007ffff7fca61c in QTest::qInvokeTestMethod (slotName=0x67f348 "testHideDontCrash()", data=0x0) at qtestcase.cpp:2012
#19 0x00007ffff7fcb10d in QTest::qInvokeTestMethods (testObject=0x7fffffffddb0) at qtestcase.cpp:2239
#20 0x00007ffff7fcb769 in QTest::qExec (testObject=0x7fffffffddb0, argc=1, argv=0x7fffffffdec8) at qtestcase.cpp:2481
#21 0x000000000040a114 in main (argc=1, argv=0x7fffffffdec8) at /home/martin/src/kf5/frameworks/frameworkintegration/autotests/ksni_unittest.cpp:52

Review Request: https://git.reviewboard.kde.org/r/122978/

The solution somehow implies that one would have always to use guarded pointers, because "somebody" might randomly delete *your* stuff away.
I'd still say that whatever takes ownership of this unparented private member (and deletes it) is the culprit.

Menu : public QMenu {
~Menu() {
   if (parent()) {
      qDebug() << parent();
      abort();
   }
}
};

just added that code: the if doesn't get executed.

found the culprit:
KStatusNotifierItem::~KStatusNotifierItem()
{
    delete d->statusNotifierWatcher;
    delete d->notificationsClient;
    delete d->systemTrayIcon;
    if (!qApp->closingDown()) {
        delete d->menu;
    }
    delete d;
}

@Marco: do  you know why KSNI takes ownership over the context menu?

I encountered exactly the same issue in my project. While I'm waiting for the upstream fix, I can only "give up" my ownership to `QSystemTrayIcon*` and use `deleteLater()` function in the destructor as a temporary workaround.

Personally I think `KStatusNotifierItem` should check if it's the parent before delete tray icon explicitly.

Git commit a153c6ea5dd472d7f9e9e7fd208916f436efea4b by Martin Gräßlin.
Committed on 16/03/2015 at 10:56.
Pushed by graesslin into branch 'master'.

Use a QPointer for QMenu of SystemTrayMenu

We need better memory management as the QMenu the SystemTrayMenu creates
might be deleted externally causing a double delete if the so-far
QScopedPoiter cleans up.

Switch to QPointer and call deleteLater if the pointer is still valid
during destroying the SystemTrayMenu.
REVIEW: 122978
CHANGELOG: Fix possible crash when destroying a QSystemTrayIcon (triggered by Trojita)

M  +25   -3    src/platformtheme/kdeplatformsystemtrayicon.cpp
M  +1    -1    src/platformtheme/kdeplatformsystemtrayicon.h

http://commits.kde.org/frameworkintegration/a153c6ea5dd472d7f9e9e7fd208916f436efea4b

*** Bug 344799 has been marked as a duplicate of this bug. ***