KCoreDirLister: fix crash when creating new folders from kfilewidget
ClosedPublic

Authored by dfaure on Sep 11 2019, 12:45 PM.

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ahmadsamir created this revision.Sep 11 2019, 12:45 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSep 11 2019, 12:45 PM
ahmadsamir requested review of this revision.Sep 11 2019, 12:45 PM
dhaumann edited the summary of this revision. (Show Details)Sep 11 2019, 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)Sep 11 2019, 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)Sep 11 2019, 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.Sep 15 2019, 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

dfaure requested changes to this revision.Sep 15 2019, 12:22 PM

Sorry it took me a long time to review this. The commit didn't fully satisfy my need to understand the underlying problem, so I had to find the time to debug the issue (thanks for the unittest, it really helps with that).

AFAICS here's what happens:

  1. We create folder1 and immediately move the view into it
  2. So by the time itemsAddedInDirectory() (the DBus notification) is called for the parent directory, there is no KDirLister showing that directory anymore. Therefore this directory isn't listed again, it's just marked as dirty in the cache, for the next time the user will go into it (see checkUpdate where it says "not in use, marked dirty"). That's the usual optimization to avoid re-listing directories that are not shown anymore.
  3. Therefore the parent directory doesn't actually have a child item for folder1
  4. We then process the KDirWatch notification (processPendingUpdates) for the parent directory, which updates the mtime of that directory item. OK.
  5. Steps 1-3 happen again the same way for folder1/folder2
  6. Then, just like step 4, we process the KDirWatch notification for folder1, but it can't find that item in the parent directory due to step 2 above. ASSERT.

The heart of the issue is that findByUrl first looks into "child items of the parent dir" then, for subdirs, falls back to the "." of the subdir (so it actually works for folder1).
On the other hand, reinsert only supports child items, not "." items.
This design is certainly quite tricky because for subdirs, the same URL is normally represented those two different items (except in this unusual scenario which skipped creating a child item in the parent directory...).

OK, so how to fix this. A KDirWatch notification for a directory is about updating the mtime and/or permissions of that directory. But that's already done in handleFileDirty, as shown by bin/kdirlistertest testDirPermissionChange. A permission change for an item we're not showing (folder1, while in folder2) also works (handleFileDirty ends up in checkUpdate which marks it as dirty; tested manually).
So... overall we don't actually need to do a processPendingUpdates for a KDirWatch notification for a directory in cache. It was added in commit 9c1e5d56cdfb8 but testDirPermissionChange actually passes without it (at least nowadays). In addition, adding pending updates in updateDirectory called by processPendingUpdates seems very unclean to me (cyclic layering).
So my suggested fix is to actually remove some code :-)

I'll post a modified patch with my suggested changes.

Thanks for pushing me to debug this, it's .... not trivial.

autotests/kfilewidgettest.cpp
517

This test fails when being run twice in a row (without the fix), or if /tmp/folder1 already exists for some unrelated reason.

I suggest using QTemporaryDir instead of QDir::tempPath, so that the deletion of the QTemporaryDir cleans up in all cases (success or failure).

I did it locally to debug this, here it is:

QTemporaryDir tempDir;
QVERIFY(tempDir.isValid());
const QString dir = tempDir.path();
const QUrl url = QUrl::fromLocalFile(dir);

(and then you can remove the cleanup code at the end of the method)

523

show() and activateWindow() are not needed for this unittest to demonstrate the failure.

I'm trying to reduce the number of stuff popping up when running ctest, since that's rather annoying if I'm doing something else at the same time :-)

534

Not needed, I get the assert even without this line.

This revision now requires changes to proceed.Sep 15 2019, 12:22 PM
dfaure updated this revision to Diff 66116.Sep 15 2019, 12:24 PM

Simplify autotest; suggest a different fix which removes unnecessary updates and preserves the assert.

dfaure resigned from this revision.Sep 15 2019, 12:24 PM
dfaure commandeered this revision.
dfaure edited reviewers, added: ahmadsamir; removed: dfaure.

Thanks for the explanation; it filled all the, various, gaps in my reasoning.

Looks good to me. (I don't have a dev account, so probably phab.kde.org doesn't show the "accept revision" or similar in its web UI for me).

autotests/kfilewidgettest.cpp
517

Yes, it does fail if the dir got created and the test didn't finish running for whatever reason; I was thinking of CI (which will nuke the whole build and start over), rather than of other people running the test locally.

523

Yes, they're quiet sudden, and disappear too quickly too. But it won't matter for KIO tests, it's just one of many many dialogs :)

This revision was not accepted when it landed; it landed in state Needs Review.Sep 15 2019, 7:30 PM
This revision was automatically updated to reflect the committed changes.

Here, in the web-UI, it says authored by dfaure, as it should, but the commit still says authored by me.

Yeah I didn't change the git commit's author. It's partly your code, partly me deleting someone else's code :-)

@dfaure: D21197 can be closed / abandoned?

@dfaure: D21197 can be closed / abandoned?

Yes, but @chinmoyr has disappeared, it seems?