dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
This is the wrong approach for thread safety (wrong level of locking). It
should come with a multithreaded unittest so it can be checked for safety (with
clang+tsan for instance). And yes the concept overall seems wrong anyway as
QIcon isn't threadsafe in the first place, AFAIK.
A separate KIconLoader instance, used only to fetch paths and not icons,
could possibly be used in a secondary thread, but only that. In which case the
fix would look very different from this.
INLINE COMMENTS
> kiconloader.cpp:616
> + S_D(d)->mIconCache->clear();
> + S_D(d)->clear();
> + S_D(d)->init(_appname, extraSearchPaths);
This locks and unlocks three times in a row, which is not only slightly
inefficient, it also means that another thread could interleave calls in
between, and get completely inconsistent state.
> kiconloader.cpp:1829
> + auto it = S_D(d)->mIconAvailability.constFind(name);
> + const auto end = S_D(d)->mIconAvailability.constEnd();
> + if (it != end && !it.value() && !S_D(d)->shouldCheckForUnknownIcons()) {
similar here, if another thread changes mIconAvailability between the previous
line and this one, you'll end up comparing iterators for a different container,
that will never work
REPOSITORY
R302 KIconThemes
REVISION DETAIL
https://phabricator.kde.org/D13627
To: anthonyfieroni, davidedmundson, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
