From 121ef3c9416836ce147f092103498c056c40cd28 Mon Sep 17 00:00:00 2001 From: Ivailo Monev Date: Tue, 26 Jan 2016 17:33:20 +0200 Subject: [PATCH] fix GC issues related to QObject connections and ownership upstream commits: https://github.com/qtproject/qtscript/commit/d7bba09f263a522d36fd4a704e6667e7a9efd297 Signed-off-by: Ivailo Monev --- src/script/api/qscriptengine.cpp | 40 +++++++++++--- src/script/api/qscriptengine_p.h | 1 + src/script/bridge/qscriptqobject.cpp | 101 +++++++++++++++++++++++++---------- src/script/bridge/qscriptqobject_p.h | 4 +- 4 files changed, 108 insertions(+), 38 deletions(-) diff --git a/src/script/api/qscriptengine.cpp b/src/script/api/qscriptengine.cpp index a16703adc..02dad2029 100644 --- a/src/script/api/qscriptengine.cpp +++ b/src/script/api/qscriptengine.cpp @@ -1312,14 +1312,7 @@ void QScriptEnginePrivate::mark(JSC::MarkStack& markStack) } #ifndef QT_NO_QOBJECT - markStack.drain(); // make sure everything is marked before marking qobject data - { - QHash::const_iterator it; - for (it = m_qobjectData.constBegin(); it != m_qobjectData.constEnd(); ++it) { - QScript::QObjectData *qdata = it.value(); - qdata->mark(markStack); - } - } + markQObjectData(markStack); #endif } @@ -1467,6 +1460,37 @@ void QScriptEnginePrivate::uncaughtException(JSC::ExecState *exec, unsigned byte #ifndef QT_NO_QOBJECT +void QScriptEnginePrivate::markQObjectData(JSC::MarkStack& markStack) +{ + QHash::const_iterator it; + // 1. Clear connection mark bits for all objects + for (it = m_qobjectData.constBegin(); it != m_qobjectData.constEnd(); ++it) { + QScript::QObjectData *qdata = it.value(); + qdata->clearConnectionMarkBits(); + } + + // 2. Iterate until no more connections are marked + int markedCount; + do { + // Drain the stack to ensure mark bits are set; this is used to determine + // whether a connection's sender object is weakly referenced + markStack.drain(); + + markedCount = 0; + for (it = m_qobjectData.constBegin(); it != m_qobjectData.constEnd(); ++it) { + QScript::QObjectData *qdata = it.value(); + markedCount += qdata->markConnections(markStack); + } + } while (markedCount > 0); + markStack.drain(); // One last time before marking wrappers + + // 3. Mark all wrappers + for (it = m_qobjectData.constBegin(); it != m_qobjectData.constEnd(); ++it) { + QScript::QObjectData *qdata = it.value(); + qdata->markWrappers(markStack); + } +} + JSC::JSValue QScriptEnginePrivate::newQObject( QObject *object, QScriptEngine::ValueOwnership ownership, const QScriptEngine::QObjectWrapOptions &options) diff --git a/src/script/api/qscriptengine_p.h b/src/script/api/qscriptengine_p.h index c2f71ee95..e5c85a3f8 100644 --- a/src/script/api/qscriptengine_p.h +++ b/src/script/api/qscriptengine_p.h @@ -324,6 +324,7 @@ public: JSC::UString translationContextFromUrl(const JSC::UString &); #ifndef QT_NO_QOBJECT + void markQObjectData(JSC::MarkStack&); JSC::JSValue newQObject(QObject *object, QScriptEngine::ValueOwnership ownership = QScriptEngine::QtOwnership, const QScriptEngine:: QObjectWrapOptions &options = 0); diff --git a/src/script/bridge/qscriptqobject.cpp b/src/script/bridge/qscriptqobject.cpp index e4601c6f7..62b8aad1b 100644 --- a/src/script/bridge/qscriptqobject.cpp +++ b/src/script/bridge/qscriptqobject.cpp @@ -61,15 +61,16 @@ namespace QScript struct QObjectConnection { - int slotIndex; + uint marked:1; + uint slotIndex:31; JSC::JSValue receiver; JSC::JSValue slot; JSC::JSValue senderWrapper; QObjectConnection(int i, JSC::JSValue r, JSC::JSValue s, JSC::JSValue sw) - : slotIndex(i), receiver(r), slot(s), senderWrapper(sw) {} - QObjectConnection() : slotIndex(-1) {} + : marked(false), slotIndex(i), receiver(r), slot(s), senderWrapper(sw) {} + QObjectConnection() : marked(false), slotIndex(0) {} bool hasTarget(JSC::JSValue r, JSC::JSValue s) const { @@ -82,12 +83,9 @@ struct QObjectConnection return (s == slot); } - void mark(JSC::MarkStack& markStack) + bool hasWeaklyReferencedSender() const { if (senderWrapper) { - // see if the sender should be marked or not; - // if the C++ object is owned by script, we don't want - // it to stay alive due to a script connection. Q_ASSERT(senderWrapper.inherits(&QScriptObject::info)); QScriptObject *scriptObject = static_cast(JSC::asObject(senderWrapper)); if (!JSC::Heap::isCellMarked(scriptObject)) { @@ -97,16 +95,23 @@ struct QObjectConnection if ((inst->ownership() == QScriptEngine::ScriptOwnership) || ((inst->ownership() == QScriptEngine::AutoOwnership) && !inst->hasParent())) { - senderWrapper = JSC::JSValue(); - } else { - markStack.append(senderWrapper); + return true; } } } + return false; + } + + void mark(JSC::MarkStack& markStack) + { + Q_ASSERT(!marked); + if (senderWrapper) + markStack.append(senderWrapper); if (receiver) markStack.append(receiver); if (slot) markStack.append(slot); + marked = true; } }; @@ -135,7 +140,8 @@ public: JSC::JSValue receiver, JSC::JSValue slot); - void mark(JSC::MarkStack&); + void clearMarkBits(); + int mark(JSC::MarkStack&); // private slots: void execute(int slotIndex, void **argv); @@ -2250,13 +2256,41 @@ QObjectConnectionManager::~QObjectConnectionManager() { } -void QObjectConnectionManager::mark(JSC::MarkStack& markStack) +void QObjectConnectionManager::clearMarkBits() { for (int i = 0; i < connections.size(); ++i) { QVector &cs = connections[i]; for (int j = 0; j < cs.size(); ++j) - cs[j].mark(markStack); + cs[j].marked = false; + } +} + +/*! + \internal + + Marks connections owned by this manager. + Returns the number of connections that were marked by this pass + (i.e., excluding connections that were already marked). +*/ +int QObjectConnectionManager::mark(JSC::MarkStack& markStack) +{ + int markedCount = 0; + for (int i = 0; i < connections.size(); ++i) { + QVector &cs = connections[i]; + for (int j = 0; j < cs.size(); ++j) { + QObjectConnection &c = cs[j]; + if (!c.marked) { + if (c.hasWeaklyReferencedSender()) { + // Don't mark the connection; we don't want the script-owned + // sender object to stay alive merely due to a connection. + } else { + c.mark(markStack); + ++markedCount; + } + } + } } + return markedCount; } bool QObjectConnectionManager::addSignalHandler( @@ -2319,26 +2353,35 @@ QObjectData::~QObjectData() } } +void QObjectData::clearConnectionMarkBits() +{ + if (connectionManager) + connectionManager->clearMarkBits(); +} + +int QObjectData::markConnections(JSC::MarkStack& markStack) +{ + if (connectionManager) + return connectionManager->mark(markStack); + return 0; +} + // 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) - connectionManager->mark(markStack); - { - QList::iterator it; - for (it = wrappers.begin(); it != wrappers.end(); ) { - const QScript::QObjectWrapperInfo &info = *it; - if (JSC::Heap::isCellMarked(info.object)) { - ++it; - } else if (info.isCollectableWhenWeaklyReferenced()) { - it = wrappers.erase(it); - } else { - markStack.append(info.object); - ++it; - } +void QObjectData::markWrappers(JSC::MarkStack& markStack) +{ + QList::iterator it; + for (it = wrappers.begin(); it != wrappers.end(); ) { + const QScript::QObjectWrapperInfo &info = *it; + if (JSC::Heap::isCellMarked(info.object)) { + ++it; + } else if (info.isCollectableWhenWeaklyReferenced()) { + it = wrappers.erase(it); + } else { + markStack.append(info.object); + ++it; } } } diff --git a/src/script/bridge/qscriptqobject_p.h b/src/script/bridge/qscriptqobject_p.h index cf44de996..6af010aa1 100644 --- a/src/script/bridge/qscriptqobject_p.h +++ b/src/script/bridge/qscriptqobject_p.h @@ -183,7 +183,9 @@ public: QScriptEngine::ValueOwnership ownership, const QScriptEngine::QObjectWrapOptions &options); - void mark(JSC::MarkStack&); + void clearConnectionMarkBits(); + int markConnections(JSC::MarkStack&); + void markWrappers(JSC::MarkStack&); private: QScriptEnginePrivate *engine; -- 2.11.0