From aaec25a798352fc222f86ab3b299384575f51dc8 Mon Sep 17 00:00:00 2001 From: Mitch Curtis Date: Wed, 27 Nov 2019 11:00:59 +0100 Subject: StackView: fix crash when recursively removing items This can happen when e.g. calling clear() in Component.onDestruction in response to a pop() call. The patch fixes the crash by warning and returning early. If users really need to do this, the clear() call can be delayed: Component.onDestruction: { Qt.callLater(function() { stackView.clear(StackView.Immediate) }) } Change-Id: If3cf07495bb34b96089522f44c36976bd6c62492 Fixes: QTBUG-80353 Reviewed-by: Richard Moe Gustavsen --- src/quicktemplates2/qquickstackview.cpp | 20 ++++++ src/quicktemplates2/qquickstackview_p_p.h | 1 + tests/auto/controls/data/tst_stackview.qml | 100 +++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+) diff --git a/src/quicktemplates2/qquickstackview.cpp b/src/quicktemplates2/qquickstackview.cpp index f7a77374..4c314904 100644 --- a/src/quicktemplates2/qquickstackview.cpp +++ b/src/quicktemplates2/qquickstackview.cpp @@ -651,6 +651,13 @@ void QQuickStackView::push(QQmlV4Function *args) void QQuickStackView::pop(QQmlV4Function *args) { Q_D(QQuickStackView); + if (d->removingElements) { + d->warn(QStringLiteral("cannot pop while already in the process of removing elements")); + args->setReturnValue(QV4::Encode::null()); + return; + } + + QScopedValueRollback removingElements(d->removingElements, true); QScopedValueRollback rollback(d->operation, QStringLiteral("pop")); int argc = args->length(); if (d->elements.count() <= 1 || argc > 2) { @@ -806,6 +813,13 @@ void QQuickStackView::pop(QQmlV4Function *args) void QQuickStackView::replace(QQmlV4Function *args) { Q_D(QQuickStackView); + if (d->removingElements) { + d->warn(QStringLiteral("cannot replace while already in the process of removing elements")); + args->setReturnValue(QV4::Encode::null()); + return; + } + + QScopedValueRollback removingElements(d->removingElements, true); QScopedValueRollback rollback(d->operation, QStringLiteral("replace")); if (args->length() <= 0) { d->warn(QStringLiteral("missing arguments")); @@ -902,6 +916,12 @@ void QQuickStackView::clear(Operation operation) if (d->elements.isEmpty()) return; + if (d->removingElements) { + d->warn(QStringLiteral("cannot clear while already in the process of removing elements")); + return; + } + + QScopedValueRollback removingElements(d->removingElements, true); if (operation != Immediate) { QQuickStackElement *exit = d->elements.pop(); exit->removal = true; diff --git a/src/quicktemplates2/qquickstackview_p_p.h b/src/quicktemplates2/qquickstackview_p_p.h index c20ce776..b8c4b817 100644 --- a/src/quicktemplates2/qquickstackview_p_p.h +++ b/src/quicktemplates2/qquickstackview_p_p.h @@ -94,6 +94,7 @@ public: void depthChange(int newDepth, int oldDepth); bool busy = false; + bool removingElements = false; QString operation; QJSValue initialItem; QQuickItem *currentItem = nullptr; diff --git a/tests/auto/controls/data/tst_stackview.qml b/tests/auto/controls/data/tst_stackview.qml index a9fbf874..c6515f77 100644 --- a/tests/auto/controls/data/tst_stackview.qml +++ b/tests/auto/controls/data/tst_stackview.qml @@ -1245,4 +1245,104 @@ TestCase { gc() verify(control.initialItem) } + + // Need to use this specific structure in order to reproduce the crash. + Component { + id: clearUponDestructionContainerComponent + + Item { + id: container + objectName: "container" + + property alias control: stackView + property var onDestructionCallback + + property Component clearUponDestructionComponent: Component { + id: clearUponDestructionComponent + + Item { + objectName: "clearUponDestructionItem" + Component.onDestruction: container.onDestructionCallback(stackView) + } + } + + StackView { + id: stackView + initialItem: Item { + objectName: "initialItem" + } + } + } + } + + // QTBUG-80353 + // Tests that calling clear() in Component.onDestruction in response to that + // item being removed (e.g. via an earlier call to clear()) results in a warning and not a crash. + function test_recursiveClearClear() { + let container = createTemporaryObject(clearUponDestructionContainerComponent, testCase, + { onDestructionCallback: function(stackView) { stackView.clear(StackView.Immediate) }}) + verify(container) + + let control = container.control + control.push(container.clearUponDestructionComponent, StackView.Immediate) + + // Shouldn't crash. + ignoreWarning(new RegExp(".*cannot clear while already in the process of removing elements")) + control.clear(StackView.Immediate) + } + + function test_recursivePopClear() { + let container = createTemporaryObject(clearUponDestructionContainerComponent, testCase, + { onDestructionCallback: function(stackView) { stackView.clear(StackView.Immediate) }}) + verify(container) + + let control = container.control + control.push(container.clearUponDestructionComponent, StackView.Immediate) + + // Pop all items except the first, removing the second item we pushed in the process. + // Shouldn't crash. + ignoreWarning(new RegExp(".*cannot clear while already in the process of removing elements")) + control.pop(null, StackView.Immediate) + } + + function test_recursivePopPop() { + let container = createTemporaryObject(clearUponDestructionContainerComponent, testCase, + { onDestructionCallback: function(stackView) { stackView.pop(null, StackView.Immediate) }}) + verify(container) + + let control = container.control + // Push an extra item so that we can call pop(null) and reproduce the conditions for the crash. + control.push(component, StackView.Immediate) + control.push(container.clearUponDestructionComponent, StackView.Immediate) + + // Pop the top item, then pop down to the first item in response. + ignoreWarning(new RegExp(".*cannot pop while already in the process of removing elements")) + control.pop(StackView.Immediate) + } + + function test_recursiveReplaceClear() { + let container = createTemporaryObject(clearUponDestructionContainerComponent, testCase, + { onDestructionCallback: function(stackView) { stackView.clear(StackView.Immediate) }}) + verify(container) + + let control = container.control + control.push(container.clearUponDestructionComponent, StackView.Immediate) + + // Replace the top item, then clear in response. + ignoreWarning(new RegExp(".*cannot clear while already in the process of removing elements")) + control.replace(component, StackView.Immediate) + } + + function test_recursiveClearReplace() { + let container = createTemporaryObject(clearUponDestructionContainerComponent, testCase, + { onDestructionCallback: function(stackView) { stackView.replace(component, StackView.Immediate) }}) + verify(container) + + let control = container.control + control.push(container.clearUponDestructionComponent, StackView.Immediate) + + // Replace the top item, then clear in response. + ignoreWarning(new RegExp(".*cannot replace while already in the process of removing elements")) + control.clear(StackView.Immediate) + } } -- cgit v1.2.3