diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp index eee9cbd793..523e952579 100644 --- a/src/corelib/kernel/qobject.cpp +++ b/src/corelib/kernel/qobject.cpp @@ -198,6 +198,8 @@ QObjectPrivate::QObjectPrivate(int version) isWindow = false; deleteLaterCalled = false; isQuickItem = false; + willBeWidget = false; + wasWidget = false; } QObjectPrivate::~QObjectPrivate() @@ -932,7 +934,7 @@ QObject::QObject(QObjectPrivate &dd, QObject *parent) QT_TRY { if (!check_parent_thread(parent, parent ? parent->d_func()->threadData.loadRelaxed() : nullptr, threadData)) parent = nullptr; - if (d->isWidget) { + if (d->willBeWidget) { if (parent) { d->parent = parent; d->parent->d_func()->children.append(this); @@ -1003,7 +1005,7 @@ QObject::~QObject() delete sharedRefcount; } - if (!d->isWidget && d->isSignalConnected(0)) { + if (!d->wasWidget && d->isSignalConnected(0)) { emit destroyed(this); } diff --git a/src/corelib/kernel/qobject.h b/src/corelib/kernel/qobject.h index b186845c40..08dbe0c348 100644 --- a/src/corelib/kernel/qobject.h +++ b/src/corelib/kernel/qobject.h @@ -108,7 +108,9 @@ public: uint isWindow : 1; // for QWindow uint deleteLaterCalled : 1; uint isQuickItem : 1; - uint unused : 23; + uint willBeWidget : 1; // for handling widget-specific bits in QObject's ctor + uint wasWidget : 1; // for properly cleaning up in QObject's dtor + uint unused : 21; int postedEvents; QDynamicMetaObjectData *metaObject; QBindingStorage bindingStorage; diff --git a/src/gui/kernel/qwindow.cpp b/src/gui/kernel/qwindow.cpp index 030628b5db..8070530033 100644 --- a/src/gui/kernel/qwindow.cpp +++ b/src/gui/kernel/qwindow.cpp @@ -232,12 +232,15 @@ QWindow::~QWindow() QGuiApplicationPrivate::currentMouseWindow = nullptr; if (QGuiApplicationPrivate::currentMousePressWindow == this) QGuiApplicationPrivate::currentMousePressWindow = nullptr; + + d->isWindow = false; } void QWindowPrivate::init(QScreen *targetScreen) { Q_Q(QWindow); + isWindow = true; parentWindow = static_cast(q->QObject::parent()); if (!parentWindow) diff --git a/src/gui/kernel/qwindow_p.h b/src/gui/kernel/qwindow_p.h index 34fddf801b..b2bd4be6a9 100644 --- a/src/gui/kernel/qwindow_p.h +++ b/src/gui/kernel/qwindow_p.h @@ -73,12 +73,6 @@ public: WindowFrameExclusive }; - QWindowPrivate() - : QObjectPrivate() - { - isWindow = true; - } - void init(QScreen *targetScreen = nullptr); #ifndef QT_NO_CURSOR diff --git a/src/widgets/kernel/qlayout.cpp b/src/widgets/kernel/qlayout.cpp index 044176d141..2867bd9fb1 100644 --- a/src/widgets/kernel/qlayout.cpp +++ b/src/widgets/kernel/qlayout.cpp @@ -570,12 +570,14 @@ void QLayout::widgetEvent(QEvent *e) case QEvent::ChildRemoved: { QChildEvent *c = (QChildEvent *)e; - if (c->child()->isWidgetType()) { + QObject *child = c->child(); + QObjectPrivate *op = QObjectPrivate::get(child); + if (op->wasWidget) { #if QT_CONFIG(menubar) - if (c->child() == d->menubar) + if (child == d->menubar) d->menubar = nullptr; #endif - removeWidgetRecursively(this, c->child()); + removeWidgetRecursively(this, child); } } break; diff --git a/src/widgets/kernel/qwidget.cpp b/src/widgets/kernel/qwidget.cpp index 7c48df9799..1a76b887ee 100644 --- a/src/widgets/kernel/qwidget.cpp +++ b/src/widgets/kernel/qwidget.cpp @@ -203,7 +203,7 @@ QWidgetPrivate::QWidgetPrivate(int version) version, QObjectPrivateVersion); #endif - isWidget = true; + willBeWidget = true; // used in QObject's ctor memset(high_attributes, 0, sizeof(high_attributes)); #ifdef QWIDGET_EXTRA_DEBUG @@ -978,6 +978,9 @@ void QWidgetPrivate::adjustFlags(Qt::WindowFlags &flags, QWidget *w) void QWidgetPrivate::init(QWidget *parentWidget, Qt::WindowFlags f) { Q_Q(QWidget); + isWidget = true; + wasWidget = true; + Q_ASSERT_X(q != parentWidget, Q_FUNC_INFO, "Cannot parent a QWidget to itself"); if (Q_UNLIKELY(!qobject_cast(QCoreApplication::instance()))) @@ -1541,6 +1544,8 @@ QWidget::~QWidget() #if QT_CONFIG(graphicseffect) delete d->graphicsEffect; #endif + + d->isWidget = false; } int QWidgetPrivate::instanceCounter = 0; // Current number of widget instances diff --git a/src/widgets/widgets/qsplitter.cpp b/src/widgets/widgets/qsplitter.cpp index 1c04f02422..7f31a955f6 100644 --- a/src/widgets/widgets/qsplitter.cpp +++ b/src/widgets/widgets/qsplitter.cpp @@ -1286,16 +1286,18 @@ int QSplitter::count() const void QSplitter::childEvent(QChildEvent *c) { Q_D(QSplitter); - if (!c->child()->isWidgetType()) { - if (Q_UNLIKELY(c->type() == QEvent::ChildAdded && qobject_cast(c->child()))) - qWarning("Adding a QLayout to a QSplitter is not supported."); - return; - } if (c->added()) { + if (!c->child()->isWidgetType()) { + if (Q_UNLIKELY(qobject_cast(c->child()))) + qWarning("Adding a QLayout to a QSplitter is not supported."); + return; + } QWidget *w = static_cast(c->child()); if (!d->blockChildAdd && !w->isWindow() && !d->findWidget(w)) d->insertWidget_helper(d->list.count(), w, false); } else if (c->polished()) { + if (!c->child()->isWidgetType()) + return; QWidget *w = static_cast(c->child()); if (!d->blockChildAdd && !w->isWindow() && d->shouldShowWidget(w)) w->show(); diff --git a/tests/auto/gui/kernel/qwindow/tst_qwindow.cpp b/tests/auto/gui/kernel/qwindow/tst_qwindow.cpp index ff5a670322..46d6f5bf44 100644 --- a/tests/auto/gui/kernel/qwindow/tst_qwindow.cpp +++ b/tests/auto/gui/kernel/qwindow/tst_qwindow.cpp @@ -110,6 +110,7 @@ private slots: void generatedMouseMove(); void keepPendingUpdateRequests(); void activateDeactivateEvent(); + void qobject_castOnDestruction(); private: QPoint m_availableTopLeft; @@ -2744,6 +2745,19 @@ void tst_QWindow::activateDeactivateEvent() QCOMPARE(w2.activateCount, 1); } +// Test that in a slot connected to destroyed() the emitter is +// is no longer a QWindow. +void tst_QWindow::qobject_castOnDestruction() +{ + QWindow window; + QObject::connect(&window, &QObject::destroyed, [](QObject *object) + { + QVERIFY(!qobject_cast(object)); + QVERIFY(!dynamic_cast(object)); + QVERIFY(!object->isWindowType()); + }); +} + #include QTEST_MAIN(tst_QWindow) diff --git a/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp b/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp index 42f336bc0b..a3698de203 100644 --- a/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp +++ b/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp @@ -247,7 +247,7 @@ private slots: void closeAndShowNativeChild(); void closeAndShowWithNativeChild(); void transientParent(); - void qobject_castInDestroyedSlot(); + void qobject_castOnDestruction(); void showHideEvent_data(); void showHideEvent(); @@ -5390,34 +5390,74 @@ void tst_QWidget::scrollNativeChildren() #endif // Mac OS -class DestroyedSlotChecker : public QObject +/* + This class is used as a slot object to test two different steps of + QWidget destruction. + + The first step is connecting the destroyed() signal to an object of + this class (through its operator()). In widgets, destroyed() is + emitted by ~QWidget, and not by ~QObject. This means that in our + operator() we expect the sender of the signal to still be a + QWidget. + + The connection realized at the first step means that now there's + an instance of this class owned by the sender object. That instance + is destroyed when the signal/slot connections are destroyed. + That happens in ~QObject, not in ~QWidget. Therefore, in the + destructor of this class, check that indeed the target is no longer + a QWidget but just a QObject. +*/ +class QObjectCastChecker { - Q_OBJECT - public: - bool wasQWidget = false; - -public slots: - void destroyedSlot(QObject *object) + explicit QObjectCastChecker(QWidget *target) + : m_target(target) { - wasQWidget = (qobject_cast(object) != nullptr || object->isWidgetType()); } + + ~QObjectCastChecker() + { + if (!m_target) + return; + + // When ~QObject is reached, check that indeed the object is no + // longer a QWidget. This relies on slots being disconnected in + // ~QObject (and this "slot object" being destroyed there). + QVERIFY(!qobject_cast(m_target)); + QVERIFY(!dynamic_cast(m_target)); + QVERIFY(!m_target->isWidgetType()); + } + + QObjectCastChecker(QObjectCastChecker &&other) noexcept + : m_target(qExchange(other.m_target, nullptr)) + {} + + QT_MOVE_ASSIGNMENT_OPERATOR_IMPL_VIA_MOVE_AND_SWAP(QObjectCastChecker) + + void swap(QObjectCastChecker &other) noexcept + { + qSwap(m_target, other.m_target); + } + + void operator()(QObject *object) const + { + // Test that in a slot connected to destroyed() the emitter is + // still a QWidget. This is because ~QWidget() itself emits the + // signal. + QVERIFY(qobject_cast(object)); + QVERIFY(dynamic_cast(object)); + QVERIFY(object->isWidgetType()); + } + +private: + Q_DISABLE_COPY(QObjectCastChecker) + QObject *m_target; }; -/* - Test that qobject_cast returns 0 in a slot - connected to QObject::destroyed. -*/ -void tst_QWidget::qobject_castInDestroyedSlot() +void tst_QWidget::qobject_castOnDestruction() { - DestroyedSlotChecker checker; - - QWidget *widget = new QWidget(); - - QObject::connect(widget, &QObject::destroyed, &checker, &DestroyedSlotChecker::destroyedSlot); - delete widget; - - QVERIFY(checker.wasQWidget); + QWidget widget; + QObject::connect(&widget, &QObject::destroyed, QObjectCastChecker(&widget)); } // Since X11 WindowManager operations are all async, and we have no way to know if the window