Review Request 117701: Fix crashers in KStartupInfo on remove while iterating

[Thomas Lübking]

> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140423/1bb4388e/attachment.html>