Skip to content

Commit

Permalink
Fix memory leak with JS imports
Browse files Browse the repository at this point in the history
Strictly speaking this is a regression introduced with commit
e22b624, making the QQmlContextData
objects reference counted, especially from the V4 QML context wrapper
objects.

That change (correct as it is) introduced an accidental circular
dependency in the simple scenario of importing a .js file in a .qml
file:

Each time the type in the .qml file is instantiated, we create a
dedicated QQmlContextData for the .js file. If the .js file has no
imports itself, that new context will get the same ctx->importedScripts
JS array as the QML context of the .qml file. That is a strong reference
via QV4::PersistentValue. That array in turn contains the
QV4::QmlContextWrapper that belongs to the imported script, which in
turn holds a strong reference (via refcount) to the script's context.

This patch breaks the circular reference when we perform context
invalidation, as the least intrusive measure.

For the auto-test to work, we must also clear the qmlContext persistent
of the QV4::Script that's used to evaluate the .js file. In subsequent
imports that persistent will be initialized to new values, so it will
only hold a strong reference to the last import, but strictly speaking
that is still a leak - hence also part of this fix.

Change-Id: I3e543c946e5e683425072dc3df7e49ca0e0c0215
Task-number: QTBUG-66189
Reviewed-by: Lars Knoll <lars.knoll@qt.io>
  • Loading branch information
tronical committed Feb 9, 2018
1 parent 94afe0d commit d4ff290
Show file tree
Hide file tree
Showing 6 changed files with 49 additions and 1 deletion.
4 changes: 3 additions & 1 deletion src/qml/jsruntime/qv4qmlcontext.cpp
Expand Up @@ -138,7 +138,9 @@ ReturnedValue QmlContextWrapper::get(const Managed *m, String *name, bool *hasPr
*hasProperty = true;
if (r.scriptIndex != -1) {
QV4::ScopedObject scripts(scope, context->importedScripts.valueRef());
return scripts->getIndexed(r.scriptIndex);
if (scripts)
return scripts->getIndexed(r.scriptIndex);
return QV4::Encode::null();
} else if (r.type.isValid()) {
return QmlTypeWrapper::create(v4, scopeObject, r.type);
} else if (r.importNamespace) {
Expand Down
2 changes: 2 additions & 0 deletions src/qml/qml/qqmlcontext.cpp
Expand Up @@ -579,6 +579,8 @@ void QQmlContextData::invalidate()
prevChild = 0;
}

importedScripts.clear();

engine = 0;
parent = 0;
}
Expand Down
1 change: 1 addition & 0 deletions src/qml/qml/qqmltypeloader.cpp
Expand Up @@ -2917,6 +2917,7 @@ QV4::ReturnedValue QQmlScriptData::scriptValueForContext(QQmlContextData *parent

m_program->qmlContext.set(scope.engine, qmlContext);
m_program->run();
m_program->qmlContext.clear();
if (scope.engine->hasException) {
QQmlError error = scope.engine->catchExceptionAsQmlError();
if (error.isValid())
Expand Down
1 change: 1 addition & 0 deletions tests/auto/qml/qqmlcontext/data/ContextLeak.js
@@ -0,0 +1 @@
var value = 42
5 changes: 5 additions & 0 deletions tests/auto/qml/qqmlcontext/data/contextLeak.qml
@@ -0,0 +1,5 @@
import QtQml 2.0
import "ContextLeak.js" as ContextLeak
QtObject {
property int value: ContextLeak.value
}
37 changes: 37 additions & 0 deletions tests/auto/qml/qqmlcontext/tst_qqmlcontext.cpp
Expand Up @@ -33,6 +33,8 @@
#include <QQmlComponent>
#include <QQmlExpression>
#include <private/qqmlcontext_p.h>
#include <private/qv4qmlcontext_p.h>
#include <private/qv4object_p.h>
#include "../../shared/util.h"

class tst_qqmlcontext : public QQmlDataTest
Expand Down Expand Up @@ -63,6 +65,7 @@ private slots:
void qobjectDerived();
void qtbug_49232();
void contextViaClosureAfterDestruction();
void contextLeak();

private:
QQmlEngine engine;
Expand Down Expand Up @@ -751,6 +754,40 @@ void tst_qqmlcontext::contextViaClosureAfterDestruction()
QCOMPARE(subObject.toString(), QLatin1String("Error: Qt.createQmlObject(): Cannot create a component in an invalid context"));
}

void tst_qqmlcontext::contextLeak()
{
QQmlEngine engine;
QQmlComponent component(&engine, testFileUrl("contextLeak.qml"));

QQmlGuardedContextData scriptContext;

{
QScopedPointer<QObject> obj(component.create());
QVERIFY(!obj.isNull());
QCOMPARE(obj->property("value").toInt(), 42);

QQmlData *ddata = QQmlData::get(obj.data());
QVERIFY(ddata);
QQmlContextData *context = ddata->context;
QVERIFY(context);
QVERIFY(!context->importedScripts.isNullOrUndefined());
QCOMPARE(int(context->importedScripts.valueRef()->as<QV4::Object>()->getLength()), 1);

{
QV4::Scope scope(ddata->jsWrapper.engine());
QV4::ScopedValue scriptContextWrapper(scope);
scriptContextWrapper = context->importedScripts.valueRef()->as<QV4::Object>()->getIndexed(0);
scriptContext = scriptContextWrapper->as<QV4::QmlContextWrapper>()->getContext();
}
}

engine.collectGarbage();

// Each time a JS file (non-pragma-shared) is imported, we create a QQmlContext(Data) for it.
// Make sure that context does not leak.
QVERIFY(scriptContext.isNull());
}

QTEST_MAIN(tst_qqmlcontext)

#include "tst_qqmlcontext.moc"

0 comments on commit d4ff290

Please sign in to comment.