diff options
author | Mitch Curtis <mitch.curtis@qt.io> | 2019-09-23 13:42:03 +0200 |
---|---|---|
committer | Mitch Curtis <mitch.curtis@qt.io> | 2019-09-30 09:22:23 +0000 |
commit | 83fbf44b980c4a072ede122f2f16921bfff8c08d (patch) | |
tree | e4dd33b52fbf24badad167981c64fa6cc5a23076 | |
parent | 100550dd22dc8eaa47405cdb3e7e461edb01a7a3 (diff) |
Fix a crash on exit when using ToolTip in a specific item hierarchy
QQuickPopup connects its parent item's (MouseArea, in this case)
windowChanged() signal to QQuickPopupPrivate::setWindow(). It does this so
that:
1) QQuickOverlay can keep track of all of the popups that it manages.
2) Fonts, palettes and locales can be resolved.
3) If the QQuickPopup component has completed loading and the popup is visible
with a valid window, start the enter transition.
The problem arises only when using a very specific item hierarchy:
Window {
width: 640
height: 480
visible: true
Item {
anchors.fill: parent
Item {
anchors.fill: parent
ColorOverlay {
source: parent
anchors.fill: parent
}
MouseArea {
anchors.fill: parent
hoverEnabled: true
ToolTip.visible: containsMouse
ToolTip.text: "ToolTip text"
}
}
}
}
When the window is closed and hence begins to be destroyed, the following events occur:
- QQuickWindow's destructor is called.
- The window's root item (QQuickRootItem) begins destruction.
- QQuickOverlay is destroyed.
- QQuickWindow's destructor is done, so the QWindow and then QObject
destructors are called.
- The QQuickItem destructor for the outer Item is called.
- The child items of the outer Item have setParentItem(nullptr) called on them,
one of which being the inner Item.
- The inner Item's setParentItem() function calls derefWindow(), which in turn
calls derefWindow() on its children. One of those children is MouseArea.
- Since the MouseArea's window is deref'd, it emits the windowChanged() signal.
MouseArea is the parentItem of the popup, so its windowChanged() signal
causes QQuickPopupPrivate::setWindow() to be called.
- setWindow() tries to remove the popup from the old overlay, which has already
been destroyed.
One approach I tried involved using QQuickOverlay::itemChange() to remove all
of the popups (via setWindow(nullptr), to ensure that their window pointer is
nullified), since that was called much earlier than the windowChanged() signal
is emitted. However, this still resulted in a heap-use-after-free in the same
place when running the newly added setOverlayParentToNull() test.
I also tried removing the popups in QQuickOverlay's destructor, but this
resulted in another heap-use-after-free (when accessing a popup in the
destructor) in tst_QQuickPopup::Universal::visible().
The remaining options were: store the window in a QPointer or return early in
overlay() if the wasDeleted member of the window was true. Using QPointer
seems like it would catch more issues than a single check in overlay(), so I
went with that.
Fixes: QTBUG-73243
Change-Id: Ieb5ce26dd76d45771d28297031ec43e27d958b5b
Reviewed-by: Mitch Curtis <mitch.curtis@qt.io>
-rw-r--r-- | src/quicktemplates2/qquickpopup_p_p.h | 2 | ||||
-rw-r--r-- | tests/auto/qquickpopup/data/toolTipCrashOnClose.qml | 94 | ||||
-rw-r--r-- | tests/auto/qquickpopup/tst_qquickpopup.cpp | 42 |
3 files changed, 137 insertions, 1 deletions
diff --git a/src/quicktemplates2/qquickpopup_p_p.h b/src/quicktemplates2/qquickpopup_p_p.h index e32fdb28..8a85f914 100644 --- a/src/quicktemplates2/qquickpopup_p_p.h +++ b/src/quicktemplates2/qquickpopup_p_p.h @@ -187,7 +187,7 @@ public: QQuickPopup::ClosePolicy closePolicy = DefaultClosePolicy; QQuickItem *parentItem = nullptr; QQuickItem *dimmer = nullptr; - QQuickWindow *window = nullptr; + QPointer<QQuickWindow> window; QQuickTransition *enter = nullptr; QQuickTransition *exit = nullptr; QQuickPopupItem *popupItem = nullptr; diff --git a/tests/auto/qquickpopup/data/toolTipCrashOnClose.qml b/tests/auto/qquickpopup/data/toolTipCrashOnClose.qml new file mode 100644 index 00000000..8de14f4c --- /dev/null +++ b/tests/auto/qquickpopup/data/toolTipCrashOnClose.qml @@ -0,0 +1,94 @@ +/**************************************************************************** +** +** Copyright (C) 2019 The Qt Company Ltd. +** Contact: https://www.qt.io/licensing/ +** +** This file is part of the test suite of the Qt Toolkit. +** +** $QT_BEGIN_LICENSE:BSD$ +** Commercial License Usage +** Licensees holding valid commercial Qt licenses may use this file in +** accordance with the commercial license agreement provided with the +** Software or, alternatively, in accordance with the terms contained in +** a written agreement between you and The Qt Company. For licensing terms +** and conditions see https://www.qt.io/terms-conditions. For further +** information use the contact form at https://www.qt.io/contact-us. +** +** BSD License Usage +** Alternatively, you may use this file under the terms of the BSD license +** as follows: +** +** "Redistribution and use in source and binary forms, with or without +** modification, are permitted provided that the following conditions are +** met: +** * Redistributions of source code must retain the above copyright +** notice, this list of conditions and the following disclaimer. +** * Redistributions in binary form must reproduce the above copyright +** notice, this list of conditions and the following disclaimer in +** the documentation and/or other materials provided with the +** distribution. +** * Neither the name of The Qt Company Ltd nor the names of its +** contributors may be used to endorse or promote products derived +** from this software without specific prior written permission. +** +** +** THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +** "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +** LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +** A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +** OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +** SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +** LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +** DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +** THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +** (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +** OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE." +** +** $QT_END_LICENSE$ +** +****************************************************************************/ + +import QtQuick 2.13 +import QtQuick.Window 2.13 +import QtQuick.Controls 2.13 +import QtGraphicalEffects 1.13 + +Window { + width: 640 + height: 480 + + readonly property bool toolTipOpened: mouseArea.ToolTip.toolTip.opened + + Component.onCompleted: contentItem.objectName = "windowContentItem" + + // For the setOverlayParentToNull test. + function nullifyOverlayParent() { + Overlay.overlay.parent = null + } + + Item { + objectName: "outerItem" + anchors.fill: parent + + Item { + objectName: "innerItem" + anchors.fill: parent + + ColorOverlay { + objectName: "colorOverlay" + source: parent + anchors.fill: parent + } + + MouseArea { + id: mouseArea + objectName: "mouseArea" + anchors.fill: parent + hoverEnabled: true + + ToolTip.visible: containsMouse + ToolTip.text: "ToolTip text" + } + } + } +} diff --git a/tests/auto/qquickpopup/tst_qquickpopup.cpp b/tests/auto/qquickpopup/tst_qquickpopup.cpp index 636b2b21..4d238663 100644 --- a/tests/auto/qquickpopup/tst_qquickpopup.cpp +++ b/tests/auto/qquickpopup/tst_qquickpopup.cpp @@ -89,6 +89,8 @@ private slots: void disabledPalette(); void disabledParentPalette(); void countChanged(); + void toolTipCrashOnClose(); + void setOverlayParentToNull(); }; void tst_QQuickPopup::initTestCase() @@ -1187,6 +1189,46 @@ void tst_QQuickPopup::countChanged() QVERIFY(window->setProperty("isModel1", false)); QTRY_COMPARE(window->property("count").toInt(), 2); } + +// QTBUG-73243 +void tst_QQuickPopup::toolTipCrashOnClose() +{ + QQuickApplicationHelper helper(this, "toolTipCrashOnClose.qml"); + + QQuickWindow *window = helper.window; + window->show(); + // TODO: Using ignoreMessage() fails in CI with macOS for release builds, + // so for now we let the warning through. +// QTest::ignoreMessage(QtWarningMsg, "ShaderEffectSource: 'recursive' must be set to true when rendering recursively."); + QVERIFY(QTest::qWaitForWindowActive(window)); + + QTest::mouseMove(window, QPoint(window->width() / 2, window->height() / 2)); + QTRY_VERIFY(window->property("toolTipOpened").toBool()); + + QVERIFY(window->close()); + // Shouldn't crash. +} + +void tst_QQuickPopup::setOverlayParentToNull() +{ + QQuickApplicationHelper helper(this, "toolTipCrashOnClose.qml"); + + QQuickWindow *window = helper.window; + window->show(); + // TODO: Using ignoreMessage() fails in CI with macOS for release builds, + // so for now we let the warning through. +// QTest::ignoreMessage(QtWarningMsg, "ShaderEffectSource: 'recursive' must be set to true when rendering recursively."); + QVERIFY(QTest::qWaitForWindowActive(window)); + + QVERIFY(QMetaObject::invokeMethod(window, "nullifyOverlayParent")); + + QTest::mouseMove(window, QPoint(window->width() / 2, window->height() / 2)); + QTRY_VERIFY(window->property("toolTipOpened").toBool()); + + QVERIFY(window->close()); + // While nullifying the overlay parent doesn't make much sense, it shouldn't crash. +} + QTEST_QUICKCONTROLS_MAIN(tst_QQuickPopup) #include "tst_qquickpopup.moc" |