From b627dd2c24c5962c1e8f9d72bcbe9ebdfe6dfceb Mon Sep 17 00:00:00 2001
From: Felix Bourbonnais <thor400.75@gmail.com>
Date: Tue, 10 May 2016 10:06:53 -0400
Subject: [PATCH] QMenuBar: nested parenting fix
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
QMenuBar now receives a parent changed event for each of its parent,
grand-parent, ... This fixes a crash caused by an invalid QWidget
pointer and makes sure the keyboard shortcuts events are relayed to the
menu bar in all parenting/re-parenting cases by installing an event
filter on each parent
Task-number: QTBUG-53205
Change-Id: I419e6cbc52e28a67fb08a848a7161b4cb8ae4ae5
Reviewed-by: Gabriel de Dietrich <gabriel.dedietrich@qt.io>
Reviewed-by: Błażej Szczygieł <spaz16@wp.pl>
---
src/widgets/widgets/qmenubar.cpp | 55 +++++++++--------
src/widgets/widgets/qmenubar_p.h | 3 +-
.../widgets/widgets/qmenubar/tst_qmenubar.cpp | 59 +++++++++++++++++++
3 files changed, 92 insertions(+), 25 deletions(-)
diff --git a/src/widgets/widgets/qmenubar.cpp b/src/widgets/widgets/qmenubar.cpp
index cca0853a8fd..3f42a07f396 100644
--- a/src/widgets/widgets/qmenubar.cpp
+++ b/src/widgets/widgets/qmenubar.cpp
@@ -706,7 +706,6 @@ void QMenuBarPrivate::init()
}
#endif
q->setBackgroundRole(QPalette::Button);
- oldWindow = oldParent = 0;
handleReparent();
q->setMouseTracking(q->style()->styleHint(QStyle::SH_MenuBar_MouseTracking, 0, q));
@@ -1330,30 +1329,41 @@ void QMenuBarPrivate::handleReparent()
{
Q_Q(QMenuBar);
QWidget *newParent = q->parentWidget();
- //Note: if parent is reparented, then window may change even if parent doesn't
- // we need to install an event filter on parent, and remove the old one
-
- if (oldParent != newParent) {
- if (oldParent)
- oldParent->removeEventFilter(q);
- if (newParent)
- newParent->installEventFilter(q);
+ //Note: if parent is reparented, then window may change even if parent doesn't.
+ // We need to install an avent filter on each parent up to the parent that is
+ // also a window (for shortcuts)
+ QWidget *newWindow = newParent ? newParent->window() : Q_NULLPTR;
+
+ QVector<QPointer<QWidget> > newParents;
+ // Remove event filters on ex-parents, keep them on still-parents
+ // The parents are always ordered in the vector
+ foreach (const QPointer<QWidget> &w, oldParents) {
+ if (w) {
+ if (newParent == w) {
+ newParents.append(w);
+ if (newParent != newWindow) //stop at the window
+ newParent = newParent->parentWidget();
+ } else {
+ w->removeEventFilter(q);
+ }
+ }
}
- //we also need event filter on top-level (for shortcuts)
- QWidget *newWindow = newParent ? newParent->window() : 0;
-
- if (oldWindow != newWindow) {
- if (oldParent && oldParent != oldWindow)
- oldWindow->removeEventFilter(q);
-
- if (newParent && newParent != newWindow)
- newWindow->installEventFilter(q);
+ // At this point, newParent is the next one to be added to newParents
+ while (newParent && newParent != newWindow) {
+ //install event filters all the way up to (excluding) the window
+ newParents.append(newParent);
+ newParent->installEventFilter(q);
+ newParent = newParent->parentWidget();
}
- oldParent = newParent;
- oldWindow = newWindow;
+ if (newParent && newWindow) {
+ // Install the event filter on the window
+ newParents.append(newParent);
+ newParent->installEventFilter(q);
+ }
+ oldParents = newParents;
if (platformMenuBar) {
if (newWindow) {
@@ -1465,10 +1475,9 @@ bool QMenuBar::event(QEvent *e)
bool QMenuBar::eventFilter(QObject *object, QEvent *event)
{
Q_D(QMenuBar);
- if (object == parent() && object) {
- if (event->type() == QEvent::ParentChange) //GrandparentChange
+ if (object && (event->type() == QEvent::ParentChange)) //GrandparentChange
d->handleReparent();
- }
+
if (object == d->leftWidget || object == d->rightWidget) {
switch (event->type()) {
case QEvent::ShowToParent:
diff --git a/src/widgets/widgets/qmenubar_p.h b/src/widgets/widgets/qmenubar_p.h
index ee615e71f34..7ef696f50ec 100644
--- a/src/widgets/widgets/qmenubar_p.h
+++ b/src/widgets/widgets/qmenubar_p.h
@@ -128,8 +128,7 @@ public:
// reparenting
void handleReparent();
- QWidget *oldParent;
- QWidget *oldWindow;
+ QVector<QPointer<QWidget> > oldParents;
QList<QAction*> hiddenActions;
//default action
diff --git a/tests/auto/widgets/widgets/qmenubar/tst_qmenubar.cpp b/tests/auto/widgets/widgets/qmenubar/tst_qmenubar.cpp
index f787d73a027..b2d15fffef4 100644
--- a/tests/auto/widgets/widgets/qmenubar/tst_qmenubar.cpp
+++ b/tests/auto/widgets/widgets/qmenubar/tst_qmenubar.cpp
@@ -132,10 +132,12 @@ private slots:
void closeOnSecondClickAndOpenOnThirdClick();
void cornerWidgets_data();
void cornerWidgets();
+ void taskQTBUG53205_crashReparentNested();
protected slots:
void onSimpleActivated( QAction*);
void onComplexActionTriggered();
+ void slotForTaskQTBUG53205();
private:
TestMenu initSimpleMenuBar(QMenuBar *mb);
@@ -148,6 +150,7 @@ private:
QAction* m_lastSimpleAcceleratorId;
int m_simpleActivatedCount;
int m_complexTriggerCount[int('k')];
+ QMenuBar* taskQTBUG53205MenuBar;
};
// Testing get/set functions
@@ -1438,5 +1441,61 @@ void tst_QMenuBar::cornerWidgets()
delete cornerLabel;
}
+
+void tst_QMenuBar::taskQTBUG53205_crashReparentNested()
+{
+ // This test was largely inspired by the test case submitted for the bug
+ QMainWindow mainWindow;
+ mainWindow.resize(300, 200);
+ centerOnScreen(&mainWindow);
+ const TestMenu testMenus = initWindowWithComplexMenuBar(mainWindow);
+ QApplication::setActiveWindow(&mainWindow);
+
+ // they can't be windows
+ QWidget hiddenParent(&mainWindow, 0);
+ //this one is going to be moved around
+ QWidget movingParent(&hiddenParent, 0);
+
+ //set up the container widget
+ QWidget containerWidget(&movingParent,0);
+
+ //set the new parent, a window
+ QScopedPointer<QWidget> windowedParent;
+ windowedParent.reset(new QWidget(Q_NULLPTR, Qt::WindowFlags()));
+ windowedParent->setGeometry(400, 10, 300, 300);
+
+ windowedParent->show();
+ QVERIFY(QTest::qWaitForWindowExposed(windowedParent.data()));
+
+ //set the "container", can't be a window
+ QWidget containedWidget(&containerWidget, 0);
+
+ taskQTBUG53205MenuBar = new QMenuBar(&containedWidget);
+
+ connect(testMenus.actions[0], &QAction::triggered, this, &tst_QMenuBar::slotForTaskQTBUG53205);
+ //now, move things around
+ //from : QMainWindow<-hiddenParent<-movingParent<-containerWidget<-containedWidget<-menuBar
+ //to windowedParent<-movingParent<-containerWidget<-containedWidget<-menuBar
+ movingParent.setParent(windowedParent.data(),0);
+ // this resets the parenting and the menu bar's window
+ taskQTBUG53205MenuBar->setParent(Q_NULLPTR);
+ taskQTBUG53205MenuBar->setParent(&containedWidget);
+ //from windowedParent<-movingParent<-containerWidget<-containedWidget<-menuBar
+ //to : QMainWindow<-hiddenParent<-movingParent<-containerWidget<-containedWidget<-menuBar
+ movingParent.setParent(&hiddenParent,0);
+ windowedParent.reset(); //make the old window invalid
+ // trigger the aciton, reset the menu bar's window, this used to crash here.
+ testMenus.actions[0]->trigger();
+}
+
+void tst_QMenuBar::slotForTaskQTBUG53205()
+{
+ QWidget *parent = taskQTBUG53205MenuBar->parentWidget();
+ taskQTBUG53205MenuBar->setParent(Q_NULLPTR);
+ taskQTBUG53205MenuBar->setParent(parent);
+}
+
+
+
QTEST_MAIN(tst_QMenuBar)
#include "tst_qmenubar.moc"
--
GitLab