D9004: Input validation of SubJobs
This revision was automatically updated to reflect the committed changes. Closed by commit R244:5da7248517bc: Input validation of SubJobs (authored by jtamate). REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9004?vs=23097&id=23183 REVISION DETAIL https://phabricator.kde.org/D9004 AFFECTED FILES src/lib/jobs/kcompositejob.cpp To: jtamate, #frameworks, dfaure, anthonyfieroni Cc: anthonyfieroni
D9004: Input validation of SubJobs
dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R244 KCoreAddons BRANCH master REVISION DETAIL https://phabricator.kde.org/D9004 To: jtamate, #frameworks, dfaure, anthonyfieroni Cc: anthonyfieroni
D9004: Input validation of SubJobs
jtamate updated this revision to Diff 23097. jtamate added a comment. - Input validation of SubJobs and disconnect signals REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9004?vs=23049&id=23097 BRANCH master REVISION DETAIL https://phabricator.kde.org/D9004 AFFECTED FILES src/lib/jobs/kcompositejob.cpp To: jtamate, #frameworks, dfaure, anthonyfieroni Cc: anthonyfieroni
D9004: Input validation of SubJobs
anthonyfieroni accepted this revision.
anthonyfieroni added inline comments.
INLINE COMMENTS
> kcompositejob.cpp:98
> {
> +Q_D(KCompositeJob);
> // Did job have an error ?
This is not needed anymore.
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D9004
To: jtamate, #frameworks, dfaure, anthonyfieroni
Cc: anthonyfieroni
D9004: Input validation of SubJobs
jtamate updated this revision to Diff 23049. jtamate added a comment. - 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. REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9004?vs=23046&id=23049 BRANCH master REVISION DETAIL https://phabricator.kde.org/D9004 AFFECTED FILES src/lib/jobs/kcompositejob.cpp To: jtamate, #frameworks, dfaure, anthonyfieroni Cc: anthonyfieroni
D9004: Input validation of SubJobs
anthonyfieroni added inline comments.
INLINE COMMENTS
> kcompositejob.cpp:104
> setErrorText(job->errorText());
> -emitResult();
> +// Finish this KCompositeJob only if it has no subjobs
> +if (d->subjobs.isEmpty()) {
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?
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D9004
To: jtamate, #frameworks, dfaure, anthonyfieroni
Cc: anthonyfieroni
D9004: Input validation of SubJobs
jtamate updated this revision to Diff 23046. jtamate added a comment. - Addressed comments by dfaure and anthonyfieroni REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9004?vs=22948&id=23046 BRANCH master REVISION DETAIL https://phabricator.kde.org/D9004 AFFECTED FILES src/lib/jobs/kcompositejob.cpp To: jtamate, #frameworks, dfaure, anthonyfieroni Cc: anthonyfieroni
D9004: Input validation of SubJobs
anthonyfieroni added inline comments. INLINE COMMENTS > dfaure wrote in kcompositejob.cpp:102 > 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. Yes, i saw in the code but i understand it a bit strange. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D9004 To: jtamate, #frameworks, dfaure, anthonyfieroni Cc: anthonyfieroni
D9004: Input validation of SubJobs
dfaure added inline comments. INLINE COMMENTS > anthonyfieroni wrote in kcompositejob.cpp:102 > Yeah, you are right, but we ever emit on error? It's finish whole composite > job (deleteLater will be called) 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. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D9004 To: jtamate, #frameworks, dfaure, anthonyfieroni Cc: anthonyfieroni
D9004: Input validation of SubJobs
anthonyfieroni added inline comments. INLINE COMMENTS > dfaure wrote in kcompositejob.cpp:102 > 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. Yeah, you are right, but we ever emit on error? It's finish whole composite job (deleteLater will be called) > kcompositejob.cpp:68 > +job->setParent(nullptr); > +job->disconnect(this); > +return true; 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. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D9004 To: jtamate, #frameworks, dfaure, anthonyfieroni Cc: anthonyfieroni
D9004: Input validation of SubJobs
dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Just do another arc diff, if your uploaded patch isn't the latest one. INLINE COMMENTS > anthonyfieroni wrote in kcompositejob.cpp:102 > Here after remove last job it should emit no? 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. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D9004 To: jtamate, #frameworks, dfaure, anthonyfieroni Cc: anthonyfieroni
D9004: Input validation of SubJobs
anthonyfieroni added a comment.
In https://phabricator.kde.org/D9004#172096, @jtamate wrote:
> - 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.
INLINE COMMENTS
> kcompositejob.cpp:104
> removeSubjob(job);
> +emitResult();
> }
if (d->subjobs.isEmpty()) {
emitResult();
}
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D9004
To: jtamate, #frameworks, dfaure, anthonyfieroni
Cc: anthonyfieroni
D9004: Input validation of SubJobs
jtamate marked 4 inline comments as done. jtamate added a comment. 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? REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D9004 To: jtamate, #frameworks, dfaure, anthonyfieroni Cc: anthonyfieroni
D9004: Input validation of SubJobs
jtamate updated this revision to Diff 22948. jtamate added a comment. - Improved input validation for SubJobs - Added the changes suggested by anthony - I can't reproduce now bug 364039 even without a patched kio REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9004?vs=22947&id=22948 BRANCH master REVISION DETAIL https://phabricator.kde.org/D9004 AFFECTED FILES src/lib/jobs/kcompositejob.cpp To: jtamate, #frameworks, dfaure, anthonyfieroni Cc: anthonyfieroni
D9004: Input validation of SubJobs
anthonyfieroni requested changes to this revision.
anthonyfieroni added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kcompositejob.cpp:67
> +if (d->subjobs.removeAll(job) > 0) {
> +job->setParent(nullptr);
> +return true;
We don't want to be notified about this job too
job->disconnect(this);
> kcompositejob.cpp:87
> Q_FOREACH (KJob *job, d->subjobs) {
> job->setParent(nullptr);
> }
Here same as line:68
> kcompositejob.cpp:99
> setErrorText(job->errorText());
> emitResult();
> }
I agree that we set first error but we shouldn't emit if job queue isn't empty.
> kcompositejob.cpp:102
>
> removeSubjob(job);
> }
Here after remove last job it should emit no?
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D9004
To: jtamate, #frameworks, dfaure, anthonyfieroni
Cc: anthonyfieroni
D9004: Input validation of SubJobs
jtamate updated this revision to Diff 22947. jtamate added a comment. - Improved input validation for SubJobs by dfaure REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9004?vs=22943&id=22947 BRANCH master REVISION DETAIL https://phabricator.kde.org/D9004 AFFECTED FILES src/lib/jobs/kcompositejob.cpp To: jtamate, #frameworks, dfaure
D9004: Input validation of SubJobs
dfaure added a comment.
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)
REPOSITORY
R244 KCoreAddons
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D9004
To: jtamate, #frameworks, dfaure
D9004: Input validation of SubJobs
dfaure requested changes to this revision. This revision now requires changes to proceed. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D9004 To: jtamate, #frameworks, dfaure
D9004: Input validation of SubJobs
dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R244 KCoreAddons BRANCH master REVISION DETAIL https://phabricator.kde.org/D9004 To: jtamate, #frameworks, dfaure
D9004: Input validation of SubJobs
jtamate created this revision. jtamate added reviewers: Frameworks, dfaure. Restricted Application added a project: Frameworks. REVISION SUMMARY Don't remove a SubJob pointer if it is not in the list of SubJobs. REPOSITORY R244 KCoreAddons BRANCH master REVISION DETAIL https://phabricator.kde.org/D9004 AFFECTED FILES src/lib/jobs/kcompositejob.cpp To: jtamate, #frameworks, dfaure
