Use object identity to detect cycles in JS-to-C++ type conversion

The documentation for v8::Object::GetIdentityHash() states that the
hash value is not guaranteed to be unique (the current implementation
just returns a random number). Hence, the hash value should not be
used to determine whether an object has already been visited during
type conversion; in the worst (and non-deterministic) case, the
conversion will be "cut off" prematurely (due to identical hash
values for two different objects), resulting in data loss.

Instead, represent the visited objects as a set of V8 object handles.
This is safe since the type conversion is always done on the stack,
within a handle scope. Use v8::Object::GetIdentityHash() merely to
implement the qHash() specialization needed for the set.

V8 already provides an operator==() for handles, and it is documented
to return true "if the objects to which they refer are identical",
which is the behavior required by the set implementation.

Task-number: QTBUG-21681
Change-Id: I1f2a1eee8f7c197c02c2ffeaaa1fc0274e8ab740
Reviewed-by: Michael Brasser <michael.brasser@nokia.com>
Reviewed-by: Simon Hausmann <simon.hausmann@nokia.com>

4 files changed, 39 insertions, 34 deletions

diff --git a/src/qml/qml/v8/qv8engine.cpp b/src/qml/qml/v8/qv8engine.cpp
index 96110ed847..8233cb7108 100644
--- a/src/qml/qml/v8/qv8engine.cpp
+++ b/src/qml/qml/v8/qv8engine.cpp
@@ -925,18 +925,17 @@ v8::Local<v8::Array> QV8Engine::variantListToJS(const QVariantList &lst)
 // of the JS Array, and elements being the JS Array's elements
 // converted to QVariants, recursively.
 QVariantList QV8Engine::variantListFromJS(v8::Handle<v8::Array> jsArray,
-                                          QSet<int> &visitedObjects)
+                                          V8ObjectSet &visitedObjects)
 {
     QVariantList result;
-    int hash = jsArray->GetIdentityHash();
-    if (visitedObjects.contains(hash))
+    if (visitedObjects.contains(jsArray))
         return result; // Avoid recursion.
     v8::HandleScope handleScope;
-    visitedObjects.insert(hash);
+    visitedObjects.insert(jsArray);
     uint32_t length = jsArray->Length();
     for (uint32_t i = 0; i < length; ++i)
         result.append(variantFromJS(jsArray->Get(i), visitedObjects));
-    visitedObjects.remove(hash);
+    visitedObjects.remove(jsArray);
     return result;
 }
 
@@ -958,7 +957,7 @@ v8::Local<v8::Object> QV8Engine::variantMapToJS(const QVariantMap &vmap)
 // of the object, and values being the values of the JS object's
 // properties converted to QVariants, recursively.
 QVariantMap QV8Engine::variantMapFromJS(v8::Handle<v8::Object> jsObject,
-                                        QSet<int> &visitedObjects)
+                                        V8ObjectSet &visitedObjects)
 {
     QVariantMap result;
 
@@ -968,18 +967,17 @@ QVariantMap QV8Engine::variantMapFromJS(v8::Handle<v8::Object> jsObject,
     if (length == 0)
         return result;
 
-    int hash = jsObject->GetIdentityHash();
-    if (visitedObjects.contains(hash))
+    if (visitedObjects.contains(jsObject))
         return result; // Avoid recursion.
 
-    visitedObjects.insert(hash);
+    visitedObjects.insert(jsObject);
     // TODO: Only object's own property names. Include non-enumerable properties.
     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), visitedObjects));
     }
-    visitedObjects.remove(hash);
+    visitedObjects.remove(jsObject);
     return result;
 }
 
