double free or corruption crash on deletion of QListWidget... what am I doing wrong?



  • Hi,
    I'm trying to do a widget that contains a QListWidget that loads another QWidgetList in a QStackedWidget depending on the selection in the first list.
    It's kind of working as expected but I'm having a crash on deletion of the object.
    I don't really see what I'm doing...
    I'm not using the QT parent mechanism for the QListWidget that are put in the QStackedWidget but rather I handle their deletion manually the method SelectManyInTwoLists::cleanInternalMaps.
    I've done this choice because the Widget is supposed to go in a Wizard and I want to be able to reset all my lists.

    Any idea where the problem is?
    PS: I'm using QString in this example but for my project it will be Pointers that is why I'm using QListWidgetItems and the data in Qt::UserRole.

    Here is my code:

    • the header
    #ifndef SELECTMANYINTWOLISTS_H
    #define SELECTMANYINTWOLISTS_H
    
    #include <QWidget>
    #include <QMap>
    #include <QSet>
    
    class QListWidget;
    class QListWidgetItem;
    
    namespace Ui {
    class SelectManyInTwoLists;
    }
    
    class SelectManyInTwoLists : public QWidget
    {
        Q_OBJECT
    
    public:
        explicit SelectManyInTwoLists(QWidget *parent = 0);
        ~SelectManyInTwoLists();
    
        void setTitles(const QString &mainTitle, const QString &titleList1, const QString &titleList2);
        bool initWidget(const QMap<QString, QSet<QString> > &allItems, const QMap<QString, QSet<QString> > &initialSelectedElements = QMap<QString, QSet<QString> >());
    
        QMap<QString, QSet<QString> > getSelectedElementViews();
    
    public slots:  
        void handlerSelectionList1(QListWidgetItem *current, QListWidgetItem *previous);
    
    private:
        void cleanInternalMaps();
    
    private:
        Ui::SelectManyInTwoLists *ui;
    
    
        QMap<QString, QSet<QString> > _allItems; //! keys are in the first lists, values are the corresponding second list
        QMap<QString, QSet<QString> > _initialSelectedElements; //! initial selections
    
        QMap<QString, ushort > _stackIndexes; //! corresponding table to get the QListWidget of the second lists
    
        QMap<ushort, QListWidget*> _secondLists; //! QListWidget that will go in the QStackedWidget
    };
    
    #endif // SELECTMANYINTWOLISTS_H
    
    
    • now the cpp:
    #include "SelectManyInTwoLists.h"
    #include "ui_SelectManyInTwoLists.h"
    #include <QDebug>
    #include <QtAlgorithms>
    
    SelectManyInTwoLists::SelectManyInTwoLists(QWidget *parent) :
        QWidget(parent),_allItems(), _initialSelectedElements(), _stackIndexes(), _secondLists(),
        ui(new Ui::SelectManyInTwoLists)
    {
        ui->setupUi(this);
        ui->list1->setSelectionMode(QAbstractItemView::SingleSelection);
        connect(ui->list1, &QListWidget::currentItemChanged, this, &SelectManyInTwoLists::handlerSelectionList1);
    }
    
    SelectManyInTwoLists::~SelectManyInTwoLists()
    {
        cleanInternalMaps();
        delete ui;
    }
    
    void SelectManyInTwoLists::setTitles(const QString &mainTitle, const QString &titleList1, const QString &titleList2)
    {
        ui->title->setText(mainTitle);
        ui->titleList1->setText(titleList1);
        ui->titleList2->setText(titleList2);
    }
    
    bool SelectManyInTwoLists::initWidget(
            const QMap<QString, QSet<QString> > &allItems,
            const QMap<QString, QSet<QString> > &initialSelectedElements)
    {
        cleanInternalMaps();
        _allItems                = allItems;
        _initialSelectedElements = initialSelectedElements;
    
        ushort index = 0;
        for (auto key : _allItems.keys())
        {
            QListWidgetItem * item1 = new QListWidgetItem(key, ui->list1); // auto deletion by the
            item1->setData(Qt::UserRole, key);
            ui->list1->addItem(item1);
    
    
            _stackIndexes[key] = index;
            QListWidget *secondList = new QListWidget();
            secondList->setSelectionMode(QAbstractItemView::ExtendedSelection);
    
            QSet<QString> selectedItems;
            if (_initialSelectedElements.contains(key))
                selectedItems = _initialSelectedElements.value(key);
    
            QList<QString> values = _allItems.value(key).toList();
            qSort(values);
            foreach (auto val, values)
            {
                QListWidgetItem * item2 = new QListWidgetItem(val, secondList); // auto deletion by the
                item2->setData(Qt::UserRole, val);
                secondList->addItem(item2);
                if (selectedItems.isEmpty() || selectedItems.contains(val))
                    secondList->item(secondList->count()-1)->setSelected(true);
            }
    
            _secondLists[index] = secondList;
            ui->stackedWidget->insertWidget(index, secondList);
            ++index;
        }
    
        ui->stackedWidget->setCurrentIndex(0);
        return true;
    }
    
    
    void SelectManyInTwoLists::handlerSelectionList1(QListWidgetItem *current, QListWidgetItem *previous)
    {
        if (current && current != previous)
        {
            ui->stackedWidget->setCurrentIndex(
                    _stackIndexes.value(current->data(Qt::UserRole).toString())
            );
        }
    }
    
    
    QMap<QString, QSet<QString> > SelectManyInTwoLists::getSelectedElementViews()
    {
        QMap<QString, QSet<QString> > selectedItems;
        for (QString elem1 : _allItems.keys())
        {
            selectedItems[elem1] = QSet<QString>();
            QListWidget *secondList = _secondLists.value(_stackIndexes[elem1]);
            foreach (QListWidgetItem *item2, secondList->selectedItems())
            {
                selectedItems[elem1].insert(item2->data(Qt::UserRole).toString());
            }
        }
        return selectedItems;
    }
    
    void SelectManyInTwoLists::cleanInternalMaps()
    {
        ui->list1->clear();
        for(int i = ui->stackedWidget->count(); i >= 0; i--)
        {
            QWidget* widget = ui->stackedWidget->widget(i);
            ui->stackedWidget->removeWidget(widget);
        }
        for (auto key : _secondLists.keys())
            delete _secondLists[key];
    
        _secondLists.clear();
    
        _stackIndexes.clear();
        _initialSelectedElements.clear();
        _allItems.clear();
    }
    
    • the main program to launch it:
    #include "SelectManyInTwoLists.h"
    #include <QApplication>
    #include <QDialog>
    #include <QDialogButtonBox>
    #include <QVBoxLayout>
    #include <QObject>
    #include <QDebug>
    
    int main(int argc, char *argv[])
    {
        QApplication a(argc, argv);
    
    
        QMap<QString, QSet<QString> > allItems;
        allItems["Fct1"] = QSet<QString>();
        allItems["Fct1"] << "Fct1_Msg1" << "Fct1_Msg2" << "Fct1_Msg3" << "Fct1_Msg4";
    
        allItems["Fct2"] = QSet<QString>();
        allItems["Fct2"] << "Fct2_Msg1" << "Fct2_Msg2";
    
        allItems["Fct3"] = QSet<QString>();
        allItems["Fct3"] << "Fct3_Msg1" << "Fct3_Msg2" << "Fct3_Msg3";
    
        QMap<QString, QSet<QString> > selectedItems;
        selectedItems["Fct1"] = QSet<QString>();
        selectedItems["Fct1"] << "Fct1_Msg4" << "Fct1_Msg2";
    
        selectedItems["Fct3"] = QSet<QString>();
        selectedItems["Fct3"] << "Fct3_Msg1" << "Fct3_Msg3";
    
    
        SelectManyInTwoLists selectMessages;
        selectMessages.setTitles("Select the messages to be mapped for each Function)",
                                 "Function", "Message");
        selectMessages.initWidget(allItems, selectedItems);
    
    
        QDialog mainDialog;
        mainDialog.setWindowModality (Qt::WindowModal); //you want your dialog modal not your widget.
    
        mainDialog.setLayoutDirection(Qt::LayoutDirection::LeftToRight);
    
        QVBoxLayout mainLayout;
        mainLayout.setMargin(4);
        mainLayout.setSpacing(6);
        mainDialog.setLayout(&mainLayout);
    
    
        QDialogButtonBox _buttonBox(QDialogButtonBox::Ok | QDialogButtonBox::Cancel);
        mainLayout.addWidget(&selectMessages);
        mainLayout.addWidget(&_buttonBox);
        QObject::connect(&_buttonBox, SIGNAL(accepted()), &mainDialog, SLOT(accept()));
        QObject::connect(&_buttonBox, SIGNAL(rejected()), &mainDialog, SLOT(reject()));
    
        mainDialog.setWindowState(mainDialog.windowState() | Qt::WindowMaximized);
        if (mainDialog.exec() == QDialog::Accepted)
        {
            QMap<QString, QSet<QString> > result = selectMessages.getSelectedElementViews();
            for (QString key : result.keys())
            {
                qDebug() << "+ key: " << key;
                if (result.value(key).isEmpty())
                    qDebug() << " has no selections...\n";
                else
                {
                    foreach (QString val, result.value(key))
                        qDebug() << "\t- val: " << val << "\n";
                }
            }
            qDebug() << "OK";
        }
        else
            qDebug() << "We didn't proceed...";
    
    
    //    return a.exec();
        return 0;
    }
    
    • and in case you want to test, here is the ui:
    <?xml version="1.0" encoding="UTF-8"?>
    <ui version="4.0">
     <class>SelectManyInTwoLists</class>
     <widget class="QWidget" name="SelectManyInTwoLists">
      <property name="geometry">
       <rect>
        <x>0</x>
        <y>0</y>
        <width>602</width>
        <height>501</height>
       </rect>
      </property>
      <property name="windowTitle">
       <string>SelectManyInTwoLists</string>
      </property>
      <layout class="QVBoxLayout" name="verticalLayout_4">
       <item alignment="Qt::AlignHCenter">
        <widget class="QLabel" name="title">
         <property name="text">
          <string>TextLabel</string>
         </property>
        </widget>
       </item>
       <item>
        <layout class="QHBoxLayout" name="horizontalLayout">
         <item>
          <layout class="QVBoxLayout" name="verticalLayout_2">
           <item alignment="Qt::AlignLeft">
            <widget class="QLabel" name="titleList1">
             <property name="text">
              <string>TextLabel</string>
             </property>
            </widget>
           </item>
           <item>
            <widget class="QListWidget" name="list1"/>
           </item>
          </layout>
         </item>
         <item>
          <layout class="QVBoxLayout" name="verticalLayout_3">
           <item alignment="Qt::AlignLeft">
            <widget class="QLabel" name="titleList2">
             <property name="text">
              <string>TextLabel</string>
             </property>
            </widget>
           </item>
           <item>
            <widget class="QStackedWidget" name="stackedWidget"/>
           </item>
          </layout>
         </item>
        </layout>
       </item>
      </layout>
     </widget>
     <layoutdefault spacing="6" margin="11"/>
     <resources/>
     <connections/>
    </ui>
    


  • @mbruel said:

    I'm not using the QT parent mechanism for the QListWidget that are put in the QStackedWidget but rather I handle their deletion manually the method SelectManyInTwoLists::cleanInternalMaps.

    Yes you are: ui->stackedWidget->insertWidget(index, secondList); make so that secondList becomes a child of stackedWidget.

    from http://doc.qt.io/qt-5/qstackedwidget.html#insertWidget :

    Inserts the given widget at the given index in the QStackedWidget. Ownership of widget is passed on to the QStackedWidget

    so you are deleting twice



  • @VRonin
    Thanks for your quick reply!
    I didn't notice the QStackedWidget was taking the ownership as it wasn't deleting it when we use QStackedWidget::removeWidget we didn't delete it... I didn't read carefully the Note saying we should reparent the removed widget.

    I've updated my clean function to remove the ownership of the QListWidget:

    void SelectManyInTwoLists::cleanInternalMaps()
    {
        ui->list1->clear();
        for(int i = ui->stackedWidget->count(); i >= 0; i--)
        {
            QWidget* widget = ui->stackedWidget->widget(i);
            if (widget)
            {
                ui->stackedWidget->removeWidget(widget);
                widget->setParent(Q_NULLPTR);
            }
        }
        for (auto key : _secondLists.keys())
            delete _secondLists[key];
    
        _secondLists.clear();
    
        _stackIndexes.clear();
        _initialSelectedElements.clear();
        _allItems.clear();
    }
    

    But I'm still having the issue...
    Any idea what is still wrong?

    PS: if I remove the call of my cleaning function from the destructor, I'm still getting the same crash... :'( I was just expecting some leaks...



  • So the problem is in main: mainLayout.addWidget(&selectMessages); the layout will take ownership of selectMessages as it was on the heap and try to delete it.

    SelectManyInTwoLists selectMessages; should become SelectManyInTwoLists* selectMessages=new SelectManyInTwoLists;

    btw you are doing this thing HORRIBLY wrong. You should really look into model-view overview: http://doc.qt.io/qt-5/model-view-programming.html

    On top of that, KItemModels (part of KDE but 100% platform independent) has already what your are after ready: https://api.kde.org/frameworks/kitemmodels/html/classKSelectionProxyModel.html


  • Qt Champions 2016

    @VRonin said:

    SelectManyInTwoLists selectMessages; should become SelectManyInTwoLists* selectMessages=new SelectManyInTwoLists;

    That's not exactly true. You can use the stack, but you must make sure you construct your objects in the correct order - the parent should always outlive the child QObject, so the destructors get invoked in the proper order (in reverse).

    @mbruel said:

    I'm not using the QT parent mechanism for the QListWidget that are put in the QStackedWidget but rather I handle their deletion manually the method SelectManyInTwoLists::cleanInternalMaps.

    It doesn't work like this. The parent-child relationship is essential for QWidgets, because it's used not only for memory management but also other things (native vs alien widgets for example). You can still handle the deletions manually, but you should set parents to your visual objects (anything that derives from QWidget).

    Kind regards.



  • @VRonin
    Sorry I left on vacation a few days. Thanks for your reply. I now see the issue...
    Why do you say I'm doing it horribly wrong? :(

    I'm not so familiar with the QT model/view schema but a little bit... I just need a widget easy to set up and easy to get the selections from that is why I went for a map and already made QListWidgets. I don't really need a model cause my data won't change. I could build a tree model but it seemed to me it would take as much time (or more) and maybe become more heavy...

    I had a quick look at KSelectionProxyModel and it is not clear to me how I could use it to easily get the same thing I want in output (a map with the selections in the second lists).



  • @kshegunov
    Thanks for the link, pretty useful information. I wasn't exactly sure how the parenting concept is working especially with the deletions.
    I understand now that there are no risk of double deletion on the heap ("if the object has a parent, the destructor automatically removes the object from its parent").
    and that we need to be careful on the stack as the order of declaration will dictate the order of deletions.
    Thanks again.



  • @mbruel said:

    don't really need a model cause my data won't change.

    That's exactly why you need a common model

    Why do you say I'm doing it horribly wrong? :(

    QWidgets are not optimised to be created and destroyed continuously. on top of that every time you create a QListWidgets you are creating a model behind the scenes and filling it.

    What you should do is create the model as a tree only once, use QListView and call QAbstractItemView::setRootIndex to show the children of only one object and save the selection as a list of QPersistentModelIndex



  • @VRonin
    I kind of see... it sounds a bit more complicated, I'll have a look at it when I find some time.

    If I understand well, I could only use 1 tree model for both my first list (all root items) and all my second lists by setting/resetting on the fly the rootIndex of the second QListWidget...
    So it's my handler SelectManyInTwoListsWidget::handlerSelectionList1 that would do that job (resetting the rootIndex of the secondList) after having saved the current selections....

    Why do I need to save the selections? Shouldn't/Couldn't the model remember which items are selected even if they are not displayed in any view? (like in a role)



  • when you set the root for the second list the selection gets reset but, as you correctly point out, you could save it in a role and manually re-enable it when you set the root


Log in to reply
 

Looks like your connection to Qt Forum was lost, please wait while we try to reconnect.