Input validation of SubJobs
ClosedPublic

Authored by jtamate on Nov 26 2017, 8:51 AM.

Details

Summary

Don't remove a SubJob pointer if it is not in the list of SubJobs.

Diff Detail

Repository
R244 KCoreAddons
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
jtamate created this revision.Nov 26 2017, 8:51 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 26 2017, 8:51 AM
jtamate requested review of this revision.Nov 26 2017, 8:51 AM
dfaure accepted this revision.Nov 26 2017, 9:32 AM
This revision is now accepted and ready to land.Nov 26 2017, 9:32 AM

Ah wait this could be made faster to avoid the double lookup.

if (d->subjobs.removeAll(job) > 0) {
    job->setParent(nullptr);
    return true;
}
return false;

(and then we don't even need the job==nullptr check, that's included in the removeAll check)

dfaure requested changes to this revision.Nov 26 2017, 9:34 AM
This revision now requires changes to proceed.Nov 26 2017, 9:34 AM
jtamate updated this revision to Diff 22947.Nov 26 2017, 10:23 AM
  • Improved input validation for SubJobs by dfaure
anthonyfieroni requested changes to this revision.Nov 26 2017, 10:30 AM
anthonyfieroni added a subscriber: anthonyfieroni.
anthonyfieroni added inline comments.
src/lib/jobs/kcompositejob.cpp
73

We don't want to be notified about this job too

job->disconnect(this);
89–91

Here same as line:68

104

I agree that we set first error but we shouldn't emit if job queue isn't empty.

108

Here after remove last job it should emit no?

This revision now requires changes to proceed.Nov 26 2017, 10:30 AM
jtamate updated this revision to Diff 22948.Nov 26 2017, 11:32 AM
  • Improved input validation for SubJobs
  • Added the changes suggested by anthony
  • I can't reproduce now bug 364039 even without a patched kio
jtamate marked 4 inline comments as done.Nov 26 2017, 11:47 AM

I've missed to do:
git add -u
git commit
before doing the last arc diff
If I do arc land (when the ship it! comes), will it do the right thing?

  • I can't reproduce now bug 364039 even without a patched kio

But to be correct as needed KIO' one should land too when David accept it.

src/lib/jobs/kcompositejob.cpp
109
if (d->subjobs.isEmpty()) {
    emitResult();
}
dfaure requested changes to this revision.Nov 26 2017, 5:28 PM

Just do another arc diff, if your uploaded patch isn't the latest one.

src/lib/jobs/kcompositejob.cpp
108

No no no this is all wrong, please don't change the logic here.
After a subjob is done, we might want to start another one.
Most KIO jobs work like that.

This revision now requires changes to proceed.Nov 26 2017, 5:28 PM
anthonyfieroni added inline comments.Nov 26 2017, 6:33 PM
src/lib/jobs/kcompositejob.cpp
74

I rethink it, sorry. Let's remove only own connections.

disconnect(job, &KJob::result, this, &KCompositeJob::slotResult);
disconnect(job, &KJob::infoMessage, this, &KCompositeJob::slotInfoMessage);

in two places.

108

Yeah, you are right, but we ever emit on error? It's finish whole composite job (deleteLater will be called)

dfaure added inline comments.Nov 26 2017, 7:30 PM
src/lib/jobs/kcompositejob.cpp
108

When a subclass calls this slotResult implementation, they expect the behaviour of "finish with an error if the subjob had an error".

But NOT emitResult in the normal case (no error), since we might want to continue by creating another subjob.

I'm still missing a proper explanation of what's happening and what shouldn't happen.

anthonyfieroni added inline comments.Nov 26 2017, 7:37 PM
src/lib/jobs/kcompositejob.cpp
108

Yes, i saw in the code but i understand it a bit strange.

jtamate updated this revision to Diff 23046.Nov 27 2017, 7:02 PM
  • Addressed comments by dfaure and anthonyfieroni
anthonyfieroni added inline comments.Nov 27 2017, 7:40 PM
src/lib/jobs/kcompositejob.cpp
104

David mean to finish job on first error, i.e. to not change this behavior. Can you verify that without empty check here and in KIO bug is fixed?

jtamate updated this revision to Diff 23049.Nov 27 2017, 8:02 PM
  • Input validation of SubJobs and disconnect signals

    I can't reproduce the bug. Probably the job already deleted (the crash in the bug) and the signals not disconnected where the problem. It doesn't crash for me even without the kio patch.
anthonyfieroni accepted this revision.Nov 27 2017, 8:07 PM
anthonyfieroni added inline comments.
src/lib/jobs/kcompositejob.cpp
98

This is not needed anymore.

jtamate updated this revision to Diff 23097.Nov 28 2017, 6:17 PM
  • Input validation of SubJobs and disconnect signals
dfaure accepted this revision.Nov 29 2017, 6:46 AM
This revision is now accepted and ready to land.Nov 29 2017, 6:46 AM
This revision was automatically updated to reflect the committed changes.