From b7af368e86874d71ffc9071c9ef009814d6a3467 Mon Sep 17 00:00:00 2001 From: Gabriel de Dietrich Date: Tue, 9 Feb 2010 09:56:25 +0100 Subject: Crash when deleting the parent of a context menu while it is being displayed Asynchronous deletion of a widget (e.g. calling deleteLater() after a timeout) could be caught by the context menu's event loop if called with exec() instead of popup(). This causes the context menu to be deleted twice in some cases, and a crash generally follows. Although this introduces a minor behaviour change, we now use popup() with the WA_DeleteOnClose attribute to display the context menu in all the contextMenuEvent() bodies, except in those where the menu was already protected by a QPointer. In QDialog::contextMenuEvent(), we use QWeakPointer to reflect the fact that the menu was previously allocated in the stack. QAbstractSpinBox, QDialog and QMdiSubWindow keep using QMenu::exec() in contextMenuEvent() as they need the QAction returned. Some auto-tests included. Reviewed-by: Olivier Task-number: QTBUG-7902 --- src/gui/dialogs/qdialog.cpp | 7 ++++--- src/gui/dialogs/qmessagebox.cpp | 4 ++-- src/gui/text/qtextcontrol.cpp | 4 ++-- src/gui/widgets/qlabel.cpp | 4 ++-- src/gui/widgets/qlineedit.cpp | 6 +++--- src/gui/widgets/qmainwindow.cpp | 12 ++++++++---- tests/auto/qlabel/tst_qlabel.cpp | 24 ++++++++++++++++++++++++ tests/auto/qlineedit/tst_qlineedit.cpp | 23 +++++++++++++++++++++++ tests/auto/qtextedit/tst_qtextedit.cpp | 22 ++++++++++++++++++++++ 9 files changed, 90 insertions(+), 16 deletions(-) diff --git a/src/gui/dialogs/qdialog.cpp b/src/gui/dialogs/qdialog.cpp index 9ff2ad8..fb0dba4 100644 --- a/src/gui/dialogs/qdialog.cpp +++ b/src/gui/dialogs/qdialog.cpp @@ -648,13 +648,14 @@ void QDialog::contextMenuEvent(QContextMenuEvent *e) while (w && w->whatsThis().size() == 0 && !w->testAttribute(Qt::WA_CustomWhatsThis)) w = w->isWindow() ? 0 : w->parentWidget(); if (w) { - QMenu p(this); - QAction *wt = p.addAction(tr("What's This?")); - if (p.exec(e->globalPos()) == wt) { + QWeakPointer p = new QMenu(this); + QAction *wt = p.data()->addAction(tr("What's This?")); + if (p.data()->exec(e->globalPos()) == wt) { QHelpEvent e(QEvent::WhatsThis, w->rect().center(), w->mapToGlobal(w->rect().center())); QApplication::sendEvent(w, &e); } + delete p.data(); } #endif } diff --git a/src/gui/dialogs/qmessagebox.cpp b/src/gui/dialogs/qmessagebox.cpp index d1b2e3f..ed437ff 100644 --- a/src/gui/dialogs/qmessagebox.cpp +++ b/src/gui/dialogs/qmessagebox.cpp @@ -92,8 +92,8 @@ public: { #ifndef QT_NO_CONTEXTMENU QMenu *menu = createStandardContextMenu(); - menu->exec(e->globalPos()); - delete menu; + menu->setAttribute(Qt::WA_DeleteOnClose); + menu->popup(e->globalPos()); #else Q_UNUSED(e); #endif diff --git a/src/gui/text/qtextcontrol.cpp b/src/gui/text/qtextcontrol.cpp index aaaa3ca..8d3923f 100644 --- a/src/gui/text/qtextcontrol.cpp +++ b/src/gui/text/qtextcontrol.cpp @@ -1762,8 +1762,8 @@ void QTextControlPrivate::contextMenuEvent(const QPoint &screenPos, const QPoint QMenu *menu = q->createStandardContextMenu(docPos, contextWidget); if (!menu) return; - menu->exec(screenPos); - delete menu; + menu->setAttribute(Qt::WA_DeleteOnClose); + menu->popup(screenPos); #endif } diff --git a/src/gui/widgets/qlabel.cpp b/src/gui/widgets/qlabel.cpp index c779312..b81f04f 100644 --- a/src/gui/widgets/qlabel.cpp +++ b/src/gui/widgets/qlabel.cpp @@ -951,8 +951,8 @@ void QLabel::contextMenuEvent(QContextMenuEvent *ev) return; } ev->accept(); - menu->exec(ev->globalPos()); - delete menu; + menu->setAttribute(Qt::WA_DeleteOnClose); + menu->popup(ev->globalPos()); #endif } diff --git a/src/gui/widgets/qlineedit.cpp b/src/gui/widgets/qlineedit.cpp index 2d2df92..141f844 100644 --- a/src/gui/widgets/qlineedit.cpp +++ b/src/gui/widgets/qlineedit.cpp @@ -2046,9 +2046,9 @@ void QLineEdit::dropEvent(QDropEvent* e) */ void QLineEdit::contextMenuEvent(QContextMenuEvent *event) { - QPointer menu = createStandardContextMenu(); - menu->exec(event->globalPos()); - delete menu; + QMenu *menu = createStandardContextMenu(); + menu->setAttribute(Qt::WA_DeleteOnClose); + menu->popup(event->globalPos()); } #if defined(Q_WS_WIN) diff --git a/src/gui/widgets/qmainwindow.cpp b/src/gui/widgets/qmainwindow.cpp index eb25417..a63f3bf 100644 --- a/src/gui/widgets/qmainwindow.cpp +++ b/src/gui/widgets/qmainwindow.cpp @@ -1555,11 +1555,15 @@ void QMainWindow::contextMenuEvent(QContextMenuEvent *event) #ifndef QT_NO_MENU QMenu *popup = createPopupMenu(); - if (popup && !popup->isEmpty()) { - popup->exec(event->globalPos()); - event->accept(); + if (popup) { + if (!popup->isEmpty()) { + popup->setAttribute(Qt::WA_DeleteOnClose); + popup->popup(event->globalPos()); + event->accept(); + } else { + delete popup; + } } - delete popup; #endif } #endif // QT_NO_CONTEXTMENU diff --git a/tests/auto/qlabel/tst_qlabel.cpp b/tests/auto/qlabel/tst_qlabel.cpp index 7099917..153149e7 100644 --- a/tests/auto/qlabel/tst_qlabel.cpp +++ b/tests/auto/qlabel/tst_qlabel.cpp @@ -121,6 +121,10 @@ private slots: void mnemonic(); void selection(); +#ifndef QT_NO_CONTEXTMENU + void taskQTBUG_7902_contextMenuCrash(); +#endif + private: QLabel *testWidget; QPointer test_box; @@ -582,5 +586,25 @@ void tst_QLabel::selection() QCOMPARE(label.selectionStart(), 6); } +#ifndef QT_NO_CONTEXTMENU +void tst_QLabel::taskQTBUG_7902_contextMenuCrash() +{ + QLabel *w = new QLabel("Test or crash?"); + w->setTextInteractionFlags(Qt::TextSelectableByMouse); + w->show(); + QTest::qWaitForWindowShown(w); + + QTimer ti; + w->connect(&ti, SIGNAL(timeout()), w, SLOT(deleteLater())); + ti.start(300); + + QContextMenuEvent *cme = new QContextMenuEvent(QContextMenuEvent::Mouse, w->rect().center()); + qApp->postEvent(w, cme); + + QTest::qWait(350); + // No crash, it's allright. +} +#endif + QTEST_MAIN(tst_QLabel) #include "tst_qlabel.moc" diff --git a/tests/auto/qlineedit/tst_qlineedit.cpp b/tests/auto/qlineedit/tst_qlineedit.cpp index 7283916..fcca58a 100644 --- a/tests/auto/qlineedit/tst_qlineedit.cpp +++ b/tests/auto/qlineedit/tst_qlineedit.cpp @@ -271,6 +271,9 @@ private slots: void taskQTBUG_4401_enterKeyClearsPassword(); void taskQTBUG_4679_moveToStartEndOfBlock(); void taskQTBUG_4679_selectToStartEndOfBlock(); +#ifndef QT_NO_CONTEXTMENU + void taskQTBUG_7902_contextMenuCrash(); +#endif protected slots: #ifdef QT3_SUPPORT @@ -3638,5 +3641,25 @@ void tst_QLineEdit::taskQTBUG_4679_selectToStartEndOfBlock() #endif // Q_OS_MAC } +#ifndef QT_NO_CONTEXTMENU +void tst_QLineEdit::taskQTBUG_7902_contextMenuCrash() +{ + // Would pass before the associated commit, but left as a guard. + QLineEdit *w = new QLineEdit; + w->show(); + QTest::qWaitForWindowShown(w); + + QTimer ti; + w->connect(&ti, SIGNAL(timeout()), w, SLOT(deleteLater())); + ti.start(200); + + QContextMenuEvent *cme = new QContextMenuEvent(QContextMenuEvent::Mouse, w->rect().center()); + qApp->postEvent(w, cme); + + QTest::qWait(300); + // No crash, it's allright. +} +#endif + QTEST_MAIN(tst_QLineEdit) #include "tst_qlineedit.moc" diff --git a/tests/auto/qtextedit/tst_qtextedit.cpp b/tests/auto/qtextedit/tst_qtextedit.cpp index deb9379..101baa5 100644 --- a/tests/auto/qtextedit/tst_qtextedit.cpp +++ b/tests/auto/qtextedit/tst_qtextedit.cpp @@ -201,6 +201,9 @@ private slots: void noWrapBackgrounds(); void preserveCharFormatAfterUnchangingSetPosition(); void twoSameInputMethodEvents(); +#ifndef QT_NO_CONTEXTMENU + void taskQTBUG_7902_contextMenuCrash(); +#endif private: void createSelection(); @@ -2202,5 +2205,24 @@ void tst_QTextEdit::twoSameInputMethodEvents() QCOMPARE(ed->document()->firstBlock().layout()->lineCount(), 1); } +#ifndef QT_NO_CONTEXTMENU +void tst_QTextEdit::taskQTBUG_7902_contextMenuCrash() +{ + QTextEdit *w = new QTextEdit; + w->show(); + QTest::qWaitForWindowShown(w); + + QTimer ti; + w->connect(&ti, SIGNAL(timeout()), w, SLOT(deleteLater())); + ti.start(200); + + QContextMenuEvent *cme = new QContextMenuEvent(QContextMenuEvent::Mouse, w->rect().center()); + qApp->postEvent(w->viewport(), cme); + + QTest::qWait(300); + // No crash, it's allright. +} +#endif + QTEST_MAIN(tst_QTextEdit) #include "tst_qtextedit.moc" -- cgit v0.12