Hover event delivery: don't modify a list we are iterating

Delivering HoverLeave events might modify or even clear the list of
hoverItems, which invalidates the iterators, and crash.

Iterate over a copy of the list instead, and write the modified
copy back to the hoverItems list if that hasn't been cleared while
delivering events. To prevent that we deliver multiple HoverLeave
events to items, check in the delivery logic that the item's entry
in the map still has a valid ID for the event point. Streamline
that code a bit by preventing multiple lookups in the flatmap, and
instead use an iterator that we can use to update the existing
entry if it's valid.

Pick-to: 6.5
Fixes: QTBUG-122686
Change-Id: I5483e75884f1ddec37c4e98ecfee35e7596756c5
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>
(cherry picked from commit d00e890e5dbcc559bd63ae996e2b31485e3b03bf)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit 60e2292e1086ffd27aedd3a25b2cce5e5605a131)

3 files changed, 60 insertions, 6 deletions

diff --git a/src/quick/util/qquickdeliveryagent.cpp b/src/quick/util/qquickdeliveryagent.cpp
index 07506e68b7..d9b071e85b 100644
--- a/src/quick/util/qquickdeliveryagent.cpp
+++ b/src/quick/util/qquickdeliveryagent.cpp
@@ -1017,7 +1017,8 @@ bool QQuickDeliveryAgentPrivate::deliverHoverEvent(
     }
 
     // Prune the list for items that are no longer hovered
-    for (auto it = hoverItems.begin(); it != hoverItems.end();) {
+    auto hoverItemsCopy = hoverItems;
+    for (auto it = hoverItemsCopy.begin(); it != hoverItemsCopy.end();) {
         auto item = (*it).first.data();
         auto hoverId = (*it).second;
         if (hoverId == currentHoverId) {
@@ -1031,9 +1032,12 @@ bool QQuickDeliveryAgentPrivate::deliverHoverEvent(
                 const bool clearHover = true;
                 deliverHoverEventToItem(item, scenePos, lastScenePos, modifiers, timestamp, clearHover);
             }
-            it = hoverItems.erase(it);
+            it = hoverItemsCopy.erase(it);
         }
     }
+    // delivery of the events might have cleared hoverItems, so don't overwrite if empty
+    if (!hoverItems.isEmpty())
+        hoverItems = hoverItemsCopy;
 
     const bool itemsAreHovered = !hoverItems.isEmpty();
     return itemsWasHovered || itemsAreHovered;
@@ -1133,7 +1137,8 @@ bool QQuickDeliveryAgentPrivate::deliverHoverEventToItem(
     const QPointF localPos = item->mapFromScene(scenePos);
     const QPointF globalPos = item->mapToGlobal(localPos);
     const bool isHovering = item->contains(localPos);
-    const bool wasHovering = hoverItems.contains(item);
+    const auto hoverItemIterator = hoverItems.find(item);
+    const bool wasHovering = hoverItemIterator != hoverItems.end() && hoverItemIterator.value() != 0;
 
     qCDebug(lcHoverTrace) << "item:" << item << "scene pos:" << scenePos << "localPos:" << localPos
                           << "wasHovering:" << wasHovering << "isHovering:" << isHovering;
@@ -1149,14 +1154,18 @@ bool QQuickDeliveryAgentPrivate::deliverHoverEventToItem(
         // Also set hoveredLeafItemFound, so that only propagate in a straight
         // line towards the root from now on.
         hoveredLeafItemFound = true;
-        hoverItems[item] = currentHoverId;
+        if (hoverItemIterator != hoverItems.end())
+            hoverItemIterator.value() = currentHoverId;
+        else
+            hoverItems[item] = currentHoverId;
+
         if (wasHovering)
             accepted = sendHoverEvent(QEvent::HoverMove, item, scenePos, lastScenePos, modifiers, timestamp);
         else
             accepted = sendHoverEvent(QEvent::HoverEnter, item, scenePos, lastScenePos, modifiers, timestamp);
     } else if (wasHovering) {
         // A leave should never stop propagation
-        hoverItems[item] = 0;
+        hoverItemIterator.value() = 0;
         sendHoverEvent(QEvent::HoverLeave, item, scenePos, lastScenePos, modifiers, timestamp);
     }
 
@@ -1197,7 +1206,10 @@ bool QQuickDeliveryAgentPrivate::deliverHoverEventToItem(
                     // Mark the whole item as updated, even if only the handler is
                     // actually in a hovered state (because of HoverHandler.margins)
                     hoveredLeafItemFound = true;
-                    hoverItems[item] = currentHoverId;
+                    if (hoverItemIterator != hoverItems.end())
+                        hoverItemIterator.value() = currentHoverId;
+                    else
+                        hoverItems[item] = currentHoverId;
                     if (hh->isBlocking()) {
                         qCDebug(lcHoverTrace) << "skipping rest of hover delivery due to blocking" << hh;
                         accepted = true;
diff --git a/tests/auto/quick/qquickdeliveryagent/data/clearItemsOnHoverLeave.qml b/tests/auto/quick/qquickdeliveryagent/data/clearItemsOnHoverLeave.qml
new file mode 100644
index 0000000000..7b37b44050
--- /dev/null
+++ b/tests/auto/quick/qquickdeliveryagent/data/clearItemsOnHoverLeave.qml
@@ -0,0 +1,32 @@
+import QtQuick
+
+Rectangle{
+    id: mainWindow
+
+    visible: true
+    width: 800
+    height: 600
+
+    Column {
+        anchors.fill: parent
+        MouseArea {
+            width: parent.width
+            height: parent.height/3
+            hoverEnabled: true
+        }
+        MouseArea {
+            id: mouseArea
+            width: parent.width
+            height: parent.height/3
+            hoverEnabled: true
+            onExited: {
+                Window.window.close();
+            }
+        }
+        MouseArea {
+            width: parent.width
+            height: parent.height / 3
+            hoverEnabled: true
+        }
+    }
+}
diff --git a/tests/auto/quick/qquickdeliveryagent/tst_qquickdeliveryagent.cpp b/tests/auto/quick/qquickdeliveryagent/tst_qquickdeliveryagent.cpp
index 8a31536be4..a9ef64a324 100644
--- a/tests/auto/quick/qquickdeliveryagent/tst_qquickdeliveryagent.cpp
+++ b/tests/auto/quick/qquickdeliveryagent/tst_qquickdeliveryagent.cpp
@@ -140,6 +140,7 @@ private slots:
     void hoverPropagation_siblings();
     void hoverEnterOnItemMove();
     void hoverEnterOnItemMoveAfterHide();
+    void clearItemsOnHoverLeave();
 
 private:
     QScopedPointer<QPointingDevice> touchDevice = QScopedPointer<QPointingDevice>(QTest::createTouchDevice());
@@ -584,6 +585,15 @@ void tst_qquickdeliveryagent::hoverEnterOnItemMoveAfterHide()
     QCOMPARE(hoverItem.hoverEnter, false);
 }
 
+void tst_qquickdeliveryagent::clearItemsOnHoverLeave()
+{
+    QQuickView window;
+    QVERIFY(QQuickTest::showView(window, testFileUrl("clearItemsOnHoverLeave.qml")));
+
+    QTest::mouseMove(&window, QPoint(10, 205)); // Move to MouseArea that triggers close
+    QTest::mouseMove(&window, QPoint(10, 405)); // Exit MouseArea that triggers close.
+}
+
 QTEST_MAIN(tst_qquickdeliveryagent)
 
 #include "tst_qquickdeliveryagent.moc"