QQuickOverlay: fix crash when a Drawer is destroyed

When a Drawer is deleted, the stack looks like this:
1 QQuickOverlayPrivate::removePopup
2 QQuickPopupPrivate::setWindow
3 QQuickPopup::setParentItem
4 QQuickPopup::~QQuickPopup
5 QQuickDrawer::~QQuickDrawer

It means that the QQuickPopup * we receive in
QQuickOverlayPrivate::removePopup() is not a QQuickDrawer * anymore.
Thus, the qobject_cast fails, we don't removeOne(), leave dangling
pointers in allDrawers, leading to a crash later in
QQuickOverlayPrivate::stackingOrderDrawers().

Store the Drawers as a list of QQuickPopup *. We downcast only in
QQuickOverlayPrivate::startDrag().

Pick-to: 6.4
Change-Id: I4840a18dec33426c58612db55937ea5988bf7650
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

src/quicktemplates/qquickoverlay.cpp

@@ -52,10 +52,10 @@ QList<QQuickPopup *> QQuickOverlayPrivate::stackingOrderPopups() const
     return popups;
 }
 
-QList<QQuickDrawer *> QQuickOverlayPrivate::stackingOrderDrawers() const
+QList<QQuickPopup *> QQuickOverlayPrivate::stackingOrderDrawers() const
 {
-    QList<QQuickDrawer *> sorted(allDrawers);
+    QList<QQuickPopup *> sorted(allDrawers);
-    std::sort(sorted.begin(), sorted.end(), [](const QQuickDrawer *one, const QQuickDrawer *another) {
+    std::sort(sorted.begin(), sorted.end(), [](const QQuickPopup *one, const QQuickPopup *another) {
         return one->z() > another->z();
     });
     return sorted;
@@ -83,8 +83,10 @@ bool QQuickOverlayPrivate::startDrag(QEvent *event, const QPointF &pos)
         }
     }
 
-    const QList<QQuickDrawer *> drawers = stackingOrderDrawers();
+    const QList<QQuickPopup *> drawers = stackingOrderDrawers();
-    for (QQuickDrawer *drawer : drawers) {
+    for (QQuickPopup *popup : drawers) {
+        QQuickDrawer *drawer = qobject_cast<QQuickDrawer *>(popup);
+        Q_ASSERT(drawer);
         QQuickDrawerPrivate *p = QQuickDrawerPrivate::get(drawer);
         if (p->startDrag(event)) {
             setMouseGrabberPopup(drawer);
@@ -240,7 +242,7 @@ void QQuickOverlayPrivate::removePopup(QQuickPopup *popup)
 {
     Q_Q(QQuickOverlay);
     allPopups.removeOne(popup);
-    if (allDrawers.removeOne(qobject_cast<QQuickDrawer *>(popup)))
+    if (allDrawers.removeOne(popup))
         q->setVisible(!allDrawers.isEmpty() || !q->childItems().isEmpty());
 }
 

src/quicktemplates/qquickoverlay_p_p.h

@@ -23,9 +23,9 @@
 QT_BEGIN_NAMESPACE
 
 class QQuickPopup;
-class QQuickDrawer;
 
-class QQuickOverlayPrivate : public QQuickItemPrivate, public QQuickItemChangeListener
+class Q_AUTOTEST_EXPORT QQuickOverlayPrivate : public QQuickItemPrivate,
+                                               public QQuickItemChangeListener
 {
 public:
     Q_DECLARE_PUBLIC(QQuickOverlay)
@@ -51,7 +51,7 @@ public:
     void setMouseGrabberPopup(QQuickPopup *popup);
 
     QList<QQuickPopup *> stackingOrderPopups() const;
-    QList<QQuickDrawer *> stackingOrderDrawers() const;
+    QList<QQuickPopup *> stackingOrderDrawers() const;
 
     void itemGeometryChanged(QQuickItem *item, QQuickGeometryChange change, const QRectF &diff) override;
 
@@ -60,7 +60,9 @@ public:
     QQmlComponent *modal = nullptr;
     QQmlComponent *modeless = nullptr;
     QList<QQuickPopup *> allPopups;
-    QList<QQuickDrawer *> allDrawers;
+    // Store drawers as QQuickPopup instead of QQuickDrawer because they're no longer
+    // QQuickDrawer by the time removePopup is called.
+    QList<QQuickPopup *> allDrawers;
     QPointer<QQuickPopup> mouseGrabberPopup;
 };
 

tests/auto/quickcontrols/qquickdrawer/tst_qquickdrawer.cpp

@@ -17,7 +17,7 @@
 #include <QtQuickTestUtils/private/qmlutils_p.h>
 #include <QtQuickTestUtils/private/visualtestutils_p.h>
 #include <QtQuickTemplates2/private/qquickapplicationwindow_p.h>
-#include <QtQuickTemplates2/private/qquickoverlay_p.h>
+#include <QtQuickTemplates2/private/qquickoverlay_p_p.h>
 #include <QtQuickTemplates2/private/qquickpopup_p_p.h>
 #include <QtQuickTemplates2/private/qquickdrawer_p.h>
 #include <QtQuickTemplates2/private/qquickbutton_p.h>
@@ -90,6 +90,8 @@ private slots:
 
     void topEdgeScreenEdge();
 
+    void bookkeepingInOverlay();
+
 private:
     QScopedPointer<QPointingDevice> touchDevice;
 };
@@ -1362,6 +1364,31 @@ void tst_QQuickDrawer::topEdgeScreenEdge()
     QTRY_COMPARE(drawer->position(), 1.0);
 }
 
+void tst_QQuickDrawer::bookkeepingInOverlay()
+{
+    QQmlEngine engine;
+    QQmlComponent component(&engine);
+    component.loadUrl(testFileUrl("window.qml"));
+
+    QScopedPointer<QObject> root(component.create());
+    QVERIFY2(!root.isNull(), qPrintable(component.errorString()));
+    QQuickWindow *window = qobject_cast<QQuickWindow *>(root.get());
+    QVERIFY(window);
+    QQuickDrawer *drawer = window->property("drawer").value<QQuickDrawer *>();
+    QVERIFY(drawer);
+    QQuickOverlay *overlay = QQuickOverlay::overlay(window);
+    QVERIFY(overlay);
+#ifdef QT_BUILD_INTERNAL
+    QQuickOverlayPrivate *overlayD = QQuickOverlayPrivate::get(overlay);
+    QVERIFY(!overlayD->stackingOrderDrawers().isEmpty());
+#endif
+
+    delete drawer;
+#ifdef QT_BUILD_INTERNAL
+    QVERIFY(overlayD->stackingOrderDrawers().isEmpty());
+#endif
+}
+
 QTEST_QUICKCONTROLS_MAIN(tst_QQuickDrawer)
 
 #include "tst_qquickdrawer.moc"