From c49fbc7a86c5d25e516091ee2aa933ab32e2606b Mon Sep 17 00:00:00 2001
From: Ulf Hermann <ulf.hermann@qt.io>
Date: Wed, 23 Mar 2022 11:36:59 +0100
Subject: V4 ArrayIterator: Protect retrieved value from GC

When constructing the iterator return object, the garbage collector may
run, and drop the element value we want to return.

Fixes: QTBUG-101700
Change-Id: I60c9b0b9fbb9e784fa089a8b5bb274d02ef7fc1f
Reviewed-by: Maximilian Goldstein <max.goldstein@qt.io>
Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
(cherry picked from commit 185760fa44f5b62f1ed3f10a458f4bc38072768f)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
---
 src/qml/jsruntime/qv4arrayiterator.cpp     |  6 +--
 tests/auto/qml/qjsengine/tst_qjsengine.cpp | 71 ++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 3 deletions(-)

diff --git a/src/qml/jsruntime/qv4arrayiterator.cpp b/src/qml/jsruntime/qv4arrayiterator.cpp
index 199b1a728a..51387edf6e 100644
--- a/src/qml/jsruntime/qv4arrayiterator.cpp
+++ b/src/qml/jsruntime/qv4arrayiterator.cpp
@@ -86,18 +86,18 @@ ReturnedValue ArrayIteratorPrototype::method_next(const FunctionObject *b, const
         return IteratorPrototype::createIterResultObject(scope.engine, Value::fromInt32(index), false);
     }
 
-    ReturnedValue elementValue = a->get(index);
+    QV4::ScopedValue elementValue(scope, a->get(index));
     CHECK_EXCEPTION();
 
     if (itemKind == ValueIteratorKind) {
-        return IteratorPrototype::createIterResultObject(scope.engine, Value::fromReturnedValue(elementValue), false);
+        return IteratorPrototype::createIterResultObject(scope.engine, elementValue, false);
     } else {
         Q_ASSERT(itemKind == KeyValueIteratorKind);
 
         ScopedArrayObject resultArray(scope, scope.engine->newArrayObject());
         resultArray->arrayReserve(2);
         resultArray->arrayPut(0, Value::fromInt32(index));
-        resultArray->arrayPut(1, Value::fromReturnedValue(elementValue));
+        resultArray->arrayPut(1, elementValue);
         resultArray->setArrayLengthUnchecked(2);
 
         return IteratorPrototype::createIterResultObject(scope.engine, resultArray, false);
diff --git a/tests/auto/qml/qjsengine/tst_qjsengine.cpp b/tests/auto/qml/qjsengine/tst_qjsengine.cpp
index e9e22d4a93..85ec400cea 100644
--- a/tests/auto/qml/qjsengine/tst_qjsengine.cpp
+++ b/tests/auto/qml/qjsengine/tst_qjsengine.cpp
@@ -281,6 +281,7 @@ private slots:
     void uiLanguage();
     void urlObject();
     void thisInConstructor();
+    void forOfAndGc();
 
 public:
     Q_INVOKABLE QJSValue throwingCppMethod1();
@@ -5501,6 +5502,76 @@ void tst_QJSEngine::thisInConstructor()
     QVERIFY(result.isObject());
 }
 
+void tst_QJSEngine::forOfAndGc()
+{
+    // We want to guard against the iterator of a for..of loop leaving the result unprotected from
+    // garbage collection. It should be possible to construct a pure JS test case, but due to the
+    // vaguaries of garbage collection it's hard to reliably trigger the crash. This test is the
+    // best I could come up with.
+
+    QQmlEngine engine;
+    QQmlComponent c(&engine);
+    c.setData(R"(
+        import QtQml
+
+        QtObject {
+            id: counter
+            property int count: 0
+
+            property DelegateModel model: DelegateModel {
+                id: filesModel
+
+                model: ListModel {
+                    Component.onCompleted: {
+                        for (let idx = 0; idx < 50; idx++)
+                            append({"i" : idx})
+                    }
+                }
+
+                groups: [
+                    DelegateModelGroup {
+                        name: "selected"
+                    }
+                ]
+
+                function getSelected() {
+                    for (let i = 0; i < items.count; ++i) {
+                        var item = items.get(i)
+                        for (let el of item.groups) {
+                            if (el === "selected")
+                                ++counter.count
+                        }
+                    }
+                }
+
+                property bool bSelect: true
+                function selectAll() {
+                    for (let i = 0; i < items.count; ++i) {
+                        if (bSelect && !items.get(i).inSelected)
+                            items.addGroups(i, 1, ["selected"])
+                        else
+                            items.removeGroups(i, 1, ["selected"])
+                        getSelected()
+                    }
+                    bSelect = !bSelect
+                }
+            }
+
+            property Timer timer: Timer {
+                running: true
+                interval: 1
+                repeat: true
+                onTriggered: filesModel.selectAll()
+            }
+        }
+    )", QUrl());
+
+    QVERIFY2(c.isReady(), qPrintable(c.errorString()));
+    QScopedPointer<QObject> o(c.create());
+
+    QTRY_VERIFY(o->property("count").toInt() > 32768);
+}
+
 QTEST_MAIN(tst_QJSEngine)
 
 #include "tst_qjsengine.moc"
-- 
cgit v1.2.3