From e5f4aa242c9ca223fbb200faae215f8dcc64e5b8 Mon Sep 17 00:00:00 2001 From: Santhosh Kumar Date: Wed, 13 Aug 2025 18:02:40 +0200 Subject: [PATCH] Reset sub-menu and content item during deletion in the quick menu The sub-menu item within the menu can have invalid reference when its created and destroyed through loader. The loader uses the incubator to create the items asynchronously (when configured asynchronous), when it becomes active and places them on the object stack. These items are cleared when the loader becomes inactive. In a case where we have the parent menu at the root, the loader places the parent menu at the bottom of the stack, and the sub-menu or any other child item within the menu is placed on top of it during its creation, and the deletion of these items happens from the top, which leads to the paren menu being deleted last. These menu items containing the sub-menu reference have not been reset when the sub-menus are deleted in this way, thus it can lead to a crash as the parent menu refers to an invalid item while resetting its state. The same happens for the contentItem within the menu. This patch ensures that the content item and sub-menu within the menu item are reset during deletion, which avoids the parent menu referring an invalid item. Fixes: QTBUG-137160 Task-number: QTBUG-139552 Pick-to: 6.9 6.8 6.5 Change-Id: Ia9688db90d6a8c8f4e4fa2aadb7e90cb3d8ea25b Reviewed-by: Oliver Eftevaag (cherry picked from commit 36216db956096d6cabdb28b08368dd76db92c652) Reviewed-by: Allan Sandfeld Jensen --- src/quicktemplates/qquickmenu.cpp | 48 ++++++++++---- src/quicktemplates/qquickmenu_p_p.h | 6 +- src/quicktemplates/qquickmenuitem_p_p.h | 2 +- .../data/loadMenuAsynchronously.qml | 62 +++++++++++++++++++ .../qquickmenu/tst_qquickmenu.cpp | 44 +++++++++++++ 5 files changed, 147 insertions(+), 15 deletions(-) create mode 100644 tests/auto/quickcontrols/qquickmenu/data/loadMenuAsynchronously.qml diff --git a/src/quicktemplates/qquickmenu.cpp b/src/quicktemplates/qquickmenu.cpp index 005ca65035..cf2dce364b 100644 --- a/src/quicktemplates/qquickmenu.cpp +++ b/src/quicktemplates/qquickmenu.cpp @@ -884,10 +884,16 @@ void QQuickMenuPrivate::itemSiblingOrderChanged(QQuickItem *) void QQuickMenuPrivate::itemDestroyed(QQuickItem *item) { - QQuickPopupPrivate::itemDestroyed(item); - int index = contentModel->indexOf(item, nullptr); - if (index != -1) - removeItem(index, item); + if (item == contentItem) { + resetContentItem(); + } else { + QQuickPopupPrivate::itemDestroyed(item); + if (contentModel) { + int index = contentModel->indexOf(item, nullptr); + if (index != -1) + removeItem(index, item); + } + } } void QQuickMenuPrivate::itemGeometryChanged(QQuickItem *item, QQuickGeometryChange, const QRectF &) @@ -1320,6 +1326,20 @@ void QQuickMenuPrivate::contentData_clear(QQmlListProperty *prop) QQuickMenuPrivate::get(q)->contentData.clear(); } +void QQuickMenuPrivate::resetContentItem() +{ + if (contentItem) { + QQuickItemPrivate::get(contentItem)->removeItemChangeListener(this, QQuickItemPrivate::Children); + QQuickItemPrivate::get(contentItem)->removeItemChangeListener(this, QQuickItemPrivate::Destroyed); + QQuickItemPrivate::get(contentItem)->removeItemChangeListener(this, QQuickItemPrivate::Geometry); + + const auto children = contentItem->childItems(); + for (QQuickItem *child : std::as_const(children)) + QQuickItemPrivate::get(child)->removeItemChangeListener(this, QQuickItemPrivate::SiblingOrder); + contentItem = nullptr; + } +} + QQuickMenu::QQuickMenu(QObject *parent) : QQuickPopup(*(new QQuickMenuPrivate), parent) { @@ -1336,6 +1356,15 @@ QQuickMenu::~QQuickMenu() << "item count:" << d->contentModel->count() << "native item count:" << d->nativeItems.count(); + // It would be better to reset the sub-menu within the menu-item during its destruction + // as there can be a chance that the parent menu use invalid reference leading to + // application crash (as mentioned in the bug report QTBUG-137160) + if (auto *menuItem = qobject_cast(d->parentItem)) { + if (menuItem->subMenu() == this) { + auto *menuItemPriv = QQuickMenuItemPrivate::get(menuItem); + menuItemPriv->setSubMenu(nullptr); + } + } // We have to remove items to ensure that our change listeners on the item // are removed. It's too late to do this in ~QQuickMenuPrivate, as // contentModel has already been destroyed before that is called. @@ -1344,14 +1373,7 @@ QQuickMenu::~QQuickMenu() while (d->contentModel->count() > 0) d->removeItem(0, d->itemAt(0), QQuickMenuPrivate::DestructionPolicy::Destroy); - if (d->contentItem) { - QQuickItemPrivate::get(d->contentItem)->removeItemChangeListener(d, QQuickItemPrivate::Children); - QQuickItemPrivate::get(d->contentItem)->removeItemChangeListener(d, QQuickItemPrivate::Geometry); - - const auto children = d->contentItem->childItems(); - for (QQuickItem *child : std::as_const(children)) - QQuickItemPrivate::get(child)->removeItemChangeListener(d, QQuickItemPrivate::SiblingOrder); - } + d->resetContentItem(); } /*! @@ -2133,10 +2155,12 @@ void QQuickMenu::contentItemChange(QQuickItem *newItem, QQuickItem *oldItem) if (oldItem) { QQuickItemPrivate::get(oldItem)->removeItemChangeListener(d, QQuickItemPrivate::Children); + QQuickItemPrivate::get(newItem)->removeItemChangeListener(d, QQuickItemPrivate::Destroyed); QQuickItemPrivate::get(oldItem)->removeItemChangeListener(d, QQuickItemPrivate::Geometry); } if (newItem) { QQuickItemPrivate::get(newItem)->addItemChangeListener(d, QQuickItemPrivate::Children); + QQuickItemPrivate::get(newItem)->addItemChangeListener(d, QQuickItemPrivate::Destroyed); QQuickItemPrivate::get(newItem)->updateOrAddGeometryChangeListener(d, QQuickGeometryChange::Width); } diff --git a/src/quicktemplates/qquickmenu_p_p.h b/src/quicktemplates/qquickmenu_p_p.h index 1a51fc27b8..bfd58b5568 100644 --- a/src/quicktemplates/qquickmenu_p_p.h +++ b/src/quicktemplates/qquickmenu_p_p.h @@ -129,6 +129,8 @@ public: QPalette defaultPalette() const override; virtual QQuickPopup::PopupType resolvedPopupType() const override; + void resetContentItem(); + bool cascade = false; bool triedToCreateNativeMenu = false; int hoverTimer = 0; @@ -137,9 +139,9 @@ public: qreal textPadding = 0; QPointer parentMenu; QPointer currentItem; - QQuickItem *contentItem = nullptr; // TODO: cleanup + QPointer contentItem; QList contentData; - QQmlObjectModel *contentModel; + QPointer contentModel; QQmlComponent *delegate = nullptr; QString title; QQuickIcon icon; diff --git a/src/quicktemplates/qquickmenuitem_p_p.h b/src/quicktemplates/qquickmenuitem_p_p.h index 3ef4981570..bc1e585985 100644 --- a/src/quicktemplates/qquickmenuitem_p_p.h +++ b/src/quicktemplates/qquickmenuitem_p_p.h @@ -47,7 +47,7 @@ public: bool highlighted = false; QQuickDeferredPointer arrow; QQuickMenu *menu = nullptr; - QQuickMenu *subMenu = nullptr; + QPointer subMenu; qreal implicitTextPadding; }; diff --git a/tests/auto/quickcontrols/qquickmenu/data/loadMenuAsynchronously.qml b/tests/auto/quickcontrols/qquickmenu/data/loadMenuAsynchronously.qml new file mode 100644 index 0000000000..3e73044a20 --- /dev/null +++ b/tests/auto/quickcontrols/qquickmenu/data/loadMenuAsynchronously.qml @@ -0,0 +1,62 @@ +import QtQuick +import QtQuick.Controls + +Item { + width:100 + height:100 + + property alias loader: menuLoader + property bool activateLoader: false + + onActivateLoaderChanged: { + if (activateLoader) { + menuTimer.start() + menuLoader.active = true + } + } + + Timer { + id: menuTimer + interval: 20 + onTriggered: menuLoader.active = false + } + + Loader { + id: menuLoader + active: false + asynchronous: true + sourceComponent: Menu { + contentItem: ListView {} + Menu {} + MenuSeparator {} + Action {} + Menu { + Action {} + Action {} + Menu {} + } + MenuItem {} + Item { + Menu {} + } + Menu { + MenuItem {} + Action {} + MenuItem {} + } + // Dummy menu items + Menu {} + Menu {} + MenuSeparator {} + Menu {} + Menu {} + MenuSeparator {} + Menu {} + Menu {} + MenuSeparator {} + Menu {} + Menu {} + } + } +} + diff --git a/tests/auto/quickcontrols/qquickmenu/tst_qquickmenu.cpp b/tests/auto/quickcontrols/qquickmenu/tst_qquickmenu.cpp index d0845851f2..9f74755423 100644 --- a/tests/auto/quickcontrols/qquickmenu/tst_qquickmenu.cpp +++ b/tests/auto/quickcontrols/qquickmenu/tst_qquickmenu.cpp @@ -19,8 +19,10 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -126,6 +128,7 @@ private slots: void mousePropagationWithinPopup(); void shortcutInNestedSubMenuAction(); void animationOnHeight(); + void loadMenuAsynchronously(); private: bool nativeMenuSupported = false; @@ -3537,6 +3540,47 @@ void tst_QQuickMenu::animationOnHeight() QCOMPARE_GE(listView->height(), listView->contentHeight()); } +void tst_QQuickMenu::loadMenuAsynchronously() +{ + QQuickView window(testFileUrl("loadMenuAsynchronously.qml")); + QCOMPARE(window.status(), QQuickView::Ready); + window.show(); + window.requestActivate(); + QVERIFY(QTest::qWaitForWindowActive(&window)); + + auto *rootItem = window.rootObject(); + QVERIFY(rootItem); + + auto *loader = rootItem->property("loader").value(); + QVERIFY(loader); + QCOMPARE(loader->active(), false); + QCOMPARE(loader->asynchronous(), true); + + auto activateLoader = rootItem->property("activateLoader").value(); + QCOMPARE(activateLoader, false); + QVERIFY(rootItem->setProperty("activateLoader", true)); + activateLoader = rootItem->property("activateLoader").value(); + QCOMPARE(activateLoader, true); + + QTRY_COMPARE(loader->active(), false); + + rootItem->setProperty("activateLoader", false); + activateLoader = rootItem->property("activateLoader").value(); + QCOMPARE(activateLoader, false); + QVERIFY(rootItem->setProperty("activateLoader", true)); + activateLoader = rootItem->property("activateLoader").value(); + QCOMPARE(activateLoader, true); + + QTRY_COMPARE(loader->active(), false); + + // The ignored warning message described below is triggered from the loader + // when progress creations not been reset. This needs to be analyzed. + // The bug report QTBUG-139552 raised to track the investigation. + auto *enginePriv = QQmlEnginePrivate::get(window.engine()); + if (enginePriv->inProgressCreations) + QTest::ignoreMessage(QtWarningMsg, QRegularExpression("There are still \\\\\"(\\d+)\\\\\" items in the process of being created at engine destruction\\.")); +} + QTEST_QUICKCONTROLS_MAIN(tst_QQuickMenu) #include "tst_qquickmenu.moc"