Fix qobject_cast on partially destroyed QWidget/QWindow

QWidget and QWindow use bits in QObjectPrivate to provide for a couple
of shortcuts -- one in qobject_cast, and another in the isWidgetType() /
isWindowType() functions in QObject. These can be optimized by simply
looking at the bits, without actually doing more expensive runtime
casts.

These bits were set on construction, but not unset on destruction.  The
result was for instance that destroying a QWidget would report that the
object was still a QWidget when ~QObject was reached.

Fix this

1) by setting the bits only when QWidget / QWindow constructors start;

2) by resetting the bits once ~QWidget / ~QWindow are completed.
Technically speaking this is not 100% correct in the presence of data
members, but luckily those classes don't have any.

Amend an existing test for QWidget (whose comment said exactly the
opposite of what the test actually did) and add a test for QWindow.

Some other code was wrongly relying on isWidgetType() returning true
for destroyed QWidgets; amend it as needed.

[ChangeLog][QtCore][QObject] Using qobject_cast on partially constructed
or destroyed QWidget/QWindow instances now yields correct results.
Similarly, using the convenience isWidgetType() / isWindowType()
functions now correctly return false on such instances. Before,
qobject_cast (and the convenience functions) would erroneously report
that a given object was a QWidget (resp. QWindow) even during that
object's construction (before QObject's constructor had completed) or
destruction (after QWidget's (resp. QWindow's) destructors had been
completed). This was semantically wrong and inconsistent with other ways
of gathering runtime type information regarding such an object (e.g.
dynamic_cast, obj->metaObject()->className() and so on).

Pick-to: 6.3
Change-Id: Ic45a887951755a9d1a3b838590f1e9f2c4ae6e92
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Marc Mutz <marc.mutz@qt.io>
Reviewed-by: Tor Arne Vestbø <tor.arne.vestbo@qt.io>

9 files changed, 102 insertions, 38 deletions

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<QWindow *>(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<QApplication *>(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<QLayout *>(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<QLayout *>(c->child())))
+                qWarning("Adding a QLayout to a QSplitter is not supported.");
+            return;
+        }
         QWidget *w = static_cast<QWidget*>(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<QWidget*>(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<QWindow *>(object));
+        QVERIFY(!dynamic_cast<QWindow *>(object));
+        QVERIFY(!object->isWindowType());
+    });
+}
+
 #include <tst_qwindow.moc>
 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;
+    explicit QObjectCastChecker(QWidget *target)
+        : m_target(target)
+    {
+    }
 
-public slots:
-    void destroyedSlot(QObject *object)
+    ~QObjectCastChecker()
     {
-        wasQWidget = (qobject_cast<QWidget *>(object) != nullptr || object->isWidgetType());
+        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<QWidget *>(m_target));
+        QVERIFY(!dynamic_cast<QWidget *>(m_target));
+        QVERIFY(!m_target->isWidgetType());
     }
-};
 
-/*
-    Test that qobject_cast<QWidget*> returns 0 in a slot
-    connected to QObject::destroyed.
-*/
-void tst_QWidget::qobject_castInDestroyedSlot()
-{
-    DestroyedSlotChecker checker;
+    QObjectCastChecker(QObjectCastChecker &&other) noexcept
+        : m_target(qExchange(other.m_target, nullptr))
+    {}
 
-    QWidget *widget = new QWidget();
+    QT_MOVE_ASSIGNMENT_OPERATOR_IMPL_VIA_MOVE_AND_SWAP(QObjectCastChecker)
 
-    QObject::connect(widget, &QObject::destroyed, &checker, &DestroyedSlotChecker::destroyedSlot);
-    delete widget;
+    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<QWidget *>(object));
+        QVERIFY(dynamic_cast<QWidget *>(object));
+        QVERIFY(object->isWidgetType());
+    }
 
-    QVERIFY(checker.wasQWidget);
+private:
+    Q_DISABLE_COPY(QObjectCastChecker)
+    QObject *m_target;
+};
+
+void tst_QWidget::qobject_castOnDestruction()
+{
+    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