Fix property caches out of sync with grouped properties that have a VME meta object

Because QQmlObjectCreator::populateInstance would take the property cache to
install from the outside and also pass it as the cache to use for the VME meta
object to install, it could happen that the wrong cache was installed - the one
supplied by engine->cache(propType) instead of the cache created together with
the VME meta-object at type compilation time.

This patch ensures that they're always in sync and correct by removing the
responsibility of the caller to supply the cache to use and install. Instead
the function will always use the cache calculated at type compile time (and
also use that when installing the VME meta object).

Installation of the property cache on the declarative data of the instance is
now done only at createInstance() time, which fortunately also simplifies the
code.

Change-Id: Ia722cd57bc48007aaf725f1f59daa2f21569e324
Reviewed-by: Lars Knoll <lars.knoll@digia.com>

5 files changed, 53 insertions, 32 deletions

diff --git a/src/qml/qml/qqmlobjectcreator.cpp b/src/qml/qml/qqmlobjectcreator.cpp
index a5f7988bf8..50fe5861a6 100644
--- a/src/qml/qml/qqmlobjectcreator.cpp
+++ b/src/qml/qml/qqmlobjectcreator.cpp
@@ -737,9 +737,7 @@ bool QQmlObjectCreator::setPropertyBinding(QQmlPropertyData *property, const QV4
         QQmlType *attachedType = tr->type;
         const int id = attachedType->attachedPropertiesId();
         QObject *qmlObject = qmlAttachedPropertiesObjectById(id, _qobject);
-        QQmlRefPointer<QQmlPropertyCache> cache = QQmlEnginePrivate::get(engine)->cache(attachedType->attachedPropertiesType());
-        Q_ASSERT(!cache.isNull());
-        if (!populateInstance(binding->value.objectIndex, qmlObject, cache, qmlObject, /*value type property*/0))
+        if (!populateInstance(binding->value.objectIndex, qmlObject, qmlObject, /*value type property*/0))
             return false;
         return true;
     }
