Review Request 118055: Fix two bugs which could lead to a crash after collapsing a folder whose (direct or indirect) children were not listed completely yet

[Emmanuel Pescosta]

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

Ship it!


Sorry for the late reply, had a stressful week.

Thanks for this patch! Couldn't reproduce the crash anymore :)

Maybe we should also add an additional parameter (enumeration) to the 
KDirLister's stop() method, so that it automatically stops updating 
all subfolders when we pass the right enum value. So that we can remove
the stop calls for each collapsed subfolder.

If you agree with me, I'll create a patch for KF5/KIO.

- Emmanuel Pescosta


On May 8, 2014, 5:42 p.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118055/
> -----------------------------------------------------------
> 
> (Updated May 8, 2014, 5:42 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Bugs: 332102
>     http://bugs.kde.org/show_bug.cgi?id=332102
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> This patch fixes two different bugs that can happen when collapsing a folder:
> 
> 
> (a) When a folder is collapsed, there might be (direct or indirect) children in m_pendingItemsToInsert because KDirLister might not have finished listing the folder or one of its subfolders. If they remained in m_pendingItemsToInsert, they would be inserted into the model at some later point, but with an invalid pointer to a parent which does not exist any more. This could cause a crash.
> 
> The solution is to insert all items from m_pendingItemsToInsert immediately when collapsing any folder, such that they can then be removed, like all other children of the collapsed folder.
> 
> One thing to note is that this can in principle change the index of the collapsed folder (namely, if any items from m_pendingItemsToInsert were inserted before this folder). In that case, the index must be adjusted.
> 
> This crash and also the index adjustment are covered by a new unit test.
> 
> 
> (b) When a folder is collapsed, we tell KDirLister to stop listing it (via KDirLister::stop()). However, there might be children of the folder which are still being listed. These children are removed from the model, but without this patch, KDirLister would still list them and possibly emit items for them at some later point. These would then be inserted into the model as top-level items (because their parent could not be found any more). This inconsistent model state could cause a crash later on (e.g., when the collapsed folder was re-expanded).
> 
> This issue is not easily unit-testable. Maybe somthing to keep in mind for KF5. One could either add a method to KDirLister which returns the listed URLs (and then verify in a unit test that KFileItemModel::setExpanded(int, bool) really does tell KDirLister to stop listing any removed URLs), or maybe even better, implement some kind of "unittest:/" kioslave which can be customized to trigger any behavior that is related to a fixed bug.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kfileitemmodel.cpp a0f9305c 
>   dolphin/src/tests/kfileitemmodeltest.cpp 99ee336 
> 
> Diff: https://git.reviewboard.kde.org/r/118055/diff/
> 
> 
> Testing
> -------
> 
> Old and new unit tests pass. No regressions found so far.
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

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