Do not copy lookups

It leads to data corruption. Also, be more careful about releasing the
property cache. We can only do that if the qobjectlookup member of the
union is active. Unfortunately we have to do a number of checks now, to
make sure it is. In order to still keep the checks inline, we move some
functions around.

Pick-to: 5.15
Fixes: QTBUG-99211
Change-Id: If6dd879e67b172e1a9035e83fbfacbe73c6c7476
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

5 files changed, 107 insertions, 67 deletions

diff --git a/src/qml/jsruntime/qv4executablecompilationunit.cpp b/src/qml/jsruntime/qv4executablecompilationunit.cpp
index 55ed888e7b..f5f76d2a9e 100644
--- a/src/qml/jsruntime/qv4executablecompilationunit.cpp
+++ b/src/qml/jsruntime/qv4executablecompilationunit.cpp
@@ -296,24 +296,8 @@ void ExecutableCompilationUnit::unlink()
     propertyCaches.clear();
 
     if (runtimeLookups) {
-        for (uint i = 0; i < data->lookupTableSize; ++i) {
-            QV4::Lookup &l = runtimeLookups[i];
-            if (l.getter == QV4::QObjectWrapper::lookupGetter
-                    || l.getter == QQmlTypeWrapper::lookupSingletonProperty) {
-                if (QQmlPropertyCache *pc = l.qobjectLookup.propertyCache)
-                    pc->release();
-            } else if (l.getter == QQmlValueTypeWrapper::lookupGetter
-                       || l.getter == QQmlTypeWrapper::lookupSingletonProperty) {
-                if (QQmlPropertyCache *pc = l.qgadgetLookup.propertyCache)
-                    pc->release();
-            }
-
-            if (l.qmlContextPropertyGetter == QQmlContextWrapper::lookupScopeObjectProperty
-                    || l.qmlContextPropertyGetter == QQmlContextWrapper::lookupContextObjectProperty) {
-                if (QQmlPropertyCache *pc = l.qobjectLookup.propertyCache)
-                    pc->release();
-            }
-        }
+        for (uint i = 0; i < data->lookupTableSize; ++i)
+            runtimeLookups[i].releasePropertyCache();
     }
 
     dependentScripts.clear();
diff --git a/src/qml/jsruntime/qv4lookup.cpp b/src/qml/jsruntime/qv4lookup.cpp
index a91fe8f371..604a6002c8 100644
--- a/src/qml/jsruntime/qv4lookup.cpp
+++ b/src/qml/jsruntime/qv4lookup.cpp
@@ -41,6 +41,7 @@
 #include "qv4jscall_p.h"
 #include "qv4string_p.h"
 #include <private/qv4identifiertable_p.h>
+#include <private/qv4qobjectwrapper_p.h>
 
 QT_BEGIN_NAMESPACE
 
@@ -144,47 +145,76 @@ ReturnedValue Lookup::getterGeneric(Lookup *l, ExecutionEngine *engine, const Va
     return l->resolvePrimitiveGetter(engine, object);
 }
 
