Fix bug when destruction fields in QWizard
authorCarl Schumann <schumann@fnal.gov>
Fri, 11 May 2012 18:40:21 +0000 (13:40 -0500)
committerQt by Nokia <qt-info@nokia.com>
Wed, 16 May 2012 20:03:30 +0000 (22:03 +0200)
Maintain the consistency of QWizardPrivate's two members:
    QVector<QWizardField> fields;
    QMap<QString, int> fieldIndexMap;
during and after calls to QWizardPrivate's
void _q_handleFieldObjectDestroyed(QObject *)
member function.  The failure to maintain this consistency
caused an out of bounds access and core dump in
QWizard's field(const QString &name) member function.

QWizard's field(const QString &name) member function expects
the values in the QMap fieldIndexMap to be indexes into the
QVector fields.  Prior to this change
_q_handleFieldObjectDestroyed only removed the appropriate
entry from the map and erased it from the vector.  It did
not decrement by one all the indexes greater than the index
that was removed from the map and erased from the vector
in the rest of the map.

For example ...
So if initially have the following mapping ...
"field0" -> 0,
"field1" -> 1, and
"field2" -> 2
with fields of size 3. After destruction of "field1" have ...
"field0" -> 0, and
"field2" -> 2
with fields of size 2.
Now attempts to look up "field2" using QWizard::field will
have an out of bounds error and possibly core dump or trigger
an internal Qt assert because an attempt to access
this->fields[2] will be made.   It should be accessing
this->fields[1], but does not because the map is no longer
consistent with the vector.

This change adds a decrement by one for all the indexes
greater than the index that was removed from the map and
erased from the vector.

Task-number: QTBUG-25691
Change-Id: Ia2a41027628a65faec4ecdd5da235ddd19746a57
Reviewed-by: Shane Kearns <shane.kearns@accenture.com>
Reviewed-by: Lars Knoll <lars.knoll@nokia.com>
src/widgets/dialogs/qwizard.cpp
tests/auto/widgets/dialogs/qwizard/qwizard.pro
tests/auto/widgets/dialogs/qwizard/tst_qwizard.cpp
tests/auto/widgets/dialogs/qwizard/tst_qwizard_2.cpp [new file with mode: 0644]

index 3632774..740ac1d 100644 (file)
@@ -1704,16 +1704,29 @@ void QWizardPrivate::_q_updateButtonStates()
 
 void QWizardPrivate::_q_handleFieldObjectDestroyed(QObject *object)
 {
+    int destroyed_index = -1;
     QVector<QWizardField>::iterator it = fields.begin();
     while (it != fields.end()) {
         const QWizardField &field = *it;
         if (field.object == object) {
+            destroyed_index = fieldIndexMap.value(field.name, -1);
             fieldIndexMap.remove(field.name);
             it = fields.erase(it);
         } else {
             ++it;
         }
     }
+    if (destroyed_index != -1) {
+        QMap<QString, int>::iterator it2 = fieldIndexMap.begin();
+        while (it2 != fieldIndexMap.end()) {
+            int index = it2.value();
+            if (index > destroyed_index) {
+                QString field_name = it2.key();
+                fieldIndexMap.insert(field_name, index-1);
+            }
+            ++it2;
+        }
+    }
 }
 
 void QWizardPrivate::setStyle(QStyle *style)
index 761511d..c6e5bce 100644 (file)
@@ -1,5 +1,5 @@
 CONFIG += testcase
 TARGET = tst_qwizard
 QT += widgets testlib
-SOURCES  += tst_qwizard.cpp
+SOURCES  += tst_qwizard.cpp tst_qwizard_2.cpp
 RESOURCES = qwizard.qrc
index 9d9b55c..c11e479 100644 (file)
@@ -111,6 +111,7 @@ private slots:
     void task177022_setFixedSize();
     void task248107_backButton();
     void task255350_fieldObjectDestroyed();
