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

[Frank Reininghaus]

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

(Updated June 4, 2014, 7:55 p.m.)


Status
------

This change has been marked as submitted.


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/e1dce4de/attachment.htm>