D13627: [KIconThemes] Isolate private data from race conditions

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  You fixed the small details I found, but the big picture is still very wrong.
  Using the same KIconLoader in multiple threads cannot work and doesn't make sense: one thread could addAppDir(), or reinit, messing up results for another thread. 
  Use a different KIconLoader per thread, and then the only thing you have to make threadsafe is any use of a global object, but not the kiconloader itself, which CANNOT be made threadsafe given its API (theme() is a perfect example).

INLINE COMMENTS

> kiconeffect.cpp:74
>  
> +KIconEffect& KIconEffect::operator=(const KIconEffect &other)
> +{

unrelated to this patch, right?

> kiconeffect.cpp:76
> +{
> +    memcpy(d, other.d, sizeof(*d));
> +    return *this;

warning, very wrong if there's a 'q' pointer...

> kiconloader.cpp:1327
> +{
> +    QPixmap pixmap = S_D(d)->loadMimeTypeIcon(_iconName, group, size, state, overlays, path_store);
> +    return pixmap;

And now such things could be written in a much more standard and readable way with a QMutexLocker at the beginning of the method, rather then the S_D macro and proxy hack.

> kiconloader.cpp:1655
>  {
> -    d->initIconThemes();
> -    if (d->mpThemeRoot) {
> -        return d->mpThemeRoot->theme;
> +    KIconTheme *theme = S_D(d)->theme();
> +    return theme;

This is not threadsafe, it's wishful-thinking-threadsafe.
Sure, the call to theme() is protected from interference from other threads, but if another thread can recreate the themes, then the pointer you got out of this method call is now invalid.

So I'll say it again: use a different KIconLoader instance in every thread, MUCH simpler, MUCH safer.

> kiconloader.h:482
> +#ifndef KICONTHEMES_NO_DEPRECATED
> +    KICONTHEMES_DEPRECATED KIconEffect *iconEffect() const;
> +#endif

unrelated to this patch, please split this out

REPOSITORY
  R302 KIconThemes

REVISION DETAIL
  https://phabricator.kde.org/D13627

To: anthonyfieroni, davidedmundson, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180622/223c75c3/attachment.html>