Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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 <qt_sanity_bot@ovi.com>
Reviewed-by: Simon Hausmann <simon.hausmann@nokia.com>
  • Loading branch information
Simon Hausmann authored and Qt by Nokia committed Oct 6, 2011
1 parent 6e7ea2f commit ab98961
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 23 deletions.
2 changes: 1 addition & 1 deletion src/declarative/qml/v8/qjsengine.cpp
Expand Up @@ -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));
}

/*!
Expand Down
19 changes: 19 additions & 0 deletions src/declarative/qml/v8/qv8engine_p.h
Expand Up @@ -228,6 +228,10 @@ class Q_DECLARATIVE_EXPORT QV8Engine
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() {}
};
Expand Down Expand Up @@ -311,6 +315,7 @@ class Q_DECLARATIVE_EXPORT QV8Engine

// Return a JS wrapper for the given QObject \a object
inline v8::Handle<v8::Value> newQObject(QObject *object);
inline v8::Handle<v8::Value> newQObject(QObject *object, const ObjectOwnership ownership);
inline bool isQObject(v8::Handle<v8::Value>);
inline QObject *toQObject(v8::Handle<v8::Value>);

Expand Down Expand Up @@ -511,6 +516,20 @@ v8::Handle<v8::Value> QV8Engine::newQObject(QObject *object)
return m_qobjectWrapper.newQObject(object);
}

v8::Handle<v8::Value> QV8Engine::newQObject(QObject *object, const ObjectOwnership ownership)
{
if (!object)
return v8::Null();

v8::Handle<v8::Value> result = newQObject(object);
QDeclarativeData *ddata = QDeclarativeData::get(object, true);
if (ownership == JavaScriptOwnership && ddata) {
ddata->indestructible = false;
ddata->explicitIndestructibleSet = true;
}
return result;
}

v8::Local<v8::String> QV8Engine::toString(const QString &string)
{
return m_stringWrapper.toString(string);
Expand Down
40 changes: 18 additions & 22 deletions tests/auto/declarative/qjsengine/tst_qjsengine.cpp
Expand Up @@ -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
Expand Down Expand Up @@ -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<QObject> 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<QObject> ptr = new QObject();
QPointer<QObject> 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);
Expand All @@ -1001,27 +998,26 @@ void tst_QJSEngine::newQObject_ownership()
QPointer<QObject> 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);
}
{
QObject *parent = new QObject();
QPointer<QObject> 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()
Expand Down Expand Up @@ -3097,21 +3093,21 @@ void tst_QJSEngine::castWithMultipleInheritance()
QCOMPARE(qjsvalue_cast<QGraphicsItem*>(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<QObject> 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()
Expand Down

0 comments on commit ab98961

Please sign in to comment.