Differential D19168

Fix crash in Dolphin when dropping trashed file in trash
ClosedPublic
Actions

Authored by hallas on Feb 19 2019, 7:57 PM.

Details

Reviewers
elvisangelaccio
ngraham
dfaure
Group Reviewers
Frameworks
Commits
R241:f56af9e2ea0a: Fix crash in Dolphin when dropping trashed file in trash
Summary

Fix crash in Dolphin when dropping trashed file in trash. The actual
crash happens because of an assertion failure in kcoredirlister_p.h:308
and this is triggered from kcoredirlister.cpp:995. What actually happens
is that the dropjob determines that it should perform a move action
which ends up being a rename operation for kio_trash. But it ends up
moving the file to itself and this triggers the above crash. The
solution is to error out in the dropjob with a KIO::ERR_DROP_ON_ITSELF
error so that the user can see that it doesn't make sense to drop a
file from the trash inside the trash again.

BUG: 378051

Test Plan

Put a file in trash
Drag and drop the file to the trash

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.
hallas created this revision.Feb 19 2019, 7:57 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 19 2019, 7:57 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
hallas requested review of this revision.Feb 19 2019, 7:57 PM
dfaure requested changes to this revision.Feb 19 2019, 10:59 PM

Looks good, just some minor improvement suggestions for the unittest.

autotests/dropjobtest.cpp
361

Would be simpler with the PMF connect syntax ;)

QSignalSpy copyingDoneSpy(copyJob, &KIO::CopyJob::copyingDone);
376

If exec() returns true, there won't be an errorString. So this line can be simplified to

QVERIFY(!job->exec());
377

QVERIFY(a==b) should always be QCOMPARE(a, b) instead, to see both values in case of a failure.

This revision now requires changes to proceed.Feb 19 2019, 10:59 PM
hallas added a comment.Feb 20 2019, 8:46 AM
In D19168#415767, @dfaure wrote:

Looks good, just some minor improvement suggestions for the unittest.

Thanks for the suggestions - I am still quite new to using QtTest :) Essentially I copied one of the other unit tests and worked my way from there.

hallas updated this revision to Diff 52132.Feb 20 2019, 8:52 AM

Implemented review comments

hallas marked 3 inline comments as done.Feb 20 2019, 8:54 AM
hallas added a comment.Feb 24 2019, 5:24 PM

@dfaure - do you have any other comments for this commit?

dfaure accepted this revision.Feb 24 2019, 5:27 PM
This revision is now accepted and ready to land.Feb 24 2019, 5:27 PM
Closed by commit R241:f56af9e2ea0a: Fix crash in Dolphin when dropping trashed file in trash (authored by hallas). · Explain WhyFeb 25 2019, 10:08 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mautotests/dropjobtest.cpp (25 lines)
Msrc/widgets/dropjob.cpp (4 lines)

Diff 52509

View Options

autotests/dropjobtest.cpp

Loading...
View Options

src/widgets/dropjob.cpp

Loading...