From 124a8c20ed27471c52f48ec9c2dcec6fba68d5e0 Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Sat, 1 Oct 2022 18:04:56 +0200 Subject: [PATCH] Make sure that palette cache keys are unique After 1d961491d817490da156769ddce6fce48a0bce4a, palettes are different if they either have different brush data, or a different private. Two privates can share data, but still must generate different cache keys. The cacheKey has so far been composted of the serial number of the Data struct, and a detach number that is incremented when we detach the private. This failed for two reasons: - the implicit copy constructor of the Data class copied the serial number, when it should have incremented it. Fix that by member- initializing the serial number rather than doing it only in the default constructor. The member initialization is also executed for the copy constructor. - the detach_no logic as it was implemented does not guarantee that two copies of the same palette that share data, but have different resolve masks (and thus different privates) have different detach_no values. Use a static serial counter for that number as well. Amend the test case to verfiy that cache keys, and the elements of the cache keys, change when they are expected to. Fixes: QTBUG-106984 Change-Id: I84d7055ce8bfe0d42f1f8e9766f3f1ad610f4ec8 Reviewed-by: Timur Pocheptsov (cherry picked from commit 109e088c7c5d0c9325966e88d55fd9f7a58f67ea) Reviewed-by: Qt Cherry-pick Bot --- src/gui/kernel/qpalette.cpp | 21 +++++-- .../auto/gui/kernel/qpalette/tst_qpalette.cpp | 57 +++++++++++++++++++ 2 files changed, 73 insertions(+), 5 deletions(-) diff --git a/src/gui/kernel/qpalette.cpp b/src/gui/kernel/qpalette.cpp index 33a885b510d..081af380745 100644 --- a/src/gui/kernel/qpalette.cpp +++ b/src/gui/kernel/qpalette.cpp @@ -35,14 +35,25 @@ class QPalettePrivate public: class Data : public QSharedData { public: - Data() : ser_no(qt_palette_count++) { } + // Every instance of Data has to have a unique serial number, even + // if it gets created by copying another - we wouldn't create a copy + // in the first place if the serial number should be the same! + Data(const Data &other) + : QSharedData(other) + { + for (int grp = 0; grp < int(QPalette::NColorGroups); grp++) { + for (int role = 0; role < int(QPalette::NColorRoles); role++) + br[grp][role] = other.br[grp][role]; + } + } + Data() = default; QBrush br[QPalette::NColorGroups][QPalette::NColorRoles]; - const int ser_no; + const int ser_no = qt_palette_count++; }; QPalettePrivate(const QExplicitlySharedDataPointer &data) - : ref(1), detach_no(0), data(data) + : ref(1), data(data) { } QPalettePrivate() : QPalettePrivate(QExplicitlySharedDataPointer(new Data)) @@ -50,7 +61,8 @@ public: QAtomicInt ref; QPalette::ResolveMask resolveMask = {0}; - int detach_no; + static inline int qt_palette_private_count = 0; + const int detach_no = ++qt_palette_private_count; QExplicitlySharedDataPointer data; }; @@ -853,7 +865,6 @@ void QPalette::detach() delete d; d = x; } - ++d->detach_no; } /*! diff --git a/tests/auto/gui/kernel/qpalette/tst_qpalette.cpp b/tests/auto/gui/kernel/qpalette/tst_qpalette.cpp index 150d58e6f4f..2e9a484b8b2 100644 --- a/tests/auto/gui/kernel/qpalette/tst_qpalette.cpp +++ b/tests/auto/gui/kernel/qpalette/tst_qpalette.cpp @@ -22,6 +22,7 @@ private Q_SLOTS: void noBrushesSetForDefaultPalette(); void cannotCheckIfInvalidBrushSet(); void checkIfBrushForCurrentGroupSet(); + void cacheKey(); }; void tst_QPalette::roleValues_data() @@ -264,5 +265,61 @@ void tst_QPalette::checkIfBrushForCurrentGroupSet() QVERIFY(p.isBrushSet(QPalette::Current, QPalette::Link)); } +void tst_QPalette::cacheKey() +{ + const QPalette defaultPalette; + // precondition: all palettes are expected to have contrasting text on base + QVERIFY(defaultPalette.base() != defaultPalette.text()); + const auto defaultCacheKey = defaultPalette.cacheKey(); + const auto defaultSerNo = defaultCacheKey >> 32; + const auto defaultDetachNo = defaultCacheKey & 0xffffffff; + + QPalette copyDifferentData(defaultPalette); + QPalette copyDifferentMask(defaultPalette); + QPalette copyDifferentMaskAndData(defaultPalette); + + QCOMPARE(defaultPalette.cacheKey(), copyDifferentData.cacheKey()); + + // deep detach of both private and data + copyDifferentData.setBrush(QPalette::Base, defaultPalette.text()); + const auto differentDataKey = copyDifferentData.cacheKey(); + const auto differentDataSerNo = differentDataKey >> 32; + const auto differentDataDetachNo = differentDataKey & 0xffffffff; + + QCOMPARE_NE(copyDifferentData.cacheKey(), defaultCacheKey); + QCOMPARE(defaultPalette.cacheKey(), defaultCacheKey); + + // shallow detach, both privates reference the same data + copyDifferentMask.setResolveMask(0xffffffffffffffff); + const auto differentMaskKey = copyDifferentMask.cacheKey(); + const auto differentMaskSerNo = differentMaskKey >> 32; + const auto differentMaskDetachNo = differentMaskKey & 0xffffffff; + QCOMPARE(differentMaskSerNo, defaultSerNo); + QCOMPARE_NE(differentMaskSerNo, defaultDetachNo); + QCOMPARE_NE(differentMaskKey, defaultCacheKey); + QCOMPARE_NE(differentMaskKey, differentDataKey); + + // shallow detach, both privates reference the same data + copyDifferentMaskAndData.setResolveMask(0xeeeeeeeeeeeeeeee); + const auto modifiedCacheKey = copyDifferentMaskAndData.cacheKey(); + QCOMPARE_NE(modifiedCacheKey, copyDifferentMask.cacheKey()); + QCOMPARE_NE(modifiedCacheKey, defaultCacheKey); + QCOMPARE_NE(modifiedCacheKey, copyDifferentData.cacheKey()); + QCOMPARE_NE(copyDifferentMask.cacheKey(), defaultCacheKey); + + // full detach - both key elements are different + copyDifferentMaskAndData.setBrush(QPalette::Base, defaultPalette.text()); + const auto modifiedAllKey = copyDifferentMaskAndData.cacheKey(); + const auto modifiedAllSerNo = modifiedAllKey >> 32; + const auto modifiedAllDetachNo = modifiedAllKey & 0xffffffff; + QCOMPARE_NE(modifiedAllSerNo, defaultSerNo); + QCOMPARE_NE(modifiedAllDetachNo, defaultDetachNo); + + QCOMPARE_NE(modifiedAllKey, copyDifferentMask.cacheKey()); + QCOMPARE_NE(modifiedAllKey, defaultCacheKey); + QCOMPARE_NE(modifiedAllKey, differentDataKey); + QCOMPARE_NE(modifiedAllKey, modifiedCacheKey); +} + QTEST_MAIN(tst_QPalette) #include "tst_qpalette.moc"