Re: Review Request 127866: Oxygen: Fix QCache usage
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127866/ --- (Updated Feb. 26, 2017, 12:28 a.m.) Status -- This change has been marked as submitted. 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
Re: Review Request 127866: Oxygen: Fix QCache usage
> 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) > > Michael Pyne wrote: > 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. > > Hugo Pereira Da Costa wrote: > Hi again Michael, > > So, I have a working implementation here that > - keeps using QCache wherever possible, passing QColor and TileSet as > values rather than as refs (in a way that is similar to the change you did > for QPixmaps) > - uses your FIFOCache for the implementation of Oxygen::Cache, which is a > "cache of caches", and thus cannot possibly use values. > > Seems to work (though I would like to test a bit more), and should fix > all the issues with Coverty. > Since I have blatently copied your code for the home-made FiFOCache, i > have added you as a copyright holder for oxygenhelper.h > is that ok with you ? Should i put your name in other places too ? > > Best, > > Hugo > > Michael Pyne wrote: > > Since I have blatently copied your code for the home-made FiFOCache, i > have added you as a copyright holder for oxygenhelper.h > is that ok with you ? Should i put your name in other places too ? > > That's perfectly fine. You don't need to mention me elsewhere, as long as > this RR is mentioned in the commit log I think that would cover any needed > attribution of work. > > Albert Astals Cid wrote: > What's the status of this, was commited? Needs further work? Should be > discarded? yes.Code was committed,review should be discarded - Hugo --- 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
Re: Review Request 127866: Oxygen: Fix QCache usage
> 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) > > Michael Pyne wrote: > 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. > > Hugo Pereira Da Costa wrote: > Hi again Michael, > > So, I have a working implementation here that > - keeps using QCache wherever possible, passing QColor and TileSet as > values rather than as refs (in a way that is similar to the change you did > for QPixmaps) > - uses your FIFOCache for the implementation of Oxygen::Cache, which is a > "cache of caches", and thus cannot possibly use values. > > Seems to work (though I would like to test a bit more), and should fix > all the issues with Coverty. > Since I have blatently copied your code for the home-made FiFOCache, i > have added you as a copyright holder for oxygenhelper.h > is that ok with you ? Should i put your name in other places too ? > > Best, > > Hugo > > Michael Pyne wrote: > > Since I have blatently copied your code for the home-made FiFOCache, i > have added you as a copyright holder for oxygenhelper.h > is that ok with you ? Should i put your name in other places too ? > > That's perfectly fine. You don't need to mention me elsewhere, as long as > this RR is mentioned in the commit log I think that would cover any needed > attribution of work. What's the status of this, was commited? Needs further work? Should be discarded? - Albert --- 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
Re: Review Request 127866: Oxygen: Fix QCache usage
> 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) > > Michael Pyne wrote: > 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. Hi again Michael, So, I have a working implementation here that - keeps using QCache wherever possible, passing QColor and TileSet as values rather than as refs (in a way that is similar to the change you did for QPixmaps) - uses your FIFOCache for the implementation of Oxygen::Cache, which is a "cache of caches", and thus cannot possibly use values. Seems to work (though I would like to test a bit more), and should fix all the issues with Coverty. Since I have blatently copied your code for the home-made FiFOCache, i have added you as a copyright holder for oxygenhelper.h is that ok with you ? Should i put your name in other places too ? Best, Hugo - Hugo --- 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,
Re: Review Request 127866: Oxygen: Fix QCache usage
> 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/
Re: Review Request 127866: Oxygen: Fix QCache usage
> 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 ... 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 --- 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 > >
Re: Review Request 127866: Oxygen: Fix QCache usage
> On May 30, 2016, 11:34 a.m., Hugo Pereira Da Costa wrote: > > Ship It! 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 --- 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 > >
Re: Review Request 127866: Oxygen: Fix QCache usage
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127866/#review96029 --- Ship it! Ship It! - Hugo Pereira Da Costa 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 > >
Re: Review Request 127866: Oxygen: Fix QCache usage
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127866/#review95728 --- +1 from me - looks much better without the while loops indeed :-) Do you know what the maximum cache limit that will be set with FIFOCache::setMaxCost is? I'm asking because FIFOCache::find is linear in the cache size. It might be a problem if this is potentially unlimited, but if it's never much more than the default value of 256, then it probabaly doesn't matter much, because the runtime cost of re-generating the cached values is probably much larger. FIFOCache looks like a simple solution to the problems that Coverity found with QCache (at least if the max cost is bounded) - nice! - Frank Reininghaus 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 > >
Re: Review Request 127866: Oxygen: Fix QCache usage
--- 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
Re: Review Request 127866: Oxygen: Fix QCache usage
> On May 15, 2016, 8:51 a.m., Hugo Pereira Da Costa wrote: > > 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) ? > > Michael Pyne wrote: > For the most part the requirements are determined by the current return > type within the code. If we return a pointer then currently it has to have > come directly out of the QCache. Since QCache is assumed to be the owner, the > calling code won't delete the pointer ; but if the caller won't delete the > pointer then we'll have a memory leak if we return a pointer to something > that had been new'd instead. > > References (i.e. QColor&) are a similar issue; it's safe enough to return > a reference to something held within the QCache, but we can't return a > reference to a local variable since that reference will invalidate as soon as > we return from the function. Of course a reference to a cached QColor may > *also* invalidate with the next call to an insert method of that cache, but > that's a separate story. > > It is unfortunate that the Qt docs are vague about this, since if the > **only** thing we had to worry about was cost being >maxCost(), we could > pretty much just mark 'ignore' for all the Coverity issues associated with > this (and I'd be fine doing that). The docs do kind of hint at that but don't > make it clear if is the only way that an entry would be deleted immediately. > > I think you're right that a loop is not a good idea... I was figuring > that eventually QCache would remove enough other items to make it work but > then I suppose QCache::insert() would have done that with the very first > attempt. > > As far as other options, I would definitely recommend against QCache for > the QColors: I'd say just hold onto specific QColors directly (perhaps in a > QHash) and, if possible, return them as values instead of references. > > I'm not sure if we could get away with the same for TileSets, but if so > it would again make things easier. If we can't we could look into making > TileSet an implicitly shared class so that we can return it by value cheaply. > > I wouldn't recommend a commit only to make Coverity happy. I've marked > other reports as "False Positive" and even "That's a bug, but we're ignoring > it" before. But it does seem to me that if a crash *is* possible (especially > in underlying library code) we should do something to avoid it. > > Either way I'll see if I can work up a revised patch but I'd still > appreciate advice on what's workable or not within Oxygen. > > Mark Gaiser wrote: > Disclaimer: i'm not an oxygen dev! I just read the patch and want to > share my opinion :) > > I'm also quite puzzeled with this change.. > > A cache is "usually" being used to store the result of a "heavy" > operation and use that result the next time to speed things up. > That principle sounds great to me and should be used in places if needed. > A better approach would be to speed up the slow operation to make caching > simply not needed, but that's a different story. > > The changes you've made now make QCache usage heavier and most certainly > much more difficult to follow because of the added while loops to keep an > hypothetical case from happening. If QCache requires measures like this then > perhaps we should not be using QCache at all. > > I understand you're trying to do your best, Michael Pyne, but a patch > like this feels like heading in the wrong direction to me.. > > I don't know if
Re: Review Request 127866: Oxygen: Fix QCache usage
> On May 15, 2016, 8:51 a.m., Hugo Pereira Da Costa wrote: > > 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) ? > > Michael Pyne wrote: > For the most part the requirements are determined by the current return > type within the code. If we return a pointer then currently it has to have > come directly out of the QCache. Since QCache is assumed to be the owner, the > calling code won't delete the pointer ; but if the caller won't delete the > pointer then we'll have a memory leak if we return a pointer to something > that had been new'd instead. > > References (i.e. QColor&) are a similar issue; it's safe enough to return > a reference to something held within the QCache, but we can't return a > reference to a local variable since that reference will invalidate as soon as > we return from the function. Of course a reference to a cached QColor may > *also* invalidate with the next call to an insert method of that cache, but > that's a separate story. > > It is unfortunate that the Qt docs are vague about this, since if the > **only** thing we had to worry about was cost being >maxCost(), we could > pretty much just mark 'ignore' for all the Coverity issues associated with > this (and I'd be fine doing that). The docs do kind of hint at that but don't > make it clear if is the only way that an entry would be deleted immediately. > > I think you're right that a loop is not a good idea... I was figuring > that eventually QCache would remove enough other items to make it work but > then I suppose QCache::insert() would have done that with the very first > attempt. > > As far as other options, I would definitely recommend against QCache for > the QColors: I'd say just hold onto specific QColors directly (perhaps in a > QHash) and, if possible, return them as values instead of references. > > I'm not sure if we could get away with the same for TileSets, but if so > it would again make things easier. If we can't we could look into making > TileSet an implicitly shared class so that we can return it by value cheaply. > > I wouldn't recommend a commit only to make Coverity happy. I've marked > other reports as "False Positive" and even "That's a bug, but we're ignoring > it" before. But it does seem to me that if a crash *is* possible (especially > in underlying library code) we should do something to avoid it. > > Either way I'll see if I can work up a revised patch but I'd still > appreciate advice on what's workable or not within Oxygen. > > Mark Gaiser wrote: > Disclaimer: i'm not an oxygen dev! I just read the patch and want to > share my opinion :) > > I'm also quite puzzeled with this change.. > > A cache is "usually" being used to store the result of a "heavy" > operation and use that result the next time to speed things up. > That principle sounds great to me and should be used in places if needed. > A better approach would be to speed up the slow operation to make caching > simply not needed, but that's a different story. > > The changes you've made now make QCache usage heavier and most certainly > much more difficult to follow because of the added while loops to keep an > hypothetical case from happening. If QCache requires measures like this then > perhaps we should not be using QCache at all. > > I understand you're trying to do your best, Michael Pyne, but a patch > like this feels like heading in the wrong direction to me.. > > I don't know if
Re: Review Request 127866: Oxygen: Fix QCache usage
> On mei 15, 2016, 8:51 a.m., Hugo Pereira Da Costa wrote: > > 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) ? > > Michael Pyne wrote: > For the most part the requirements are determined by the current return > type within the code. If we return a pointer then currently it has to have > come directly out of the QCache. Since QCache is assumed to be the owner, the > calling code won't delete the pointer ; but if the caller won't delete the > pointer then we'll have a memory leak if we return a pointer to something > that had been new'd instead. > > References (i.e. QColor&) are a similar issue; it's safe enough to return > a reference to something held within the QCache, but we can't return a > reference to a local variable since that reference will invalidate as soon as > we return from the function. Of course a reference to a cached QColor may > *also* invalidate with the next call to an insert method of that cache, but > that's a separate story. > > It is unfortunate that the Qt docs are vague about this, since if the > **only** thing we had to worry about was cost being >maxCost(), we could > pretty much just mark 'ignore' for all the Coverity issues associated with > this (and I'd be fine doing that). The docs do kind of hint at that but don't > make it clear if is the only way that an entry would be deleted immediately. > > I think you're right that a loop is not a good idea... I was figuring > that eventually QCache would remove enough other items to make it work but > then I suppose QCache::insert() would have done that with the very first > attempt. > > As far as other options, I would definitely recommend against QCache for > the QColors: I'd say just hold onto specific QColors directly (perhaps in a > QHash) and, if possible, return them as values instead of references. > > I'm not sure if we could get away with the same for TileSets, but if so > it would again make things easier. If we can't we could look into making > TileSet an implicitly shared class so that we can return it by value cheaply. > > I wouldn't recommend a commit only to make Coverity happy. I've marked > other reports as "False Positive" and even "That's a bug, but we're ignoring > it" before. But it does seem to me that if a crash *is* possible (especially > in underlying library code) we should do something to avoid it. > > Either way I'll see if I can work up a revised patch but I'd still > appreciate advice on what's workable or not within Oxygen. Disclaimer: i'm not an oxygen dev! I just read the patch and want to share my opinion :) I'm also quite puzzeled with this change.. A cache is "usually" being used to store the result of a "heavy" operation and use that result the next time to speed things up. That principle sounds great to me and should be used in places if needed. A better approach would be to speed up the slow operation to make caching simply not needed, but that's a different story. The changes you've made now make QCache usage heavier and most certainly much more difficult to follow because of the added while loops to keep an hypothetical case from happening. If QCache requires measures like this then perhaps we should not be using QCache at all. I understand you're trying to do your best, Michael Pyne, but a patch like this feels like heading in the wrong direction to me.. I don't know if it's usefull, but there is als QPixmapCache [1] (which uses a QCache internally, but seems to behave as a
Re: Review Request 127866: Oxygen: Fix QCache usage
> On May 15, 2016, 8:51 a.m., Hugo Pereira Da Costa wrote: > > 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) ? For the most part the requirements are determined by the current return type within the code. If we return a pointer then currently it has to have come directly out of the QCache. Since QCache is assumed to be the owner, the calling code won't delete the pointer ; but if the caller won't delete the pointer then we'll have a memory leak if we return a pointer to something that had been new'd instead. References (i.e. QColor&) are a similar issue; it's safe enough to return a reference to something held within the QCache, but we can't return a reference to a local variable since that reference will invalidate as soon as we return from the function. Of course a reference to a cached QColor may *also* invalidate with the next call to an insert method of that cache, but that's a separate story. It is unfortunate that the Qt docs are vague about this, since if the **only** thing we had to worry about was cost being >maxCost(), we could pretty much just mark 'ignore' for all the Coverity issues associated with this (and I'd be fine doing that). The docs do kind of hint at that but don't make it clear if is the only way that an entry would be deleted immediately. I think you're right that a loop is not a good idea... I was figuring that eventually QCache would remove enough other items to make it work but then I suppose QCache::insert() would have done that with the very first attempt. As far as other options, I would definitely recommend against QCache for the QColors: I'd say just hold onto specific QColors directly (perhaps in a QHash) and, if possible, return them as values instead of references. I'm not sure if we could get away with the same for TileSets, but if so it would again make things easier. If we can't we could look into making TileSet an implicitly shared class so that we can return it by value cheaply. I wouldn't recommend a commit only to make Coverity happy. I've marked other reports as "False Positive" and even "That's a bug, but we're ignoring it" before. But it does seem to me that if a crash *is* possible (especially in underlying library code) we should do something to avoid it. Either way I'll see if I can work up a revised patch but I'd still appreciate advice on what's workable or not within Oxygen. - Michael --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127866/#review95480 --- 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
Re: Review Request 127866: Oxygen: Fix QCache usage
--- 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 > >
Review Request 127866: Oxygen: Fix QCache usage
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127866/ --- 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
