Re: Review Request 113877: Use QMutableMapIterator for removing KStartupInfoIds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113877/#review44506 --- Ship it! tier1/kwindowsystem/src/kstartupinfo.cpp http://git.reviewboard.kde.org/r/113877/#comment31792 The TODO seems to be very old and a leftover, I would remove it, now that we know what this is actually for. Or replace with give kdesu time to get a password - David Faure On Nov. 26, 2013, 6:31 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. 26, 2013, 6:31 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/autotests/kstartupinfo_unittest.cpp 17890ff tier1/kwindowsystem/src/kstartupinfo.cpp c2ff7c2 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 0x750131e5 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #1 0x75016398 in __GI_abort () at abort.c:90 #2 0x75c3d402 in qt_message_fatal (context=..., message=...) at global/qlogging.cpp:979 #3 0x75c3afce in QMessageLogger::fatal (this=0x7fffbbc0, msg=0x75f8d9f8 ASSERT: \%s\ in file %s, line %d) at global/qlogging.cpp:384 #4 0x75c369e6 in qt_assert (assertion=0x75f8b7f0 size == 0 || offset 0 || size_t(offset) = sizeof(QArrayData), file=0x75f8b7b0 ../../include/QtCore/../../src/corelib/tools/qarraydata.h, line=62) at global/qglobal.cpp:2088 #5 0x75c29bf8 in QArrayData::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:61 #6 0x75c3015e in QTypedArrayDatachar::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:207 #7 0x75c2fde7 in QByteArray::constData (this=0x6f4760) at ../../include/QtCore/../../src/corelib/tools/qbytearray.h:433 #8 0x75c5 in qstrcmp (str1=..., str2=...) at tools/qbytearray.cpp:347 #9 0x77ba4625 in operator (a1=..., a2=...) at /opt/qt5/include/QtCore/qbytearray.h:550 #10 0x77ba0eb9 in KStartupInfoId::operator (this=0x693858, id_P=...) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:1175 #11 0x77ba67b5 in qMapLessThanKeyKStartupInfoId (key1=..., key2=...) at /opt/qt5/include/QtCore/qmap.h:75 #12 0x77ba6ef3 in QMapNodeKStartupInfoId, KStartupInfo::Data::lowerBound (this=0x693840, akey=...) at /opt/qt5/include/QtCore/qmap.h:145 #13 0x77ba66b4 in QMapDataKStartupInfoId, KStartupInfo::Data::findNode (this=0x700870, akey=...) at /opt/qt5/include/QtCore/qmap.h:292 #14 0x77ba5a73 in QMapKStartupInfoId, KStartupInfo::Data::remove (this=0x65acc0, akey=...) at /opt/qt5/include/QtCore/qmap.h:897 #15 0x77b9dd42 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 0x77ba039b 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 0x77ba02bf in KStartupInfo::Private::startups_cleanup (this=0x65acb0) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:965 #18 0x77ba3d1c in KStartupInfo::qt_static_metacall (_o=0x7fffdc60, _c=QMetaObject::InvokeMetaMethod, _id=3, _a=0x7fffbf70) at /opt/build/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/moc_kstartupinfo.cpp:106 #19 0x75ef8c9f in QMetaObject::activate (sender=0x684d50, signalOffset=3, local_signal_index=0, argv=0x0) at kernel/qobject.cpp:3530 #20 0x75ef84f0 in QMetaObject::activate (sender=0x684d50, m=0x763ab980 QTimer::staticMetaObject, local_signal_index=0, argv=0x0) at kernel/qobject.cpp:3405 #21 0x75f88fc3 in QTimer::timeout (this=0x684d50) at .moc/moc_qtimer.cpp:189 #22 0x75f03bc5 in QTimer::timerEvent (this=0x684d50, e=0x7fffca30) at
Re: Review Request 113877: Use QMutableMapIterator for removing KStartupInfoIds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113877/#review44557 --- This review has been submitted with commit 2391542aea6fe7b64cd98f983881553fb4cde3ea by Martin Gräßlin to branch frameworks. - Commit Hook On Nov. 26, 2013, 6:31 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. 26, 2013, 6:31 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/autotests/kstartupinfo_unittest.cpp 17890ff tier1/kwindowsystem/src/kstartupinfo.cpp c2ff7c2 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 0x750131e5 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #1 0x75016398 in __GI_abort () at abort.c:90 #2 0x75c3d402 in qt_message_fatal (context=..., message=...) at global/qlogging.cpp:979 #3 0x75c3afce in QMessageLogger::fatal (this=0x7fffbbc0, msg=0x75f8d9f8 ASSERT: \%s\ in file %s, line %d) at global/qlogging.cpp:384 #4 0x75c369e6 in qt_assert (assertion=0x75f8b7f0 size == 0 || offset 0 || size_t(offset) = sizeof(QArrayData), file=0x75f8b7b0 ../../include/QtCore/../../src/corelib/tools/qarraydata.h, line=62) at global/qglobal.cpp:2088 #5 0x75c29bf8 in QArrayData::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:61 #6 0x75c3015e in QTypedArrayDatachar::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:207 #7 0x75c2fde7 in QByteArray::constData (this=0x6f4760) at ../../include/QtCore/../../src/corelib/tools/qbytearray.h:433 #8 0x75c5 in qstrcmp (str1=..., str2=...) at tools/qbytearray.cpp:347 #9 0x77ba4625 in operator (a1=..., a2=...) at /opt/qt5/include/QtCore/qbytearray.h:550 #10 0x77ba0eb9 in KStartupInfoId::operator (this=0x693858, id_P=...) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:1175 #11 0x77ba67b5 in qMapLessThanKeyKStartupInfoId (key1=..., key2=...) at /opt/qt5/include/QtCore/qmap.h:75 #12 0x77ba6ef3 in QMapNodeKStartupInfoId, KStartupInfo::Data::lowerBound (this=0x693840, akey=...) at /opt/qt5/include/QtCore/qmap.h:145 #13 0x77ba66b4 in QMapDataKStartupInfoId, KStartupInfo::Data::findNode (this=0x700870, akey=...) at /opt/qt5/include/QtCore/qmap.h:292 #14 0x77ba5a73 in QMapKStartupInfoId, KStartupInfo::Data::remove (this=0x65acc0, akey=...) at /opt/qt5/include/QtCore/qmap.h:897 #15 0x77b9dd42 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 0x77ba039b 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 0x77ba02bf in KStartupInfo::Private::startups_cleanup (this=0x65acb0) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:965 #18 0x77ba3d1c in KStartupInfo::qt_static_metacall (_o=0x7fffdc60, _c=QMetaObject::InvokeMetaMethod, _id=3, _a=0x7fffbf70) at /opt/build/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/moc_kstartupinfo.cpp:106 #19 0x75ef8c9f in QMetaObject::activate (sender=0x684d50, signalOffset=3, local_signal_index=0, argv=0x0) at kernel/qobject.cpp:3530 #20 0x75ef84f0 in QMetaObject::activate (sender=0x684d50, m=0x763ab980 QTimer::staticMetaObject, local_signal_index=0, argv=0x0) at kernel/qobject.cpp:3405 #21 0x75f88fc3 in QTimer::timeout (this=0x684d50) at .moc/moc_qtimer.cpp:189 #22 0x75f03bc5 in QTimer::timerEvent (this=0x684d50, e=0x7fffca30) at kernel/qtimer.cpp:255 #23 0x75ef2314 in QObject::event (this=0x684d50, e=0x7fffca30) at kernel/qobject.cpp:1101 #24 0x7710e24e in
Re: Review Request 113877: Use QMutableMapIterator for removing KStartupInfoIds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113877/ --- (Updated Nov. 27, 2013, 6:10 a.m.) Status -- This change has been marked as submitted. 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/autotests/kstartupinfo_unittest.cpp 17890ff tier1/kwindowsystem/src/kstartupinfo.cpp c2ff7c2 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 0x750131e5 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #1 0x75016398 in __GI_abort () at abort.c:90 #2 0x75c3d402 in qt_message_fatal (context=..., message=...) at global/qlogging.cpp:979 #3 0x75c3afce in QMessageLogger::fatal (this=0x7fffbbc0, msg=0x75f8d9f8 ASSERT: \%s\ in file %s, line %d) at global/qlogging.cpp:384 #4 0x75c369e6 in qt_assert (assertion=0x75f8b7f0 size == 0 || offset 0 || size_t(offset) = sizeof(QArrayData), file=0x75f8b7b0 ../../include/QtCore/../../src/corelib/tools/qarraydata.h, line=62) at global/qglobal.cpp:2088 #5 0x75c29bf8 in QArrayData::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:61 #6 0x75c3015e in QTypedArrayDatachar::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:207 #7 0x75c2fde7 in QByteArray::constData (this=0x6f4760) at ../../include/QtCore/../../src/corelib/tools/qbytearray.h:433 #8 0x75c5 in qstrcmp (str1=..., str2=...) at tools/qbytearray.cpp:347 #9 0x77ba4625 in operator (a1=..., a2=...) at /opt/qt5/include/QtCore/qbytearray.h:550 #10 0x77ba0eb9 in KStartupInfoId::operator (this=0x693858, id_P=...) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:1175 #11 0x77ba67b5 in qMapLessThanKeyKStartupInfoId (key1=..., key2=...) at /opt/qt5/include/QtCore/qmap.h:75 #12 0x77ba6ef3 in QMapNodeKStartupInfoId, KStartupInfo::Data::lowerBound (this=0x693840, akey=...) at /opt/qt5/include/QtCore/qmap.h:145 #13 0x77ba66b4 in QMapDataKStartupInfoId, KStartupInfo::Data::findNode (this=0x700870, akey=...) at /opt/qt5/include/QtCore/qmap.h:292 #14 0x77ba5a73 in QMapKStartupInfoId, KStartupInfo::Data::remove (this=0x65acc0, akey=...) at /opt/qt5/include/QtCore/qmap.h:897 #15 0x77b9dd42 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 0x77ba039b 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 0x77ba02bf in KStartupInfo::Private::startups_cleanup (this=0x65acb0) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:965 #18 0x77ba3d1c in KStartupInfo::qt_static_metacall (_o=0x7fffdc60, _c=QMetaObject::InvokeMetaMethod, _id=3, _a=0x7fffbf70) at /opt/build/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/moc_kstartupinfo.cpp:106 #19 0x75ef8c9f in QMetaObject::activate (sender=0x684d50, signalOffset=3, local_signal_index=0, argv=0x0) at kernel/qobject.cpp:3530 #20 0x75ef84f0 in QMetaObject::activate (sender=0x684d50, m=0x763ab980 QTimer::staticMetaObject, local_signal_index=0, argv=0x0) at kernel/qobject.cpp:3405 #21 0x75f88fc3 in QTimer::timeout (this=0x684d50) at .moc/moc_qtimer.cpp:189 #22 0x75f03bc5 in QTimer::timerEvent (this=0x684d50, e=0x7fffca30) at kernel/qtimer.cpp:255 #23 0x75ef2314 in QObject::event (this=0x684d50, e=0x7fffca30) at kernel/qobject.cpp:1101 #24 0x7710e24e in QApplicationPrivate::notify_helper (this=0x60f010, receiver=0x684d50, e=0x7fffca30) at kernel/qapplication.cpp:3467 #25 0x7710b805 in QApplication::notify (this=0x7fffdc90, receiver=0x684d50, e=0x7fffca30) at kernel/qapplication.cpp:2888 #26 0x75eb6908 in QCoreApplication::notifyInternal (this=0x7fffdc90, receiver=0x684d50, event=0x7fffca30) at kernel/qcoreapplication.cpp:878 #27 0x75eba4f1 in QCoreApplication::sendEvent
Re: Review Request 113877: Use QMutableMapIterator for removing KStartupInfoIds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113877/#review44397 --- Any chance for a review? We really need to tie up loose ends now. - Kevin Ottens 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 0x750131e5 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #1 0x75016398 in __GI_abort () at abort.c:90 #2 0x75c3d402 in qt_message_fatal (context=..., message=...) at global/qlogging.cpp:979 #3 0x75c3afce in QMessageLogger::fatal (this=0x7fffbbc0, msg=0x75f8d9f8 ASSERT: \%s\ in file %s, line %d) at global/qlogging.cpp:384 #4 0x75c369e6 in qt_assert (assertion=0x75f8b7f0 size == 0 || offset 0 || size_t(offset) = sizeof(QArrayData), file=0x75f8b7b0 ../../include/QtCore/../../src/corelib/tools/qarraydata.h, line=62) at global/qglobal.cpp:2088 #5 0x75c29bf8 in QArrayData::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:61 #6 0x75c3015e in QTypedArrayDatachar::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:207 #7 0x75c2fde7 in QByteArray::constData (this=0x6f4760) at ../../include/QtCore/../../src/corelib/tools/qbytearray.h:433 #8 0x75c5 in qstrcmp (str1=..., str2=...) at tools/qbytearray.cpp:347 #9 0x77ba4625 in operator (a1=..., a2=...) at /opt/qt5/include/QtCore/qbytearray.h:550 #10 0x77ba0eb9 in KStartupInfoId::operator (this=0x693858, id_P=...) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:1175 #11 0x77ba67b5 in qMapLessThanKeyKStartupInfoId (key1=..., key2=...) at /opt/qt5/include/QtCore/qmap.h:75 #12 0x77ba6ef3 in QMapNodeKStartupInfoId, KStartupInfo::Data::lowerBound (this=0x693840, akey=...) at /opt/qt5/include/QtCore/qmap.h:145 #13 0x77ba66b4 in QMapDataKStartupInfoId, KStartupInfo::Data::findNode (this=0x700870, akey=...) at /opt/qt5/include/QtCore/qmap.h:292 #14 0x77ba5a73 in QMapKStartupInfoId, KStartupInfo::Data::remove (this=0x65acc0, akey=...) at /opt/qt5/include/QtCore/qmap.h:897 #15 0x77b9dd42 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 0x77ba039b 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 0x77ba02bf in KStartupInfo::Private::startups_cleanup (this=0x65acb0) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:965 #18 0x77ba3d1c in KStartupInfo::qt_static_metacall (_o=0x7fffdc60, _c=QMetaObject::InvokeMetaMethod, _id=3, _a=0x7fffbf70) at /opt/build/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/moc_kstartupinfo.cpp:106 #19 0x75ef8c9f in QMetaObject::activate (sender=0x684d50, signalOffset=3, local_signal_index=0, argv=0x0) at kernel/qobject.cpp:3530 #20 0x75ef84f0 in QMetaObject::activate (sender=0x684d50, m=0x763ab980 QTimer::staticMetaObject, local_signal_index=0, argv=0x0) at kernel/qobject.cpp:3405 #21 0x75f88fc3 in QTimer::timeout (this=0x684d50) at .moc/moc_qtimer.cpp:189 #22 0x75f03bc5 in QTimer::timerEvent (this=0x684d50, e=0x7fffca30) at kernel/qtimer.cpp:255 #23 0x75ef2314 in QObject::event (this=0x684d50, e=0x7fffca30) at kernel/qobject.cpp:1101 #24 0x7710e24e in QApplicationPrivate::notify_helper (this=0x60f010,
Re: Review Request 113877: Use QMutableMapIterator for removing KStartupInfoIds
On Nov. 25, 2013, 1:48 p.m., Kevin Ottens wrote: Any chance for a review? We really need to tie up loose ends now. 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 ;-) - Frank --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113877/#review44397 --- 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 0x750131e5 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #1 0x75016398 in __GI_abort () at abort.c:90 #2 0x75c3d402 in qt_message_fatal (context=..., message=...) at global/qlogging.cpp:979 #3 0x75c3afce in QMessageLogger::fatal (this=0x7fffbbc0, msg=0x75f8d9f8 ASSERT: \%s\ in file %s, line %d) at global/qlogging.cpp:384 #4 0x75c369e6 in qt_assert (assertion=0x75f8b7f0 size == 0 || offset 0 || size_t(offset) = sizeof(QArrayData), file=0x75f8b7b0 ../../include/QtCore/../../src/corelib/tools/qarraydata.h, line=62) at global/qglobal.cpp:2088 #5 0x75c29bf8 in QArrayData::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:61 #6 0x75c3015e in QTypedArrayDatachar::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:207 #7 0x75c2fde7 in QByteArray::constData (this=0x6f4760) at ../../include/QtCore/../../src/corelib/tools/qbytearray.h:433 #8 0x75c5 in qstrcmp (str1=..., str2=...) at tools/qbytearray.cpp:347 #9 0x77ba4625 in operator (a1=..., a2=...) at /opt/qt5/include/QtCore/qbytearray.h:550 #10 0x77ba0eb9 in KStartupInfoId::operator (this=0x693858, id_P=...) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:1175 #11 0x77ba67b5 in qMapLessThanKeyKStartupInfoId (key1=..., key2=...) at /opt/qt5/include/QtCore/qmap.h:75 #12 0x77ba6ef3 in QMapNodeKStartupInfoId, KStartupInfo::Data::lowerBound (this=0x693840, akey=...) at /opt/qt5/include/QtCore/qmap.h:145 #13 0x77ba66b4 in QMapDataKStartupInfoId, KStartupInfo::Data::findNode (this=0x700870, akey=...) at /opt/qt5/include/QtCore/qmap.h:292 #14 0x77ba5a73 in QMapKStartupInfoId, KStartupInfo::Data::remove (this=0x65acc0, akey=...) at /opt/qt5/include/QtCore/qmap.h:897 #15 0x77b9dd42 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 0x77ba039b 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 0x77ba02bf in KStartupInfo::Private::startups_cleanup (this=0x65acb0) at
Re: Review Request 113877: Use QMutableMapIterator for removing KStartupInfoIds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113877/#review6 --- 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. - David Faure 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 0x750131e5 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #1 0x75016398 in __GI_abort () at abort.c:90 #2 0x75c3d402 in qt_message_fatal (context=..., message=...) at global/qlogging.cpp:979 #3 0x75c3afce in QMessageLogger::fatal (this=0x7fffbbc0, msg=0x75f8d9f8 ASSERT: \%s\ in file %s, line %d) at global/qlogging.cpp:384 #4 0x75c369e6 in qt_assert (assertion=0x75f8b7f0 size == 0 || offset 0 || size_t(offset) = sizeof(QArrayData), file=0x75f8b7b0 ../../include/QtCore/../../src/corelib/tools/qarraydata.h, line=62) at global/qglobal.cpp:2088 #5 0x75c29bf8 in QArrayData::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:61 #6 0x75c3015e in QTypedArrayDatachar::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:207 #7 0x75c2fde7 in QByteArray::constData (this=0x6f4760) at ../../include/QtCore/../../src/corelib/tools/qbytearray.h:433 #8 0x75c5 in qstrcmp (str1=..., str2=...) at tools/qbytearray.cpp:347 #9 0x77ba4625 in operator (a1=..., a2=...) at /opt/qt5/include/QtCore/qbytearray.h:550 #10 0x77ba0eb9 in KStartupInfoId::operator (this=0x693858, id_P=...) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:1175 #11 0x77ba67b5 in qMapLessThanKeyKStartupInfoId (key1=..., key2=...) at /opt/qt5/include/QtCore/qmap.h:75 #12 0x77ba6ef3 in QMapNodeKStartupInfoId, KStartupInfo::Data::lowerBound (this=0x693840, akey=...) at /opt/qt5/include/QtCore/qmap.h:145 #13 0x77ba66b4 in QMapDataKStartupInfoId, KStartupInfo::Data::findNode (this=0x700870, akey=...) at /opt/qt5/include/QtCore/qmap.h:292 #14 0x77ba5a73 in QMapKStartupInfoId, KStartupInfo::Data::remove (this=0x65acc0, akey=...) at /opt/qt5/include/QtCore/qmap.h:897 #15 0x77b9dd42 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 0x77ba039b 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 0x77ba02bf in KStartupInfo::Private::startups_cleanup (this=0x65acb0) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:965 #18 0x77ba3d1c in KStartupInfo::qt_static_metacall (_o=0x7fffdc60, _c=QMetaObject::InvokeMetaMethod, _id=3, _a=0x7fffbf70) at /opt/build/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/moc_kstartupinfo.cpp:106 #19 0x75ef8c9f in QMetaObject::activate (sender=0x684d50, signalOffset=3, local_signal_index=0, argv=0x0) at kernel/qobject.cpp:3530 #20 0x75ef84f0 in QMetaObject::activate (sender=0x684d50, m=0x763ab980 QTimer::staticMetaObject, local_signal_index=0, argv=0x0) at
Re: Review Request 113877: Use QMutableMapIterator for removing KStartupInfoIds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113877/ --- (Updated Nov. 18, 2013, 7:04 a.m.) Review request for KDE Frameworks. Changes --- changed to data driven testing 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 (updated) - 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 0x750131e5 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #1 0x75016398 in __GI_abort () at abort.c:90 #2 0x75c3d402 in qt_message_fatal (context=..., message=...) at global/qlogging.cpp:979 #3 0x75c3afce in QMessageLogger::fatal (this=0x7fffbbc0, msg=0x75f8d9f8 ASSERT: \%s\ in file %s, line %d) at global/qlogging.cpp:384 #4 0x75c369e6 in qt_assert (assertion=0x75f8b7f0 size == 0 || offset 0 || size_t(offset) = sizeof(QArrayData), file=0x75f8b7b0 ../../include/QtCore/../../src/corelib/tools/qarraydata.h, line=62) at global/qglobal.cpp:2088 #5 0x75c29bf8 in QArrayData::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:61 #6 0x75c3015e in QTypedArrayDatachar::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:207 #7 0x75c2fde7 in QByteArray::constData (this=0x6f4760) at ../../include/QtCore/../../src/corelib/tools/qbytearray.h:433 #8 0x75c5 in qstrcmp (str1=..., str2=...) at tools/qbytearray.cpp:347 #9 0x77ba4625 in operator (a1=..., a2=...) at /opt/qt5/include/QtCore/qbytearray.h:550 #10 0x77ba0eb9 in KStartupInfoId::operator (this=0x693858, id_P=...) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:1175 #11 0x77ba67b5 in qMapLessThanKeyKStartupInfoId (key1=..., key2=...) at /opt/qt5/include/QtCore/qmap.h:75 #12 0x77ba6ef3 in QMapNodeKStartupInfoId, KStartupInfo::Data::lowerBound (this=0x693840, akey=...) at /opt/qt5/include/QtCore/qmap.h:145 #13 0x77ba66b4 in QMapDataKStartupInfoId, KStartupInfo::Data::findNode (this=0x700870, akey=...) at /opt/qt5/include/QtCore/qmap.h:292 #14 0x77ba5a73 in QMapKStartupInfoId, KStartupInfo::Data::remove (this=0x65acc0, akey=...) at /opt/qt5/include/QtCore/qmap.h:897 #15 0x77b9dd42 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 0x77ba039b 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 0x77ba02bf in KStartupInfo::Private::startups_cleanup (this=0x65acb0) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:965 #18 0x77ba3d1c in KStartupInfo::qt_static_metacall (_o=0x7fffdc60, _c=QMetaObject::InvokeMetaMethod, _id=3, _a=0x7fffbf70) at /opt/build/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/moc_kstartupinfo.cpp:106 #19 0x75ef8c9f in QMetaObject::activate (sender=0x684d50, signalOffset=3, local_signal_index=0, argv=0x0) at kernel/qobject.cpp:3530 #20 0x75ef84f0 in QMetaObject::activate (sender=0x684d50, m=0x763ab980 QTimer::staticMetaObject, local_signal_index=0, argv=0x0) at kernel/qobject.cpp:3405 #21 0x75f88fc3 in QTimer::timeout (this=0x684d50) at .moc/moc_qtimer.cpp:189 #22 0x75f03bc5 in QTimer::timerEvent (this=0x684d50, e=0x7fffca30) at kernel/qtimer.cpp:255 #23 0x75ef2314 in QObject::event (this=0x684d50, e=0x7fffca30) at kernel/qobject.cpp:1101 #24 0x7710e24e in QApplicationPrivate::notify_helper (this=0x60f010, receiver=0x684d50, e=0x7fffca30) at kernel/qapplication.cpp:3467 #25 0x7710b805 in QApplication::notify (this=0x7fffdc90, receiver=0x684d50, e=0x7fffca30) at kernel/qapplication.cpp:2888 #26 0x75eb6908 in QCoreApplication::notifyInternal (this=0x7fffdc90, receiver=0x684d50, event=0x7fffca30) at kernel/qcoreapplication.cpp:878 #27 0x75eba4f1 in QCoreApplication::sendEvent
Re: Review Request 113877: Use QMutableMapIterator for removing KStartupInfoIds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113877/#review43717 --- I think the root cause of the corruption is that there is a ++it in line 981, and then remove_startup_info_internal( key ) in the following line, which calls QMap::remove(key). The latter invalidates all existing iterators for the corresponding map, so any later use of it is unsafe. An alternative would be to use it = startups.erase(it) instead. QMap::erase(it) always returns a valid iterator pointing to the next item (which may be end()). That approach could also be combined with your refactored function (which looks much nicer than the threefold repetition of mostly the same code, nice!), but I guess it's a matter of taste if you prefer the Java-style iterators, or you use the STL-style ones in the correct and safe way. I prefer the STL ones though, because you can still use *it and it-foo() to dereference them. Most (or all?) Qt and STL containers support it = container.erase(it) for the removal of an item that an iterator points to, and having it point to the next item (or end()) afterwards. - Frank Reininghaus On Nov. 15, 2013, 9:48 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. 15, 2013, 9:48 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/autotests/kstartupinfo_unittest.cpp 17890ff tier1/kwindowsystem/src/kstartupinfo.cpp 402cc97 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 0x750131e5 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #1 0x75016398 in __GI_abort () at abort.c:90 #2 0x75c3d402 in qt_message_fatal (context=..., message=...) at global/qlogging.cpp:979 #3 0x75c3afce in QMessageLogger::fatal (this=0x7fffbbc0, msg=0x75f8d9f8 ASSERT: \%s\ in file %s, line %d) at global/qlogging.cpp:384 #4 0x75c369e6 in qt_assert (assertion=0x75f8b7f0 size == 0 || offset 0 || size_t(offset) = sizeof(QArrayData), file=0x75f8b7b0 ../../include/QtCore/../../src/corelib/tools/qarraydata.h, line=62) at global/qglobal.cpp:2088 #5 0x75c29bf8 in QArrayData::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:61 #6 0x75c3015e in QTypedArrayDatachar::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:207 #7 0x75c2fde7 in QByteArray::constData (this=0x6f4760) at ../../include/QtCore/../../src/corelib/tools/qbytearray.h:433 #8 0x75c5 in qstrcmp (str1=..., str2=...) at tools/qbytearray.cpp:347 #9 0x77ba4625 in operator (a1=..., a2=...) at /opt/qt5/include/QtCore/qbytearray.h:550 #10 0x77ba0eb9 in KStartupInfoId::operator (this=0x693858, id_P=...) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:1175 #11 0x77ba67b5 in qMapLessThanKeyKStartupInfoId (key1=..., key2=...) at /opt/qt5/include/QtCore/qmap.h:75 #12 0x77ba6ef3 in QMapNodeKStartupInfoId, KStartupInfo::Data::lowerBound (this=0x693840, akey=...) at /opt/qt5/include/QtCore/qmap.h:145 #13 0x77ba66b4 in QMapDataKStartupInfoId, KStartupInfo::Data::findNode (this=0x700870, akey=...) at /opt/qt5/include/QtCore/qmap.h:292 #14 0x77ba5a73 in QMapKStartupInfoId, KStartupInfo::Data::remove (this=0x65acc0, akey=...) at /opt/qt5/include/QtCore/qmap.h:897 #15 0x77b9dd42 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 0x77ba039b 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 0x77ba02bf in KStartupInfo::Private::startups_cleanup (this=0x65acb0) at
Re: Review Request 113877: Use QMutableMapIterator for removing KStartupInfoIds
On Nov. 15, 2013, 11:26 a.m., Frank Reininghaus wrote: I think the root cause of the corruption is that there is a ++it in line 981, and then remove_startup_info_internal( key ) in the following line, which calls QMap::remove(key). The latter invalidates all existing iterators for the corresponding map, so any later use of it is unsafe. An alternative would be to use it = startups.erase(it) instead. QMap::erase(it) always returns a valid iterator pointing to the next item (which may be end()). That approach could also be combined with your refactored function (which looks much nicer than the threefold repetition of mostly the same code, nice!), but I guess it's a matter of taste if you prefer the Java-style iterators, or you use the STL-style ones in the correct and safe way. I prefer the STL ones though, because you can still use *it and it-foo() to dereference them. Most (or all?) Qt and STL containers support it = container.erase(it) for the removal of an item that an iterator points to, and having it point to the next item (or end()) afterwards. ah I didn't see erase() at all. It was not mentioned in the doc: Items can be removed from the map in several ways. One way is to call remove(); this will remove any item with the given key. Another way is to use QMutableMapIterator::remove(). In addition, you can clear the entire map using clear(). That's why I went for QMutableMapIterator. It's not like I prefer the Java-style iterators. - Martin --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113877/#review43717 --- On Nov. 15, 2013, 10:48 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. 15, 2013, 10:48 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/autotests/kstartupinfo_unittest.cpp 17890ff tier1/kwindowsystem/src/kstartupinfo.cpp 402cc97 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 0x750131e5 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #1 0x75016398 in __GI_abort () at abort.c:90 #2 0x75c3d402 in qt_message_fatal (context=..., message=...) at global/qlogging.cpp:979 #3 0x75c3afce in QMessageLogger::fatal (this=0x7fffbbc0, msg=0x75f8d9f8 ASSERT: \%s\ in file %s, line %d) at global/qlogging.cpp:384 #4 0x75c369e6 in qt_assert (assertion=0x75f8b7f0 size == 0 || offset 0 || size_t(offset) = sizeof(QArrayData), file=0x75f8b7b0 ../../include/QtCore/../../src/corelib/tools/qarraydata.h, line=62) at global/qglobal.cpp:2088 #5 0x75c29bf8 in QArrayData::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:61 #6 0x75c3015e in QTypedArrayDatachar::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:207 #7 0x75c2fde7 in QByteArray::constData (this=0x6f4760) at ../../include/QtCore/../../src/corelib/tools/qbytearray.h:433 #8 0x75c5 in qstrcmp (str1=..., str2=...) at tools/qbytearray.cpp:347 #9 0x77ba4625 in operator (a1=..., a2=...) at /opt/qt5/include/QtCore/qbytearray.h:550 #10 0x77ba0eb9 in KStartupInfoId::operator (this=0x693858, id_P=...) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:1175 #11 0x77ba67b5 in qMapLessThanKeyKStartupInfoId (key1=..., key2=...) at /opt/qt5/include/QtCore/qmap.h:75 #12 0x77ba6ef3 in QMapNodeKStartupInfoId, KStartupInfo::Data::lowerBound (this=0x693840, akey=...) at /opt/qt5/include/QtCore/qmap.h:145 #13 0x77ba66b4 in QMapDataKStartupInfoId, KStartupInfo::Data::findNode (this=0x700870, akey=...) at /opt/qt5/include/QtCore/qmap.h:292 #14 0x77ba5a73 in QMapKStartupInfoId, KStartupInfo::Data::remove (this=0x65acc0, akey=...) at /opt/qt5/include/QtCore/qmap.h:897 #15 0x77b9dd42 in
Re: Review Request 113877: Use QMutableMapIterator for removing KStartupInfoIds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113877/ --- (Updated Nov. 15, 2013, 11:42 a.m.) Review request for KDE Frameworks. Changes --- change to stl-style iterators. 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 (updated) - tier1/kwindowsystem/autotests/kstartupinfo_unittest.cpp 17890ff tier1/kwindowsystem/src/kstartupinfo.cpp 402cc97 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 0x750131e5 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #1 0x75016398 in __GI_abort () at abort.c:90 #2 0x75c3d402 in qt_message_fatal (context=..., message=...) at global/qlogging.cpp:979 #3 0x75c3afce in QMessageLogger::fatal (this=0x7fffbbc0, msg=0x75f8d9f8 ASSERT: \%s\ in file %s, line %d) at global/qlogging.cpp:384 #4 0x75c369e6 in qt_assert (assertion=0x75f8b7f0 size == 0 || offset 0 || size_t(offset) = sizeof(QArrayData), file=0x75f8b7b0 ../../include/QtCore/../../src/corelib/tools/qarraydata.h, line=62) at global/qglobal.cpp:2088 #5 0x75c29bf8 in QArrayData::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:61 #6 0x75c3015e in QTypedArrayDatachar::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:207 #7 0x75c2fde7 in QByteArray::constData (this=0x6f4760) at ../../include/QtCore/../../src/corelib/tools/qbytearray.h:433 #8 0x75c5 in qstrcmp (str1=..., str2=...) at tools/qbytearray.cpp:347 #9 0x77ba4625 in operator (a1=..., a2=...) at /opt/qt5/include/QtCore/qbytearray.h:550 #10 0x77ba0eb9 in KStartupInfoId::operator (this=0x693858, id_P=...) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:1175 #11 0x77ba67b5 in qMapLessThanKeyKStartupInfoId (key1=..., key2=...) at /opt/qt5/include/QtCore/qmap.h:75 #12 0x77ba6ef3 in QMapNodeKStartupInfoId, KStartupInfo::Data::lowerBound (this=0x693840, akey=...) at /opt/qt5/include/QtCore/qmap.h:145 #13 0x77ba66b4 in QMapDataKStartupInfoId, KStartupInfo::Data::findNode (this=0x700870, akey=...) at /opt/qt5/include/QtCore/qmap.h:292 #14 0x77ba5a73 in QMapKStartupInfoId, KStartupInfo::Data::remove (this=0x65acc0, akey=...) at /opt/qt5/include/QtCore/qmap.h:897 #15 0x77b9dd42 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 0x77ba039b 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 0x77ba02bf in KStartupInfo::Private::startups_cleanup (this=0x65acb0) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:965 #18 0x77ba3d1c in KStartupInfo::qt_static_metacall (_o=0x7fffdc60, _c=QMetaObject::InvokeMetaMethod, _id=3, _a=0x7fffbf70) at /opt/build/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/moc_kstartupinfo.cpp:106 #19 0x75ef8c9f in QMetaObject::activate (sender=0x684d50, signalOffset=3, local_signal_index=0, argv=0x0) at kernel/qobject.cpp:3530 #20 0x75ef84f0 in QMetaObject::activate (sender=0x684d50, m=0x763ab980 QTimer::staticMetaObject, local_signal_index=0, argv=0x0) at kernel/qobject.cpp:3405 #21 0x75f88fc3 in QTimer::timeout (this=0x684d50) at .moc/moc_qtimer.cpp:189 #22 0x75f03bc5 in QTimer::timerEvent (this=0x684d50, e=0x7fffca30) at kernel/qtimer.cpp:255 #23 0x75ef2314 in QObject::event (this=0x684d50, e=0x7fffca30) at kernel/qobject.cpp:1101 #24 0x7710e24e in QApplicationPrivate::notify_helper (this=0x60f010, receiver=0x684d50, e=0x7fffca30) at kernel/qapplication.cpp:3467 #25 0x7710b805 in QApplication::notify (this=0x7fffdc90, receiver=0x684d50, e=0x7fffca30) at kernel/qapplication.cpp:2888 #26 0x75eb6908 in QCoreApplication::notifyInternal (this=0x7fffdc90, receiver=0x684d50, event=0x7fffca30) at kernel/qcoreapplication.cpp:878 #27 0x75eba4f1 in QCoreApplication::sendEvent
Re: Review Request 113877: Use QMutableMapIterator for removing KStartupInfoIds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113877/#review43727 --- I'm not a KWindowSystem expert, but anyway, here are some more things that I noticed, and that might or might not be worth considering: 1. I think that you can simplify the code in KStartupInfo::Private::startups_cleanup_internal(bool) slightly by using it-age++; if (it-silent() == Data::Yes) etc. Might be a matter of taste though ;-) I think that this is slightly easier to read, but the compiler will most likely generate the same code for both variants. 2. Since your three test functions share the vast majority of their code, one could use data-driven testing: http://qt-project.org/doc/qt-5.1/qttestlib/tutorial2.html That could make future maintenance of the test functions easier, and it also makes it easier to see what the difference between the three tests is. 3. If I read the test code correctly, the unit test will have a runtime of at least 27 seconds because of the QTest::qWait(int) calls, right? I think that tests should run as fast as possible - if they don't, it gives people an excellent excuse for not running them, which greatly increases the risk of regressions. For the test that checks if the QSignalSpy finally has a certain count(), one could use a loop over a qWait(int) with a smaller timeout that terminates if the count() has reached the expected value. (For the case that the expected count() was 1, we had kWaitForSignal, but this is probably not an option for tier 1. Internally, that function uses a QEventLoop which has its quit() slot connected to the expected signal, I think). I'm not sure about the test that waits for 21 seconds and passes if the count() is zero though. I guess that the 21 seconds are required for safety, such that the test has a predictable outcome even if runs on a slow system with heavy load? Some knowledge of the code to be tested might be required to come up with a less time-consuming solution. If there was a signal that was emitted if everything went OK, one could simply wait for it. - Frank Reininghaus On Nov. 15, 2013, 10:42 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. 15, 2013, 10:42 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/autotests/kstartupinfo_unittest.cpp 17890ff tier1/kwindowsystem/src/kstartupinfo.cpp 402cc97 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 0x750131e5 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #1 0x75016398 in __GI_abort () at abort.c:90 #2 0x75c3d402 in qt_message_fatal (context=..., message=...) at global/qlogging.cpp:979 #3 0x75c3afce in QMessageLogger::fatal (this=0x7fffbbc0, msg=0x75f8d9f8 ASSERT: \%s\ in file %s, line %d) at global/qlogging.cpp:384 #4 0x75c369e6 in qt_assert (assertion=0x75f8b7f0 size == 0 || offset 0 || size_t(offset) = sizeof(QArrayData), file=0x75f8b7b0 ../../include/QtCore/../../src/corelib/tools/qarraydata.h, line=62) at global/qglobal.cpp:2088 #5 0x75c29bf8 in QArrayData::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:61 #6 0x75c3015e in QTypedArrayDatachar::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:207 #7 0x75c2fde7 in QByteArray::constData (this=0x6f4760) at ../../include/QtCore/../../src/corelib/tools/qbytearray.h:433 #8 0x75c5 in qstrcmp (str1=..., str2=...) at tools/qbytearray.cpp:347 #9 0x77ba4625 in operator (a1=..., a2=...) at /opt/qt5/include/QtCore/qbytearray.h:550 #10 0x77ba0eb9 in KStartupInfoId::operator (this=0x693858, id_P=...) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:1175 #11 0x77ba67b5 in qMapLessThanKeyKStartupInfoId (key1=..., key2=...) at /opt/qt5/include/QtCore/qmap.h:75 #12
Re: Review Request 113877: Use QMutableMapIterator for removing KStartupInfoIds
On Nov. 15, 2013, 1:31 p.m., Frank Reininghaus wrote: I'm not a KWindowSystem expert, but anyway, here are some more things that I noticed, and that might or might not be worth considering: 1. I think that you can simplify the code in KStartupInfo::Private::startups_cleanup_internal(bool) slightly by using it-age++; if (it-silent() == Data::Yes) etc. Might be a matter of taste though ;-) I think that this is slightly easier to read, but the compiler will most likely generate the same code for both variants. 2. Since your three test functions share the vast majority of their code, one could use data-driven testing: http://qt-project.org/doc/qt-5.1/qttestlib/tutorial2.html That could make future maintenance of the test functions easier, and it also makes it easier to see what the difference between the three tests is. 3. If I read the test code correctly, the unit test will have a runtime of at least 27 seconds because of the QTest::qWait(int) calls, right? I think that tests should run as fast as possible - if they don't, it gives people an excellent excuse for not running them, which greatly increases the risk of regressions. For the test that checks if the QSignalSpy finally has a certain count(), one could use a loop over a qWait(int) with a smaller timeout that terminates if the count() has reached the expected value. (For the case that the expected count() was 1, we had kWaitForSignal, but this is probably not an option for tier 1. Internally, that function uses a QEventLoop which has its quit() slot connected to the expected signal, I think). I'm not sure about the test that waits for 21 seconds and passes if the count() is zero though. I guess that the 21 seconds are required for safety, such that the test has a predictable outcome even if runs on a slow system with heavy load? Some knowledge of the code to be tested might be required to come up with a less time-consuming solution. If there was a signal that was emitted if everything went OK, one could simply wait for it. I think that tests should run as fast as possible yup, that's what it does :-( The default would be a timeout of one minute. For the silent case the timeout - is for what reason ever - multiplied by 20. That's where the long time comes from. The problem is that the race condition causing the problem could only be hit when the timeout was reached. So we have to wait those 20 secs. I added another sec to be sure that the timeout has fired - we also depend on X here and so a small safety net is needed. And unfortunately in the silent case no signal is emitted. Thanks for your feedback. I will try to change to a useful data driven test function. - Martin --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113877/#review43727 --- On Nov. 15, 2013, 11:42 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. 15, 2013, 11:42 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/autotests/kstartupinfo_unittest.cpp 17890ff tier1/kwindowsystem/src/kstartupinfo.cpp 402cc97 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 0x750131e5 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #1 0x75016398 in __GI_abort () at abort.c:90 #2 0x75c3d402 in qt_message_fatal (context=..., message=...) at global/qlogging.cpp:979 #3 0x75c3afce in QMessageLogger::fatal (this=0x7fffbbc0, msg=0x75f8d9f8 ASSERT: \%s\ in file %s, line %d) at global/qlogging.cpp:384 #4 0x75c369e6 in qt_assert (assertion=0x75f8b7f0 size == 0 || offset 0 || size_t(offset) = sizeof(QArrayData), file=0x75f8b7b0 ../../include/QtCore/../../src/corelib/tools/qarraydata.h, line=62) at global/qglobal.cpp:2088 #5 0x75c29bf8 in QArrayData::data (this=0x6cb830) at