@@ -1265,7 +1263,7 @@ v8::Handle<v8::Value> QV8Engine::variantToJS(const QVariant &value)
 // RegExp -> QVariant(QRegExp)
 // [Any other object] -> QVariantMap(...)
 QVariant QV8Engine::variantFromJS(v8::Handle<v8::Value> value,
-                                  QSet<int> &visitedObjects)
+                                  V8ObjectSet &visitedObjects)
 {
     Q_ASSERT(!value.IsEmpty());
     if (value->IsUndefined())
diff --git a/src/qml/qml/v8/qv8engine_p.h b/src/qml/qml/v8/qv8engine_p.h
index d885c777f7..9abdb842f9 100644
--- a/src/qml/qml/v8/qv8engine_p.h
+++ b/src/qml/qml/v8/qv8engine_p.h
@@ -237,6 +237,7 @@ public:
 
 class Q_QML_PRIVATE_EXPORT QV8Engine
 {
+    typedef QSet<v8::Handle<v8::Object> > V8ObjectSet;
 public:
     static QV8Engine* get(QJSEngine* q) { Q_ASSERT(q); return q->handle(); }
     static QJSEngine* get(QV8Engine* d) { Q_ASSERT(d); return d->q; }
@@ -372,15 +373,15 @@ public:
 
     v8::Local<v8::Array> variantListToJS(const QVariantList &lst);
     inline QVariantList variantListFromJS(v8::Handle<v8::Array> jsArray)
-    { QSet<int> visitedObjects; return variantListFromJS(jsArray, visitedObjects); }
+    { V8ObjectSet visitedObjects; return variantListFromJS(jsArray, visitedObjects); }
 
     v8::Local<v8::Object> variantMapToJS(const QVariantMap &vmap);
     inline QVariantMap variantMapFromJS(v8::Handle<v8::Object> jsObject)
-    { QSet<int> visitedObjects; return variantMapFromJS(jsObject, visitedObjects); }
+    { V8ObjectSet visitedObjects; return variantMapFromJS(jsObject, visitedObjects); }
 
     v8::Handle<v8::Value> variantToJS(const QVariant &value);
     inline QVariant variantFromJS(v8::Handle<v8::Value> value)
-    { QSet<int> visitedObjects; return variantFromJS(value, visitedObjects); }
+    { V8ObjectSet visitedObjects; return variantFromJS(value, visitedObjects); }
 
     v8::Handle<v8::Value> jsonValueToJS(const QJsonValue &value);
     QJsonValue jsonValueFromJS(v8::Handle<v8::Value> value);
@@ -474,9 +475,9 @@ protected:
     double qtDateTimeToJsDate(const QDateTime &dt);
 
 private:
-    QVariantList variantListFromJS(v8::Handle<v8::Array> jsArray, QSet<int> &visitedObjects);
-    QVariantMap variantMapFromJS(v8::Handle<v8::Object> jsObject, QSet<int> &visitedObjects);
-    QVariant variantFromJS(v8::Handle<v8::Value> value, QSet<int> &visitedObjects);
+    QVariantList variantListFromJS(v8::Handle<v8::Array> jsArray, V8ObjectSet &visitedObjects);
+    QVariantMap variantMapFromJS(v8::Handle<v8::Object> jsObject, V8ObjectSet &visitedObjects);
+    QVariant variantFromJS(v8::Handle<v8::Value> value, V8ObjectSet &visitedObjects);
 
     static v8::Persistent<v8::Object> *findOwnerAndStrength(QObject *object, bool *shouldBeStrong);
 
@@ -615,6 +616,13 @@ QV8Engine::Deletable *QV8Engine::extensionData(int index) const
         return 0;
 }
 
+// Needed for V8ObjectSet
+template<>
+inline uint qHash<v8::Handle<v8::Object> >(const v8::Handle<v8::Object> &object, uint /*seed*/)
+{
+    return object->GetIdentityHash();
+}
+
 QT_END_NAMESPACE
 
 #endif // QQMLV8ENGINE_P_H
diff --git a/src/qml/qml/v8/qv8jsonwrapper.cpp b/src/qml/qml/v8/qv8jsonwrapper.cpp
index ff8b69f580..5ba59b2263 100644
--- a/src/qml/qml/v8/qv8jsonwrapper.cpp
+++ b/src/qml/qml/v8/qv8jsonwrapper.cpp
@@ -82,7 +82,7 @@ v8::Handle<v8::Value> QV8JsonWrapper::fromJsonValue(const QJsonValue &value)
 }
 
 QJsonValue QV8JsonWrapper::toJsonValue(v8::Handle<v8::Value> value,
-                                       QSet<int> &visitedObjects)
+                                       V8ObjectSet &visitedObjects)
 {
     if (value->IsString())
         return QJsonValue(QJSConverter::toString(value.As<v8::String>()));
@@ -109,22 +109,21 @@ v8::Local<v8::Object> QV8JsonWrapper::fromJsonObject(const QJsonObject &object)
 }
 
 QJsonObject QV8JsonWrapper::toJsonObject(v8::Handle<v8::Value> value,
-                                         QSet<int> &visitedObjects)
+                                         V8ObjectSet &visitedObjects)
 {
     QJsonObject result;
     if (!value->IsObject() || value->IsArray() || value->IsFunction())
         return result;
 
     v8::Handle<v8::Object> v8object(value.As<v8::Object>());
-    int hash = v8object->GetIdentityHash();
-    if (visitedObjects.contains(hash)) {
+    if (visitedObjects.contains(v8object)) {
         // Avoid recursion.
         // For compatibility with QVariant{List,Map} conversion, we return an
         // empty object (and no error is thrown).
         return result;
     }
 
-    visitedObjects.insert(hash);
+    visitedObjects.insert(v8object);
 
     v8::Local<v8::Array> propertyNames = m_engine->getOwnPropertyNames(v8object);
     uint32_t length = propertyNames->Length();
@@ -136,7 +135,7 @@ QJsonObject QV8JsonWrapper::toJsonObject(v8::Handle<v8::Value> value,
                           toJsonValue(propertyValue, visitedObjects));
     }
 
-    visitedObjects.remove(hash);
+    visitedObjects.remove(v8object);
 
     return result;
 }
