QtQml: Fix return type constructions when calling methods

We now expect the return type to be initialized in all cases. This is in
line with what the various metacall() methods expect. Unifying this
behavior makes it much easier to reason about and avoids complicated
bugs and memory leaks. The code generated by QmlCompiler always passes
initialized values for the return type anyway.

Amends commit 4f1b9156a48e44cf1f127a4563d0ac69ab436f12
Amends commit 02c4c817fe1cfa4766c56759be99fb081382a586

Change-Id: I26c016676dd82c91d6ef81762b5c4b599f6f7f72
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>

5 files changed, 46 insertions, 31 deletions

diff --git a/src/qml/jsruntime/qv4jscall_p.h b/src/qml/jsruntime/qv4jscall_p.h
index 82a83fd06f..06a8e62761 100644
--- a/src/qml/jsruntime/qv4jscall_p.h
+++ b/src/qml/jsruntime/qv4jscall_p.h
@@ -125,7 +125,8 @@ ReturnedValue convertAndCall(
         types[i + 1] = argumentType;
         if (const qsizetype argumentSize = argumentType.sizeOf()) {
             Q_ALLOCA_VAR(void, argument, argumentSize);
-            argumentType.construct(argument);
+            if (argumentType.flags() & QMetaType::NeedsConstruction)
+                argumentType.construct(argument);
             if (i < argc)
                 ExecutionEngine::metaTypeFromJS(argv[i], argumentType, argument);
             values[i + 1] = argument;
@@ -139,6 +140,8 @@ ReturnedValue convertAndCall(
     if (const qsizetype returnSize = types[0].sizeOf()) {
         Q_ALLOCA_ASSIGN(void, returnValue, returnSize);
         values[0] = returnValue;
+        if (types[0].flags() & QMetaType::NeedsConstruction)
+            types[0].construct(returnValue);
     } else {
         values[0] = nullptr;
     }
@@ -151,13 +154,16 @@ ReturnedValue convertAndCall(
     ReturnedValue result;
     if (values[0]) {
         result = engine->metaTypeToJS(types[0], values[0]);
-        types[0].destruct(values[0]);
+        if (types[0].flags() & QMetaType::NeedsDestruction)
+            types[0].destruct(values[0]);
     } else {
         result = Encode::undefined();
     }
 
-    for (qsizetype i = 1, end = numFunctionArguments + 1; i < end; ++i)
-        types[i].destruct(values[i]);
+    for (qsizetype i = 1, end = numFunctionArguments + 1; i < end; ++i) {
+        if (types[i].flags() & QMetaType::NeedsDestruction)
+            types[i].destruct(values[i]);
+    }
 
     return result;
 }
@@ -477,10 +483,9 @@ void coerceAndCall(
         memcpy(transformedArguments, argv, (argc + 1) * sizeof(void *));
 
         if (frameReturn == QMetaType::fromType<QVariant>()) {
-            void *returnValue = argv[0];
-            new (returnValue) QVariant(returnType);
-            transformedResult = transformedArguments[0]
-                    = static_cast<QVariant *>(returnValue)->data();
+            QVariant *returnValue = static_cast<QVariant *>(argv[0]);
+            *returnValue = QVariant(returnType);
+            transformedResult = transformedArguments[0] = returnValue->data();
             returnsQVariantWrapper = true;
         } else if (returnType.sizeOf() > 0) {
             Q_ALLOCA_ASSIGN(void, transformedResult, returnType.sizeOf());
@@ -545,8 +550,11 @@ void coerceAndCall(
     call(transformedArguments, numFunctionArguments);
 
     if (transformedResult && !returnsQVariantWrapper) {
-        if (frameReturn.sizeOf() > 0)
+        if (frameReturn.sizeOf() > 0) {
+            if (frameReturn.flags() & QMetaType::NeedsDestruction)
+                frameReturn.destruct(argv[0]);
             coerce(engine, returnType, transformedResult, frameReturn, argv[0]);
+        }
         if (returnType.flags() & QMetaType::NeedsDestruction)
             returnType.destruct(transformedResult);
     }
diff --git a/src/qml/qml/qqmlbinding.cpp b/src/qml/qml/qqmlbinding.cpp
index 434d92d3d2..a7fe363058 100644
--- a/src/qml/qml/qqmlbinding.cpp
+++ b/src/qml/qml/qqmlbinding.cpp
@@ -675,20 +675,21 @@ void QQmlBinding::doUpdate(const DeleteWatcher &watcher, QQmlPropertyData::Write
     if (v4Function && v4Function->kind == QV4::Function::AotCompiled && !hasBoundFunction()) {
         const auto returnType = v4Function->aotCompiledFunction->returnType;
         if (returnType == QMetaType::fromType<QVariant>()) {
-            // It expects uninitialized memory
-            Q_ALLOCA_VAR(QVariant, result, sizeof(QVariant));
-            const bool isUndefined = !evaluate(result, returnType);
+            QVariant result;
+            const bool isUndefined = !evaluate(&result, returnType);
             if (canWrite())
-                error = !write(result->data(), result->metaType(), isUndefined, flags);
-            result->~QVariant();
+                error = !write(result.data(), result.metaType(), isUndefined, flags);
         } else {
             const auto size = returnType.sizeOf();
             if (Q_LIKELY(size > 0)) {
                 Q_ALLOCA_VAR(void, result, size);
+                if (returnType.flags() & QMetaType::NeedsConstruction)
+                    returnType.construct(result);
                 const bool isUndefined = !evaluate(result, returnType);
                 if (canWrite())
                     error = !write(result, returnType, isUndefined, flags);
-                returnType.destruct(result);
+                if (returnType.flags() & QMetaType::NeedsDestruction)
+                    returnType.destruct(result);
             } else if (canWrite()) {
                 error = !write(QV4::Encode::undefined(), true, flags);
             }
diff --git a/src/qml/qml/qqmlpropertybinding_p.h b/src/qml/qml/qqmlpropertybinding_p.h
index 50688fa245..edf11bc221 100644
--- a/src/qml/qml/qqmlpropertybinding_p.h
+++ b/src/qml/qml/qqmlpropertybinding_p.h
@@ -286,19 +286,27 @@ bool QQmlPropertyBinding::evaluate(QMetaType metaType, void *dataPtr)
     if (!hasBoundFunction()) {
         Q_ASSERT(metaType.sizeOf() > 0);
 
-        // No need to construct here. evaluate() expects uninitialized memory.
-        const auto size = [&]() -> qsizetype {
+        using Tuple = std::tuple<qsizetype, bool, bool>;
+        const auto [size, needsConstruction, needsDestruction] = [&]() -> Tuple {
             switch (type) {
-                case QMetaType::QObjectStar: return sizeof(QObject *);
-                case QMetaType::Bool: return sizeof(bool);
-                case QMetaType::Int: return (sizeof(int));
-                case QMetaType::Double: return (sizeof(double));
-                case QMetaType::Float: return (sizeof(float));
-                case QMetaType::QString: return (sizeof(QString));
-                default: return metaType.sizeOf();
+            case QMetaType::QObjectStar: return Tuple(sizeof(QObject *), false, false);
+            case QMetaType::Bool:        return Tuple(sizeof(bool), false, false);
+            case QMetaType::Int:         return Tuple(sizeof(int), false, false);
+            case QMetaType::Double:      return Tuple(sizeof(double), false, false);
+            case QMetaType::Float:       return Tuple(sizeof(float), false, false);
+            case QMetaType::QString:     return Tuple(sizeof(QString), true, true);
+            default: {
+                const auto flags = metaType.flags();
+                return Tuple(
+                        metaType.sizeOf(),
+                        flags & QMetaType::NeedsConstruction,
+                        flags & QMetaType::NeedsDestruction);
+            }
             }
         }();
         Q_ALLOCA_VAR(void, result, size);
+        if (needsConstruction)
+            metaType.construct(result);
 
         const bool evaluatedToUndefined = !jsExpression()->evaluate(&result, &metaType, 0);
         if (!handleErrorAndUndefined(evaluatedToUndefined))
@@ -326,10 +334,12 @@ bool QQmlPropertyBinding::evaluate(QMetaType metaType, void *dataPtr)
 
         const bool hasChanged = !metaType.equals(result, dataPtr);
         if (hasChanged) {
-            metaType.destruct(dataPtr);
+            if (needsDestruction)
+                metaType.destruct(dataPtr);
             metaType.construct(dataPtr, result);
         }
-        metaType.destruct(result);
+        if (needsDestruction)
+            metaType.destruct(result);
         return hasChanged;
     }
 
diff --git a/src/qml/qml/qqmlvmemetaobject.cpp b/src/qml/qml/qqmlvmemetaobject.cpp
index fb90e3e800..1aaff365e9 100644
--- a/src/qml/qml/qqmlvmemetaobject.cpp
+++ b/src/qml/qml/qqmlvmemetaobject.cpp
@@ -1119,14 +1119,10 @@ int QQmlVMEMetaObject::metaCall(QObject *o, QMetaObject::Call c, int _id, void *
                 if (arguments && arguments->names) {
                     const quint32 parameterCount = arguments->names->size();
                     Q_ASSERT(parameterCount == function->formalParameterCount());
-                    if (void *result = a[0])
-                        arguments->types[0].destruct(result);
                     function->call(nullptr, a, arguments->types, parameterCount);
                 } else {
                     Q_ASSERT(function->formalParameterCount() == 0);
                     const QMetaType returnType = methodData->propType();
-                    if (void *result = a[0])
-                        returnType.destruct(result);
                     function->call(nullptr, a, &returnType, 0);
                 }
 
diff --git a/src/qmlcompiler/qqmljscompiler.cpp b/src/qmlcompiler/qqmljscompiler.cpp
index 2f1bc7a391..69d80283e3 100644
--- a/src/qmlcompiler/qqmljscompiler.cpp
+++ b/src/qmlcompiler/qqmljscompiler.cpp
@@ -468,7 +468,7 @@ void wrapCall(const QQmlPrivate::AOTCompiledContext *aotContext, void *dataPtr,
        binding(aotContext, argumentsPtr);
     } else {
         if (dataPtr) {
-           new (dataPtr) return_type(binding(aotContext, argumentsPtr));
+           *static_cast<return_type *>(dataPtr) = binding(aotContext, argumentsPtr);
         } else {
            binding(aotContext, argumentsPtr);
         }