Review Request 127866: Oxygen: Fix QCache usage

[Michael Pyne]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127866/
-----------------------------------------------------------

(Updated May 22, 2016, 4:20 a.m.)


Review request for kde-workspace and Hugo Pereira Da Costa.


Changes
-------

Updates patch based on much of the feedback, with major reverts to the way I approached TileSets and QColor in particular. For the TileSets I went ahead and implemented what I was talking about regarding a simple FIFO-based cache that holds shared pointers. I used QSharedPointer<> for this since it doesn't require subclassing from QSharedData -- but if this is the only part of the code that uses TileSet it might make sense to subclass from QSharedData instead.

Although it builds, installs, makes it through oxygen-demo5 benchmark and all the rest, I'm not sure if the return value changes for TileSet are ABI-safe (do we track public API for this part of Oxygen? Does anything in a different library or application link to this?).

On the other hand, if we can change return value, we should also be able to do that for QColor which will significantly improve that portion of the code, as right now the code doesn't 'cache' QColor at all anymore, we just dump them into a QMap that stays alive throughout the process lifetime.

After reviewing the QCache sources I'm pretty sure this is all actually only a problem if you try to ::insert() into QCache with a cost > maxCost -- but we have codepaths in Oxygen that appear to lead to either reducing maxCost or disabling the cache entirely so I can understand why Coverity would be wary. But as long as we've convinced that we're not ever inserting entries into a cache with an invalid cost, I can also just flag those Coverity entries to be ignored if that would be easier, and then drop this RR.


Repository: oxygen


Description
-------

This should mostly complete the QCache fixes I kicked off in a previous RR, 127837. Hugo noted there were many other similar usages, and boy he wasn't kidding! ;) The long story short is that these usages can theoretically cause use-after-free behavior (which can lead to crashes and even undefined behavior if the compiler ever gets smart).

*NOTE* It is -much- easier to review if you download the diff to your git repository for oxygen and then run "git diff -b" to ignore whitespace changes, particularly for the QPixmap changes.

For QPixmaps we return values instead of pointers, so we simply make a separate copy to be cached when we do insert. For QColor we return references to values so we *must* return pointers, and those have to be owned by a QCache to avoid memleaks. So I added a helper function to loop until the cache accepts the new entry. TileSets are a similar concern, except those have manual loops since I was uncertain about whether TileSet's copy constructor was the best idea or not.

This fixes a ton of Coverity issues (59717 - 259733, 259739, 259742 - 259752, 1336154, 1336155) and might be associated with Qt bug 38142 and KDE bug 219055 (which doesn't actually appear to be a dupe of a different bug to me...).


Diffs (updated)
-----

  kdecoration/oxygendecohelper.cpp aa75eca 
  kstyle/oxygenstyle.cpp e428606 
  kstyle/oxygenstylehelper.h 9510a60 
  kstyle/oxygenstylehelper.cpp 612ba37 
  liboxygen/oxygenhelper.h a6453a0 
  liboxygen/oxygenhelper.cpp 4843604 
  liboxygen/oxygenshadowcache.cpp 907e586 

Diff: https://git.reviewboard.kde.org/r/127866/diff/


Testing
-------

Compiled without warnings, installed and ran `oxygen-demo5 -style oxygen`. Used the GUI Benchmark feature to automatically cycle through all the listed features -- no crashes or obvious rendering errors.


Thanks,

Michael Pyne

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20160522/2aec148d/attachment.htm>