Bug 343976 - Segfault in SysTray triggered by Trojita
Summary: Segfault in SysTray triggered by Trojita
Status: RESOLVED FIXED
Alias: None
Product: frameworks-frameworkintegration
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: unspecified
Platform: Other Linux
: NOR crash
Target Milestone: ---
Assignee: kdelibs bugs
URL:
Keywords:
: 344799 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-02-09 19:12 UTC by Jan Kundrát
Modified: 2015-04-10 08:25 UTC (History)
8 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
attachment-1672-0.html (1.00 KB, text/html)
2015-02-12 00:53 UTC, Elias Probst
Details
GDB Backtrace of Trojita crash (38.19 KB, text/plain)
2015-03-03 17:27 UTC, Elias Probst
Details
Backtrace of Trojita crash with patch applied to 'frameworkintegration' (39.95 KB, text/plain)
2015-03-04 09:27 UTC, Elias Probst
Details
Possible patch removing QScopedPointer (1.33 KB, patch)
2015-03-05 07:36 UTC, Martin Flöser
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Kundrát 2015-02-09 19:12:01 UTC
Three users reported earlier today crashes during the first run of a Qt5 build Trojita (extragear/pim/trojita) which does not use KDE or KF5 libraries. Here's a report from Valgrind (also at https://gist.github.com/eliasp/4f0ea86a45c117b8ed22, courtesy of eliasp on IRC):

==9425== Invalid read of size 8
==9425== at 0x4FC39D2: QWidget::removeAction(QAction*) (qscopedpointer.h:135)
==9425== by 0x1706AC32: SystemTrayMenu::removeMenuItem(QPlatformMenuItem*) (kdeplatformsystemtrayicon.cpp:93)
==9425== by 0x50DEBEE: QMenu::actionEvent(QActionEvent*) (qmenu.cpp:3054)
==9425== by 0x4FCB724: QWidget::event(QEvent*) (qwidget.cpp:9022)
==9425== by 0x50E3F7A: QMenu::event(QEvent*) (qmenu.cpp:2515)
==9425== by 0x4F8BF0B: QApplicationPrivate::notify_helper(QObject*, QEvent*) (qapplication.cpp:3722)
==9425== by 0x4F90F1F: QApplication::notify(QObject*, QEvent*) (qapplication.cpp:3505)
==9425== by 0x84C13E4: QCoreApplication::notifyInternal(QObject*, QEvent*) (qcoreapplication.cpp:932)
==9425== by 0x4FC3A4A: QWidget::removeAction(QAction*) (qcoreapplication.h:228)
==9425== by 0x4F82960: QAction::~QAction() (qaction.cpp:573)
==9425== by 0x4F82AC8: QAction::~QAction() (qaction.cpp:592)
==9425== by 0x84EF5BB: QObjectPrivate::deleteChildren() (qobject.cpp:1950)
==9425== Address 0x219318a8 is 8 bytes inside a block of size 48 free'd
==9425== at 0x4C2A55C: operator delete(void*) (vg_replace_malloc.c:502)
==9425== by 0x17E6A9A7: KStatusNotifierItem::~KStatusNotifierItem() (kstatusnotifieritem.cpp:71)
==9425== by 0x17E6AE68: KStatusNotifierItem::~KStatusNotifierItem() (kstatusnotifieritem.cpp:74)
==9425== by 0x1706A072: KDEPlatformSystemTrayIcon::cleanup() (kdeplatformsystemtrayicon.cpp:264)
==9425== by 0x52C040A: QSystemTrayIcon::~QSystemTrayIcon() (qsystemtrayicon.cpp:144)
==9425== by 0x52C0428: QSystemTrayIcon::~QSystemTrayIcon() (qsystemtrayicon.cpp:145)
==9425== by 0x441CA5: Gui::MainWindow::removeSysTray() (Window.cpp:784)
==9425== by 0x45235F: Gui::MainWindow::slotShowSettings() (Window.cpp:1147)
==9425== by 0x45EAF9: Gui::MainWindow::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) (moc_Window.cpp:393)
==9425== by 0x84F1A35: QObject::event(QEvent*) (qobject.cpp:1245)
==9425== by 0x4FCB423: QWidget::event(QEvent*) (qwidget.cpp:9083)
==9425== by 0x50B970A: QMainWindow::event(QEvent*) (qmainwindow.cpp:1495) 

This is what Trojita is doing with the systray. The code calls removeSysTray() followed by a toggleSysTray() during the initial setup:

void MainWindow::createSysTray()
{
    if (m_trayIcon)
        return;

    qApp->setQuitOnLastWindowClosed(false);

    m_trayIcon = new QSystemTrayIcon(this);
    handleTrayIconChange();

    QAction* quitAction = new QAction(tr("&Quit"), m_trayIcon);
    connect(quitAction, SIGNAL(triggered()), qApp, SLOT(quit()));

    QMenu *trayIconMenu = new QMenu(this);
    trayIconMenu->addAction(quitAction);
    m_trayIcon->setContextMenu(trayIconMenu);

    // QMenu cannot be a child of QSystemTrayIcon, and we don't want the QMenu in MainWindow scope.
    connect(m_trayIcon, SIGNAL(destroyed()), trayIconMenu, SLOT(deleteLater()));

    connect(m_trayIcon, SIGNAL(activated(QSystemTrayIcon::ActivationReason)),
            this, SLOT(slotIconActivated(QSystemTrayIcon::ActivationReason)));
    connect(imapModel(), SIGNAL(messageCountPossiblyChanged(QModelIndex)), this, SLOT(handleTrayIconChange()));
    m_trayIcon->setVisible(true);
    m_trayIcon->show();
}

