Review Request 127866: Oxygen: Fix QCache usage

[Hugo Pereira Da Costa]

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



To be honest, I am quite puzzle by this whole thing.
Now, every insertion in the cache requires at least two searches in there and (in many case) at least one copy constructor being called. This is quite expansive ... (even though this happens only if the object is not found in the cache).

Also: not sure I understand what issue we are trying to fix and how: why is it that if the object inserted in the cache is immediately deleted, just retrying an indefinite amount of time will "fix" the issue. Are we not just transforming a crash into a freeze (infinite loop) ? 

The Qt documentation is very vague about cases where the object is deleted immediately, and the only case it mentions is: " In particular, if cost is greater than maxCost(), the object will be deleted immediately."
Well, in such cases (that should not appear here), the infinite loop will not help. Right ? 
Since we have no idea on how "predictible" the other deletion cases are, I don't think the fix is a good fix. 

Does that mean that we should change the code in order to use references rather than pointer everywhere ? (as you did in the first patch on this topic) ? 

Or get rid of using QCache (because this absence of guarantee at the insertion stage is too much of a pain to handle) ? 

Or just commit and wait for bug reports about freezes ? (but with a happy coverty) ?

- Hugo Pereira Da Costa


On May 8, 2016, 5:03 a.m., Michael Pyne wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127866/
> -----------------------------------------------------------
> 
> (Updated May 8, 2016, 5:03 a.m.)
> 
> 
> Review request for kde-workspace and Hugo Pereira Da Costa.
> 
> 
> 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
> -----
> 
>   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/20160515/fc68503a/attachment.htm>