Don't remove a SubJob pointer if it is not in the list of SubJobs.
Details
Details
- Reviewers
dfaure anthonyfieroni - Group Reviewers
Frameworks - Commits
- R244:5da7248517bc: Input validation of SubJobs
Diff Detail
Diff Detail
- Repository
- R244 KCoreAddons
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
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)
Comment Actions
- Improved input validation for SubJobs
- Added the changes suggested by anthony
- I can't reproduce now bug 364039 even without a patched kio
Comment Actions
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?
Comment Actions
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();
} | |
Comment Actions
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. | |
| 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) | |
| 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. | |
| src/lib/jobs/kcompositejob.cpp | ||
|---|---|---|
| 108 | Yes, i saw in the code but i understand it a bit strange. | |
| 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? | |
Comment Actions
- 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.
| src/lib/jobs/kcompositejob.cpp | ||
|---|---|---|
| 98 | This is not needed anymore. | |