@@ -151,22 +150,21 @@ v8::Local<v8::Array> QV8JsonWrapper::fromJsonArray(const QJsonArray &array)
 }
 
 QJsonArray QV8JsonWrapper::toJsonArray(v8::Handle<v8::Value> value,
-                                       QSet<int> &visitedObjects)
+                                       V8ObjectSet &visitedObjects)
 {
     QJsonArray result;
     if (!value->IsArray())
         return result;
 
     v8::Handle<v8::Array> v8array(value.As<v8::Array>());
-    int hash = v8array->GetIdentityHash();
-    if (visitedObjects.contains(hash)) {
+    if (visitedObjects.contains(v8array)) {
         // Avoid recursion.
         // For compatibility with QVariant{List,Map} conversion, we return an
         // empty array (and no error is thrown).
         return result;
     }
 
-    visitedObjects.insert(hash);
+    visitedObjects.insert(v8array);
 
     uint32_t length = v8array->Length();
     for (uint32_t i = 0; i < length; ++i) {
@@ -175,7 +173,7 @@ QJsonArray QV8JsonWrapper::toJsonArray(v8::Handle<v8::Value> value,
             result.append(toJsonValue(element, visitedObjects));
     }
 
-    visitedObjects.remove(hash);
+    visitedObjects.remove(v8array);
 
     return result;
 }
diff --git a/src/qml/qml/v8/qv8jsonwrapper_p.h b/src/qml/qml/v8/qv8jsonwrapper_p.h
index 887e1069f8..31a5ceba3e 100644
--- a/src/qml/qml/v8/qv8jsonwrapper_p.h
+++ b/src/qml/qml/v8/qv8jsonwrapper_p.h
@@ -66,6 +66,7 @@ QT_BEGIN_NAMESPACE
 class QV8Engine;
 class QV8JsonWrapper
 {
+    typedef QSet<v8::Handle<v8::Object> > V8ObjectSet;
 public:
     QV8JsonWrapper();
     ~QV8JsonWrapper();
@@ -75,20 +76,20 @@ public:
 
     v8::Handle<v8::Value> fromJsonValue(const QJsonValue &value);
     inline QJsonValue toJsonValue(v8::Handle<v8::Value> value)
-    { QSet<int> visitedObjects; return toJsonValue(value, visitedObjects); }
+    { V8ObjectSet visitedObjects; return toJsonValue(value, visitedObjects); }
 
     v8::Local<v8::Object> fromJsonObject(const QJsonObject &object);
     inline QJsonObject toJsonObject(v8::Handle<v8::Value> value)
-    { QSet<int> visitedObjects; return toJsonObject(value, visitedObjects); }
+    { V8ObjectSet visitedObjects; return toJsonObject(value, visitedObjects); }
 
     v8::Local<v8::Array> fromJsonArray(const QJsonArray &array);
     inline QJsonArray toJsonArray(v8::Handle<v8::Value> value)
-    { QSet<int> visitedObjects; return toJsonArray(value, visitedObjects); }
+    { V8ObjectSet visitedObjects; return toJsonArray(value, visitedObjects); }
 
 private:
-    QJsonValue toJsonValue(v8::Handle<v8::Value> value, QSet<int> &visitedObjects);
-    QJsonObject toJsonObject(v8::Handle<v8::Value> value, QSet<int> &visitedObjects);
-    QJsonArray toJsonArray(v8::Handle<v8::Value> value, QSet<int> &visitedObjects);
+    QJsonValue toJsonValue(v8::Handle<v8::Value> value, V8ObjectSet &visitedObjects);
+    QJsonObject toJsonObject(v8::Handle<v8::Value> value, V8ObjectSet &visitedObjects);
+    QJsonArray toJsonArray(v8::Handle<v8::Value> value, V8ObjectSet &visitedObjects);
 
     QV8Engine *m_engine;
 };