Drop Qt::DirectConnection in Job connections
Closed, ResolvedPublic

Description

It seems that removing the Qt::DirectConnection from Job::connectToArchiveInterfaceSignals() fixes the crash. For a rationale, see the comments by David in: https://git.reviewboard.kde.org/r/128113/

In particular:

I see what the problem is.

You're mixing KJob and QThread, which is only fine if none of the KJob API is called from the secondary thread. I.e. the KJob should only sit on top of the thread and report its progress (e.g. via signals emitted by the thread). This is almost what you're doing, except for this:

connect(archiveInterface(), &ReadOnlyArchiveInterface::finished, this, &Job::onFinished, Qt::DirectConnection);

Due to the DirectConnection, this will call onFinished, and therefore KJob::emitResult, from the secondary thread. This leads to race conditions inside KJob, and it leads to deletion of the job happening in the wrong thread.
This should definitely not be a direct connection.

This would also fix the race on m_isRunning that your code currently has (read/written in main thread (isRunning() and start()), but set to true in secondary thread in emitResult()).

TODO:

  • Check whether this does not re-open bugs 193908 and 222392

Details

Differential Revisions
D2225: Jobs: drop direct connection

Related Objects

Unfortunately, removing the Qt::DirectConnection makes Ark 16.04 hang if we close it while loading a huge zip file. Huge tar archives are fine though, yet another proof that a QProcess run in a QThread was really a bad idea.

However, we should still consider to remove the Qt::DirectConnection on master.

elvisangelaccio renamed this task from Fix bug 362977 on 16.04 to Drop Qt::DirectConnection in Job connections.Jul 3 2016, 3:42 PM
elvisangelaccio lowered the priority of this task from High to Normal.
elvisangelaccio updated the task description. (Show Details)
elvisangelaccio moved this task from In progress to Backlog on the Ark board.
elvisangelaccio moved this task from Backlog to In progress on the Ark board.Jul 16 2016, 10:33 AM
elvisangelaccio raised the priority of this task from Normal to High.
elvisangelaccio moved this task from In progress to Under review on the Ark board.Jul 19 2016, 4:43 PM
elvisangelaccio moved this task from Under review to Done on the Ark board.Jul 19 2016, 8:46 PM
elvisangelaccio closed this task as Resolved.Aug 16 2016, 5:15 PM