KCoreDirLister: fix crash when creating new folders from kfilewidget
Needs ReviewPublic

Authored by ahmadsamir on Wed, Sep 11, 12:45 PM.

Details

Reviewers
dfaure
Group Reviewers
Frameworks
Summary

When creating multiple nested new folders, one at a time, in the "save as"
dialog, where folders are created and entered, a dirlister would hit an
assert (in DirItem::reinsert()), because one of the created folders would
eventually get inserted in pendingUpdates.

Add a unit test in kfilewidgettest.

BUG: 401916
FIXED-IN: 5.63.0

Test Plan
  • Open a file in e.g. okular, then "save as"
  • Create a new folder from the dialog, then another ... etc, usually 2-3 new folders would hit the assert (keep going, the number varies apparently) and the app would crash
  • Apply the patch then try again, it shouldn't crash

Diff Detail

Repository
R241 KIO
Branch
ahmad/kcoredirlister (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16564
Build 16582: arc lint + arc unit
ahmadsamir created this revision.Wed, Sep 11, 12:45 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptWed, Sep 11, 12:45 PM
ahmadsamir requested review of this revision.Wed, Sep 11, 12:45 PM
dhaumann edited the summary of this revision. (Show Details)Wed, Sep 11, 12:52 PM

Since this crash is in KCoreDirLister, I wonder whether we can find a unit test that reproduces this without the KFileWidget.

But in any case - thanks for having a look into this, since it will fixes a crash in many applications.

Bug https://bugs.kde.org/show_bug.cgi?id=401916 will also be fixed.

dhaumann edited the summary of this revision. (Show Details)Wed, Sep 11, 12:57 PM

A different patch was proposed with D21197 in May 2019.

By the way, this will likely also fix:

ngraham edited the summary of this revision. (Show Details)Wed, Sep 11, 1:48 PM

Since this crash is in KCoreDirLister, I wonder whether we can find a unit test that reproduces this without the KFileWidget.

I think the main issue here is kdirlister inserting items into pendingUpdates, then trying to use an invalid, end(), iterator in DirItem::reinsert(); so it's about the pace things happen:

  • with KFileWidget, the successive creating and entering of multiple nested folders puts things in pending updates; creating the nested folders in one go a/b/c/d (which is possible), doesn't cause the crash
  • "Theoretically" with dolphin or gwenview, copying folders to a pen drive where presumably the latter is slow, so kdirlister is inserting items in pendingUpdates, the same with ripping an audio cd (these are slow by design it seems) and lastly copying files to a smart phone via kde-connect.

A different patch was proposed with D21197 in May 2019.

I didn't know about D21197, technically it's the same patch; from the other patch, I could be wrong but:

(*it).url() == oldUrl

will always be true, so it's redundant.

A couple of stuff I found while I was stuck^Wworking on this issue, (sharing them here because better minds than mine could have a better fix):

  • It happens in slotResult() when the jobUrl is a child of an item dir in pendingUpdates
  • The parentDir in reinsert() has DirItem->lstItems.size() == 0

When creating a new folder in the "save as" dialog, I see this sequence:
slotFilesAdded() -> itemsAddedInDirectory() -> updateDirectory(), in the latter checkUpdate(dir) is always false.

Using dolphin to create a new folder, then enter it, then create a new folder inside... etc, it's the same sequence but checkUpdate(dir) is usually always true. That confirms my idea that it's about pace, things happening too fast, or too slow in the case of the pen drive / audio cd / smart phone, and kdirlister is guarding against being flooded by FAM events.

ahmadsamir updated this revision to Diff 66104.Sun, Sep 15, 8:43 AM
ahmadsamir edited the summary of this revision. (Show Details)
ahmadsamir edited the test plan for this revision. (Show Details)
ahmadsamir removed a reviewer: chinmoyr.

Refer to one bug report, the others are dupes