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<QSharedData> 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<const
void>, 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 <allan.jensen@qt.io>
Reviewed-by: Lars Knoll <lars.knoll@qt.io>
Reviewed-by: Olivier Goffart (Woboq GmbH) <ogoffart@woboq.com>

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 <private/qsimd_p.h>
 
+#include <QtCore/qsharedpointer.h>
+
 QT_BEGIN_NAMESPACE
 
 #if defined(Q_CC_GNU)
@@ -335,7 +337,11 @@ struct QSpanData
         QGradientData gradient;
         QTextureData texture;
     };
-    QExplicitlySharedDataPointer<const QSharedData> cachedGradient;
+    class Pinnable {
+    protected:
+        ~Pinnable() {}
+    }; // QSharedPointer<const void> is not supported
+    QSharedPointer<const Pinnable> 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<quint64, QExplicitlySharedDataPointer<const CacheInfo> > QGradientColorTableHash;
+    typedef QMultiHash<quint64, QSharedPointer<const CacheInfo>> QGradientColorTableHash;
 
-    inline QExplicitlySharedDataPointer<const CacheInfo> getBuffer(const QGradient &gradient, int opacity) {
+    inline QSharedPointer<const CacheInfo> 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<const CacheInfo> &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<const CacheInfo> addCacheElement(quint64 hash_val, const QGradient &gradient, int opacity) {
+    QSharedPointer<const CacheInfo> 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<CacheInfo> cache_entry(new CacheInfo (gradient.stops(), opacity, gradient.interpolationMode()));
+        auto cache_entry = QSharedPointer<CacheInfo>::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<const QLinearGradient *>(brush.gradient());
             gradient.alphaColor = !brush.isOpaque() || alpha != 256;
 
-            QExplicitlySharedDataPointer<const QGradientCache::CacheInfo> 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<const QRadialGradient *>(brush.gradient());
             gradient.alphaColor = !brush.isOpaque() || alpha != 256;
 
-            QExplicitlySharedDataPointer<const QGradientCache::CacheInfo> 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<const QConicalGradient *>(brush.gradient());
             gradient.alphaColor = !brush.isOpaque() || alpha != 256;
 
-            QExplicitlySharedDataPointer<const QGradientCache::CacheInfo> 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;