From eb52c31c5b8cb6f05fce598519034c4fc32c4c3f Mon Sep 17 00:00:00 2001
From: Volker Hilsheimer <volker.hilsheimer@qt.io>
Date: Thu, 17 Mar 2022 15:23:55 +0100
Subject: When clearing selected items, iterate over a copy of the QSet

Selection change handlers of the items might call a method that
implicitly recreates the selectedItems QSet, which then invalidates
the iterators of the ranged for loop, resulting in crashes.

Iterate over a copy of the set instead.

Add a test case that crashen without the fix.

Fixes: QTBUG-101651
Change-Id: I6da6f4043fe1906b0186931a37283f635cb5a404
Reviewed-by: Axel Spoerl <axel.spoerl@qt.io>
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>
(cherry picked from commit 5e566ab373bd6161f82b00c5a1f24bd210051140)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
---
 src/widgets/graphicsview/qgraphicsscene.cpp        |  6 +-
 .../qgraphicsscene/tst_qgraphicsscene.cpp          | 69 ++++++++++++++++++++++
 2 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/src/widgets/graphicsview/qgraphicsscene.cpp b/src/widgets/graphicsview/qgraphicsscene.cpp
index f41ced1a38..d00d69c47c 100644
--- a/src/widgets/graphicsview/qgraphicsscene.cpp
+++ b/src/widgets/graphicsview/qgraphicsscene.cpp
@@ -2285,9 +2285,11 @@ void QGraphicsScene::clearSelection()
 
     // Disable emitting selectionChanged
     ++d->selectionChanging;
-    bool changed = !d->selectedItems.isEmpty();
+    // iterate over a copy, as clearing selection might invalidate selectedItems
+    const auto selectedItems = d->selectedItems;
+    bool changed = !selectedItems.isEmpty();
 
-    for (QGraphicsItem *item : qAsConst(d->selectedItems))
+    for (QGraphicsItem *item : selectedItems)
         item->setSelected(false);
     d->selectedItems.clear();
 
diff --git a/tests/auto/widgets/graphicsview/qgraphicsscene/tst_qgraphicsscene.cpp b/tests/auto/widgets/graphicsview/qgraphicsscene/tst_qgraphicsscene.cpp
index b8991b5a9a..9a8b94852d 100644
--- a/tests/auto/widgets/graphicsview/qgraphicsscene/tst_qgraphicsscene.cpp
+++ b/tests/auto/widgets/graphicsview/qgraphicsscene/tst_qgraphicsscene.cpp
@@ -294,6 +294,8 @@ private slots:
     void taskQTBUG_42915_focusNextPrevChild();
     void taskQTBUG_85088_previewTextfailWhenLostFocus();
 
+    void deleteItemsOnChange();
+
 private:
     QRect m_availableGeometry = QGuiApplication::primaryScreen()->availableGeometry();
     QSize m_testSize;
@@ -4969,5 +4971,72 @@ void tst_QGraphicsScene::taskQTBUG_85088_previewTextfailWhenLostFocus()
     QCOMPARE(simpleTextItem->toPlainText(), str + str);
 }
 
+void tst_QGraphicsScene::deleteItemsOnChange()
+{
+    QGraphicsScene scene;
+
+    class SelectionItem : public QGraphicsRectItem {
+    public:
+        QRectF boundingRect() const override { return QRectF(); }
+    };
+
+    class ChangeItem : public QGraphicsItem
+    {
+    public:
+        ChangeItem()
+        {
+            setFlag(QGraphicsItem::ItemIsSelectable, true);
+            setFlag(QGraphicsItem::ItemIsMovable, true);
+        }
+        QRectF boundingRect() const override
+        {
+            return QRectF(0,0,100,100);
+        }
+
+    protected:
+        void paint(QPainter *painter, const QStyleOptionGraphicsItem *, QWidget *) override
+        {
+            painter->fillRect(boundingRect().toRect(), isSelected() ? Qt::yellow : Qt::cyan);
+        }
+
+        QVariant itemChange(GraphicsItemChange change, const QVariant &value) override
+        {
+            if (change != QGraphicsItem::ItemSelectedHasChanged)
+                return QGraphicsItem::itemChange(change, value);
+            if (value.toBool()) {
+                selectionRect = new SelectionItem;
+                scene()->addItem(selectionRect);
+            } else {
+                // this recreates the selectedItems QSet inside of QGraphicsScene,
+                // invalidating iterators. See QTBUG-101651.
+                scene()->selectedItems();
+                delete selectionRect;
+                selectionRect = nullptr;
+            }
+            return QGraphicsItem::itemChange(change, value);
+        }
+    private:
+        SelectionItem *selectionRect = nullptr;
+    };
+
+    ChangeItem item1;
+    item1.setPos(0, 0);
+    ChangeItem item2;
+    item1.setPos(50, 50);
+
+    scene.addItem(&item1);
+    scene.addItem(&item2);
+
+    QGraphicsView view;
+    view.setScene(&scene);
+    view.show();
+
+    QVERIFY(QTest::qWaitForWindowExposed(&view));
+
+    // this should not crash - see QTBUG-101651
+    QTest::mouseClick(view.viewport(), Qt::LeftButton, {}, QPoint(120, 120));
+    QTest::mouseClick(view.viewport(), Qt::LeftButton, {}, QPoint(25, 25));
+}
+
 QTEST_MAIN(tst_QGraphicsScene)
 #include "tst_qgraphicsscene.moc"
-- 
cgit v1.2.3