Re: Review Request 117701: Fix crashers in KStartupInfo on remove while iterating
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117701/ --- (Updated April 25, 2014, 7:03 a.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks, Àlex Fiestas and Bhushan Shah. Bugs: 332681 https://bugs.kde.org/show_bug.cgi?id=332681 Repository: kwindowsystem Description --- Fix crashers in KStartupInfo on remove while iterating This is similar to the change ... just for more cases. If the API was used with ::checkStartup it could happen that an item got removed from the startups while iterating the list of startups. Thus the data corrupted resulting in a crash when getting a similar crash. In this case the code now uses erase instead of remove to have a valid iterator. BUG: 332681 Diffs - autotests/kstartupinfo_unittest.cpp 29fa320bc2c82e7e04a7322111bcdba44b7078c6 src/kstartupinfo.cpp 6a95ce2d2eb79abbfbd072fa922f458da30d37eb Diff: https://git.reviewboard.kde.org/r/117701/diff/ Testing --- See new unit tests which crash without the change. For people wanting to run the test: be warned it has the power to crash plasma-shell and kwin. Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 117701: Fix crashers in KStartupInfo on remove while iterating
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117701/#review56470 --- This review has been submitted with commit ba3fe29fe342d20caf2ec694858d81b97afbfc2c by Martin Gräßlin to branch master. - Commit Hook On April 24, 2014, 5:27 a.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117701/ --- (Updated April 24, 2014, 5:27 a.m.) Review request for KDE Frameworks, Àlex Fiestas and Bhushan Shah. Bugs: 332681 https://bugs.kde.org/show_bug.cgi?id=332681 Repository: kwindowsystem Description --- Fix crashers in KStartupInfo on remove while iterating This is similar to the change ... just for more cases. If the API was used with ::checkStartup it could happen that an item got removed from the startups while iterating the list of startups. Thus the data corrupted resulting in a crash when getting a similar crash. In this case the code now uses erase instead of remove to have a valid iterator. BUG: 332681 Diffs - autotests/kstartupinfo_unittest.cpp 29fa320bc2c82e7e04a7322111bcdba44b7078c6 src/kstartupinfo.cpp 6a95ce2d2eb79abbfbd072fa922f458da30d37eb Diff: https://git.reviewboard.kde.org/r/117701/diff/ Testing --- See new unit tests which crash without the change. For people wanting to run the test: be warned it has the power to crash plasma-shell and kwin. Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 117701: Fix crashers in KStartupInfo on remove while iterating
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117701/#review56269 --- src/kstartupinfo.cpp https://git.reviewboard.kde.org/r/117701/#comment39336 this function is so ugly that i wonder whether removal by value is actually required at all and in case it should try to find the iterator and use that in case of success - instead of the ugly ::operator[] ;-) src/kstartupinfo.cpp https://git.reviewboard.kde.org/r/117701/#comment39334 this is somehow not equivalent to the other variant (ignores silent/uninited) - the iterator should perhaps go with a map pointer/reference to stress this (and as a matching map usage is required anyway) src/kstartupinfo.cpp https://git.reviewboard.kde.org/r/117701/#comment39335 ahhh... this one's the (semi-)obvious crasher =) (I had first only seen the other loops, which looked pretty harmless) Though i had assumed that the non-const iterator would have detached the list anyway. - Thomas Lübking On April 23, 2014, 9:35 a.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117701/ --- (Updated April 23, 2014, 9:35 a.m.) Review request for KDE Frameworks, Àlex Fiestas and Bhushan Shah. Bugs: 332681 https://bugs.kde.org/show_bug.cgi?id=332681 Repository: kwindowsystem Description --- Fix crashers in KStartupInfo on remove while iterating This is similar to the change ... just for more cases. If the API was used with ::checkStartup it could happen that an item got removed from the startups while iterating the list of startups. Thus the data corrupted resulting in a crash when getting a similar crash. In this case the code now uses erase instead of remove to have a valid iterator. BUG: 332681 Diffs - autotests/kstartupinfo_unittest.cpp 29fa320bc2c82e7e04a7322111bcdba44b7078c6 src/kstartupinfo.cpp 6a95ce2d2eb79abbfbd072fa922f458da30d37eb Diff: https://git.reviewboard.kde.org/r/117701/diff/ Testing --- See new unit tests which crash without the change. For people wanting to run the test: be warned it has the power to crash plasma-shell and kwin. Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 117701: Fix crashers in KStartupInfo on remove while iterating
On April 23, 2014, 3:03 p.m., Thomas Lübking wrote: src/kstartupinfo.cpp, line 413 https://git.reviewboard.kde.org/r/117701/diff/1/?file=267866#file267866line413 this is somehow not equivalent to the other variant (ignores silent/uninited) - the iterator should perhaps go with a map pointer/reference to stress this (and as a matching map usage is required anyway) my understand is that silent/unintended was dead code for these cases as it only iterated over the startups and that if it's in startups it's not in silent or unintended. I'll re-check that. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117701/#review56269 --- On April 23, 2014, 11:35 a.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117701/ --- (Updated April 23, 2014, 11:35 a.m.) Review request for KDE Frameworks, Àlex Fiestas and Bhushan Shah. Bugs: 332681 https://bugs.kde.org/show_bug.cgi?id=332681 Repository: kwindowsystem Description --- Fix crashers in KStartupInfo on remove while iterating This is similar to the change ... just for more cases. If the API was used with ::checkStartup it could happen that an item got removed from the startups while iterating the list of startups. Thus the data corrupted resulting in a crash when getting a similar crash. In this case the code now uses erase instead of remove to have a valid iterator. BUG: 332681 Diffs - autotests/kstartupinfo_unittest.cpp 29fa320bc2c82e7e04a7322111bcdba44b7078c6 src/kstartupinfo.cpp 6a95ce2d2eb79abbfbd072fa922f458da30d37eb Diff: https://git.reviewboard.kde.org/r/117701/diff/ Testing --- See new unit tests which crash without the change. For people wanting to run the test: be warned it has the power to crash plasma-shell and kwin. Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 117701: Fix crashers in KStartupInfo on remove while iterating
On April 23, 2014, 3:03 p.m., Thomas Lübking wrote:
src/kstartupinfo.cpp, line 413
https://git.reviewboard.kde.org/r/117701/diff/1/?file=267866#file267866line413
this is somehow not equivalent to the other variant (ignores
silent/uninited) - the iterator should perhaps go with a map
pointer/reference to stress this (and as a matching map usage is required
anyway)
Martin Gräßlin wrote:
my understand is that silent/unintended was dead code for these cases as
it only iterated over the startups and that if it's in startups it's not in
silent or unintended. I'll re-check that.
yep, unintended and ignored do not matter in that case as it's either in
startups or in silent_startups or in unintended_startups:
if (update_P) { // change: without any new: first
//qDebug() adding uninited;
uninited_startups.insert(id_P, data_P);
} else if (data_P.silent() != Data::Yes || flags AnnounceSilenceChanges) {
//qDebug() adding;
startups.insert(id_P, data_P);
emit q-gotNewStartup(id_P, data_P);
} else { // new silenced, and silent shouldn't be announced
//qDebug() adding silent;
silent_startups.insert(id_P, data_P);
}
- Martin
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117701/#review56269
---
On April 23, 2014, 11:35 a.m., Martin Gräßlin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117701/
---
(Updated April 23, 2014, 11:35 a.m.)
Review request for KDE Frameworks, Àlex Fiestas and Bhushan Shah.
Bugs: 332681
https://bugs.kde.org/show_bug.cgi?id=332681
Repository: kwindowsystem
Description
---
Fix crashers in KStartupInfo on remove while iterating
This is similar to the change ... just for more cases. If the API was
used with ::checkStartup it could happen that an item got removed from
the startups while iterating the list of startups. Thus the data
corrupted resulting in a crash when getting a similar crash.
In this case the code now uses erase instead of remove to have a valid
iterator.
BUG: 332681
Diffs
-
autotests/kstartupinfo_unittest.cpp
29fa320bc2c82e7e04a7322111bcdba44b7078c6
src/kstartupinfo.cpp 6a95ce2d2eb79abbfbd072fa922f458da30d37eb
Diff: https://git.reviewboard.kde.org/r/117701/diff/
Testing
---
See new unit tests which crash without the change. For people wanting to run
the test: be warned it has the power to crash plasma-shell and kwin.
Thanks,
Martin Gräßlin
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 117701: Fix crashers in KStartupInfo on remove while iterating
On April 23, 2014, 3:03 p.m., Thomas Lübking wrote:
src/kstartupinfo.cpp, line 413
https://git.reviewboard.kde.org/r/117701/diff/1/?file=267866#file267866line413
this is somehow not equivalent to the other variant (ignores
silent/uninited) - the iterator should perhaps go with a map
pointer/reference to stress this (and as a matching map usage is required
anyway)
Martin Gräßlin wrote:
my understand is that silent/unintended was dead code for these cases as
it only iterated over the startups and that if it's in startups it's not in
silent or unintended. I'll re-check that.
Martin Gräßlin wrote:
yep, unintended and ignored do not matter in that case as it's either in
startups or in silent_startups or in unintended_startups:
if (update_P) { // change: without any new: first
//qDebug() adding uninited;
uninited_startups.insert(id_P, data_P);
} else if (data_P.silent() != Data::Yes || flags
AnnounceSilenceChanges) {
//qDebug() adding;
startups.insert(id_P, data_P);
emit q-gotNewStartup(id_P, data_P);
} else { // new silenced, and silent shouldn't be announced
//qDebug() adding silent;
silent_startups.insert(id_P, data_P);
}
But yes, it might be a good idea to either rename the new method or add the map
as a reference. Though I think rename is better.
- Martin
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117701/#review56269
---
On April 23, 2014, 11:35 a.m., Martin Gräßlin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117701/
---
(Updated April 23, 2014, 11:35 a.m.)
Review request for KDE Frameworks, Àlex Fiestas and Bhushan Shah.
Bugs: 332681
https://bugs.kde.org/show_bug.cgi?id=332681
Repository: kwindowsystem
Description
---
Fix crashers in KStartupInfo on remove while iterating
This is similar to the change ... just for more cases. If the API was
used with ::checkStartup it could happen that an item got removed from
the startups while iterating the list of startups. Thus the data
corrupted resulting in a crash when getting a similar crash.
In this case the code now uses erase instead of remove to have a valid
iterator.
BUG: 332681
Diffs
-
autotests/kstartupinfo_unittest.cpp
29fa320bc2c82e7e04a7322111bcdba44b7078c6
src/kstartupinfo.cpp 6a95ce2d2eb79abbfbd072fa922f458da30d37eb
Diff: https://git.reviewboard.kde.org/r/117701/diff/
Testing
---
See new unit tests which crash without the change. For people wanting to run
the test: be warned it has the power to crash plasma-shell and kwin.
Thanks,
Martin Gräßlin
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 117701: Fix crashers in KStartupInfo on remove while iterating
On April 23, 2014, 1:03 p.m., Thomas Lübking wrote:
src/kstartupinfo.cpp, line 413
https://git.reviewboard.kde.org/r/117701/diff/1/?file=267866#file267866line413
this is somehow not equivalent to the other variant (ignores
silent/uninited) - the iterator should perhaps go with a map
pointer/reference to stress this (and as a matching map usage is required
anyway)
Martin Gräßlin wrote:
my understand is that silent/unintended was dead code for these cases as
it only iterated over the startups and that if it's in startups it's not in
silent or unintended. I'll re-check that.
Martin Gräßlin wrote:
yep, unintended and ignored do not matter in that case as it's either in
startups or in silent_startups or in unintended_startups:
if (update_P) { // change: without any new: first
//qDebug() adding uninited;
uninited_startups.insert(id_P, data_P);
} else if (data_P.silent() != Data::Yes || flags
AnnounceSilenceChanges) {
//qDebug() adding;
startups.insert(id_P, data_P);
emit q-gotNewStartup(id_P, data_P);
} else { // new silenced, and silent shouldn't be announced
//qDebug() adding silent;
silent_startups.insert(id_P, data_P);
}
Martin Gräßlin wrote:
But yes, it might be a good idea to either rename the new method or add
the map as a reference. Though I think rename is better.
I actually meant, whether it's in actual use at all (yes, is) or could fully be
replaced by a variant taking an iterator and a map (no, can't) - there's no
ignored in my copy ;-)
I'd then suggest to rename it to removeAllStartupInfoInternal() and fix it (to
operate on iterators) in another patch.
- Thomas
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117701/#review56269
---
On April 23, 2014, 9:35 a.m., Martin Gräßlin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117701/
---
(Updated April 23, 2014, 9:35 a.m.)
Review request for KDE Frameworks, Àlex Fiestas and Bhushan Shah.
Bugs: 332681
https://bugs.kde.org/show_bug.cgi?id=332681
Repository: kwindowsystem
Description
---
Fix crashers in KStartupInfo on remove while iterating
This is similar to the change ... just for more cases. If the API was
used with ::checkStartup it could happen that an item got removed from
the startups while iterating the list of startups. Thus the data
corrupted resulting in a crash when getting a similar crash.
In this case the code now uses erase instead of remove to have a valid
iterator.
BUG: 332681
Diffs
-
autotests/kstartupinfo_unittest.cpp
29fa320bc2c82e7e04a7322111bcdba44b7078c6
src/kstartupinfo.cpp 6a95ce2d2eb79abbfbd072fa922f458da30d37eb
Diff: https://git.reviewboard.kde.org/r/117701/diff/
Testing
---
See new unit tests which crash without the change. For people wanting to run
the test: be warned it has the power to crash plasma-shell and kwin.
Thanks,
Martin Gräßlin
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 117701: Fix crashers in KStartupInfo on remove while iterating
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117701/#review56333 --- Ship it! Ship It! - Thomas Lübking On April 23, 2014, 9:35 a.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117701/ --- (Updated April 23, 2014, 9:35 a.m.) Review request for KDE Frameworks, Àlex Fiestas and Bhushan Shah. Bugs: 332681 https://bugs.kde.org/show_bug.cgi?id=332681 Repository: kwindowsystem Description --- Fix crashers in KStartupInfo on remove while iterating This is similar to the change ... just for more cases. If the API was used with ::checkStartup it could happen that an item got removed from the startups while iterating the list of startups. Thus the data corrupted resulting in a crash when getting a similar crash. In this case the code now uses erase instead of remove to have a valid iterator. BUG: 332681 Diffs - autotests/kstartupinfo_unittest.cpp 29fa320bc2c82e7e04a7322111bcdba44b7078c6 src/kstartupinfo.cpp 6a95ce2d2eb79abbfbd072fa922f458da30d37eb Diff: https://git.reviewboard.kde.org/r/117701/diff/ Testing --- See new unit tests which crash without the change. For people wanting to run the test: be warned it has the power to crash plasma-shell and kwin. Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 117701: Fix crashers in KStartupInfo on remove while iterating
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117701/ --- (Updated April 24, 2014, 7:27 a.m.) Review request for KDE Frameworks, Àlex Fiestas and Bhushan Shah. Changes --- Changed the method name to removeStartupInfoInternal and added a small documentation to what it does. Will now change the other one in another review request. Bugs: 332681 https://bugs.kde.org/show_bug.cgi?id=332681 Repository: kwindowsystem Description --- Fix crashers in KStartupInfo on remove while iterating This is similar to the change ... just for more cases. If the API was used with ::checkStartup it could happen that an item got removed from the startups while iterating the list of startups. Thus the data corrupted resulting in a crash when getting a similar crash. In this case the code now uses erase instead of remove to have a valid iterator. BUG: 332681 Diffs (updated) - autotests/kstartupinfo_unittest.cpp 29fa320bc2c82e7e04a7322111bcdba44b7078c6 src/kstartupinfo.cpp 6a95ce2d2eb79abbfbd072fa922f458da30d37eb Diff: https://git.reviewboard.kde.org/r/117701/diff/ Testing --- See new unit tests which crash without the change. For people wanting to run the test: be warned it has the power to crash plasma-shell and kwin. Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
