QWidget: Clean up state that depends on QWindow from ~QWidgetWindow()

We were assuming that QWidget was in full control of QWidgetWindow
destruction, via deleteTLSysExtra(), and that we could limit any
cleanups to that function.

But in some situations QWidgetWindow is destructed from other code paths,
in which case we were left with dangling pointers to the QWidgetWindow
in both QTLWExtra, as well as the backingstore.

This can happen if there's a child widget hierarchy where there is not
a 1:1 mapping between QWidgets and QWindows, for example if the window
attribute WA_DontCreateNativeAncestors has been set. In this situation
our normal recursion into children in QWidget::destroy() stops at the
first widget without a window handle. When we then delete the top level
QWindow, the QWindow destructor will delete any child QWindows, which
includes our leaf QWidgetWindow.

We should probably fix up QWidget::destroy to continue looking for
children, even if we don't destroy the current child. But independently
of that we should make sure the QWidgetWindow cleans up when it's being
deleted, regardless of how it ended up there.

There's further room to clean up the deleteTLSysExtra() function and
friends, but that's been left for a later time.

Fixes: QTBUG-120509
Change-Id: Ib691df164d7c9c83cb464c0a6bf3bc2116e8ca43
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>
(cherry picked from commit 12203e94f5a34b59b6a7389402c854e823135a35)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit 4f4ffa8a67ac34396a0f16898ffd7b5987e0864d)
(cherry picked from commit 06a1e3b3cf47a103aea429053d273810ae576893)

3 files changed, 45 insertions, 27 deletions

diff --git a/src/widgets/kernel/qwidget.cpp b/src/widgets/kernel/qwidget.cpp
index 844bb51467..8d1f9f263c 100644
--- a/src/widgets/kernel/qwidget.cpp
+++ b/src/widgets/kernel/qwidget.cpp
@@ -1660,10 +1660,9 @@ void QWidgetPrivate::deleteExtra()
         if (QStyleSheetStyle *proxy = qt_styleSheet(extra->style))
             proxy->deref();
 #endif
-        if (extra->topextra) {
+        if (extra->topextra)
             deleteTLSysExtra();
-            // extra->topextra->backingStore destroyed in QWidgetPrivate::deleteTLSysExtra()
-        }
+
         // extra->xic destroyed in QWidget::destroy()
         extra.reset();
     }
@@ -1673,34 +1672,11 @@ void QWidgetPrivate::deleteSysExtra()
 {
 }
 
-static void deleteBackingStore(QWidgetPrivate *d)
-{
-    QTLWExtra *topData = d->topData();
-
-    delete topData->backingStore;
-    topData->backingStore = nullptr;
-}
-
 void QWidgetPrivate::deleteTLSysExtra()
 {
     if (extra && extra->topextra) {
-        //the qplatformbackingstore may hold a reference to the window, so the backingstore
-        //needs to be deleted first.
-
-        extra->topextra->repaintManager.reset(nullptr);
-        deleteBackingStore(this);
-        extra->topextra->widgetTextures.clear();
-
-        //the toplevel might have a context with a "qglcontext associated with it. We need to
-        //delete the qglcontext before we delete the qplatformopenglcontext.
-        //One unfortunate thing about this is that we potentially create a glContext just to
-        //delete it straight afterwards.
-        if (extra->topextra->window) {
-            extra->topextra->window->destroy();
-        }
         delete extra->topextra->window;
         extra->topextra->window = nullptr;
-
     }
 }
 
@@ -12255,7 +12231,7 @@ void QWidget::setBackingStore(QBackingStore *store)
         return;
 
     QBackingStore *oldStore = topData->backingStore;
-    deleteBackingStore(d);
+    delete topData->backingStore;
     topData->backingStore = store;
 
     QWidgetRepaintManager *repaintManager = d->maybeRepaintManager();
diff --git a/src/widgets/kernel/qwidgetwindow.cpp b/src/widgets/kernel/qwidgetwindow.cpp
index 99c54b9d98..ff666a7e49 100644
--- a/src/widgets/kernel/qwidgetwindow.cpp
+++ b/src/widgets/kernel/qwidgetwindow.cpp
@@ -145,6 +145,21 @@ QWidgetWindow::QWidgetWindow(QWidget *widget)
 
 QWidgetWindow::~QWidgetWindow()
 {
+    if (!m_widget)
+        return;
+
+    QTLWExtra *topData = QWidgetPrivate::get(m_widget)->topData();
+    Q_ASSERT(topData);
+
+    // The QPlaformBackingStore may hold a reference to the window,
+    // so the backingstore needs to be deleted first.
+    topData->repaintManager.reset(nullptr);
+    delete topData->backingStore;
+    topData->backingStore = nullptr;
+    topData->widgetTextures.clear();
+
+    // Too late to do anything beyond this point
+    topData->window = nullptr;
 }
 
 #if QT_CONFIG(accessibility)
diff --git a/tests/auto/widgets/kernel/qwidget_window/tst_qwidget_window.cpp b/tests/auto/widgets/kernel/qwidget_window/tst_qwidget_window.cpp
index addcc7d321..83e6415569 100644
--- a/tests/auto/widgets/kernel/qwidget_window/tst_qwidget_window.cpp
+++ b/tests/auto/widgets/kernel/qwidget_window/tst_qwidget_window.cpp
@@ -113,6 +113,8 @@ private slots:
 
     void resetFocusObjectOnDestruction();
 
+    void cleanupOnDestruction();
+
 private:
     QSize m_testWidgetSize;
     const int m_fuzz;
@@ -1672,5 +1674,30 @@ void tst_QWidget_window::resetFocusObjectOnDestruction()
     QCOMPARE(focusObjectChangedSpy.last().last().value<QObject*>(), nullptr);
 }
 
+class CreateDestroyWidget : public QWidget
+{
+public:
+    using QWidget::create;
+    using QWidget::destroy;
+};
+
+void tst_QWidget_window::cleanupOnDestruction()
+{
+    CreateDestroyWidget widget;
+    QWidget child(&widget);
+
+    QWidget grandChild(&child);
+    // Ensure there's not a 1:1 native window hierarhcy that we could
+    // recurse during QWidget::destroy(), triggering the issue that
+    // we were failing to clean up when not destroyed via QWidget.
+    grandChild.setAttribute(Qt::WA_DontCreateNativeAncestors);
+    grandChild.winId();
+
+    widget.destroy();
+    widget.create();
+
+    widget.show();
+}
+
 QTEST_MAIN(tst_QWidget_window)
 #include "tst_qwidget_window.moc"