From 71b3d18ea7962a01413966f793e8187c9e91685e Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Thu, 17 Mar 2022 12:56:01 +0100 Subject: [PATCH] Notify about focus object changes upon widget destruction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the active QWidget gets destroyed, then QWidgetWindow::focusObject will return nullptr. If then no other object takes focus, then we'd never emit change signals, and QGuiApplication's _q_updateFocusObject (which then informs the input context and emits signals) didn't get called. This left the input context with a dangling focus object pointer, which resulted in crashes. If the QWidget clears its focus, but the corresponding window doesn't know that it had focus, then fall back to the widget's focus widget to see if we have a change in focus, so that signals get emitted. Add a test case that shows that we didn't call _q_updateFocusObject by counting emissions of the QGuiApplication::focusObjectChanged signal, which we emit in this function. The signal is emitted more than once both when showing a widget, and now also when destroying a widget that has a focus child. The former is a previous issue, the latter is an improvement to not emitting the signal at all. Pick-to: 6.3 6.2 Fixes: QTBUG-101423 Fixes: QTBUG-101321 Change-Id: Ib96a397211d442f52ce795a3eebd055a0ef51b0d Reviewed-by: Tor Arne Vestbø --- src/gui/kernel/qguiapplication.cpp | 9 ++++- src/widgets/kernel/qwidget.cpp | 9 ++++- .../qwidget_window/tst_qwidget_window.cpp | 36 +++++++++++++++++++ 3 files changed, 52 insertions(+), 2 deletions(-) diff --git a/src/gui/kernel/qguiapplication.cpp b/src/gui/kernel/qguiapplication.cpp index 883b22b7ed..0935f654eb 100644 --- a/src/gui/kernel/qguiapplication.cpp +++ b/src/gui/kernel/qguiapplication.cpp @@ -2498,8 +2498,15 @@ void QGuiApplicationPrivate::processActivatedEvent(QWindowSystemInterfacePrivate if (self) { self->notifyActiveWindowChange(previous); - if (previousFocusObject != qApp->focusObject()) + if (previousFocusObject != qApp->focusObject() || + // We are getting an activation change but there is no new focusObject, and we also + // don't have a previousFocusObject in the previously active window anymore. This can + // happen when window gets destroyed (see QWidgetWindow::focusObject returning nullptr + // when already in the QWidget destructor), so update the focusObject to avoid dangling + // pointers. See also QWidget::clearFocus(), which tries to cover for this as well. + (previous && previousFocusObject == nullptr && qApp->focusObject() == nullptr)) { self->_q_updateFocusObject(qApp->focusObject()); + } } emit qApp->focusWindowChanged(newFocus); diff --git a/src/widgets/kernel/qwidget.cpp b/src/widgets/kernel/qwidget.cpp index 3926436b07..5a17a45c74 100644 --- a/src/widgets/kernel/qwidget.cpp +++ b/src/widgets/kernel/qwidget.cpp @@ -6707,7 +6707,14 @@ void QWidget::clearFocus() } QTLWExtra *extra = window()->d_func()->maybeTopData(); - QObject *originalFocusObject = (extra && extra->window) ? extra->window->focusObject() : nullptr; + QObject *originalFocusObject = nullptr; + if (extra && extra->window) { + originalFocusObject = extra->window->focusObject(); + // the window's focus object might already be nullptr if we are in the destructor, but we still + // need to update QGuiApplication and input context if we have a focus widget. + if (!originalFocusObject) + originalFocusObject = focusWidget(); + } QWidget *w = this; while (w) { 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 5ae4526af2..9016d84e91 100644 --- a/tests/auto/widgets/kernel/qwidget_window/tst_qwidget_window.cpp +++ b/tests/auto/widgets/kernel/qwidget_window/tst_qwidget_window.cpp @@ -43,6 +43,7 @@ #include #include #include +#include #include #include #include @@ -131,6 +132,8 @@ private slots: void mouseMoveWithPopup_data(); void mouseMoveWithPopup(); + void resetFocusObjectOnDestruction(); + private: QSize m_testWidgetSize; const int m_fuzz; @@ -1574,5 +1577,38 @@ void tst_QWidget_window::mouseMoveWithPopup() QCOMPARE(topLevel.popup->mouseReleaseCount, 1); } +void tst_QWidget_window::resetFocusObjectOnDestruction() +{ + QSignalSpy focusObjectChangedSpy(qApp, &QGuiApplication::focusObjectChanged); + + // single top level widget that has focus + std::unique_ptr widget(new QWidget); + widget->setObjectName("Widget 1"); + widget->setFocus(); + widget->show(); + QVERIFY(QTest::qWaitForWindowActive(widget.get())); + + int activeCount = focusObjectChangedSpy.count(); + widget.reset(); + QVERIFY(focusObjectChangedSpy.count() > activeCount); + QCOMPARE(focusObjectChangedSpy.last().last().value(), nullptr); + focusObjectChangedSpy.clear(); + + // top level widget with focused child + widget.reset(new QWidget); + widget->setObjectName("Widget 2"); + QWidget *child = new QWidget(widget.get()); + child->setObjectName("Child widget"); + child->setFocus(); + widget->show(); + QVERIFY(QTest::qWaitForWindowActive(widget.get())); + + activeCount = focusObjectChangedSpy.count(); + widget.reset(); + // we might get more than one signal emission + QVERIFY(focusObjectChangedSpy.count() > activeCount); + QCOMPARE(focusObjectChangedSpy.last().last().value(), nullptr); +} + QTEST_MAIN(tst_QWidget_window) #include "tst_qwidget_window.moc"