From ab989615c05bbf15ef7f5b91d3daaf2423a490ea Mon Sep 17 00:00:00 2001 From: Simon Hausmann Date: Fri, 29 Jul 2011 16:04:26 +0200 Subject: [PATCH] Fix QJSEngine::newQObject ownership behaviour Ensure the indestructible flag is set to false for objects wrapped through QJSEngine::newQObject, to ensure that they get deleted upon gc when they don't have a parent. Re-enabled the QJSEngine::garbageCollect and ownership auto-tests that verified this behaviour. Change-Id: I181bff0cc44d071d650a2f73494e49cce6ad538e Reviewed-on: http://codereview.qt-project.org/2398 Reviewed-by: Qt Sanity Bot Reviewed-by: Simon Hausmann --- src/declarative/qml/v8/qjsengine.cpp | 2 +- src/declarative/qml/v8/qv8engine_p.h | 19 +++++++++ tests/auto/declarative/qjsengine/tst_qjsengine.cpp | 40 +++++++++----------- 3 files changed, 38 insertions(+), 23 deletions(-) diff --git a/src/declarative/qml/v8/qjsengine.cpp b/src/declarative/qml/v8/qjsengine.cpp index 3db7fc4..e466afc 100644 --- a/src/declarative/qml/v8/qjsengine.cpp +++ b/src/declarative/qml/v8/qjsengine.cpp @@ -378,7 +378,7 @@ QJSValue QJSEngine::newQObject(QObject *object) Q_D(QJSEngine); QScriptIsolate api(d, QScriptIsolate::NotNullEngine); v8::HandleScope handleScope; - return d->scriptValueFromInternal(d->newQObject(object)); + return d->scriptValueFromInternal(d->newQObject(object, QV8Engine::JavaScriptOwnership)); } /*! diff --git a/src/declarative/qml/v8/qv8engine_p.h b/src/declarative/qml/v8/qv8engine_p.h index 185eb54..e8163fb 100644 --- a/src/declarative/qml/v8/qv8engine_p.h +++ b/src/declarative/qml/v8/qv8engine_p.h @@ -228,6 +228,10 @@ public: QV8Engine(QJSEngine* qq,QJSEngine::ContextOwnership ownership = QJSEngine::CreateNewContext); ~QV8Engine(); + // ### TODO get rid of it, do we really need CppOwnership? + // This enum should be in sync with QDeclarativeEngine::ObjectOwnership + enum ObjectOwnership { CppOwnership, JavaScriptOwnership }; + struct Deletable { virtual ~Deletable() {} }; @@ -311,6 +315,7 @@ public: // Return a JS wrapper for the given QObject \a object inline v8::Handle newQObject(QObject *object); + inline v8::Handle newQObject(QObject *object, const ObjectOwnership ownership); inline bool isQObject(v8::Handle); inline QObject *toQObject(v8::Handle); @@ -511,6 +516,20 @@ v8::Handle QV8Engine::newQObject(QObject *object) return m_qobjectWrapper.newQObject(object); } +v8::Handle QV8Engine::newQObject(QObject *object, const ObjectOwnership ownership) +{ + if (!object) + return v8::Null(); + + v8::Handle result = newQObject(object); + QDeclarativeData *ddata = QDeclarativeData::get(object, true); + if (ownership == JavaScriptOwnership && ddata) { + ddata->indestructible = false; + ddata->explicitIndestructibleSet = true; + } + return result; +} + v8::Local QV8Engine::toString(const QString &string) { return m_stringWrapper.toString(string); diff --git a/tests/auto/declarative/qjsengine/tst_qjsengine.cpp b/tests/auto/declarative/qjsengine/tst_qjsengine.cpp index 6907fb0..6b18bac 100644 --- a/tests/auto/declarative/qjsengine/tst_qjsengine.cpp +++ b/tests/auto/declarative/qjsengine/tst_qjsengine.cpp @@ -182,9 +182,7 @@ private slots: void castWithPrototypeChain(); #endif void castWithMultipleInheritance(); -#if 0 // ###FIXME: ScriptOwnership void collectGarbage(); -#endif #if 0 // ###FIXME: no reportAdditionalMemoryCost API void reportAdditionalMemoryCost(); #endif @@ -965,34 +963,33 @@ void tst_QJSEngine::newQObject() void tst_QJSEngine::newQObject_ownership() { -#if 0 // FIXME: ownership tests need to be revivewed - QScriptEngine eng; + QJSEngine eng; { QPointer ptr = new QObject(); QVERIFY(ptr != 0); { - QScriptValue v = eng.newQObject(ptr, QScriptEngine::ScriptOwnership); + QJSValue v = eng.newQObject(ptr); } - eng.evaluate("gc()"); + collectGarbage_helper(eng); if (ptr) - QEXPECT_FAIL("", "In the JSC-based back-end, script-owned QObjects are not always deleted immediately during GC", Continue); + QApplication::sendPostedEvents(ptr, QEvent::DeferredDelete); QVERIFY(ptr == 0); } { - QPointer ptr = new QObject(); + QPointer ptr = new QObject(this); QVERIFY(ptr != 0); { - QScriptValue v = eng.newQObject(ptr, QScriptEngine::QtOwnership); + QJSValue v = eng.newQObject(ptr); } QObject *before = ptr; - eng.evaluate("gc()"); + collectGarbage_helper(eng); QVERIFY(ptr == before); delete ptr; } { QObject *parent = new QObject(); QObject *child = new QObject(parent); - QScriptValue v = eng.newQObject(child, QScriptEngine::QtOwnership); + QJSValue v = eng.newQObject(child); QCOMPARE(v.toQObject(), child); delete parent; QCOMPARE(v.toQObject(), (QObject *)0); @@ -1001,12 +998,12 @@ void tst_QJSEngine::newQObject_ownership() QPointer ptr = new QObject(); QVERIFY(ptr != 0); { - QScriptValue v = eng.newQObject(ptr, QScriptEngine::AutoOwnership); + QJSValue v = eng.newQObject(ptr); } - eng.evaluate("gc()"); + collectGarbage_helper(eng); // no parent, so it should be like ScriptOwnership if (ptr) - QEXPECT_FAIL("", "In the JSC-based back-end, script-owned QObjects are not always deleted immediately during GC", Continue); + QApplication::sendPostedEvents(ptr, QEvent::DeferredDelete); QVERIFY(ptr == 0); } { @@ -1014,14 +1011,13 @@ void tst_QJSEngine::newQObject_ownership() QPointer child = new QObject(parent); QVERIFY(child != 0); { - QScriptValue v = eng.newQObject(child, QScriptEngine::AutoOwnership); + QJSValue v = eng.newQObject(child); } - eng.evaluate("gc()"); + collectGarbage_helper(eng); // has parent, so it should be like QtOwnership QVERIFY(child != 0); delete parent; } -#endif } void tst_QJSEngine::newQObject_promoteObject() @@ -3097,21 +3093,21 @@ void tst_QJSEngine::castWithMultipleInheritance() QCOMPARE(qjsvalue_cast(v), (QGraphicsItem *)&klz); } -#if 0 // ###FIXME: ScriptOwnership void tst_QJSEngine::collectGarbage() { - QScriptEngine eng; + QJSEngine eng; eng.evaluate("a = new Object(); a = new Object(); a = new Object()"); - QScriptValue a = eng.newObject(); + QJSValue a = eng.newObject(); a = eng.newObject(); a = eng.newObject(); QPointer ptr = new QObject(); QVERIFY(ptr != 0); - (void)eng.newQObject(ptr, QScriptEngine::ScriptOwnership); + (void)eng.newQObject(ptr); collectGarbage_helper(eng); + if (ptr) + QApplication::sendPostedEvents(ptr, QEvent::DeferredDelete); QVERIFY(ptr == 0); } -#endif #if 0 // ###FIXME: no reportAdditionalMemoryCost API void tst_QJSEngine::reportAdditionalMemoryCost() -- 1.7.2.5