D19170: Fix crash while moving files

hallas closed this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D19170 To: hallas, #frameworks, elvisangelaccio, dfaure Cc: cfeck, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D19170: Fix crash while moving files

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH fix_crash_while_moving_files (branched from master) REVISION DETAIL https://phabricator.kde.org/D19170 To: hallas, #frameworks, elvisangelaccio, dfaure Cc: cfeck, dhaumann,

D19170: Fix crash while moving files

hallas marked 4 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D19170 To: hallas, #frameworks, elvisangelaccio, dfaure Cc: cfeck, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D19170: Fix crash while moving files

hallas updated this revision to Diff 53032. hallas added a comment. Set job to nullptr before stopping job running in parallel REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19170?vs=52643=53032 BRANCH fix_crash_while_moving_files (branched from master)

D19170: Fix crash while moving files

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Thanks for testing the fix - and indeed the reverse case is useful to have too. INLINE COMMENTS > hallas wrote in filecopyjob.cpp:495 > Should we also set d->m_chmodJob to

D19170: Fix crash while moving files

hallas added inline comments. INLINE COMMENTS > filecopyjob.cpp:495 > +} else if (job == d->m_chmodJob) { > +if (d->m_delJob) { > +d->m_delJob->kill(Quietly); Should we also set d->m_chmodJob to nullptr here, just like the m_putJob handling branch? >

D19170: Fix crash while moving files

hallas updated this revision to Diff 52643. hallas added a comment. Updated fix. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19170?vs=52240=52643 BRANCH fix_crash_while_moving_files (branched from master) REVISION DETAIL

D19170: Fix crash while moving files

hallas added a comment. In D19170#418234 , @dfaure wrote: > I think it's an unwanted fact that the chmod job runs in parallel with the del job. > A "return" after creating the chmod job would fix all this. > > But of course it should be

D19170: Fix crash while moving files

dfaure added a comment. I think it's an unwanted fact that the chmod job runs in parallel with the del job. A "return" after creating the chmod job would fix all this. But of course it should be possible to have two concurrent subjobs (one on dest, one on src), this whole issue just

D19170: Fix crash while moving files

hallas added a comment. In D19170#417274 , @dfaure wrote: > Let's find out :-) > > Note that SlaveBase already has a warning in case a slave emits finished() or error() twice; on the other hand this might not be the issue here, it could be

D19170: Fix crash while moving files

dfaure added a comment. Let's find out :-) Note that SlaveBase already has a warning in case a slave emits finished() or error() twice; on the other hand this might not be the issue here, it could be the job itself being buggy (and then it would be in a single location). But yeah I'd

D19170: Fix crash while moving files

hallas added a comment. In D19170#417235 , @dfaure wrote: > Great analysis, thanks! Now this makes a lot more sense ;-) > > The thing that I don't get, is why the subjob would emit anything after result(). I.e. step 7 is not supposed to

D19170: Fix crash while moving files

dfaure added a comment. Great analysis, thanks! Now this makes a lot more sense ;-) The thing that I don't get, is why the subjob would emit anything after result(). I.e. step 7 is not supposed to happen, at all. cfeck: I think you're referring to warning(), which does pop up a

D19170: Fix crash while moving files

cfeck added a comment. That a nested event loop of an error dialog caused the crashes makes sense, because they were reported for alien drives (NTFS) or transfers with permission problems. What actual error dialogs did you get and are they reproducible? REPOSITORY R241 KIO REVISION

D19170: Fix crash while moving files

hallas edited the summary of this revision. hallas edited the test plan for this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D19170 To: hallas, #frameworks, elvisangelaccio, dfaure Cc: dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D19170: Fix crash while moving files

hallas updated this revision to Diff 52240. hallas added a comment. Implement a proper fix REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19170?vs=52108=52240 BRANCH fix_crash_while_moving_files (branched from master) REVISION DETAIL

D19170: Fix crash while moving files

hallas added a comment. I think I have finally found the root cause of this. The following events occurs: 1. A FileCopyJob is created and added as a SubJob to CopyJob 2. CopyJob::slotResult is notified 3. CopyJobPrivate::slotResultCopyingFiles is called because it is in state

D19170: Fix crash while moving files

hallas added a comment. In D19170#415760 , @dfaure wrote: > If some code is deleting this job from a slot connected to it, that code needs to be fixed. This hack isn't a fix, it will only create more problems.. > > E.g. if some other bad

D19170: Fix crash while moving files

hallas added a comment. In D19170#416120 , @hallas wrote: > In D19170#416067 , @hallas wrote: > > > In D19170#415760 , @dfaure wrote: > > > > > If some

D19170: Fix crash while moving files

hallas added a comment. In D19170#416067 , @hallas wrote: > In D19170#415760 , @dfaure wrote: > > > If some code is deleting this job from a slot connected to it, that code needs to be fixed. This

D19170: Fix crash while moving files

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. If some code is deleting this job from a slot connected to it, that code needs to be fixed. This hack isn't a fix, it will only create more problems.. E.g. if some other bad

D19170: Fix crash while moving files

dhaumann added a reviewer: dfaure. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D19170 To: hallas, #frameworks, elvisangelaccio, dfaure Cc: dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D19170: Fix crash while moving files

dhaumann added inline comments. INLINE COMMENTS > kjob.cpp:105 > > +if (isAutoDelete()) { > +deleteLater(); Alternatively to moving the code, you could also use a `QPointer that(this);` to monitor whether the QObject is still existing. Then, you'd have to call if (that &&

D19170: Fix crash while moving files

hallas created this revision. hallas added reviewers: Frameworks, elvisangelaccio. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. hallas requested review of this revision. REVISION SUMMARY Fix crash while moving files. The backtrace points towards the KJob