From 25793276e52240e4dfad297dc5b9eb282ed3f5e6 Mon Sep 17 00:00:00 2001 From: Chris Adams Date: Tue, 13 Mar 2012 13:30:39 +1000 Subject: [PATCH] Fix crash caused by dereferencing collected v8 data If a var property of a QObject is read after the v8 data associated with the qobject has been deleted but prior to the DeferredDelete event being processed, the varProperties array will be null and a crash will occur. This patch ensures that we check for this condition in both the access and set codepaths for var properties, and also ensures that an object which has previously been queued for deletion cannot be referenced in JS. Finally, it adds a unit test to ensure that we don't regress. Task-number: QTBUG-24748 Change-Id: Idde384ca01e18f4dcf9e376e9379f2c5eb410e14 Reviewed-by: Michael Brasser --- src/qml/qml/qqmldata_p.h | 7 +++-- src/qml/qml/qqmlvmemetaobject.cpp | 25 ++++++++++++----- src/qml/qml/qqmlvmemetaobject_p.h | 2 +- src/qml/qml/v8/qv8qobjectwrapper.cpp | 9 ++++-- .../data/ComponentWithVarProp.qml | 5 ++++ .../data/propertyVarOwnership.5.qml | 28 +++++++++++++++++++ tests/auto/qml/qqmlecmascript/testtypes.h | 8 +++++- .../qml/qqmlecmascript/tst_qqmlecmascript.cpp | 9 ++++++ 8 files changed, 79 insertions(+), 14 deletions(-) create mode 100644 tests/auto/qml/qqmlecmascript/data/ComponentWithVarProp.qml create mode 100644 tests/auto/qml/qqmlecmascript/data/propertyVarOwnership.5.qml diff --git a/src/qml/qml/qqmldata_p.h b/src/qml/qml/qqmldata_p.h index e4ba44583d..09d1a23510 100644 --- a/src/qml/qml/qqmldata_p.h +++ b/src/qml/qml/qqmldata_p.h @@ -78,8 +78,8 @@ class Q_QML_EXPORT QQmlData : public QAbstractDeclarativeData public: QQmlData() : ownMemory(true), ownContext(false), indestructible(true), explicitIndestructibleSet(false), - hasTaintedV8Object(false), notifyList(0), context(0), outerContext(0), bindings(0), - nextContextObject(0), prevContextObject(0), bindingBitsSize(0), bindingBits(0), + hasTaintedV8Object(false), isQueuedForDeletion(false), notifyList(0), context(0), outerContext(0), + bindings(0), nextContextObject(0), prevContextObject(0), bindingBitsSize(0), bindingBits(0), lineNumber(0), columnNumber(0), deferredComponent(0), deferredIdx(0), v8objectid(0), propertyCache(0), guards(0), extendedData(0) { init(); @@ -110,7 +110,8 @@ class Q_QML_EXPORT QQmlData : public QAbstractDeclarativeData quint32 indestructible:1; quint32 explicitIndestructibleSet:1; quint32 hasTaintedV8Object:1; - quint32 dummy:27; + quint32 isQueuedForDeletion:1; + quint32 dummy:26; struct NotifyList { quint64 connectionMask; diff --git a/src/qml/qml/qqmlvmemetaobject.cpp b/src/qml/qml/qqmlvmemetaobject.cpp index ecfde203c6..cc4ba091ce 100644 --- a/src/qml/qml/qqmlvmemetaobject.cpp +++ b/src/qml/qml/qqmlvmemetaobject.cpp @@ -832,15 +832,17 @@ v8::Handle QQmlVMEMetaObject::readVarProperty(int id) { Q_ASSERT(id >= firstVarPropertyIndex); - ensureVarPropertiesAllocated(); - return varProperties->Get(id - firstVarPropertyIndex); + if (ensureVarPropertiesAllocated()) + return varProperties->Get(id - firstVarPropertyIndex); + return v8::Handle(); } QVariant QQmlVMEMetaObject::readPropertyAsVariant(int id) { if (id >= firstVarPropertyIndex) { - ensureVarPropertiesAllocated(); - return QQmlEnginePrivate::get(ctxt->engine)->v8engine()->toVariant(varProperties->Get(id - firstVarPropertyIndex), -1); + if (ensureVarPropertiesAllocated()) + return QQmlEnginePrivate::get(ctxt->engine)->v8engine()->toVariant(varProperties->Get(id - firstVarPropertyIndex), -1); + return QVariant(); } else { if (data[id].dataType() == QMetaType::QObjectStar) { return QVariant::fromValue(data[id].asQObject()); @@ -853,7 +855,8 @@ QVariant QQmlVMEMetaObject::readPropertyAsVariant(int id) void QQmlVMEMetaObject::writeVarProperty(int id, v8::Handle value) { Q_ASSERT(id >= firstVarPropertyIndex); - ensureVarPropertiesAllocated(); + if (!ensureVarPropertiesAllocated()) + return; // Importantly, if the current value is a scarce resource, we need to ensure that it // gets automatically released by the engine if no other references to it exist. @@ -882,7 +885,8 @@ void QQmlVMEMetaObject::writeVarProperty(int id, v8::Handle value) void QQmlVMEMetaObject::writeProperty(int id, const QVariant &value) { if (id >= firstVarPropertyIndex) { - ensureVarPropertiesAllocated(); + if (!ensureVarPropertiesAllocated()) + return; // Importantly, if the current value is a scarce resource, we need to ensure that it // gets automatically released by the engine if no other references to it exist. @@ -1029,10 +1033,17 @@ void QQmlVMEMetaObject::setVMEProperty(int index, v8::Handle v) return writeVarProperty(index - propOffset, v); } -void QQmlVMEMetaObject::ensureVarPropertiesAllocated() +bool QQmlVMEMetaObject::ensureVarPropertiesAllocated() { if (!varPropertiesInitialized) allocateVarPropertiesArray(); + + // in some situations, the QObject's v8object (and associated v8 data, + // such as the varProperties array) will have been cleaned up, but the + // QObject ptr will not yet have been deleted (eg, waiting on deleteLater). + // In this situation, the varProperties handle will be (and should remain) + // empty. + return !varProperties.IsEmpty(); } // see also: QV8GCCallback::garbageCollectorPrologueCallback() diff --git a/src/qml/qml/qqmlvmemetaobject_p.h b/src/qml/qml/qqmlvmemetaobject_p.h index 84c88fdd7b..1b5ceb8203 100644 --- a/src/qml/qml/qqmlvmemetaobject_p.h +++ b/src/qml/qml/qqmlvmemetaobject_p.h @@ -195,7 +195,7 @@ class Q_AUTOTEST_EXPORT QQmlVMEMetaObject : public QAbstractDynamicMetaObject, static void VarPropertiesWeakReferenceCallback(v8::Persistent object, void* parameter); static void GcPrologueCallback(QV8GCCallback::Node *node); inline void allocateVarPropertiesArray(); - inline void ensureVarPropertiesAllocated(); + inline bool ensureVarPropertiesAllocated(); void connectAlias(int aliasId); QBitArray aConnected; diff --git a/src/qml/qml/v8/qv8qobjectwrapper.cpp b/src/qml/qml/v8/qv8qobjectwrapper.cpp index a483346dd1..3faea2c97b 100644 --- a/src/qml/qml/v8/qv8qobjectwrapper.cpp +++ b/src/qml/qml/v8/qv8qobjectwrapper.cpp @@ -883,8 +883,10 @@ static void WeakQObjectReferenceCallback(v8::Persistent handle, void QQmlData *ddata = QQmlData::get(object, false); if (ddata) { ddata->v8object.Clear(); - if (!object->parent() && !ddata->indestructible) + if (!object->parent() && !ddata->indestructible) { + ddata->isQueuedForDeletion = true; object->deleteLater(); + } } } @@ -1043,12 +1045,15 @@ v8::Handle QV8QObjectWrapper::newQObject(QObject *object) if (QObjectPrivate::get(object)->wasDeleted) return v8::Null(); - + QQmlData *ddata = QQmlData::get(object, true); if (!ddata) return v8::Undefined(); + if (ddata->isQueuedForDeletion) + return v8::Null(); + if (ddata->v8objectid == m_id && !ddata->v8object.IsEmpty()) { // We own the v8object return v8::Local::New(ddata->v8object); diff --git a/tests/auto/qml/qqmlecmascript/data/ComponentWithVarProp.qml b/tests/auto/qml/qqmlecmascript/data/ComponentWithVarProp.qml new file mode 100644 index 0000000000..3105b8c79f --- /dev/null +++ b/tests/auto/qml/qqmlecmascript/data/ComponentWithVarProp.qml @@ -0,0 +1,5 @@ +import QtQuick 2.0 + +Item { + property var varprop: true +} diff --git a/tests/auto/qml/qqmlecmascript/data/propertyVarOwnership.5.qml b/tests/auto/qml/qqmlecmascript/data/propertyVarOwnership.5.qml new file mode 100644 index 0000000000..ad5807b1cf --- /dev/null +++ b/tests/auto/qml/qqmlecmascript/data/propertyVarOwnership.5.qml @@ -0,0 +1,28 @@ +import QtQuick 2.0 +import Qt.test 1.0 as ModuleApi + +Item { + id: testOwnership + property bool test: false + + function runTest() { + var o; + var c = Qt.createComponent("ComponentWithVarProp.qml"); + if (c.status == Component.Ready) { + o = c.createObject(); + } else { + return; // failed to create component. + } + o.varprop = true; // causes initialization of varProperties. + ModuleApi.trackObject(o); // stores QObject ptr + if (ModuleApi.trackedObject() == null) return; // is still valid, should have a valid v8object. + o = new Date(); // causes object to be gc-able. + gc(); // collect object's v8object + varProperties, queues deleteLater. + if (ModuleApi.trackedObject() != null) return; // v8object was previously collected. + ModuleApi.setTrackedObjectProperty("varprop"); // deferences varProperties of object. + test = !(ModuleApi.trackedObjectProperty("varprop")); // deferences varProperties of object. + // if we didn't crash, success. + } +} + + diff --git a/tests/auto/qml/qqmlecmascript/testtypes.h b/tests/auto/qml/qqmlecmascript/testtypes.h index 65b84d66cb..54fab26405 100644 --- a/tests/auto/qml/qqmlecmascript/testtypes.h +++ b/tests/auto/qml/qqmlecmascript/testtypes.h @@ -982,7 +982,7 @@ class testQObjectApi : public QObject public: testQObjectApi(QObject* parent = 0) - : QObject(parent), m_testProperty(0), m_testWritableProperty(0), m_testWritableFinalProperty(0), m_methodCallCount(0) + : QObject(parent), m_testProperty(0), m_testWritableProperty(0), m_testWritableFinalProperty(0), m_methodCallCount(0), m_trackedObject(0) { } @@ -992,6 +992,11 @@ class testQObjectApi : public QObject Q_INVOKABLE int qobjectEnumTestMethod(MyEnum val) { return (static_cast(val) + 5); } Q_INVOKABLE int qobjectTestMethod(int increment = 1) { m_methodCallCount += increment; return m_methodCallCount; } + Q_INVOKABLE void trackObject(QObject *obj) { m_trackedObject = obj; } + Q_INVOKABLE QObject *trackedObject() const { return m_trackedObject; } + Q_INVOKABLE void setTrackedObjectProperty(const QString &propName) const { m_trackedObject->setProperty(qPrintable(propName), QVariant(5)); } + Q_INVOKABLE QVariant trackedObjectProperty(const QString &propName) const { return m_trackedObject->property(qPrintable(propName)); } + int qobjectTestProperty() const { return m_testProperty; } void setQObjectTestProperty(int tp) { m_testProperty = tp; emit qobjectTestPropertyChanged(tp); } @@ -1011,6 +1016,7 @@ class testQObjectApi : public QObject int m_testWritableProperty; int m_testWritableFinalProperty; int m_methodCallCount; + QObject *m_trackedObject; }; class CircularReferenceObject : public QObject, diff --git a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp index cca008eb0c..3bbbc2c79c 100644 --- a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp +++ b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp @@ -3906,6 +3906,15 @@ void tst_qqmlecmascript::propertyVarOwnership() delete object; } + // Garbage collection cannot result in attempted dereference of empty handle + { + QQmlComponent component(&engine, testFileUrl("propertyVarOwnership.5.qml")); + QObject *object = component.create(); + QVERIFY(object != 0); + QMetaObject::invokeMethod(object, "runTest"); + QCOMPARE(object->property("test").toBool(), true); + delete object; + } } void tst_qqmlecmascript::propertyVarImplicitOwnership()