KCompositeJob: connect slotResult to finished, not result
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.