+static inline void setupObjectLookupTwoClasses(Lookup *l, const Lookup &first, const Lookup &second)
+{
+    Heap::InternalClass *ic1 = first.objectLookup.ic;
+    const uint offset1 = first.objectLookup.offset;
+    Heap::InternalClass *ic2 = second.objectLookup.ic;
+    const uint offset2 = second.objectLookup.offset;
+
+    l->objectLookupTwoClasses.ic = ic1;
+    l->objectLookupTwoClasses.ic2 = ic2;
+    l->objectLookupTwoClasses.offset = offset1;
+    l->objectLookupTwoClasses.offset2 = offset2;
+}
+
+static inline void setupProtoLookupTwoClasses(Lookup *l, const Lookup &first, const Lookup &second)
+{
+    const quintptr protoId1 = first.protoLookup.protoId;
+    const Value *data1 = first.protoLookup.data;
+    const quintptr protoId2 = second.protoLookup.protoId;
+    const Value *data2 = second.protoLookup.data;
+
+    l->protoLookupTwoClasses.protoId = protoId1;
+    l->protoLookupTwoClasses.protoId2 = protoId2;
+    l->protoLookupTwoClasses.data = data1;
+    l->protoLookupTwoClasses.data2 = data2;
+}
+
 ReturnedValue Lookup::getterTwoClasses(Lookup *l, ExecutionEngine *engine, const Value &object)
 {
     if (const Object *o = object.as<Object>()) {
-        Lookup first = *l;
-        Lookup second = *l;
 
-        ReturnedValue result = second.resolveGetter(engine, o);
-
-        if (first.getter == getter0Inline && (second.getter == getter0Inline || second.getter == getter0MemberData)) {
-            l->objectLookupTwoClasses.ic = first.objectLookup.ic;
-            l->objectLookupTwoClasses.ic2 = second.objectLookup.ic;
-            l->objectLookupTwoClasses.offset = first.objectLookup.offset;
-            l->objectLookupTwoClasses.offset2 = second.objectLookup.offset;
-            l->getter = second.getter == getter0Inline ? getter0Inlinegetter0Inline : getter0Inlinegetter0MemberData;
+        // Do the resolution on a second lookup, then merge.
+        Lookup second;
+        memset(&second, 0, sizeof(Lookup));
+        second.nameIndex = l->nameIndex;
+        second.getter = getterGeneric;
+        const ReturnedValue result = second.resolveGetter(engine, o);
+
+        if (l->getter == getter0Inline
+                && (second.getter == getter0Inline || second.getter == getter0MemberData)) {
+            setupObjectLookupTwoClasses(l, *l, second);
+            l->getter = (second.getter == getter0Inline)
+                    ? getter0Inlinegetter0Inline
+                    : getter0Inlinegetter0MemberData;
             return result;
         }
-        if (first.getter == getter0MemberData && (second.getter == getter0Inline || second.getter == getter0MemberData)) {
-            l->objectLookupTwoClasses.ic = second.objectLookup.ic;
-            l->objectLookupTwoClasses.ic2 = first.objectLookup.ic;
-            l->objectLookupTwoClasses.offset = second.objectLookup.offset;
-            l->objectLookupTwoClasses.offset2 = first.objectLookup.offset;
-            l->getter = second.getter == getter0Inline ? getter0Inlinegetter0MemberData : getter0MemberDatagetter0MemberData;
+
+        if (l->getter == getter0MemberData
+                && (second.getter == getter0Inline || second.getter == getter0MemberData)) {
+            setupObjectLookupTwoClasses(l, second, *l);
+            l->getter = (second.getter == getter0Inline)
+                    ? getter0Inlinegetter0MemberData
+                    : getter0MemberDatagetter0MemberData;
             return result;
         }
-        if (first.getter == getterProto && second.getter == getterProto) {
-            l->protoLookupTwoClasses.protoId = first.protoLookup.protoId;
-            l->protoLookupTwoClasses.protoId2 = second.protoLookup.protoId;
-            l->protoLookupTwoClasses.data = first.protoLookup.data;
-            l->protoLookupTwoClasses.data2 = second.protoLookup.data;
+
+
+        if (l->getter == getterProto && second.getter == getterProto) {
+            setupProtoLookupTwoClasses(l, *l, second);
             l->getter = getterProtoTwoClasses;
             return result;
         }
-        if (first.getter == getterProtoAccessor && second.getter == getterProtoAccessor) {
-            l->protoLookupTwoClasses.protoId = first.protoLookup.protoId;
-            l->protoLookupTwoClasses.protoId2 = second.protoLookup.protoId;
-            l->protoLookupTwoClasses.data = first.protoLookup.data;
-            l->protoLookupTwoClasses.data2 = second.protoLookup.data;
+
+        if (l->getter == getterProtoAccessor && second.getter == getterProtoAccessor) {
+            setupProtoLookupTwoClasses(l, *l, second);
             l->getter = getterProtoAccessorTwoClasses;
             return result;
         }
 
+        // If any of the above options were true, the propertyCache was inactive.
+        second.releasePropertyCache();
     }
 
     l->getter = getterFallback;
@@ -371,7 +401,19 @@ ReturnedValue Lookup::getterIndexed(Lookup *l, ExecutionEngine *engine, const Va
     }
     l->getter = getterFallback;
     return getterFallback(l, engine, object);
+}
 
+ReturnedValue Lookup::getterQObject(Lookup *lookup, ExecutionEngine *engine, const Value &object)
+{
+    const auto revertLookup = [lookup, engine, &object]() {
+        lookup->qobjectLookup.propertyCache->release();
+        lookup->qobjectLookup.propertyCache = nullptr;
+        lookup->getter = Lookup::getterGeneric;
+        return Lookup::getterGeneric(lookup, engine, object);
+    };
+
+    return QObjectWrapper::lookupGetterImpl(
+                lookup, engine, object, /*useOriginalProperty*/ false, revertLookup);
 }
 
 ReturnedValue Lookup::primitiveGetterProto(Lookup *l, ExecutionEngine *engine, const Value &object)
@@ -463,23 +505,30 @@ bool Lookup::setterGeneric(Lookup *l, ExecutionEngine *engine, Value &object, co
 
 bool Lookup::setterTwoClasses(Lookup *l, ExecutionEngine *engine, Value &object, const Value &value)
 {
-    Lookup first = *l;
-    Lookup second = *l;
+    // A precondition of this method is that l->objectLookup is the active variant of the union.
+    Q_ASSERT(l->setter == setter0MemberData || l->setter == setter0Inline);
 
     if (object.isObject()) {
+
+        // As l->objectLookup is active, we can stash some members here, before resolving.
+        Heap::InternalClass *ic = l->objectLookup.ic;
+        const uint index = l->objectLookup.index;
+
         if (!l->resolveSetter(engine, static_cast<Object *>(&object), value)) {
             l->setter = setterFallback;
             return false;
         }
 
         if (l->setter == Lookup::setter0MemberData || l->setter == Lookup::setter0Inline) {
-            l->objectLookupTwoClasses.ic = first.objectLookup.ic;
-            l->objectLookupTwoClasses.ic2 = second.objectLookup.ic;
-            l->objectLookupTwoClasses.offset = first.objectLookup.index;
-            l->objectLookupTwoClasses.offset2 = second.objectLookup.index;
+            l->objectLookupTwoClasses.ic = ic;
+            l->objectLookupTwoClasses.ic2 = ic;
+            l->objectLookupTwoClasses.offset = index;
+            l->objectLookupTwoClasses.offset2 = index;
             l->setter = setter0setter0;
             return true;
         }
+
+        l->releasePropertyCache();
     }
 
     l->setter = setterFallback;
diff --git a/src/qml/jsruntime/qv4lookup_p.h b/src/qml/jsruntime/qv4lookup_p.h
index deb23d8c58..ce46186827 100644
--- a/src/qml/jsruntime/qv4lookup_p.h
+++ b/src/qml/jsruntime/qv4lookup_p.h
@@ -56,11 +56,17 @@
 #include "qv4context_p.h"
 #include "qv4object_p.h"
 #include "qv4internalclass_p.h"
+#include "qv4qmlcontext_p.h"
+#include <private/qqmltypewrapper_p.h>
+#include <private/qqmlvaluetypewrapper_p.h>
 
 QT_BEGIN_NAMESPACE
 
 namespace QV4 {
 
+// Note: We cannot hide the copy ctor and assignment operator of this class because it needs to
+//       be trivially copyable. But you should never ever copy it. There are refcounted members
+//       in there.
 struct Q_QML_PRIVATE_EXPORT Lookup {
     union {
         ReturnedValue (*getter)(Lookup *l, ExecutionEngine *engine, const Value &object);
@@ -187,6 +193,7 @@ struct Q_QML_PRIVATE_EXPORT Lookup {
     static ReturnedValue getterProtoAccessor(Lookup *l, ExecutionEngine *engine, const Value &object);
     static ReturnedValue getterProtoAccessorTwoClasses(Lookup *l, ExecutionEngine *engine, const Value &object);
     static ReturnedValue getterIndexed(Lookup *l, ExecutionEngine *engine, const Value &object);
+    static ReturnedValue getterQObject(Lookup *l, ExecutionEngine *engine, const Value &object);
 
     static ReturnedValue primitiveGetterProto(Lookup *l, ExecutionEngine *engine, const Value &object);
     static ReturnedValue primitiveGetterAccessor(Lookup *l, ExecutionEngine *engine, const Value &object);
@@ -216,6 +223,20 @@ struct Q_QML_PRIVATE_EXPORT Lookup {
     void clear() {
         memset(&markDef, 0, sizeof(markDef));
     }
+
+    void releasePropertyCache()
+    {
+        if (getter == getterQObject
+                || getter == QQmlTypeWrapper::lookupSingletonProperty
+                || qmlContextPropertyGetter == QQmlContextWrapper::lookupScopeObjectProperty
+                || qmlContextPropertyGetter == QQmlContextWrapper::lookupContextObjectProperty) {
+            if (QQmlPropertyCache *pc = qobjectLookup.propertyCache)
+                pc->release();
+        } else if (getter == QQmlValueTypeWrapper::lookupGetter) {
+            if (QQmlPropertyCache *pc = qgadgetLookup.propertyCache)
+                pc->release();
+        }
+    }
 };
 
 Q_STATIC_ASSERT(std::is_standard_layout<Lookup>::value);
@@ -226,9 +247,7 @@ Q_STATIC_ASSERT(offsetof(Lookup, getter) == 0);
 inline void setupQObjectLookup(
         Lookup *lookup, const QQmlData *ddata, QQmlPropertyData *propertyData)
 {
-    if (QQmlPropertyCache *cache = lookup->qobjectLookup.propertyCache)
-        cache->release();
-
+    lookup->releasePropertyCache();
     Q_ASSERT(ddata->propertyCache != nullptr);
     lookup->qobjectLookup.propertyCache = ddata->propertyCache;
     lookup->qobjectLookup.propertyCache->addref();
diff --git a/src/qml/jsruntime/qv4qobjectwrapper.cpp b/src/qml/jsruntime/qv4qobjectwrapper.cpp
index 91d7712be0..29780590b4 100644
--- a/src/qml/jsruntime/qv4qobjectwrapper.cpp
+++ b/src/qml/jsruntime/qv4qobjectwrapper.cpp
@@ -876,22 +876,10 @@ ReturnedValue QObjectWrapper::virtualResolveLookupGetter(const Object *object, E
     }
 
     QV4::setupQObjectLookup(lookup, ddata, property, This);
-    lookup->getter = QV4::QObjectWrapper::lookupGetter;
+    lookup->getter = QV4::Lookup::getterQObject;
     return lookup->getter(lookup, engine, *object);
 }
 
-ReturnedValue QObjectWrapper::lookupGetter(Lookup *lookup, ExecutionEngine *engine, const Value &object)
-{
-    const auto revertLookup = [lookup, engine, &object]() {
-        lookup->qobjectLookup.propertyCache->release();
-        lookup->qobjectLookup.propertyCache = nullptr;
-        lookup->getter = Lookup::getterGeneric;
-        return Lookup::getterGeneric(lookup, engine, object);
-    };
-
-    return lookupGetterImpl(lookup, engine, object, /*useOriginalProperty*/ false, revertLookup);
-}
-
 bool QObjectWrapper::virtualResolveLookupSetter(Object *object, ExecutionEngine *engine, Lookup *lookup,
                                                 const Value &value)
 {
diff --git a/src/qml/jsruntime/qv4qobjectwrapper_p.h b/src/qml/jsruntime/qv4qobjectwrapper_p.h
index ae1b5b679b..51197243ad 100644
--- a/src/qml/jsruntime/qv4qobjectwrapper_p.h
+++ b/src/qml/jsruntime/qv4qobjectwrapper_p.h
@@ -183,7 +183,7 @@ struct Q_QML_EXPORT QObjectWrapper : public Object
     static ReturnedValue getProperty(ExecutionEngine *engine, QObject *object, QQmlPropertyData *property);
 
     static ReturnedValue virtualResolveLookupGetter(const Object *object, ExecutionEngine *engine, Lookup *lookup);
-    static ReturnedValue lookupGetter(Lookup *l, ExecutionEngine *engine, const Value &object);
+
     template <typename ReversalFunctor> static ReturnedValue lookupGetterImpl(Lookup *l, ExecutionEngine *engine, const Value &object, bool useOriginalProperty, ReversalFunctor revert);
     static bool virtualResolveLookupSetter(Object *object, ExecutionEngine *engine, Lookup *lookup, const Value &value);