Review Request 118507: Fix crash if a kioslave adds multiple items with the same URL and reports them as deleted

[Emmanuel Pescosta]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118507/#review59142
-----------------------------------------------------------

Ship it!


Ship It!

- Emmanuel Pescosta


On June 3, 2014, 9:34 p.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118507/
> -----------------------------------------------------------
> 
> (Updated June 3, 2014, 9:34 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Bugs: 335672
>     http://bugs.kde.org/show_bug.cgi?id=335672
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> When opening the URL "man:", there are multiple items with the same name (for example, _exit is shown twice here). When opening a new tab, the kioslave reports some items as deleted (I have not quite understood why). The problem is that it reports some of the duplicate items twice in the list of deleted items. This confuses KFileItemModel and corrupts the internal data structures, and finally, causes a crash.
> 
> The fix is to remove all duplicates from KItemRangeList::fromSortedContainer(const Container& container).
> 
> I added a unit test for KFileItemModel, which revealed a problem with my first attempt to fix it, which is attached to the bug report (the first fix worked only if the last two items in the list were not duplicates - in that case, it caused an inifinite loop and let the memory usage grow indefinitely).
> 
> Therefore, I added another unit test, which tests the function fromSortedContainer with quite a few different data sets.
> 
> Note that with the change in fromSortedContainer, we would still get a crash in KFileItemModel::index(const KUrl&) when filtering the view. This is because the number of items in m_items and m_itemData can never be the same (as it should usually be when all URLs have been added to the hash m_items) if some URLs are reported multiple times by the kioslave. Therefore, I removed the assert. I kind of liked it (it helped to analyze https://bugs.kde.org/show_bug.cgi?id=332102 ), but we should not risk a crash if a kioslave misbehaves.
> 
> In the hope that it will make analyzing bugs like https://bugs.kde.org/show_bug.cgi?id=332102 easier, I replaced the assert with some debug output that tells us about the corrupt state of the model. I added a static bool variable that is set to false as soon as the debug output has been printed once, and then disables all further debug output from this function. This prevents that the Konsole or .xsession-errors are flooded with output if the function is called many times.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kfileitemmodel.cpp de3c3eb 
>   dolphin/src/kitemviews/kitemrange.h 70927b9 
>   dolphin/src/tests/CMakeLists.txt 4ba68dd 
>   dolphin/src/tests/kfileitemmodeltest.cpp 48e72e8 
>   dolphin/src/tests/kitemrangetest.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/118507/diff/
> 
> 
> Testing
> -------
> 
> I cannot reproduce the crash any more. Old and new unit tests pass. Filtering "man:" does not cause a crash any more (note to myself: maybe I should add a unit test for the "filter view which contains one URL multiple times" issue too).
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20140604/f7bff77c/attachment.htm>