Skip to content

KCompositeJob: connect slotResult to finished, not result

Igor Kushnir requested to merge work/kcompositejob-kjob-finished-not-result into master

Quote from KJob::finished signal documentation:

if you store a list of jobs and they might get killed silently, then you must connect to this instead of result(), to avoid dangling pointers in your list.

If a subjob is killed quietly or destroyed, it does not emit KJob::result, and KCompositeJob (as well as its subclass that overrides slotResult) is not notified. The composite job keeps waiting for this subjob to finish or tries to start it when a previous subjob finishes. The destroyed job remains a dangling pointer in the subjob list and eventually causes a segmentation fault unless KCompositeJob and its subclass(es) happen to not dereference it ever again.

An unfortunate consequence of this connection to the not-always-emitted KJob::result signal is that a job that may end up in a KCompositeJob's subjob list must never be killed quietly. For example, some jobs in KDevelop are killed with KillVerbosity==EmitResult only to work around this potential segfault. Inevitably, some segfault possibilities are missed and users report crashes. It is often difficult to figure out which subjob caused a crash or steps to reproduce, because the subjob may be accessed long after it becomes a dangling pointer (e.g. when a KCompositeJob's subclass itself is killed and tries to kill the seemingly still-running job). When the crash cause is finally identified, the usual fix looks more like a workaround: kill() => kill(EmitResult). And what if it is really preferable to kill the subjob quietly, because its result() signal is undesired for some reason?

When &KCompositeJob::slotResult is connected to &KJob::finished, there is no more need to worry if a particular job can possibly be added to a subjob list. The KillVerbosity can be picked based on application logic rather than on KCompositeJob's arbitrary requirement.

Some KCompositeJob subclasses have come to rely on slotResult not being called when a subjob is killed quietly (for example, this is an assumption in KDevelop's ExecuteCompositeJob::doKill()). Hopefully there are few such subclasses and they can be adjusted to the new KCompositeJob's behavior without too much effort.

Possibly related KDevelop crashes that may be fixed by this change: https://bugs.kde.org/buglist.cgi?quicksearch=ExecuteCompositeJob%3A%3AdoKill&list_id=1793250

kdevelop/kdevelop!167 (merged) is an example of a workaround that will not be necessary once KDevelop requires a KF version that contains this fix.

Merge request reports