Icon: solve threading issue on when the source is http
ClosedPublic

Authored by apol on Nov 7 2019, 2:26 AM.

Details

Summary

For starters, QQmlEngine::networkAccessManager is not reentrant and
would give
us a warning along the lines of:

QObject::QObject|QNetworkAccessManager::QNetworkAccessManager QObject:
Cannot create children for a parent that is in a different thread.
(Parent is QQmlApplicationEngine(0x5563bf8dcad0), parent's thread is
QThread(0x5563bf825f70), current thread is
QSGRenderThread(0x5563bfef7830)

That happens because Icon::findIcon is called from Icon::updatePaintNode
which is called from the render thread.

This patch changes it so the QNAM bits happen upon polish, so
downloading the image will be handled by the main thread and we will
only be grabbing it from an already prepared QImage.

Test Plan

I don't get the warning anymore and if I agressively scroll I don't
get crashes. This will potentially fix a bunch of bugs in Discover since we use
extensively Kirigami.Icon + http.

Diff Detail

Repository
R169 Kirigami
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18551
Build 18569: arc lint + arc unit
apol created this revision.Nov 7 2019, 2:26 AM
Restricted Application added a project: Kirigami. · View Herald TranscriptNov 7 2019, 2:26 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
apol requested review of this revision.Nov 7 2019, 2:26 AM
apol updated this revision to Diff 69372.Nov 7 2019, 2:33 AM

deadlock

As a reminder, please ensure that all QNetworkAccessManager code is built to force handling of redirects on - code that interacts with KDE.org and does not explicitly handle redirects is not supported by Sysadmin.

Can we just use queued connection, it'll avoid mutex weirdness.

apol added a comment.Nov 7 2019, 12:28 PM

Can we just use queued connection, it'll avoid mutex weirdness.

Well, no. In fact, m_source is being accessed too unguarded from the different threads.

Certainly doing all this inside updatePaintNode was wrong. So, concept wise ++

I don't understand the mutex, the main point of the patch is so that we only do network stuff on the main thread. During updatePaintNode the GUI thread is explicitly locked.

This patch changes it so the QNAM bits happen upon setSource

The other option is to do all the resolving of the final QImage inside ::updatePolish()
You get all the advantages of it happening only once per frame and all the advantages of keeping everything simple in the main thread.

It's what we do in Plasma::IconItem Plasma::SvgItem and since we ported to that it made everything cleaner, faster and safer.

apol updated this revision to Diff 69420.Nov 8 2019, 2:42 AM

Simplify the multithread situation with updatePolish

apol updated this revision to Diff 69421.Nov 8 2019, 2:43 AM

Update commit text

apol edited the summary of this revision. (Show Details)Nov 8 2019, 2:44 AM
apol updated this revision to Diff 69422.Nov 8 2019, 2:46 AM
apol edited the summary of this revision. (Show Details)

simplify connect

davidedmundson accepted this revision.Nov 18 2019, 9:33 PM
This revision is now accepted and ready to land.Nov 18 2019, 9:33 PM
This revision was automatically updated to reflect the committed changes.