From 7d898ae38e42f44dabcc972ad81cb7f05ecd8ad3 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Tue, 10 Jan 2017 12:49:11 +0100 Subject: [PATCH] QGradientCache: fix a new/delete mismatch Commit f839f536 fixed a data race in the gradient cache by reference-counting the CacheInfo objects stored in the cache. To this end, QSpanData gained a ref-counted pointer to the CacheInfo whose members it references to keep the object alive for as long as the QSpanData object needs it. However, since CacheInfo is only later defined in qpaintengine_raster.cpp, the counted pointer's payload was chosen as CacheInfo's base class, QSharedData. As it turns out, e.g. in the QPainter test, the data race was real and so QSpanData ends up being the entity that destroys (at least some) CacheInfos, either in its destructor, or in the setup() method. Since QSharedData's destructor is not virtual, and QExplicitlySharedDataPointer knows nothing of the CacheInfo-ness of its payload, we end up calling the destructor of the base class, and not the CacheInfo one. Fix by using QSharedPointer instead, which stores the correct deleter internally. Ideally, QSpanData would contain a QSharedPointer, but QSharedPointer's implementation is deficient in that respect and does not compile when instantiated with void, and we can't use std::shared_ptr, yet, so introduce an arbitrary base class, Pinnable, to be used instead. Change-Id: I5573c599d5464278d3a8e4248d887ef9ffcd7b70 Reviewed-by: Allan Sandfeld Jensen Reviewed-by: Lars Knoll Reviewed-by: Olivier Goffart (Woboq GmbH) --- src/gui/painting/qdrawhelper_p.h | 8 +++++++- src/gui/painting/qpaintengine_raster.cpp | 18 +++++++++--------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/gui/painting/qdrawhelper_p.h b/src/gui/painting/qdrawhelper_p.h index e537c343bb..0e46962784 100644 --- a/src/gui/painting/qdrawhelper_p.h +++ b/src/gui/painting/qdrawhelper_p.h @@ -64,6 +64,8 @@ #include "private/qrasterdefs_p.h" #include +#include + QT_BEGIN_NAMESPACE #if defined(Q_CC_GNU) @@ -335,7 +337,11 @@ struct QSpanData QGradientData gradient; QTextureData texture; }; - QExplicitlySharedDataPointer cachedGradient; + class Pinnable { + protected: + ~Pinnable() {} + }; // QSharedPointer is not supported + QSharedPointer cachedGradient; void init(QRasterBuffer *rb, const QRasterPaintEngine *pe); diff --git a/src/gui/painting/qpaintengine_raster.cpp b/src/gui/painting/qpaintengine_raster.cpp index df96a993e3..6d5eaf5aed 100644 --- a/src/gui/painting/qpaintengine_raster.cpp +++ b/src/gui/painting/qpaintengine_raster.cpp @@ -4150,7 +4150,7 @@ void QRasterBuffer::flushToARGBImage(QImage *target) const class QGradientCache { public: - struct CacheInfo : public QSharedData + struct CacheInfo : QSpanData::Pinnable { inline CacheInfo(QGradientStops s, int op, QGradient::InterpolationMode mode) : stops(qMove(s)), opacity(op), interpolationMode(mode) {} @@ -4161,9 +4161,9 @@ public: QGradient::InterpolationMode interpolationMode; }; - typedef QMultiHash > QGradientColorTableHash; + typedef QMultiHash> QGradientColorTableHash; - inline QExplicitlySharedDataPointer getBuffer(const QGradient &gradient, int opacity) { + inline QSharedPointer getBuffer(const QGradient &gradient, int opacity) { quint64 hash_val = 0; const QGradientStops stops = gradient.stops(); @@ -4177,7 +4177,7 @@ public: return addCacheElement(hash_val, gradient, opacity); else { do { - const QExplicitlySharedDataPointer &cache_info = it.value(); + const auto &cache_info = it.value(); if (cache_info->stops == stops && cache_info->opacity == opacity && cache_info->interpolationMode == gradient.interpolationMode()) return cache_info; ++it; @@ -4193,12 +4193,12 @@ protected: inline void generateGradientColorTable(const QGradient& g, QRgba64 *colorTable, int size, int opacity) const; - QExplicitlySharedDataPointer addCacheElement(quint64 hash_val, const QGradient &gradient, int opacity) { + QSharedPointer addCacheElement(quint64 hash_val, const QGradient &gradient, int opacity) { if (cache.size() == maxCacheSize()) { // may remove more than 1, but OK cache.erase(cache.begin() + (qrand() % maxCacheSize())); } - QExplicitlySharedDataPointer cache_entry(new CacheInfo (gradient.stops(), opacity, gradient.interpolationMode())); + auto cache_entry = QSharedPointer::create(gradient.stops(), opacity, gradient.interpolationMode()); generateGradientColorTable(gradient, cache_entry->buffer64, paletteSize(), opacity); for (int i = 0; i < GRADIENT_STOPTABLE_SIZE; ++i) cache_entry->buffer32[i] = cache_entry->buffer64[i].toArgb32(); @@ -4438,7 +4438,7 @@ void QSpanData::setup(const QBrush &brush, int alpha, QPainter::CompositionMode const QLinearGradient *g = static_cast(brush.gradient()); gradient.alphaColor = !brush.isOpaque() || alpha != 256; - QExplicitlySharedDataPointer cacheInfo = qt_gradient_cache()->getBuffer(*g, alpha); + auto cacheInfo = qt_gradient_cache()->getBuffer(*g, alpha); cachedGradient = cacheInfo; gradient.colorTable32 = cacheInfo->buffer32; gradient.colorTable64 = cacheInfo->buffer64; @@ -4460,7 +4460,7 @@ void QSpanData::setup(const QBrush &brush, int alpha, QPainter::CompositionMode const QRadialGradient *g = static_cast(brush.gradient()); gradient.alphaColor = !brush.isOpaque() || alpha != 256; - QExplicitlySharedDataPointer cacheInfo = qt_gradient_cache()->getBuffer(*g, alpha); + auto cacheInfo = qt_gradient_cache()->getBuffer(*g, alpha); cachedGradient = cacheInfo; gradient.colorTable32 = cacheInfo->buffer32; gradient.colorTable64 = cacheInfo->buffer64; @@ -4486,7 +4486,7 @@ void QSpanData::setup(const QBrush &brush, int alpha, QPainter::CompositionMode const QConicalGradient *g = static_cast(brush.gradient()); gradient.alphaColor = !brush.isOpaque() || alpha != 256; - QExplicitlySharedDataPointer cacheInfo = qt_gradient_cache()->getBuffer(*g, alpha); + auto cacheInfo = qt_gradient_cache()->getBuffer(*g, alpha); cachedGradient = cacheInfo; gradient.colorTable32 = cacheInfo->buffer32; gradient.colorTable64 = cacheInfo->buffer64;