From d4ff2907162e20e2288357c428b0df88f7396f92 Mon Sep 17 00:00:00 2001
From: Simon Hausmann <simon.hausmann@qt.io>
Date: Thu, 8 Feb 2018 16:43:08 +0100
Subject: Fix memory leak with JS imports

Strictly speaking this is a regression introduced with commit
e22b624d9ab1f36021adb9cdbfa9b37054282bb8, 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>
---
 tests/auto/qml/qqmlcontext/data/ContextLeak.js  |  1 +
 tests/auto/qml/qqmlcontext/data/contextLeak.qml |  5 ++++
 tests/auto/qml/qqmlcontext/tst_qqmlcontext.cpp  | 37 +++++++++++++++++++++++++
 3 files changed, 43 insertions(+)
 create mode 100644 tests/auto/qml/qqmlcontext/data/ContextLeak.js
 create mode 100644 tests/auto/qml/qqmlcontext/data/contextLeak.qml

(limited to 'tests/auto/qml')

diff --git a/tests/auto/qml/qqmlcontext/data/ContextLeak.js b/tests/auto/qml/qqmlcontext/data/ContextLeak.js
new file mode 100644
index 0000000000..e43b1bb230
--- /dev/null
+++ b/tests/auto/qml/qqmlcontext/data/ContextLeak.js
@@ -0,0 +1 @@
+var value = 42
diff --git a/tests/auto/qml/qqmlcontext/data/contextLeak.qml b/tests/auto/qml/qqmlcontext/data/contextLeak.qml
new file mode 100644
index 0000000000..515b3a1aa2
--- /dev/null
+++ b/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
+}
diff --git a/tests/auto/qml/qqmlcontext/tst_qqmlcontext.cpp b/tests/auto/qml/qqmlcontext/tst_qqmlcontext.cpp
index 5f57b9ebb0..574e6e2834 100644
--- a/tests/auto/qml/qqmlcontext/tst_qqmlcontext.cpp
+++ b/tests/auto/qml/qqmlcontext/tst_qqmlcontext.cpp
@@ -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
@@ -63,6 +65,7 @@ private slots:
     void qobjectDerived();
     void qtbug_49232();
     void contextViaClosureAfterDestruction();
+    void contextLeak();
 
 private:
     QQmlEngine engine;
@@ -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"
-- 
cgit v1.2.3