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

Review Request #118055 - Created May 8, 2014 and submitted

Information
Frank Reininghaus
kde-baseapps
KDE/4.13
332102
Reviewers
dolphin
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.
Old and new unit tests pass. No regressions found so far.
Christoph Feck
Emmanuel Pescosta
Commit Hook
This review has been submitted with commit 4642301ea0f4b87a3da2e7f810af6154b29dd612 by Frank Reininghaus to branch KDE/4.13.
Frank Reininghaus
Review request changed

Status: Closed (submitted)

Loading...