From 933d695ecb5d8e2dbe107e5fd15428a7484ede8f Mon Sep 17 00:00:00 2001 From: Ulf Hermann Date: Fri, 1 Mar 2024 15:16:54 +0100 Subject: [PATCH] QProperty: Destroy binding when refcount is 0 This has to be done from all places where we deref it. Otherwise we leak memory. Pick-to: 6.6 6.5 Fixes: QTBUG-116086 Change-Id: I57307ac746205578a920d2bb1e159b66ebd9b2cc Reviewed-by: Fabian Kosmale (cherry picked from commit 717dc9450ffc13ef8209a10073552ac4574a4160) Reviewed-by: Qt Cherry-pick Bot --- src/corelib/kernel/qproperty.cpp | 7 --- src/corelib/kernel/qproperty_p.h | 18 +++++++- .../kernel/qproperty/tst_qproperty.cpp | 44 +++++++++++++++++++ 3 files changed, 60 insertions(+), 9 deletions(-) diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp index c2ade0599b9..4819ac4d57b 100644 --- a/src/corelib/kernel/qproperty.cpp +++ b/src/corelib/kernel/qproperty.cpp @@ -753,13 +753,6 @@ void QPropertyObserverPointer::setChangeHandler(QPropertyObserver::ChangeHandler ptr->next.setTag(QPropertyObserver::ObserverNotifiesChangeHandler); } -void QPropertyObserverPointer::setBindingToNotify(QPropertyBindingPrivate *binding) -{ - Q_ASSERT(ptr->next.tag() != QPropertyObserver::ObserverIsPlaceholder); - ptr->binding = binding; - ptr->next.setTag(QPropertyObserver::ObserverNotifiesBinding); -} - /*! \internal The same as setBindingToNotify, but assumes that the tag is already correct. diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h index 220a0064e96..8ae6664a2b6 100644 --- a/src/corelib/kernel/qproperty_p.h +++ b/src/corelib/kernel/qproperty_p.h @@ -143,7 +143,13 @@ struct QPropertyObserverPointer unlink_common(); } - void setBindingToNotify(QPropertyBindingPrivate *binding); + void setBindingToNotify(QPropertyBindingPrivate *binding) + { + Q_ASSERT(ptr->next.tag() != QPropertyObserver::ObserverIsPlaceholder); + ptr->binding = binding; + ptr->next.setTag(QPropertyObserver::ObserverNotifiesBinding); + } + void setBindingToNotify_unsafe(QPropertyBindingPrivate *binding); void setChangeHandler(QPropertyObserver::ChangeHandler changeHandler); @@ -941,7 +947,15 @@ QBindingObserverPtr::QBindingObserverPtr(QPropertyObserver *observer) noexcept : QPropertyObserverPointer{d}.binding()->addRef(); } -QBindingObserverPtr::~QBindingObserverPtr() { if (d) QPropertyObserverPointer{d}.binding()->deref(); } +QBindingObserverPtr::~QBindingObserverPtr() +{ + if (!d) + return; + + QPropertyBindingPrivate *bindingPrivate = binding(); + if (!bindingPrivate->deref()) + QPropertyBindingPrivate::destroyAndFreeMemory(bindingPrivate); +} QPropertyBindingPrivate *QBindingObserverPtr::binding() const noexcept { return QPropertyObserverPointer{d}.binding(); } diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp index af207cea7b4..cc7edb8bf2d 100644 --- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp +++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp @@ -108,6 +108,8 @@ private slots: void propertyAdaptorBinding(); void propertyUpdateViaSignaledProperty(); + + void derefFromObserver(); }; namespace { @@ -315,6 +317,7 @@ void tst_QProperty::bindingAfterUse() void tst_QProperty::bindingFunctionDtorCalled() { + DtorCounter::counter = 0; DtorCounter dc; { QProperty prop; @@ -2531,6 +2534,47 @@ void tst_QProperty::propertyUpdateViaSignaledProperty() QCOMPARE(o.bindable2(), 36); } +void tst_QProperty::derefFromObserver() +{ + int triggered = 0; + QProperty source(11); + + DtorCounter::counter = 0; + DtorCounter dc; + + QProperty target([&triggered, &source, dc]() mutable { + dc.shouldIncrement = true; + return ++triggered + source.value(); + }); + QCOMPARE(triggered, 1); + + { + auto propObserver = std::make_unique(); + QPropertyObserverPointer propObserverPtr { propObserver.get() }; + propObserverPtr.setBindingToNotify(QPropertyBindingPrivate::get(target.binding())); + + QBindingObserverPtr bindingPtr(propObserver.get()); + + QCOMPARE(triggered, 1); + source = 25; + QCOMPARE(triggered, 2); + QCOMPARE(target, 27); + + target.setBinding([]() { return 8; }); + QCOMPARE(target, 8); + + // The QBindingObserverPtr still holds on to the binding. + QCOMPARE(dc.counter, 0); + } + + // The binding is actually gone now. + QCOMPARE(dc.counter, 1); + + source = 26; + QCOMPARE(triggered, 2); + QCOMPARE(target, 8); +} + QTEST_MAIN(tst_QProperty); #undef QT_SOURCE_LOCATION_NAMESPACE