Fix GC issues related to QObject connections and ownership

Issue 1: Even if a sender object was only weakly referenced, the
connection's slot function would still get marked. If the slot was a
closure, its scope could hold a reference to the sender object, so by
marking the closure, the sender would get marked, too - even if there
were no other references to the closure outside of the QObject
connection structure. This would cause the sender object to stay
alive, rather than being garbage-collected (i.e., it leaked).

Issue 2: It's possible that a closure used as a slot in a connection
for one QObject holds the only reference to another QObject that has
connections of its own. In that case, if the first object is
explicitly referenced, the second object (and its connections) should
get marked. But depending on the order in which the connections were
marked, the second object might get treated incorrectly.

This commit solves both issues by introducing an iterative scheme for
marking connections. The first pass marks only connections whose
sender object is referenced elsewhere in the JS environment. The
second pass marks connections whose sender object is referenced by
slots of the connections marked in the first pass. And so on, until
no more connections should be marked. At that point, any remaining
unmarked connections are effectively dead (belonging to QObjects that
can be reclaimed by the GC).

Task-number: QTBUG-26590
Change-Id: I50aa66f7fe407a6827b6f2a12e027275a2fb4655
Reviewed-by: Simon Hausmann <simon.hausmann@nokia.com>

2 files changed, 68 insertions, 0 deletions

diff --git a/tests/auto/qscriptextqobject/qscriptextqobject.pro b/tests/auto/qscriptextqobject/qscriptextqobject.pro
index 8ddebef..784037f 100644
--- a/tests/auto/qscriptextqobject/qscriptextqobject.pro
+++ b/tests/auto/qscriptextqobject/qscriptextqobject.pro
@@ -2,4 +2,5 @@ TARGET = tst_qscriptextqobject
 CONFIG += testcase
 QT = core gui widgets script testlib
 SOURCES  += tst_qscriptextqobject.cpp
+include(../shared/util.pri)
 DEFINES += QT_DISABLE_DEPRECATED_BEFORE=0
diff --git a/tests/auto/qscriptextqobject/tst_qscriptextqobject.cpp b/tests/auto/qscriptextqobject/tst_qscriptextqobject.cpp
index bbf72ad..43e640a 100644
--- a/tests/auto/qscriptextqobject/tst_qscriptextqobject.cpp
+++ b/tests/auto/qscriptextqobject/tst_qscriptextqobject.cpp
@@ -50,6 +50,8 @@
 #include <qpushbutton.h>
 #include <qlineedit.h>
 
+#include "../shared/util.h"
+
 struct CustomType
 {
     QString string;
@@ -586,6 +588,8 @@ private slots:
     void nestedObjectAsSlotArgument();
     void propertyAccessThroughActivationObject();
     void connectionRemovedAfterQueuedCall();
+    void collectQObjectWithClosureSlot();
+    void collectQObjectWithClosureSlot2();
 
 private:
     QScriptEngine *m_engine;
@@ -3705,5 +3709,68 @@ void tst_QScriptExtQObject::connectionRemovedAfterQueuedCall()
     QVERIFY(m_engine->evaluate("pass").toBool());
 }
 
+// QTBUG-26590
+void tst_QScriptExtQObject::collectQObjectWithClosureSlot()
+{
+    QScriptEngine eng;
+    QScriptValue fun = eng.evaluate("(function(obj) {\n"
+        "    obj.mySignal.connect(function() { obj.mySlot(); });\n"
+        "})");
+    QVERIFY(fun.isFunction());
+
+    QPointer<MyQObject> obj = new MyQObject;
+    {
+        QScriptValue wrapper = eng.newQObject(obj, QScriptEngine::ScriptOwnership);
+        QVERIFY(fun.call(QScriptValue(), QScriptValueList() << wrapper).isUndefined());
+    }
+    QVERIFY(obj != 0);
+    QCOMPARE(obj->qtFunctionInvoked(), -1);
+    obj->emitMySignal();
+    QCOMPARE(obj->qtFunctionInvoked(), 20);
+
+    collectGarbage_helper(eng);
+    // The closure that's connected to obj's signal has the only JS reference
+    // to obj, and the closure is only referenced by the connection. Hence, obj
+    // should have been collected.
+    if (obj != 0)
+        QEXPECT_FAIL("", "Test can fail because the garbage collector is conservative", Continue);
+    QVERIFY(obj == 0);
+}
+
+void tst_QScriptExtQObject::collectQObjectWithClosureSlot2()
+{
+    QScriptEngine eng;
+    QScriptValue fun = eng.evaluate("(function(obj1, obj2) {\n"
+        "    obj2.mySignal.connect(function() { obj1.mySlot(); });\n"
+        "    obj1.mySignal.connect(function() { obj2.mySlot(); });\n"
+        "})");
+    QVERIFY(fun.isFunction());
+
+    QPointer<MyQObject> obj1 = new MyQObject;
+    QScriptValue wrapper1 = eng.newQObject(obj1, QScriptEngine::ScriptOwnership);
+    QPointer<MyQObject> obj2 = new MyQObject;
+    {
+        QScriptValue wrapper2 = eng.newQObject(obj2, QScriptEngine::ScriptOwnership);
+        QVERIFY(fun.call(QScriptValue(), QScriptValueList() << wrapper1 << wrapper2).isUndefined());
+    }
+    QVERIFY(obj1 != 0);
+    QVERIFY(obj2 != 0);
+
+    collectGarbage_helper(eng);
+    // obj1 is referenced by a QScriptValue, so it (and its connections) should not be collected.
+    QVERIFY(obj1 != 0);
+    // obj2 is referenced from the closure that's connected to obj1's signal, so it
+    // (and its connections) should not be collected.
+    QVERIFY(obj2 != 0);
+
+    QCOMPARE(obj2->qtFunctionInvoked(), -1);
+    obj1->emitMySignal();
+    QCOMPARE(obj2->qtFunctionInvoked(), 20);
+
+    QCOMPARE(obj1->qtFunctionInvoked(), -1);
+    obj2->emitMySignal();
+    QCOMPARE(obj1->qtFunctionInvoked(), 20);
+}
+
 QTEST_MAIN(tst_QScriptExtQObject)
 #include "tst_qscriptextqobject.moc"