[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: Review Request 127866: Oxygen: Fix QCache usage
From: Michael Pyne <mpyne () kde ! org>
Date: 2016-05-30 15:17:10
Message-ID: 20160530151710.29452.59460 () mimi ! kde ! org
[Download RAW message or body]
--===============5559554249179650076==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
> On May 30, 2016, 11:34 a.m., Hugo Pereira Da Costa wrote:
> > Ship It!
>
> Hugo Pereira Da Costa wrote:
> err. Wait ...
> There are rendering issues here once the patch is applied.
> See http://wstaw.org/m/2016/05/30/plasma-desktopY12228.png
> (left is "before", right is "after").
> So something seems wrong with the background gradient.
> I'll investigate a bit ...
>
> Hugo Pereira Da Costa wrote:
> Hi again,
> So, thinking more about it, and actually answering the questions raised in the \
> review:
> - do we track public API for this part of Oxygen? Does anything in a different \
> library or application link to this? No we don't this is supposed to be an \
> "internal" (as in private) library, used only by oxygen style and decoration. No \
> ABI/API guarantee.
> - QColor: can we change the return type. I would say yes, (from reference to \
> value), and return to using QCache
> - Tilesets: I would be inclined to changing the return type here too, using values, \
> and assuming the copy constructor does not cost much, based on the \
> implicit-shareness nature of pixmaps, and keep a built-in QCache here (which should \
> be more efficient than the (otherwise nice) FIFO.
> Now I understand that this is a lot of going back and forth, and a job I should \
> rather do myself, as the maintainer of the code. So I would propose to "postpone" \
> this RR for now, and for me to locally
> - take the QPixmap change from this patch
> - implement something similar for QColor and TileSet
> - test.
> Then if I manage to do that in a reasonable time (e.g. this week), drop the review \
> and commit my change instead (with proper credits where due). Otherwise (because of \
> me being too busy with other stuff), just commit this review (once the problem \
> mentionned above is fixed, though I could not investigate yet).
> What do you think ?
>
> (also: I need to sanitize this run-time changing of the cache use and max-cost)
Hugo,
That all sounds fair. And if it's all too much difficulty, then we might be able to \
lean on the hinted-at-but-not-quite-documented QCache behavior that ::insert() only \
fails if the item being inserted has a cost higher than maxCost. In that case I could \
ignore the Coverity issues (since Coverity can't statically prove that items are \
always < maxCost) and then drop the RR (and perhaps ask Qt to document more \
stringently that guarantee for QCache::insert).
Still though, I think the QColor return type change would make sense even independent \
of this RR.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127866/#review96029
-----------------------------------------------------------
On May 22, 2016, 4:20 a.m., Michael Pyne wrote:
>
> -----------------------------------------------------------
> 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.
>
>
> 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
> -----
>
> 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
>
>
--===============5559554249179650076==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 7bit
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 \
solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> \
<tr> <td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/127866/">https://git.reviewboard.kde.org/r/127866/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On May 30th, 2016, 11:34 a.m. UTC, <b>Hugo Pereira \
Da Costa</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Ship It!</pre> </blockquote>
<p>On May 30th, 2016, 11:39 a.m. UTC, <b>Hugo Pereira Da Costa</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">err. \
Wait ... There are rendering issues here once the patch is applied.
See http://wstaw.org/m/2016/05/30/plasma-desktopY12228.png
(left is "before", right is "after").
So something seems wrong with the background gradient.
I'll investigate a bit ...</p></pre>
</blockquote>
<p>On May 30th, 2016, 12:39 p.m. UTC, <b>Hugo Pereira Da Costa</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Hi \
again, So, thinking more about it, and actually answering the questions raised in the \
review:</p> <ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 \
1em;line-height: inherit;white-space: normal;"> <li style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: normal;"> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">do we \
track public API for this part of Oxygen? Does anything in a different library or \
application link to this? No we don't this is supposed to be an "internal" (as in \
private) library, used only by oxygen style and decoration. No ABI/API guarantee. \
</p> </li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: normal;"> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">QColor: can we change the return type. \
I would say yes, (from reference to value), and return to using QCache</p> </li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: normal;">Tilesets: I would be inclined to changing the return \
type here too, using values, and assuming the copy constructor does not cost much, \
based on the implicit-shareness nature of pixmaps, and keep a built-in QCache here \
(which should be more efficient than the (otherwise nice) FIFO. </li> </ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Now I understand that this is a lot of going back and \
forth, and a job I should rather do myself, as the maintainer of the code. So I would \
propose to "postpone" this RR for now, and for me to locally
- take the QPixmap change from this patch
- implement something similar for QColor and TileSet
- test.
Then if I manage to do that in a reasonable time (e.g. this week), drop the review \
and commit my change instead (with proper credits where due). Otherwise (because of \
me being too busy with other stuff), just commit this review (once the problem \
mentionned above is fixed, though I could not investigate yet).</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">What do you think ? </p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">(also: I need to \
sanitize this run-time changing of the cache use and max-cost)</p></pre> \
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Hugo,
That all sounds fair. And if it's all too much difficulty, then we might be able \
to lean on the hinted-at-but-not-quite-documented QCache behavior that ::insert() \
only fails if the item being inserted has a cost higher than maxCost. In that case I \
could ignore the Coverity issues (since Coverity can't statically prove that \
items are always < maxCost) and then drop the RR (and perhaps ask Qt to document \
more stringently that guarantee for QCache::insert).
Still though, I think the QColor return type change would make sense even independent \
of this RR.</pre> <br />
<p>- Michael</p>
<br />
<p>On May 22nd, 2016, 4:20 a.m. UTC, Michael Pyne wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: \
1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; \
-webkit-border-radius: 6px;"> <tr>
<td>
<div>Review request for kde-workspace and Hugo Pereira Da Costa.</div>
<div>By Michael Pyne.</div>
<p style="color: grey;"><i>Updated May 22, 2016, 4:20 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
oxygen
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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).</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;"><em style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
normal;">NOTE</em> 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.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">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 <em style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
normal;">must</em> 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.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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...).</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Compiled without warnings, installed and ran <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">oxygen-demo5 -style oxygen</code>. Used the GUI Benchmark \
feature to automatically cycle through all the listed features -- no crashes or \
obvious rendering errors.</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kdecoration/oxygendecohelper.cpp <span style="color: grey">(aa75eca)</span></li>
<li>kstyle/oxygenstyle.cpp <span style="color: grey">(e428606)</span></li>
<li>kstyle/oxygenstylehelper.h <span style="color: grey">(9510a60)</span></li>
<li>kstyle/oxygenstylehelper.cpp <span style="color: grey">(612ba37)</span></li>
<li>liboxygen/oxygenhelper.h <span style="color: grey">(a6453a0)</span></li>
<li>liboxygen/oxygenhelper.cpp <span style="color: grey">(4843604)</span></li>
<li>liboxygen/oxygenshadowcache.cpp <span style="color: grey">(907e586)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127866/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============5559554249179650076==--
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic