Assigning empty object to Q_PROPERTY(QVariantMap)

Correct the evaluation of an empty javascript object during assignment
to a QVariantMap property.

Task-number: QTBUG-23586
Change-Id: Ifa891a017690a36bd5837bc6b4dd0e47eb515a46
Reviewed-by: Martin Jones <martin.jones@nokia.com>

4 files changed, 132 insertions, 8 deletions

diff --git a/src/declarative/qml/v8/qv8engine.cpp b/src/declarative/qml/v8/qv8engine.cpp
index 448734638f..dbe8fbe774 100644
--- a/src/declarative/qml/v8/qv8engine.cpp
+++ b/src/declarative/qml/v8/qv8engine.cpp
@@ -501,10 +501,6 @@ QVariant QV8Engine::toBasicVariant(v8::Handle<v8::Value> value)
     if (!value->IsFunction()) {
         v8::Context::Scope scope(context());
         v8::Handle<v8::Object> object = value->ToObject();
-        v8::Local<v8::Array> properties = object->GetPropertyNames();
-        int length = properties->Length();
-        if (length == 0)
-            return QVariant();
         return variantMapFromJS(object);
     }
 
@@ -1088,14 +1084,19 @@ v8::Local<v8::Object> QV8Engine::variantMapToJS(const QVariantMap &vmap)
 QVariantMap QV8Engine::variantMapFromJS(v8::Handle<v8::Object> jsObject)
 {
     QVariantMap result;
+
+    v8::HandleScope handleScope;
+    v8::Handle<v8::Array> propertyNames = jsObject->GetPropertyNames();
+    uint32_t length = propertyNames->Length();
+    if (length == 0)
+        return result;
+
     int hash = jsObject->GetIdentityHash();
     if (visitedConversionObjects.contains(hash))
         return result; // Avoid recursion.
+
     visitedConversionObjects.insert(hash);
-    v8::HandleScope handleScope;
     // TODO: Only object's own property names. Include non-enumerable properties.
-    v8::Handle<v8::Array> propertyNames = jsObject->GetPropertyNames();
-    uint32_t length = propertyNames->Length();
     for (uint32_t i = 0; i < length; ++i) {
         v8::Handle<v8::Value> name = propertyNames->Get(i);
         result.insert(QJSConverter::toString(name->ToString()), variantFromJS(jsObject->Get(name)));
diff --git a/tests/auto/declarative/qdeclarativeproperty/data/assignEmptyVariantMap.qml b/tests/auto/declarative/qdeclarativeproperty/data/assignEmptyVariantMap.qml
new file mode 100644
index 0000000000..a9e51c1255
--- /dev/null
+++ b/tests/auto/declarative/qdeclarativeproperty/data/assignEmptyVariantMap.qml
@@ -0,0 +1,5 @@
+import QtQuick 2.0
+
+Item {
+    Component.onCompleted: { o.variantMap = {}; }
+}
diff --git a/tests/auto/declarative/qdeclarativeproperty/tst_qdeclarativeproperty.cpp b/tests/auto/declarative/qdeclarativeproperty/tst_qdeclarativeproperty.cpp
index 348c2582c0..5604df885f 100644
--- a/tests/auto/declarative/qdeclarativeproperty/tst_qdeclarativeproperty.cpp
+++ b/tests/auto/declarative/qdeclarativeproperty/tst_qdeclarativeproperty.cpp
@@ -41,6 +41,7 @@
 #include <qtest.h>
 #include <QtDeclarative/qdeclarativeengine.h>
 #include <QtDeclarative/qdeclarativecomponent.h>
+#include <QtDeclarative/qdeclarativecontext.h>
 #include <QtDeclarative/qdeclarativeproperty.h>
 #include <QtDeclarative/private/qdeclarativeproperty_p.h>
 #include <private/qdeclarativebinding_p.h>
@@ -124,10 +125,14 @@ private slots:
     void urlHandling_data();
     void urlHandling();
 
+    void variantMapHandling_data();
+    void variantMapHandling();
+
     // Bugs
     void crashOnValueProperty();
     void aliasPropertyBindings();
     void noContext();
+    void assignEmptyVariantMap();
 
     void copy();
 private:
@@ -188,6 +193,7 @@ class PropertyObject : public QObject
     Q_PROPERTY(QRect rectProperty READ rectProperty)
     Q_PROPERTY(QRect wrectProperty READ wrectProperty WRITE setWRectProperty)
     Q_PROPERTY(QUrl url READ url WRITE setUrl)
+    Q_PROPERTY(QVariantMap variantMap READ variantMap WRITE setVariantMap)
     Q_PROPERTY(int resettableProperty READ resettableProperty WRITE setResettableProperty RESET resetProperty)
     Q_PROPERTY(int propertyWithNotify READ propertyWithNotify WRITE setPropertyWithNotify NOTIFY oddlyNamedNotifySignal)
     Q_PROPERTY(MyQmlObject *qmlObject READ qmlObject)
@@ -205,6 +211,9 @@ public:
     QUrl url() { return m_url; }
     void setUrl(const QUrl &u) { m_url = u; }
 
+    QVariantMap variantMap() const { return m_variantMap; }
+    void setVariantMap(const QVariantMap &variantMap) { m_variantMap = variantMap; }
+
     int resettableProperty() const { return m_resetProperty; }
     void setResettableProperty(int r) { m_resetProperty = r; }
     void resetProperty() { m_resetProperty = 9; }
@@ -213,6 +222,7 @@ public:
     void setPropertyWithNotify(int i) { m_propertyWithNotify = i; emit oddlyNamedNotifySignal(); }
 
     MyQmlObject *qmlObject() { return &m_qmlObject; }
+
 signals:
     void clicked();
     void oddlyNamedNotifySignal();
@@ -221,6 +231,7 @@ private:
     int m_resetProperty;
     QRect m_rect;
     QUrl m_url;
+    QVariantMap m_variantMap;
     int m_propertyWithNotify;
     MyQmlObject m_qmlObject;
 };
@@ -1196,6 +1207,32 @@ void tst_qdeclarativeproperty::write()
         QCOMPARE(o.url(), result);
     }
 
+    // VariantMap-property
+    QVariantMap vm;
+    vm.insert("key", "value");
+
+    {
+        PropertyObject o;
+        QDeclarativeProperty p(&o, "variantMap");
+
+        QCOMPARE(p.write(vm), true);
+        QCOMPARE(o.variantMap(), vm);
+
+        QDeclarativeProperty p2(&o, "variantMap", engine.rootContext());
+
+        QCOMPARE(p2.write(vm), true);
+        QCOMPARE(o.variantMap(), vm);
+    }
+    {   // static
+        PropertyObject o;
+
+        QCOMPARE(QDeclarativeProperty::write(&o, "variantMap", vm), true);
+        QCOMPARE(o.variantMap(), vm);
+
+        QCOMPARE(QDeclarativeProperty::write(&o, "variantMap", vm, engine.rootContext()), true);
+        QCOMPARE(o.variantMap(), vm);
+    }
+
     // Attached property
     {
         QDeclarativeComponent component(&engine);
@@ -1319,7 +1356,6 @@ void tst_qdeclarativeproperty::writeObjectToList()
     QCOMPARE(list.at(0), qobject_cast<QObject*>(object));
 }
 
-Q_DECLARE_METATYPE(QList<QObject *>);
 void tst_qdeclarativeproperty::writeListToList()
 {
     QDeclarativeComponent containerComponent(&engine);
@@ -1446,6 +1482,59 @@ void tst_qdeclarativeproperty::urlHandling()
     }
 }
 
+void tst_qdeclarativeproperty::variantMapHandling_data()
+{
+    QTest::addColumn<QVariantMap>("vm");
+
+    // Object literals
+    {
+        QVariantMap m;
+        QTest::newRow("{}") << m;
+    }
+    {
+        QVariantMap m;
+        m["a"] = QVariantMap();
+        QTest::newRow("{ a:{} }") << m;
+    }
+    {
+        QVariantMap m, m2;
+        m2["b"] = 10;
+        m2["c"] = 20;
+        m["a"] = m2;
+        QTest::newRow("{ a:{b:10, c:20} }") << m;
+    }
+    {
+        QVariantMap m;
+        m["a"] = 10;
+        m["b"] = QVariantList() << 20 << 30;
+        QTest::newRow("{ a:10, b:[20, 30]}") << m;
+    }
+
+    // Cyclic objects
+    {
+        QVariantMap m;
+        m["p"] = QVariantMap();
+        QTest::newRow("var o={}; o.p=o") << m;
+    }
+    {
+        QVariantMap m;
+        m["p"] = 123;
+        m["q"] = QVariantMap();
+        QTest::newRow("var o={}; o.p=123; o.q=o") << m;
+    }
+}
+
+void tst_qdeclarativeproperty::variantMapHandling()
+{
+    QFETCH(QVariantMap, vm);
+
+    PropertyObject o;
+    QDeclarativeProperty p(&o, "variantMap");
+
+    QCOMPARE(p.write(vm), true);
+    QCOMPARE(o.variantMap(), vm);
+}
+
 void tst_qdeclarativeproperty::crashOnValueProperty()
 {
     QDeclarativeEngine *engine = new QDeclarativeEngine;
@@ -1596,6 +1685,29 @@ void tst_qdeclarativeproperty::noContext()
     delete b;
 }
 
+void tst_qdeclarativeproperty::assignEmptyVariantMap()
+{
+    PropertyObject o;
+
+    QVariantMap map;
+    map.insert("key", "value");
+    o.setVariantMap(map);
+    QCOMPARE(o.variantMap().count(), 1);
+    QCOMPARE(o.variantMap().isEmpty(), false);
+
+    QDeclarativeContext context(&engine);
+    context.setContextProperty("o", &o);
+
+    QDeclarativeComponent component(&engine, testFileUrl("assignEmptyVariantMap.qml"));
+    QObject *obj = component.create(&context);
+    QVERIFY(obj);
+
+    QCOMPARE(o.variantMap().count(), 0);
+    QCOMPARE(o.variantMap().isEmpty(), true);
+
+    delete obj;
+}
+
 void tst_qdeclarativeproperty::initTestCase()
 {
     QDeclarativeDataTest::initTestCase();
diff --git a/tests/auto/declarative/qjsvalue/tst_qjsvalue.cpp b/tests/auto/declarative/qjsvalue/tst_qjsvalue.cpp
index 5a9ccc521c..e3e259dcbc 100644
--- a/tests/auto/declarative/qjsvalue/tst_qjsvalue.cpp
+++ b/tests/auto/declarative/qjsvalue/tst_qjsvalue.cpp
@@ -4023,6 +4023,12 @@ void tst_QJSValue::nestedObjectToVariant_data()
     // Object literals
     {
         QVariantMap m;
+        QTest::newRow("{}")
+            << QString::fromLatin1("({})")
+            << QVariant(m);
+    }
+    {
+        QVariantMap m;
         m["a"] = QVariantMap();
         QTest::newRow("{ a:{} }")
             << QString::fromLatin1("({ a:{} })")