@@ -776,8 +774,6 @@ bool QQmlObjectCreator::setPropertyBinding(QQmlPropertyData *property, const QV4
         const QV4::CompiledData::Object *obj = qmlUnit->objectAt(binding->value.objectIndex);
         if (stringAt(obj->inheritedTypeNameIndex).isEmpty()) {
 
-            QQmlEnginePrivate *enginePrivate = QQmlEnginePrivate::get(engine);
-            QQmlRefPointer<QQmlPropertyCache> groupObjectPropertyCache;
             QObject *groupObject = 0;
             QQmlValueType *valueType = 0;
             QQmlPropertyData *valueTypeProperty = 0;
@@ -792,7 +788,6 @@ bool QQmlObjectCreator::setPropertyBinding(QQmlPropertyData *property, const QV4
 
                 valueType->read(_qobject, property->coreIndex);
 
-                groupObjectPropertyCache = enginePrivate->cache(valueType);
                 groupObject = valueType;
                 valueTypeProperty = property;
             } else {
@@ -803,20 +798,10 @@ bool QQmlObjectCreator::setPropertyBinding(QQmlPropertyData *property, const QV4
                     return false;
                 }
 
-                if (QQmlData *groupDeclarativeData = QQmlData::get(groupObject))
-                    groupObjectPropertyCache = groupDeclarativeData->propertyCache;
-                if (!groupObjectPropertyCache)
-                    groupObjectPropertyCache = enginePrivate->propertyCacheForType(property->propType);
-                if (!groupObjectPropertyCache) {
-                    recordError(binding->location, tr("Invalid grouped property access"));
-                    return false;
-                }
-
-
                 bindingTarget = groupObject;
             }
 
-            if (!populateInstance(binding->value.objectIndex, groupObject, groupObjectPropertyCache, bindingTarget, valueTypeProperty))
+            if (!populateInstance(binding->value.objectIndex, groupObject, bindingTarget, valueTypeProperty))
                 return false;
 
             if (valueType)
@@ -1139,6 +1124,12 @@ QObject *QQmlObjectCreator::createInstance(int index, QObject *parent)
 
     QQmlRefPointer<QQmlPropertyCache> cache = propertyCaches.value(index);
     Q_ASSERT(!cache.isNull());
+    if (installPropertyCache) {
+        if (ddata->propertyCache)
+            ddata->propertyCache->release();;
+        ddata->propertyCache = cache;
+        ddata->propertyCache->addref();
+    }
 
     QObject *scopeObject = instance;
     qSwap(_scopeObject, scopeObject);
@@ -1153,7 +1144,7 @@ QObject *QQmlObjectCreator::createInstance(int index, QObject *parent)
 
     qSwap(_qmlContext, qmlContext);
 
-    bool result = populateInstance(index, instance, cache, /*binding target*/instance, /*value type property*/0, installPropertyCache, bindingsToSkip);
+    bool result = populateInstance(index, instance, /*binding target*/instance, /*value type property*/0, bindingsToSkip);
 
     qSwap(_qmlContext, qmlContext);
     qSwap(_scopeObject, scopeObject);
@@ -1262,14 +1253,13 @@ void QQmlObjectCreator::clear()
     phase = Done;
 }
 
-bool QQmlObjectCreator::populateInstance(int index, QObject *instance, QQmlRefPointer<QQmlPropertyCache> cache, QObject *bindingTarget, QQmlPropertyData *valueTypeProperty, bool installPropertyCache, const QBitArray &bindingsToSkip)
+bool QQmlObjectCreator::populateInstance(int index, QObject *instance, QObject *bindingTarget, QQmlPropertyData *valueTypeProperty, const QBitArray &bindingsToSkip)
 {
     const QV4::CompiledData::Object *obj = qmlUnit->objectAt(index);
     Q_QML_VME_PROFILE(sharedState->profiler, push());
 
     QQmlData *declarativeData = QQmlData::get(instance, /*create*/true);
 
-    qSwap(_propertyCache, cache);
     qSwap(_qobject, instance);
     qSwap(_valueTypeProperty, valueTypeProperty);
     qSwap(_compiledObject, obj);
@@ -1280,28 +1270,25 @@ bool QQmlObjectCreator::populateInstance(int index, QObject *instance, QQmlRefPo
     QV4::Scope valueScope(v4);
     QV4::ScopedValue scopeObjectProtector(valueScope);
 
+    QQmlRefPointer<QQmlPropertyCache> cache = propertyCaches.value(index);
+
     QQmlVMEMetaObject *vmeMetaObject = 0;
     const QByteArray data = vmeMetaObjectData.value(index);
     if (!data.isEmpty()) {
+        Q_ASSERT(!cache.isNull());
         // install on _object
-        vmeMetaObject = new QQmlVMEMetaObject(_qobject, _propertyCache, reinterpret_cast<const QQmlVMEMetaData*>(data.constData()));
+        vmeMetaObject = new QQmlVMEMetaObject(_qobject, cache, reinterpret_cast<const QQmlVMEMetaData*>(data.constData()));
         if (_ddata->propertyCache)
             _ddata->propertyCache->release();
-        Q_ASSERT(installPropertyCache);
+        _ddata->propertyCache = cache;
+        _ddata->propertyCache->addref();
         scopeObjectProtector = _ddata->jsWrapper.value();
     } else {
         vmeMetaObject = QQmlVMEMetaObject::get(_qobject);
     }
-    if (installPropertyCache) {
-        Q_ASSERT(_propertyCache);
-        _ddata->propertyCache = _propertyCache;
-        _ddata->propertyCache->addref();
-    }
-
+    qSwap(_propertyCache, cache);
     qSwap(_vmeMetaObject, vmeMetaObject);
 
-    QVector<QQmlAbstractBinding*> createdBindings(_compiledObject->nBindings, 0);
-
     QBitArray bindingSkipList = bindingsToSkip;
     {
         QHash<int, QBitArray>::ConstIterator deferredBindings = compiledData->deferredBindingsPerObject.find(index);
diff --git a/src/qml/qml/qqmlobjectcreator_p.h b/src/qml/qml/qqmlobjectcreator_p.h
index 6717257bcf..9fd52a9f48 100644
--- a/src/qml/qml/qqmlobjectcreator_p.h
+++ b/src/qml/qml/qqmlobjectcreator_p.h
@@ -98,8 +98,8 @@ private:
 
     QObject *createInstance(int index, QObject *parent = 0);
 
-    bool populateInstance(int index, QObject *instance, QQmlRefPointer<QQmlPropertyCache> cache,
-                          QObject *bindingTarget, QQmlPropertyData *valueTypeProperty, bool installPropertyCache = true,
+    bool populateInstance(int index, QObject *instance,
+                          QObject *bindingTarget, QQmlPropertyData *valueTypeProperty,
                           const QBitArray &bindingsToSkip = QBitArray());
 
     void setupBindings(const QBitArray &bindingsToSkip);
diff --git a/src/qml/qml/qqmlvmemetaobject_p.h b/src/qml/qml/qqmlvmemetaobject_p.h
index 778559d0c4..50d1cf75b7 100644
--- a/src/qml/qml/qqmlvmemetaobject_p.h
+++ b/src/qml/qml/qqmlvmemetaobject_p.h
@@ -174,6 +174,9 @@ public:
 
     virtual QAbstractDynamicMetaObject *toDynamicMetaObject(QObject *o);
 
+    // Used by auto-tests for inspection
+    QQmlPropertyCache *propertyCache() const { return cache; }
+
     static inline QQmlVMEMetaObject *get(QObject *o);
     static QQmlVMEMetaObject *getForProperty(QObject *o, int coreIndex);
     static QQmlVMEMetaObject *getForMethod(QObject *o, int coreIndex);
diff --git a/tests/auto/qml/qqmllanguage/data/propertyCacheInSync.qml b/tests/auto/qml/qqmllanguage/data/propertyCacheInSync.qml
new file mode 100644
index 0000000000..35fa4be6eb
--- /dev/null
+++ b/tests/auto/qml/qqmllanguage/data/propertyCacheInSync.qml
@@ -0,0 +1,9 @@
+import QtQuick 2.0
+Item {
+    anchors {
+        margins: 50
+        Behavior on margins {
+            NumberAnimation { duration: 10 }
+        }
+    }
+}
diff --git a/tests/auto/qml/qqmllanguage/tst_qqmllanguage.cpp b/tests/auto/qml/qqmllanguage/tst_qqmllanguage.cpp
index c7a26c72db..fa0c393d66 100644
--- a/tests/auto/qml/qqmllanguage/tst_qqmllanguage.cpp
+++ b/tests/auto/qml/qqmllanguage/tst_qqmllanguage.cpp
@@ -56,6 +56,7 @@
 #include <private/qqmlmetatype_p.h>
 #include <private/qqmlglobal_p.h>
 #include <private/qqmlscriptstring_p.h>
+#include <private/qqmlvmemetaobject_p.h>
 
 #include "testtypes.h"
 #include "testhttpserver.h"
@@ -219,6 +220,7 @@ private slots:
     void customParserEvaluateEnum();
 
     void preservePropertyCacheOnGroupObjects();
+    void propertyCacheInSync();
 
 private:
     QQmlEngine engine;
@@ -3602,6 +3604,26 @@ void tst_qqmllanguage::preservePropertyCacheOnGroupObjects()
     QCOMPARE(pd->propType, qMetaTypeId<int>());
 }
 
+void tst_qqmllanguage::propertyCacheInSync()
+{
+    QQmlComponent component(&engine, testFile("propertyCacheInSync.qml"));
+    VERIFY_ERRORS(0);
+    QScopedPointer<QObject> o(component.create());
+    QVERIFY(!o.isNull());
+    QObject *anchors = qvariant_cast<QObject*>(o->property("anchors"));
+    QVERIFY(anchors);
+    QQmlVMEMetaObject *vmemo = QQmlVMEMetaObject::get(anchors);
+    QVERIFY(vmemo);
+    QQmlPropertyCache *vmemoCache = vmemo->propertyCache();
+    QVERIFY(vmemoCache);
+    QQmlData *ddata = QQmlData::get(anchors);
+    QVERIFY(ddata);
+    QVERIFY(ddata->propertyCache);
+    // Those always have to be in sync and correct.
+    QVERIFY(ddata->propertyCache == vmemoCache);
+    QCOMPARE(anchors->property("margins").toInt(), 50);
+}
+
 QTEST_MAIN(tst_qqmllanguage)
 
 #include "tst_qqmllanguage.moc"