Review Request 113877: Use QMutableMapIterator for removing KStartupInfoIds

Mon Nov 25 19:00:36 UTC 2013

> On Nov. 25, 2013, 5:29 p.m., David Faure wrote:
> > AFAIK the rule for Qt unittests is that they should run in under 5 seconds. They don't all manage that, nor do we, but it gives an indication.
> > 
> > 30 seconds or more is definitely too much.
> > "For the silent case the timeout - is for what reason ever - multiplied by 20." << is this something that comes from our own code, from Qt, or from X itself? Depending on the answer, we could imagine some hidden symbol or env var in order to tune that behavior from unittests.
> >
> 
> Martin Gräßlin wrote:
>     It's our own code:
>     unsigned int tout = timeout;
>     if ((*it).silent() == Data::Yes) // TODO
>         tout *= 20;
>     
>     Seems to have been introduced with ffac2c941cfd1187ca024c92f0a179b64229474d back in 2003

Looking at the callers for setSilent(), I see that this is triggered by StartupNotify=false in desktop files (no reason to give these a longer timeout).
But it's also triggered by http://lxr.kde.org/source/kde/kde-runtime/kdesu/kdesu/kdesu.cpp#415
That explains it: the timeout is made much longer so that the user has time to type a password in the kdesu dialog box.

So, if we agree that the feature is useful, the solution is simply to adjust the timeout for the unittest.
My suggestion would be qgetenv("KSTARTUPINFO_TIMEOUT").toInt() having precedence over the timeout calculated by the code (in all cases, silent or not).

- David

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113877/#review44446
-----------------------------------------------------------

