QQmlListWrapper: throw type errors or warnings if append fails

Some list implementations don't accept nullptr object and don't grow the
underlying list. This breaks assumptions made in e.g. unshift() or
set_length(), which increase the size of the list by appending nullptr
objects.

Throw type errors if lists don't come back with the correct size after
the calls to append. For the push method, which has accepted this
behavior for a long time, we only generate a runtime warning and return
the actual length of the list.

Pick-to: 6.6 6.5
Change-Id: I4623292d2ec2b3343f119d789064ee69ebe90cb5
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>

src/qml/qml/qqmllistwrapper.cpp

@@ -3,6 +3,8 @@
 
 #include "qqmllistwrapper_p.h"
 
+#include <QtQml/qqmlinfo.h>
+
 #include <private/qqmllist_p.h>
 
 #include <private/qv4arrayiterator_p.h>
@@ -283,7 +285,11 @@ ReturnedValue PropertyListPrototype::method_push(const FunctionObject *b, const
             property->append(property, argv[i].as<QV4::QObjectWrapper>()->object());
     }
 
-    return Encode(uint(length + argc));
+    const auto actualLength = property->count(property);
+    if (actualLength != length + argc)
+        qmlWarning(property->object) << "List didn't append all objects";
+
+    return Encode(uint(actualLength));
 }
 
 ReturnedValue PropertyListPrototype::method_shift(const FunctionObject *b, const Value *thisObject, const Value *, int)
@@ -444,12 +450,16 @@ ReturnedValue PropertyListPrototype::method_unshift(const FunctionObject *b, con
 
     for (int i = 0; i < argc; ++i)
         property->append(property, nullptr);
+    if (property->count(property) != argc + len)
+        return scope.engine->throwTypeError(u"List doesn't append null objects"_s);
 
     for (qsizetype k = len; k > 0; --k)
         property->replace(property, k + argc - 1, property->at(property, k - 1));
 
-    for (int i = 0; i < argc; ++i)
-        property->replace(property, i, argv[i].as<QObjectWrapper>()->object());
+    for (int i = 0; i < argc; ++i) {
+        const auto *wrapper = argv[i].as<QObjectWrapper>();
+        property->replace(property, i, wrapper ? wrapper->object() : nullptr);
+    }
 
     return Encode(uint(len + argc));
 }
@@ -656,11 +666,18 @@ ReturnedValue PropertyListPrototype::method_set_length(const FunctionObject *b,
     }
 
     if (!property->append)
-            return scope.engine->throwTypeError(u"List doesn't define an Append function"_s);
+        return scope.engine->throwTypeError(u"List doesn't define an Append function"_s);
 
     for (uint i = count; i < newLength; ++i)
         property->append(property, nullptr);
 
+    count = property->count(property);
+    if (!qIsAtMostUintLimit(count))
+        return scope.engine->throwRangeError(QString::fromLatin1("List length out of range."));
+
+    if (uint(count) != newLength)
+        return scope.engine->throwTypeError(u"List doesn't append null objects"_s);
+
     return true;
 
 }

tests/auto/qml/qqmllistreference/data/listIgnoresNull.qml

@@ -0,0 +1,12 @@
+import QtQml
+import Test
+
+TestItem {
+    id: testItem
+
+    Component.onCompleted : {
+        testItem.data.push(null);
+        testItem.data.length = 5;
+        testItem.data.unshift(null);
+    }
+}

tests/auto/qml/qqmllistreference/tst_qqmllistreference.cpp

@@ -58,6 +58,8 @@ private slots:
     void jsArrayMethods();
     void jsArrayMethodsWithParams_data();
     void jsArrayMethodsWithParams();
+    void listIgnoresNull_data() { modeData(); }
+    void listIgnoresNull();
 };
 
 class TestType : public QObject
@@ -73,12 +75,18 @@ public:
         SyntheticClearAndReplace,
         SyntheticRemoveLast,
         SyntheticRemoveLastAndReplace,
-        AutomaticPointer
+        AutomaticPointer,
+        IgnoreNullValues,
     };
 
     static void append(QQmlListProperty<TestType> *p, TestType *v) {
         reinterpret_cast<QList<TestType *> *>(p->data)->append(v);
     }
+    static void appendNoNullValues(QQmlListProperty<TestType> *p, TestType *v) {
+        if (!v)
+            return;
+        reinterpret_cast<QList<TestType *> *>(p->data)->append(v);
+    }
     static qsizetype count(QQmlListProperty<TestType> *p) {
         return reinterpret_cast<QList<TestType *> *>(p->data)->size();
     }
@@ -121,6 +129,10 @@ public:
         case AutomaticPointer:
             property = QQmlListProperty<TestType>(this, &data);
             break;
+        case IgnoreNullValues:
+            property = QQmlListProperty<TestType>(this, &data, appendNoNullValues, count, at, clear,
+                                                  replace, removeLast);
+            break;
         }
     }
 
@@ -142,6 +154,7 @@ void tst_qqmllistreference::modeData()
     QTest::addRow("SyntheticClearAndReplace") << TestType::SyntheticClearAndReplace;
     QTest::addRow("SyntheticRemoveLast") << TestType::SyntheticRemoveLast;
     QTest::addRow("SyntheticRemoveLastAndReplace") << TestType::SyntheticRemoveLastAndReplace;
+    QTest::addRow("IgnoreNullValues") << TestType::IgnoreNullValues;
 }
 
 void tst_qqmllistreference::initTestCase()
@@ -1036,6 +1049,39 @@ void tst_qqmllistreference::jsArrayMethodsWithParams()
     QCOMPARE(object->property("listPropertyLastIndexOf"), object->property("jsArrayLastIndexOf"));
 }
 
+/*!
+    Some of our list implementations ignore attempts to append a null object.
+    This should result in warnings or type errors, and not crash our wrapper
+    code.
+*/
+void tst_qqmllistreference::listIgnoresNull()
+{
+    QFETCH(const TestType::Mode, mode);
+    static TestType::Mode globalMode;
+    globalMode = mode;
+    struct TestItem : public TestType
+    {
+        TestItem() : TestType(globalMode) {}
+    };
+
+    const auto id = qmlRegisterType<TestItem>("Test", 1, 0, "TestItem");
+    const auto unregister = qScopeGuard([id]{
+        QQmlPrivate::qmlunregister(QQmlPrivate::TypeRegistration, id);
+    });
+
+    QQmlEngine engine;
+    QQmlComponent component(&engine, testFileUrl("listIgnoresNull.qml"));
+
+    // For lists that don't append null values, creating the component shouldn't crash
+    // in the onCompleted handler, but generate type errors and warnings.
+    if (mode == TestType::IgnoreNullValues) {
+        QTest::ignoreMessage(QtWarningMsg, QRegularExpression(".* QML TestItem: List didn't append all objects$"));
+        QTest::ignoreMessage(QtWarningMsg, QRegularExpression(".* TypeError: List doesn't append null objects$"));
+    }
+    QScopedPointer<QObject> object( component.create() );
+    QVERIFY(object != nullptr);
+}
+
 QTEST_MAIN(tst_qqmllistreference)
 
 #include "tst_qqmllistreference.moc"