From 64ab935e0a69b1ad5bdb6f59dbe3f9304716c02a Mon Sep 17 00:00:00 2001 From: Louis du Verdier Date: Sat, 28 Dec 2019 14:53:03 +0100 Subject: Stabilize QQuickItemParticle This commit fixes two crashes and a memory leak in QQuickItemParticle. The first crash was caused by the list m_loadables that kept pointers on QQuickParticleData, that could in the meantime become dangling pointers if the associated particle expired or got deleted by a call to the reset() method of the particle system. Its role was to keep a list of particles that did not have a delegate instantiated yet, in order to create them in the tick() method. This list is removed and the list of particles to handle is directly deduced in the tick() method. The second crash was caused by the list m_deletables that could in some situations (generally due to a reset()) contain multiple times the same delegate, and therefore cause a double delete in processDeletables(). This list is changed to a set to avoid this situation with a minimum impact on the rest of the code. The memory leak was caused by the m_managed list of delegates that was not freed when QQuickItemParticle gets deleted. Task-number: QTBUG-71193 Change-Id: I6fe30ee59a9a0bb90c14c62c7a48a50f465a9e0c Reviewed-by: Mikhail Svetkin --- src/particles/qquickitemparticle.cpp | 75 +++++++++------------- src/particles/qquickitemparticle_p.h | 3 +- .../particles/qquickitemparticle/data/loader.qml | 65 +++++++++++++++++++ .../qquickitemparticle/tst_qquickitemparticle.cpp | 37 +++++++++++ 4 files changed, 134 insertions(+), 46 deletions(-) create mode 100644 tests/auto/particles/qquickitemparticle/data/loader.qml diff --git a/src/particles/qquickitemparticle.cpp b/src/particles/qquickitemparticle.cpp index fc28864746..de74c8a2c4 100644 --- a/src/particles/qquickitemparticle.cpp +++ b/src/particles/qquickitemparticle.cpp @@ -136,6 +136,7 @@ QQuickItemParticle::QQuickItemParticle(QQuickItem *parent) : QQuickItemParticle::~QQuickItemParticle() { delete clock; + qDeleteAll(m_managed); } void QQuickItemParticle::freeze(QQuickItem* item) @@ -165,7 +166,8 @@ void QQuickItemParticle::give(QQuickItem *item) void QQuickItemParticle::initialize(int gIdx, int pIdx) { - m_loadables << m_system->groupData[gIdx]->data[pIdx];//defer to other thread + Q_UNUSED(gIdx); + Q_UNUSED(pIdx); } void QQuickItemParticle::commit(int, int) @@ -196,45 +198,39 @@ void QQuickItemParticle::tick(int time) Q_UNUSED(time);//only needed because QTickAnimationProxy expects one processDeletables(); - foreach (QQuickParticleData* d, m_loadables){ - Q_ASSERT(d); - if (m_stasis.contains(d->delegate)) - qWarning() << "Current model particles prefers overwrite:false"; - //remove old item from the particle that is dying to make room for this one - if (d->delegate) { - m_deletables << d->delegate; - d->delegate = nullptr; - } - if (!m_pendingItems.isEmpty()){ - d->delegate = m_pendingItems.front(); - m_pendingItems.pop_front(); - }else if (m_delegate){ - d->delegate = qobject_cast(m_delegate->create(qmlContext(this))); - if (d->delegate) - m_managed << d->delegate; - } - if (d && d->delegate){//###Data can be zero if creating an item leads to a reset - this screws things up. - d->delegate->setX(d->curX(m_system) - d->delegate->width() / 2); //TODO: adjust for system? - d->delegate->setY(d->curY(m_system) - d->delegate->height() / 2); - QQuickItemParticleAttached* mpa = qobject_cast(qmlAttachedPropertiesObject(d->delegate)); - if (mpa){ - mpa->m_mp = this; - mpa->attach(); + for (auto groupId : groupIds()) { + for (QQuickParticleData* d : qAsConst(m_system->groupData[groupId]->data)) { + if (!d->delegate && d->t != -1 && d->stillAlive(m_system)) { + if (!m_pendingItems.isEmpty()){ + d->delegate = m_pendingItems.front(); + m_pendingItems.pop_front(); + }else if (m_delegate){ + d->delegate = qobject_cast(m_delegate->create(qmlContext(this))); + if (d->delegate) + m_managed << d->delegate; + } + if (d && d->delegate){//###Data can be zero if creating an item leads to a reset - this screws things up. + d->delegate->setX(d->curX(m_system) - d->delegate->width() / 2); //TODO: adjust for system? + d->delegate->setY(d->curY(m_system) - d->delegate->height() / 2); + QQuickItemParticleAttached* mpa = qobject_cast(qmlAttachedPropertiesObject(d->delegate)); + if (mpa){ + mpa->m_mp = this; + mpa->attach(); + } + d->delegate->setParentItem(this); + if (m_fade) + d->delegate->setOpacity(0.); + d->delegate->setVisible(false);//Will be set to true when we prepare the next frame + m_activeCount++; + } } - d->delegate->setParentItem(this); - if (m_fade) - d->delegate->setOpacity(0.); - d->delegate->setVisible(false);//Will be set to true when we prepare the next frame - m_activeCount++; } } - m_loadables.clear(); } void QQuickItemParticle::reset() { QQuickParticlePainter::reset(); - m_loadables.clear(); // delete all managed items which had their logical particles cleared // but leave it alone if the logical particle is maintained @@ -244,7 +240,7 @@ void QQuickItemParticle::reset() lost.remove(d->delegate); } } - m_deletables.append(lost.values()); + m_deletables.unite(lost); //TODO: This doesn't yet handle calling detach on taken particles in the system reset case processDeletables(); } @@ -253,18 +249,9 @@ void QQuickItemParticle::reset() QSGNode* QQuickItemParticle::updatePaintNode(QSGNode* n, UpdatePaintNodeData* d) { //Dummy update just to get painting tick - if (m_pleaseReset){ + if (m_pleaseReset) m_pleaseReset = false; - //Refill loadables, delayed here so as to only happen once per frame max - //### Constant resetting might lead to m_loadables never being populated when tick() occurs - for (auto groupId : groupIds()) { - for (QQuickParticleData* d : qAsConst(m_system->groupData[groupId]->data)) { - if (!d->delegate && d->t != -1 && d->stillAlive(m_system)) { - m_loadables << d; - } - } - } - } + prepareNextFrame(); update();//Get called again diff --git a/src/particles/qquickitemparticle_p.h b/src/particles/qquickitemparticle_p.h index 7867add4e4..b278008460 100644 --- a/src/particles/qquickitemparticle_p.h +++ b/src/particles/qquickitemparticle_p.h @@ -106,9 +106,8 @@ protected: private: void processDeletables(); void tick(int time = 0); - QList m_deletables; + QSet m_deletables; QList m_managed; - QList< QQuickParticleData* > m_loadables; bool m_fade; QList m_pendingItems; diff --git a/tests/auto/particles/qquickitemparticle/data/loader.qml b/tests/auto/particles/qquickitemparticle/data/loader.qml new file mode 100644 index 0000000000..beac7a0410 --- /dev/null +++ b/tests/auto/particles/qquickitemparticle/data/loader.qml @@ -0,0 +1,65 @@ +/**************************************************************************** +** +** Copyright (C) 2020 The Qt Company Ltd. +** Contact: https://www.qt.io/licensing/ +** +** This file is part of the test suite of the Qt Toolkit. +** +** $QT_BEGIN_LICENSE:GPL-EXCEPT$ +** Commercial License Usage +** Licensees holding valid commercial Qt licenses may use this file in +** accordance with the commercial license agreement provided with the +** Software or, alternatively, in accordance with the terms contained in +** a written agreement between you and The Qt Company. For licensing terms +** and conditions see https://www.qt.io/terms-conditions. For further +** information use the contact form at https://www.qt.io/contact-us. +** +** GNU General Public License Usage +** Alternatively, this file may be used under the terms of the GNU +** General Public License version 3 as published by the Free Software +** Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT +** included in the packaging of this file. Please review the following +** information to ensure the GNU General Public License requirements will +** be met: https://www.gnu.org/licenses/gpl-3.0.html. +** +** $QT_END_LICENSE$ +** +****************************************************************************/ + +import QtQuick 2.0 +import QtQuick.Particles 2.0 + +Rectangle { + color: "black" + width: 320 + height: 320 + + Component { + id: component + + ParticleSystem { + id: sys + objectName: "system" + anchors.fill: parent + running: visible + + ItemParticle { + delegate: Image { source: "../../shared/star.png" } + } + + Emitter { + //0,0 position + size: 32 + emitRate: 10 + lifeSpan: 150000 + } + } + } + + Loader { + id: loader + objectName: "loader" + sourceComponent: component + anchors.fill: parent + } +} diff --git a/tests/auto/particles/qquickitemparticle/tst_qquickitemparticle.cpp b/tests/auto/particles/qquickitemparticle/tst_qquickitemparticle.cpp index d9791cdb33..ea4e7a97a6 100644 --- a/tests/auto/particles/qquickitemparticle/tst_qquickitemparticle.cpp +++ b/tests/auto/particles/qquickitemparticle/tst_qquickitemparticle.cpp @@ -45,6 +45,8 @@ private slots: void test_basic(); void test_deletion(); void test_noDeletion(); + void test_noCrashOnReset(); + void test_noLeakWhenDeleted(); }; void tst_qquickitemparticle::initTestCase() @@ -107,6 +109,41 @@ void tst_qquickitemparticle::test_noDeletion() delete view; } +void tst_qquickitemparticle::test_noCrashOnReset() +{ + QQuickView* view = createView(testFileUrl("basic.qml"), 600); + QQuickParticleSystem* system = view->rootObject()->findChild("system"); + + for (int i = 0; i < 10; ++i) { + ensureAnimTime(16, system->m_animation); + system->reset(); + } + + delete view; +} + +void tst_qquickitemparticle::test_noLeakWhenDeleted() +{ + QQuickView* view = createView(testFileUrl("loader.qml"), 500); + QQuickParticleSystem* system = view->rootObject()->findChild("system"); + ensureAnimTime(100, system->m_animation); + + auto particles = qAsConst(system->groupData[0]->data); + QVERIFY(!particles.isEmpty()); + + QQuickParticleData* firstParticleData = particles.first(); + QPointer firstParticleDelegate = firstParticleData->delegate; + QVERIFY(!firstParticleDelegate.isNull()); + + QQuickItem* loader = view->rootObject()->findChild("loader"); + loader->setProperty("active", false); //This should destroy the ParticleSystem, ItemParticle and Emitter + + QTest::qWait(1); //Process events to make sure the loader is properly unloaded + QVERIFY(firstParticleDelegate.isNull()); //Delegates should be deleted + + delete view; +} + QTEST_MAIN(tst_qquickitemparticle); #include "tst_qquickitemparticle.moc" -- cgit v1.2.3