Ensure QObject wrappers are garbage-collected if appropriate

Address the "###" comment.
When QScriptEngine::newQObject() is called with the option
PreferExistingWrapperObject, the resulting wrapper object is cached
by the engine, in case it will be needed later (e.g., by a subsequent
newQObject() call for the same QObject, with the same wrap options).

But if a QObject wrapper object is only referenced by the QtScript
internals (i.e., not reachable from the JSC stack/heap, or kept in a
QScriptValue), the wrapper should not be kept alive if the ownership
is ScriptOwnership, or if the ownership is AutoOwnership and the C++
object has no parent.

If the wrapper is marked in that case, it won't get collected, and
hence the C++ object will be kept alive, too. In practice, QtScript
appears to leak memory (the objects will only be destroyed when the
engine is destroyed).

Our copy of JSC doesn't have a concept of weak references; the
ClientData callback in the JSC markRoots() function (which causes
QScriptEnginePrivate::mark() to be called) was moved to the end. This
enables the wrapper and connection marking logic to determine whether
a wrapper can be safely discarded (if it hasn't been marked by JSC by
this point, it must be a weak reference).

Task-number: QTBUG-21993
Change-Id: I61f6aafc91f080b80d3f5859148e645b80d9b653
Reviewed-by: Olivier Goffart <ogoffart@woboq.com>

1 files changed, 13 insertions, 5 deletions

diff --git a/src/script/bridge/qscriptqobject.cpp b/src/script/bridge/qscriptqobject.cpp
index 2bc2547..15288a7 100644
--- a/src/script/bridge/qscriptqobject.cpp
+++ b/src/script/bridge/qscriptqobject.cpp
@@ -97,7 +97,7 @@ struct QObjectConnection
                 QObjectDelegate *inst = static_cast<QObjectDelegate*>(delegate);
                 if ((inst->ownership() == QScriptEngine::ScriptOwnership)
                     || ((inst->ownership() == QScriptEngine::AutoOwnership)
-                        && inst->value() && !inst->value()->parent())) {
+                        && !inst->hasParent())) {
                     senderWrapper = JSC::JSValue();
                 } else {
                     markStack.append(senderWrapper);
@@ -2199,6 +2199,10 @@ QObjectData::~QObjectData()
     }
 }
 
+// This function assumes all objects reachable elsewhere in the JS environment
+// (stack, heap) have been marked already (see QScriptEnginePrivate::mark()).
+// This determines whether any of QtScript's internal QObject wrappers are only
+// weakly referenced and can be discarded.
 void QObjectData::mark(JSC::MarkStack& markStack)
 {
     if (connectionManager)
@@ -2207,10 +2211,14 @@ void QObjectData::mark(JSC::MarkStack& markStack)
         QList<QScript::QObjectWrapperInfo>::iterator it;
         for (it = wrappers.begin(); it != wrappers.end(); ) {
             const QScript::QObjectWrapperInfo &info = *it;
-            // ### don't mark if there are no other references.
-            // we need something like isMarked()
-            markStack.append(info.object);
-            ++it;
+            if (JSC::Heap::isCellMarked(info.object)) {
+                ++it;
+            } else if (info.isCollectableWhenWeaklyReferenced()) {
+                it = wrappers.erase(it);
+            } else {
+                markStack.append(info.object);
+                ++it;
+            }
         }
     }
 }