+    void taskQTBUG_25691_fieldObjectDestroyed2();
 
     /*
         Things that could be added:
@@ -2643,5 +2644,15 @@ void tst_QWizard::task255350_fieldObjectDestroyed()
     delete page;
 }
 
+// Global taskQTBUG_25691_fieldObjectDestroyed2 is defined in
+// tst_qwizard_2.cpp to avoid cluttering up this file with
+// the QWizardPage subclasses, etc. required to complete this
+// test.
+void taskQTBUG_25691_fieldObjectDestroyed2(void);
+void tst_QWizard::taskQTBUG_25691_fieldObjectDestroyed2()
+{
+    ::taskQTBUG_25691_fieldObjectDestroyed2();
+}
+
 QTEST_MAIN(tst_QWizard)
 #include "tst_qwizard.moc"
diff --git a/tests/auto/widgets/dialogs/qwizard/tst_qwizard_2.cpp b/tests/auto/widgets/dialogs/qwizard/tst_qwizard_2.cpp
new file mode 100644 (file)
index 0000000..a04c324
--- /dev/null
@@ -0,0 +1,208 @@
+/****************************************************************************
+**
+** Copyright (C) 2012 Nokia Corporation and/or its subsidiary(-ies).
+** Contact: http://www.qt-project.org/
+**
+** This file is part of the test suite of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:LGPL$
+** GNU Lesser General Public License Usage
+** This file may be used under the terms of the GNU Lesser General Public
+** License version 2.1 as published by the Free Software Foundation and
+** appearing in the file LICENSE.LGPL included in the packaging of this
+** file. Please review the following information to ensure the GNU Lesser
+** General Public License version 2.1 requirements will be met:
+** http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html.
+**
+** In addition, as a special exception, Nokia gives you certain additional
+** rights. These rights are described in the Nokia Qt LGPL Exception
+** version 1.1, included in the file LGPL_EXCEPTION.txt in this package.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU General
+** Public License version 3.0 as published by the Free Software Foundation
+** and appearing in the file LICENSE.GPL included in the packaging of this
+** file. Please review the following information to ensure the GNU General
+** Public License version 3.0 requirements will be met:
+** http://www.gnu.org/copyleft/gpl.html.
+**
+** Other Usage
+** Alternatively, this file may be used in accordance with the terms and
+** conditions contained in a signed written agreement between you and Nokia.
+**
+**
+**
+**
+**
+**
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+
+#include <QComboBox>
+#include <QDebug>
+#include <QLineEdit>
+#include <QMainWindow>
+#include <QVBoxLayout>
+#include <QWizard>
+#include <QWizardPage>
+
+#include <QtTest/QtTest>
+
+class taskQTBUG_25691 : public QWizard
+{
+    Q_OBJECT
+public:
+    taskQTBUG_25691( QWidget * parent = 0 );
+
+    ~taskQTBUG_25691(void);
+};
+
+class taskQTBUG_25691Page1 : public QWizardPage
+{
+    Q_OBJECT
+public:
+    taskQTBUG_25691Page1( QWidget * parent = 0 );
+
+    ~taskQTBUG_25691Page1(void);
+};
+
+class taskQTBUG_25691Page2 : public QWizardPage
+{
+    Q_OBJECT
+public:
+    taskQTBUG_25691Page2( QWidget * parent = 0 );
+
+    virtual void initializePage(void);
+
+    ~taskQTBUG_25691Page2(void);
+
+private:
+    QVBoxLayout * layout;
+    QLineEdit * field0_value;
+    QLineEdit * field1_value;
+    QLineEdit * field2_value;
+};
+
+
+taskQTBUG_25691::taskQTBUG_25691( QWidget * parent )
+    : QWizard( parent )
+{
+    this->addPage( new taskQTBUG_25691Page1 );
+    this->addPage( new taskQTBUG_25691Page2 );
+    this->show();
+}
+
+taskQTBUG_25691::~taskQTBUG_25691(void)
+{
+}
+
+taskQTBUG_25691Page1::taskQTBUG_25691Page1( QWidget * parent )
+    : QWizardPage( parent )
+{
+    QComboBox * field0_needed = new QComboBox( this );
+    field0_needed->addItem( "No" );
+    field0_needed->addItem( "Yes" );
+    field0_needed->setCurrentIndex(0);
+    this->registerField( "field0_needed", field0_needed );
+
+    QComboBox * field1_needed = new QComboBox( this );
+    field1_needed->addItem( "No" );
+    field1_needed->addItem( "Yes" );
+    field1_needed->setCurrentIndex(0);
+    this->registerField( "field1_needed", field1_needed );
+
+    QComboBox * field2_needed = new QComboBox( this );
+    field2_needed->addItem( "No" );
+    field2_needed->addItem( "Yes" );
+    field2_needed->setCurrentIndex(0);
+    this->registerField( "field2_needed", field2_needed );
+
+    QVBoxLayout * layout = new QVBoxLayout;
+    layout->addWidget( field0_needed );
+    layout->addWidget( field1_needed );
+    layout->addWidget( field2_needed );
+    this->setLayout( layout );
+}
+
+taskQTBUG_25691Page1::~taskQTBUG_25691Page1(void)
+{
+}
+
+taskQTBUG_25691Page2::taskQTBUG_25691Page2( QWidget * parent )
+    : QWizardPage( parent )
+{
+    this->layout = new QVBoxLayout;
+    this->setLayout( this->layout );
+
+    this->field0_value = 0;
+    this->field1_value = 0;
+    this->field2_value = 0;
+}
+
+void taskQTBUG_25691Page2::initializePage(void)
+{
+    QWizard * wizard = this->wizard();
+    bool field0_needed = wizard->field( "field0_needed" ).toBool();
+    bool field1_needed = wizard->field( "field1_needed" ).toBool();
+    bool field2_needed = wizard->field( "field2_needed" ).toBool();
+
+    if ( field0_needed  &&  this->field0_value == 0 ){
+        this->field0_value = new QLineEdit( "field0_default" );
+        this->registerField( "field0_value", this->field0_value );
+        this->layout->addWidget( this->field0_value );
+    } else if ( ! field0_needed  &&  this->field0_value != 0 ){
+        this->layout->removeWidget( this->field0_value );
+        delete this->field0_value;
+        this->field0_value = 0;
+    }
+
+    if ( field1_needed  &&  this->field1_value == 0  ){
+        this->field1_value = new QLineEdit( "field1_default" );
+        this->registerField( "field1_value", this->field1_value );
+        this->layout->addWidget( this->field1_value );
+    } else if ( ! field1_needed  &&  this->field1_value != 0 ){
+        this->layout->removeWidget( this->field1_value );
+        delete this->field1_value;
+        this->field1_value = 0;
+    }
+
+    if ( field2_needed  &&  this->field2_value == 0  ){
+        this->field2_value = new QLineEdit( "field2_default" );
+        this->registerField( "field2_value", this->field2_value );
+        this->layout->addWidget( this->field2_value );
+    } else if ( ! field2_needed  &&  this->field2_value != 0 ){
+        this->layout->removeWidget( this->field2_value );
+        delete this->field2_value;
+        this->field2_value = 0;
+    }
+}
+
+taskQTBUG_25691Page2::~taskQTBUG_25691Page2(void)
+{
+}
+
+void taskQTBUG_25691_fieldObjectDestroyed2(void)
+{
+    QMainWindow mw;
+    taskQTBUG_25691 wb( &mw );
+
+    wb.setField( "field0_needed", true );
+    wb.setField( "field1_needed", true );
+    wb.setField( "field2_needed", true );
+    wb.next(); // Results in registration of all three field_Nvalue fields
+    wb.back(); // Back up to cancel need for field1_value
+    wb.setField( "field1_needed", false ); // cancel need for field1_value
+    wb.next(); // Results in destruction of field field1_value's widget
+    wb.next(); // Commit wizard's results
+
+    // Now collect the value from fields that was not destroyed.
+    QString field0_value = wb.field( "field0_value" ).toString();
+    QCOMPARE( field0_value, QString("field0_default") );
+
+    QString field2_value = wb.field( "field2_value" ).toString();
+    QCOMPARE( field2_value, QString("field2_default") );
+}
+
+#include "tst_qwizard_2.moc"