From 71a18fd64c93afa00f4d89de2ba47663f41eba2c Mon Sep 17 00:00:00 2001 From: Simon Hausmann Date: Mon, 18 Sep 2017 16:26:17 +0200 Subject: Fix qmlClearTypeRegistrations() not dropping all registrations In commit 48c09a85ce397979c7e706e3694c879ffe456e09 we added the undeletableTypes container to hold a reference on C++ registered types to keep the indices returned by the public qmlRegisterType() API stable. Since qmlClearTypeRegistrations() is API that also resets those indices, we must also clear the undeletableTypes container to avoid leaking memory. Change-Id: I2038c00913f894d58aca3714d64d497493585326 Reviewed-by: Lars Knoll --- src/qml/qml/qqmlmetatype.cpp | 8 +++ src/qml/qml/qqmlmetatype_p.h | 1 + .../qml/qqmlenginecleanup/qqmlenginecleanup.pro | 2 +- .../qqmlenginecleanup/tst_qqmlenginecleanup.cpp | 59 +++++++++++++++++++--- 4 files changed, 61 insertions(+), 9 deletions(-) diff --git a/src/qml/qml/qqmlmetatype.cpp b/src/qml/qml/qqmlmetatype.cpp index 8f5d11a96f..ac670bdabb 100644 --- a/src/qml/qml/qqmlmetatype.cpp +++ b/src/qml/qml/qqmlmetatype.cpp @@ -1144,6 +1144,13 @@ void QQmlType::derefHandle(QQmlTypePrivate *priv) delete priv; } +int QQmlType::refCount(QQmlTypePrivate *priv) +{ + if (priv) + return priv->refCount; + return -1; +} + namespace { template void removeQQmlTypePrivate(QQmlTypeContainer &container, const QQmlTypePrivate *reference) @@ -1334,6 +1341,7 @@ void qmlClearTypeRegistrations() // Declared in qqml.h data->urlToNonFileImportType.clear(); data->metaObjectToType.clear(); data->uriToModule.clear(); + data->undeletableTypes.clear(); QQmlEnginePrivate::baseModulesUninitialized = true; //So the engine re-registers its types #if QT_CONFIG(library) diff --git a/src/qml/qml/qqmlmetatype_p.h b/src/qml/qml/qqmlmetatype_p.h index 9a7736ffcd..74b1cf0e06 100644 --- a/src/qml/qml/qqmlmetatype_p.h +++ b/src/qml/qml/qqmlmetatype_p.h @@ -245,6 +245,7 @@ public: QQmlTypePrivate *priv() const { return d; } static void refHandle(QQmlTypePrivate *priv); static void derefHandle(QQmlTypePrivate *priv); + static int refCount(QQmlTypePrivate *priv); enum RegistrationType { CppType = 0, diff --git a/tests/auto/qml/qqmlenginecleanup/qqmlenginecleanup.pro b/tests/auto/qml/qqmlenginecleanup/qqmlenginecleanup.pro index 5bcec9f5b4..90508609a8 100644 --- a/tests/auto/qml/qqmlenginecleanup/qqmlenginecleanup.pro +++ b/tests/auto/qml/qqmlenginecleanup/qqmlenginecleanup.pro @@ -6,4 +6,4 @@ include (../../shared/util.pri) SOURCES += tst_qqmlenginecleanup.cpp -QT += testlib qml +QT += testlib qml qml-private diff --git a/tests/auto/qml/qqmlenginecleanup/tst_qqmlenginecleanup.cpp b/tests/auto/qml/qqmlenginecleanup/tst_qqmlenginecleanup.cpp index d0a8b6401f..7e9a1524b0 100644 --- a/tests/auto/qml/qqmlenginecleanup/tst_qqmlenginecleanup.cpp +++ b/tests/auto/qml/qqmlenginecleanup/tst_qqmlenginecleanup.cpp @@ -31,6 +31,8 @@ #include #include #include +#include +#include //Separate test, because if engine cleanup attempts fail they can easily break unrelated tests class tst_qqmlenginecleanup : public QQmlDataTest @@ -44,41 +46,82 @@ private slots: void test_valueTypeProviderModule(); // QTBUG-43004 }; +// A wrapper around QQmlComponent to ensure the temporary reference counts +// on the type data as a result of the main thread <> loader thread communication +// are dropped. Regular Synchronous loading will leave us with an event posted +// to the gui thread and an extra refcount that will only be dropped after the +// event delivery. A plain sendPostedEvents() however is insufficient because +// we can't be sure that the event is posted after the constructor finished. +class CleanlyLoadingComponent : public QQmlComponent +{ +public: + CleanlyLoadingComponent(QQmlEngine *engine, const QUrl &url) + : QQmlComponent(engine, url, QQmlComponent::Asynchronous) + { waitForLoad(); } + CleanlyLoadingComponent(QQmlEngine *engine, const QString &fileName) + : QQmlComponent(engine, fileName, QQmlComponent::Asynchronous) + { waitForLoad(); } + + void waitForLoad() + { + QTRY_VERIFY(status() == QQmlComponent::Ready || status() == QQmlComponent::Error); + } +}; + void tst_qqmlenginecleanup::test_qmlClearTypeRegistrations() { //Test for preventing memory leaks is in tests/manual/qmltypememory QQmlEngine* engine; - QQmlComponent* component; + CleanlyLoadingComponent* component; QUrl testFile = testFileUrl("types.qml"); + const auto qmlTypeForTestType = []() { + return QQmlMetaType::qmlType(QStringLiteral("TestTypeCpp"), QStringLiteral("Test"), 2, 0); + }; + + QVERIFY(!qmlTypeForTestType().isValid()); qmlRegisterType("Test", 2, 0, "TestTypeCpp"); + QVERIFY(qmlTypeForTestType().isValid()); + engine = new QQmlEngine; - component = new QQmlComponent(engine, testFile); + component = new CleanlyLoadingComponent(engine, testFile); QVERIFY(component->isReady()); - delete engine; delete component; - qmlClearTypeRegistrations(); + delete engine; + + { + auto cppType = qmlTypeForTestType(); + + qmlClearTypeRegistrations(); + QVERIFY(!qmlTypeForTestType().isValid()); + + // cppType should hold the last ref, qmlClearTypeRegistration should have wiped + // all internal references. + QCOMPARE(QQmlType::refCount(cppType.priv()), 1); + } //2nd run verifies that types can reload after a qmlClearTypeRegistrations qmlRegisterType("Test", 2, 0, "TestTypeCpp"); + QVERIFY(qmlTypeForTestType().isValid()); engine = new QQmlEngine; - component = new QQmlComponent(engine, testFile); + component = new CleanlyLoadingComponent(engine, testFile); QVERIFY(component->isReady()); - delete engine; delete component; + delete engine; qmlClearTypeRegistrations(); + QVERIFY(!qmlTypeForTestType().isValid()); //3nd run verifies that TestTypeCpp is no longer registered engine = new QQmlEngine; - component = new QQmlComponent(engine, testFile); + component = new CleanlyLoadingComponent(engine, testFile); QVERIFY(component->isError()); QCOMPARE(component->errorString(), testFile.toString() +":33 module \"Test\" is not installed\n"); - delete engine; delete component; + delete engine; } static void cleanState(QQmlEngine **e) -- cgit v1.2.3