<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/113877/">http://git.reviewboard.kde.org/r/113877/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 25th, 2013, 1:48 p.m. UTC, <b>Kevin Ottens</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Any chance for a review? We really need to tie up loose ends now.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think that this is a very nice piece of work, and the new data-driven test also looks good to me. The only thing I'm concerned about is the run time of the test - I like unit testing very much, but tests with a very long run time might make people avoid "make test", and thus bear the risk that we see more regressions in the future. Maybe this review request is not the best place to discuss this issue though, in particular if it should go in soon in order to not delay the splitting (and also to fix the crash, of course). Maybe one could discuss requirements for unit tests at some later point. (One possible solution would be to make this a manual test. Yes, it would mean that people will run this test less often, but if it prevents that people stop running "make test" altogether, maybe it's still worth considering).
I don't really feel qualified to approve changes in kwindowsystem though ;-)</pre>
<br />
<p>- Frank</p>
<br />
<p>On November 18th, 2013, 6:04 a.m. UTC, Martin Gräßlin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated Nov. 18, 2013, 6:04 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdelibs
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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@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
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>tier1/kwindowsystem/src/kstartupinfo.cpp <span style="color: grey">(402cc97)</span></li>
<li>tier1/kwindowsystem/autotests/kstartupinfo_unittest.cpp <span style="color: grey">(17890ff)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/113877/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>