void MainWindow::removeSysTray()
{
    delete m_trayIcon;
    m_trayIcon = 0;

    qApp->setQuitOnLastWindowClosed(true);
}

void MainWindow::slotToggleSysTray()
{
    bool showSystray = m_settings->value(Common::SettingsNames::guiShowSystray, QVariant(true)).toBool();
    if (showSystray && !m_trayIcon) {
        createSysTray();
    } else if (!showSystray && m_trayIcon) {
        removeSysTray();
    }
}

Reproducible: Always

Steps to Reproduce:
1. Build Qt5 version of Trojita (cmake -DWITH_QT5=ON)
2. rm -f ~/.config/flaska.net/trojita-mehwtf.conf
3. `trojita --profile mehwtf`, type "localhost" in IMAP -> Server and SMTP -> Server, press OK, wittness the crash



I do not think that the QSysTrayIcon dance we're doing is correct because it used to work just fine in Qt4, and it also works well with a git Qt 5.4.1 (a couple weeks old) when run within Plasma4. On the other hand, three users reported crashes under Plasma5, two on Gentoo, one on Arch.
Comment 1 Jan Kundrát 2015-02-11 14:30:43 UTC
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
Comment 2 Elias Probst 2015-02-12 00:53:45 UTC
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.
Comment 3 Martin Flöser 2015-03-03 16:30:15 UTC
for those being able to reproduce: could you please provide a gdb backtrace?
Comment 4 Elias Probst 2015-03-03 17:27:32 UTC
Created attachment 91392 [details]
GDB Backtrace of Trojita crash

Full backtrace attached.
Comment 5 Martin Flöser 2015-03-03 17:55:18 UTC
Thanks. Looks rather obvious: SystemTrayMenu::m_menu gets deleted and then deleted again due to being a QScopedPointer.
Comment 6 Martin Flöser 2015-03-03 17:57:33 UTC
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()
Comment 7 Elias Probst 2015-03-04 09:27:30 UTC
Created attachment 91407 [details]
Backtrace of Trojita crash with patch applied to 'frameworkintegration'

Still crashes with this patch.
Backtrace attached.
Comment 8 Martin Flöser 2015-03-04 09:36:03 UTC
thanks for testing. Well it helped it's a different crash now.
Comment 9 Thomas Lübking 2015-03-04 22:53:28 UTC
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));
Comment 10 Martin Flöser 2015-03-05 06:52:51 UTC
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);
Comment 11 Martin Flöser 2015-03-05 07:36:30 UTC
Created attachment 91429 [details]
Possible patch removing QScopedPointer

I only compile tested. Please test whether this solves the problem.
Comment 12 Thomas Lübking 2015-03-05 07:57:29 UTC
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.
Comment 13 Martin Flöser 2015-03-05 08:02:49 UTC
(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.
Comment 14 Thomas Lübking 2015-03-05 09:57:47 UTC
+ 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?
Comment 15 Martin Flöser 2015-03-05 10:54:38 UTC
The reparenting might happen at:

void SystemTrayMenuItem::setMenu(QPlatformMenu *menu)
{
    if (SystemTrayMenu *ourMenu = qobject_cast<SystemTrayMenu *>(menu)) {
        m_action->setMenu(ourMenu->menu());
    }
}
Comment 16 Thomas Lübking 2015-03-05 11:00:21 UTC
Nope, QAction does not take ownership of the menu and setMenu isn't virtual.
(Also, QAction is no QWidget)
Comment 17 Jan Kundrát 2015-03-08 18:33:29 UTC
I can confirm that https://bugsfiles.kde.org/attachment.cgi?id=91429 fixes the problem on Plasma 5.2.1. Thanks!
Comment 18 Jan Kundrát 2015-03-12 17:18:04 UTC
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
Comment 19 Martin Flöser 2015-03-16 10:52:38 UTC
(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
Comment 20 Martin Flöser 2015-03-16 10:59:18 UTC
Review Request: https://git.reviewboard.kde.org/r/122978/
Comment 21 Thomas Lübking 2015-03-16 13:33:39 UTC
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();
   }
}
};
Comment 22 Martin Flöser 2015-03-16 13:48:05 UTC
just added that code: the if doesn't get executed.
Comment 23 Martin Flöser 2015-03-16 13:52:00 UTC
found the culprit:
KStatusNotifierItem::~KStatusNotifierItem()
{
    delete d->statusNotifierWatcher;
    delete d->notificationsClient;
    delete d->systemTrayIcon;
    if (!qApp->closingDown()) {
        delete d->menu;
    }
    delete d;
}
Comment 24 Martin Flöser 2015-03-16 13:54:58 UTC
@Marco: do  you know why KSNI takes ownership over the context menu?
Comment 25 Symeon Huang 2015-03-16 22:51:36 UTC
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.
Comment 26 Martin Flöser 2015-03-17 07:06:51 UTC
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
Comment 27 Eugene Shalygin 2015-04-10 08:25:08 UTC
*** Bug 344799 has been marked as a duplicate of this bug. ***