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.


  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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180621/984dc201/attachment-0001.html>