diff options
author | Giuseppe D'Angelo <giuseppe.dangelo@kdab.com> | 2015-05-08 13:49:54 +0200 |
---|---|---|
committer | Giuseppe D'Angelo <giuseppe.dangelo@kdab.com> | 2015-05-09 08:41:23 +0000 |
commit | 1ac1ae05f551d45772ff2a840dba128abf04171e (patch) | |
tree | a71c5963c2bb119f50f90568bd6ed6cbd76ad1c3 | |
parent | 4b7d70886f710767c2b163a08da44d3a7b80479c (diff) |
QDialogButtonBox: prevent crashes on deletions in slots
As usual, user code connected to signals emitted from Qt may destroy
an object's internal status while a signal is being emitted.
Guard a bit QDialogButtonBox signal emissions to prevent
crashes in case the button or a dialog get deleted from a slot
connected to clicked(). Also, be sure to emit the corresponding
accepted/rejected/etc. signal.
Change-Id: I7b1888070a8f2f56aa60923a17f90fb5efef145c
Task-number: QTBUG-45835
Reviewed-by: Friedemann Kleint <Friedemann.Kleint@theqtcompany.com>
Reviewed-by: Olivier Goffart (Woboq GmbH) <ogoffart@woboq.com>
Reviewed-by: Marc Mutz <marc.mutz@kdab.com>
-rw-r--r-- | src/widgets/widgets/qdialogbuttonbox.cpp | 12 | ||||
-rw-r--r-- | tests/auto/widgets/widgets/qdialogbuttonbox/tst_qdialogbuttonbox.cpp | 66 |
2 files changed, 77 insertions, 1 deletions
diff --git a/src/widgets/widgets/qdialogbuttonbox.cpp b/src/widgets/widgets/qdialogbuttonbox.cpp index 237eb775b9..3e8c08f923 100644 --- a/src/widgets/widgets/qdialogbuttonbox.cpp +++ b/src/widgets/widgets/qdialogbuttonbox.cpp @@ -852,9 +852,19 @@ void QDialogButtonBoxPrivate::_q_handleButtonClicked() { Q_Q(QDialogButtonBox); if (QAbstractButton *button = qobject_cast<QAbstractButton *>(q->sender())) { + // Can't fetch this *after* emitting clicked, as clicked may destroy the button + // or change its role. Now changing the role is not possible yet, but arguably + // both clicked and accepted/rejected/etc. should be emitted "atomically" + // depending on whatever role the button had at the time of the click. + const QDialogButtonBox::ButtonRole buttonRole = q->buttonRole(button); + QPointer<QDialogButtonBox> guard(q); + emit q->clicked(button); - switch (q->buttonRole(button)) { + if (!guard) + return; + + switch (buttonRole) { case QPlatformDialogHelper::AcceptRole: case QPlatformDialogHelper::YesRole: emit q->accepted(); diff --git a/tests/auto/widgets/widgets/qdialogbuttonbox/tst_qdialogbuttonbox.cpp b/tests/auto/widgets/widgets/qdialogbuttonbox/tst_qdialogbuttonbox.cpp index 9c549365ff..38b473e5ae 100644 --- a/tests/auto/widgets/widgets/qdialogbuttonbox/tst_qdialogbuttonbox.cpp +++ b/tests/auto/widgets/widgets/qdialogbuttonbox/tst_qdialogbuttonbox.cpp @@ -90,6 +90,7 @@ private slots: // void buttons(); void testDelete(); + void testSignalEmissionAfterDelete_QTBUG_45835(); void testRemove(); void testMultipleAdd(); void testStandardButtonMapping_data(); @@ -111,6 +112,7 @@ private: tst_QDialogButtonBox::tst_QDialogButtonBox() { + qRegisterMetaType<QAbstractButton *>(); } tst_QDialogButtonBox::~tst_QDialogButtonBox() @@ -414,6 +416,70 @@ void tst_QDialogButtonBox::testDelete() QCOMPARE(buttonBox.buttons().count(), 0); } +class ObjectDeleter : public QObject +{ + Q_OBJECT +public slots: + void deleteButton(QAbstractButton *button) + { + delete button; + } + + void deleteSender() + { + delete sender(); + } +}; + +void tst_QDialogButtonBox::testSignalEmissionAfterDelete_QTBUG_45835() +{ + { + QDialogButtonBox buttonBox; + QCOMPARE(buttonBox.buttons().count(), 0); + + QSignalSpy buttonClickedSpy(&buttonBox, &QDialogButtonBox::clicked); + QVERIFY(buttonClickedSpy.isValid()); + + QSignalSpy buttonBoxAcceptedSpy(&buttonBox, &QDialogButtonBox::accepted); + QVERIFY(buttonBoxAcceptedSpy.isValid()); + + QPushButton *button = buttonBox.addButton("Test", QDialogButtonBox::AcceptRole); + QCOMPARE(buttonBox.buttons().count(), 1); + + ObjectDeleter objectDeleter; + connect(&buttonBox, &QDialogButtonBox::clicked, &objectDeleter, &ObjectDeleter::deleteButton); + + button->click(); + + QCOMPARE(buttonBox.buttons().count(), 0); + QCOMPARE(buttonClickedSpy.count(), 1); + QCOMPARE(buttonBoxAcceptedSpy.count(), 1); + } + + { + QPointer<QDialogButtonBox> buttonBox(new QDialogButtonBox); + QCOMPARE(buttonBox->buttons().count(), 0); + + QSignalSpy buttonClickedSpy(buttonBox.data(), &QDialogButtonBox::clicked); + QVERIFY(buttonClickedSpy.isValid()); + + QSignalSpy buttonBoxAcceptedSpy(buttonBox.data(), &QDialogButtonBox::accepted); + QVERIFY(buttonBoxAcceptedSpy.isValid()); + + QPushButton *button = buttonBox->addButton("Test", QDialogButtonBox::AcceptRole); + QCOMPARE(buttonBox->buttons().count(), 1); + + ObjectDeleter objectDeleter; + connect(buttonBox.data(), &QDialogButtonBox::clicked, &objectDeleter, &ObjectDeleter::deleteSender); + + button->click(); + + QVERIFY(buttonBox.isNull()); + QCOMPARE(buttonClickedSpy.count(), 1); + QCOMPARE(buttonBoxAcceptedSpy.count(), 0); + } +} + void tst_QDialogButtonBox::testMultipleAdd() { // Add a button into the thing multiple times. |