QMenuBar: nested parenting fix

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

@@ -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
+    //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;
 
-    if (oldParent != newParent) {
-        if (oldParent)
-            oldParent->removeEventFilter(q);
-        if (newParent)
-            newParent->installEventFilter(q);
+    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:

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

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"