On Nov. 18, 2013, 6:04 a.m., Martin Gräßlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113877/
> -----------------------------------------------------------
> 
> (Updated Nov. 18, 2013, 6:04 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> If there were multiple KStartupInfoIds which were removed at the same time, it was possible that the map got changed while being iterated, thus the data corrupted and we hit a crash later on. This changes to a QMutableMapIterator for checking the cleanup and allowing to safely remove ids during the iteration.
> 
> 
> Diffs
> -----
> 
>   tier1/kwindowsystem/src/kstartupinfo.cpp 402cc97 
>   tier1/kwindowsystem/autotests/kstartupinfo_unittest.cpp 17890ff 
> 
> Diff: http://git.reviewboard.kde.org/r/113877/diff/
> 
> 
> Testing
> -------
> 
> When running the unit tests on the old code base it isn't guaranteed that it crashes as it's a race and also depends on X. Just run a few times and it will certainly hit the abort condition.
> 
> For reference the backtrace of the crash before fix:
> #0  0x00007ffff50131e5 in __GI_raise (sig=sig at entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
> #1  0x00007ffff5016398 in __GI_abort () at abort.c:90
> #2  0x00007ffff5c3d402 in qt_message_fatal (context=..., message=...) at global/qlogging.cpp:979
> #3  0x00007ffff5c3afce in QMessageLogger::fatal (this=0x7fffffffbbc0, msg=0x7ffff5f8d9f8 "ASSERT: \"%s\" in file %s, line %d") at global/qlogging.cpp:384
> #4  0x00007ffff5c369e6 in qt_assert (assertion=0x7ffff5f8b7f0 "size == 0 || offset < 0 || size_t(offset) >= sizeof(QArrayData)", file=0x7ffff5f8b7b0 "../../include/QtCore/../../src/corelib/tools/qarraydata.h", line=62)
>     at global/qglobal.cpp:2088
> #5  0x00007ffff5c29bf8 in QArrayData::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:61
> #6  0x00007ffff5c3015e in QTypedArrayData<char>::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:207
> #7  0x00007ffff5c2fde7 in QByteArray::constData (this=0x6f4760) at ../../include/QtCore/../../src/corelib/tools/qbytearray.h:433
> #8  0x00007ffff5c53333 in qstrcmp (str1=..., str2=...) at tools/qbytearray.cpp:347
> #9  0x00007ffff7ba4625 in operator< (a1=..., a2=...) at /opt/qt5/include/QtCore/qbytearray.h:550
> #10 0x00007ffff7ba0eb9 in KStartupInfoId::operator< (this=0x693858, id_P=...) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:1175
> #11 0x00007ffff7ba67b5 in qMapLessThanKey<KStartupInfoId> (key1=..., key2=...) at /opt/qt5/include/QtCore/qmap.h:75
> #12 0x00007ffff7ba6ef3 in QMapNode<KStartupInfoId, KStartupInfo::Data>::lowerBound (this=0x693840, akey=...) at /opt/qt5/include/QtCore/qmap.h:145
> #13 0x00007ffff7ba66b4 in QMapData<KStartupInfoId, KStartupInfo::Data>::findNode (this=0x700870, akey=...) at /opt/qt5/include/QtCore/qmap.h:292
> #14 0x00007ffff7ba5a73 in QMap<KStartupInfoId, KStartupInfo::Data>::remove (this=0x65acc0, akey=...) at /opt/qt5/include/QtCore/qmap.h:897
> #15 0x00007ffff7b9dd42 in KStartupInfo::Private::remove_startup_info_internal (this=0x65acb0, id_P=...) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:399
> #16 0x00007ffff7ba039b in KStartupInfo::Private::startups_cleanup_internal (this=0x65acb0, age_P=true) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:984
> #17 0x00007ffff7ba02bf in KStartupInfo::Private::startups_cleanup (this=0x65acb0) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:965
> #18 0x00007ffff7ba3d1c in KStartupInfo::qt_static_metacall (_o=0x7fffffffdc60, _c=QMetaObject::InvokeMetaMethod, _id=3, _a=0x7fffffffbf70) at /opt/build/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/moc_kstartupinfo.cpp:106
> #19 0x00007ffff5ef8c9f in QMetaObject::activate (sender=0x684d50, signalOffset=3, local_signal_index=0, argv=0x0) at kernel/qobject.cpp:3530
> #20 0x00007ffff5ef84f0 in QMetaObject::activate (sender=0x684d50, m=0x7ffff63ab980 <QTimer::staticMetaObject>, local_signal_index=0, argv=0x0) at kernel/qobject.cpp:3405
> #21 0x00007ffff5f88fc3 in QTimer::timeout (this=0x684d50) at .moc/moc_qtimer.cpp:189
> #22 0x00007ffff5f03bc5 in QTimer::timerEvent (this=0x684d50, e=0x7fffffffca30) at kernel/qtimer.cpp:255
> #23 0x00007ffff5ef2314 in QObject::event (this=0x684d50, e=0x7fffffffca30) at kernel/qobject.cpp:1101
> #24 0x00007ffff710e24e in QApplicationPrivate::notify_helper (this=0x60f010, receiver=0x684d50, e=0x7fffffffca30) at kernel/qapplication.cpp:3467
> #25 0x00007ffff710b805 in QApplication::notify (this=0x7fffffffdc90, receiver=0x684d50, e=0x7fffffffca30) at kernel/qapplication.cpp:2888
> #26 0x00007ffff5eb6908 in QCoreApplication::notifyInternal (this=0x7fffffffdc90, receiver=0x684d50, event=0x7fffffffca30) at kernel/qcoreapplication.cpp:878
> #27 0x00007ffff5eba4f1 in QCoreApplication::sendEvent (receiver=0x684d50, event=0x7fffffffca30) at ../../include/QtCore/../../src/corelib/kernel/qcoreapplication.h:232
> #28 0x00007ffff5f2c450 in QTimerInfoList::activateTimers (this=0x654ec0) at kernel/qtimerinfo_unix.cpp:643
> #29 0x00007ffff5f2d764 in timerSourceDispatch (source=0x654e60) at kernel/qeventdispatcher_glib.cpp:185
> #30 0x00007ffff25a8ea6 in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> #31 0x00007ffff25a91f8 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> #32 0x00007ffff25a929c in g_main_context_iteration () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> #33 0x00007ffff5f2e0f9 in QEventDispatcherGlib::processEvents (this=0x64ed40, flags=...) at kernel/qeventdispatcher_glib.cpp:426
> #34 0x00007fffefcca744 in QPAEventDispatcherGlib::processEvents (this=0x64ed40, flags=...) at eventdispatchers/qeventdispatcher_glib.cpp:123
> #35 0x00007ffff5eb6e46 in QCoreApplication::processEvents (flags=..., maxtime=11000) at kernel/qcoreapplication.cpp:1076
> #36 0x0000000000403ecc in QTest::qWait (ms=11000) at /opt/qt5/include/QtTest/qtestsystem.h:66
> #37 0x000000000040690c in KStartupInfo_UnitTest::dontCrashCleanup (this=0x7fffffffdc50) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/autotests/kstartupinfo_unittest.cpp:128
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20131125/3e720b77/attachment-0001.html>