QComboBox and smart pointer conflict?

CJha

Hi, I am facing a weird issue, my QWidget subclass is throwing an exception after its destructor only when the value of QComboBox in its UI is changed.

I have a structure where I have a namespace that contains a module that has some widgets. I have designed the structure in such a way that the module is hidden away from the main application. Here are the files:

main.cpp

#include "mainwindow.h"
#include <QtWidgets/QApplication>

int main(int argc, char *argv[])
{
    QApplication a(argc, argv);
    guitest::setup();
    MainWindow w;
    w.show();
    return a.exec();
}

mainwindow.h

#pragma once

#include <QtWidgets/QMainWindow>
#include "ui_mainwindow.h"

#include "common.h"

class MainWindow : public QMainWindow
{
    Q_OBJECT

public:
    MainWindow(QWidget *parent = Q_NULLPTR);

private:
    Ui::MainWindowClass ui;

private slots:
    void on_pushButton_clicked(); // ComboWid button in my main UI
};

mainwindow.cpp

#include "mainwindow.h"

MainWindow::MainWindow(QWidget *parent)
    : QMainWindow(parent)
{
    ui.setupUi(this);
}

void MainWindow::on_pushButton_clicked()
{
    guitest::showComboWidWindow();
}

common.h - contains my namespace that acts as a seperation

#pragma once

namespace guitest
{
	void setup();
	void showComboWidWindow();
}

common.cpp

#pragma once

#include <memory>
#include "common.h"
#include "combowid.h"

namespace guitest
{
	std::unique_ptr<ComboWid> comboWid{nullptr};

	void setup()
	{
		comboWid = std::make_unique<ComboWid>();
	}
	
	void showComboWidWindow()
	{
		comboWid->show();
	}
}

combowid.h

#pragma once

#include <QWidget>
#include "ui_combowid.h"

#include <QDebug>

class ComboWid : public QWidget
{
	Q_OBJECT

public:
	ComboWid(QWidget *parent = Q_NULLPTR);
	~ComboWid();

private:
	Ui::ComboWid ui;
};

combowid.cpp

#include "combowid.h"

ComboWid::ComboWid(QWidget *parent)
	: QWidget(parent)
{
	ui.setupUi(this);
}

ComboWid::~ComboWid()
{
	qDebug() << "\nComboWid is Getting destroyed\n"; // to see if it is getting destroyed properly (i.e. no memory leaks)
}

My MainWindow UI looks like this:

My ComboWid UI:

None of the widgets in the ComboWid is connected to anything, these are just placed there. When I click on the "ComboWid" push button in my main window the "ComboWid" window is shown. If I do not change the value of combo boxes (i.e. normal combo box and font combo box) then when I close my main window the ComboWid is destroyed properly. If I change the value of either combo boxes in my ComboWid UI then even though the destructor of the ComboWid is called correctly I get the following access violation error:

This only happens with QComboBox and its subclasses, if I change the value of any of the other widgets it does not affect the destructor of ComboWid and there are no exceptions thrown. Also, I have noticed that if I use normal pointers instead of smart pointers in common.cpp then this problem is no longer there, but then the ComboWid is not destroyed at application close. Is there a conflict between smart pointers and how the QComboBox is implemented? I am using Qt 5.15 with Visual Studio 2019 (16.10.4).

EDIT: I found out a solution to circumvent this this problem by using raw pointers and QGuiApplication::lastWindowClosed() signal. Now my common.cpp looks like this:

#pragma once

#include <QApplication>
#include <memory>
#include "common.h"
#include "combowid.h"

namespace guitest
{
	ComboWid* comboWid{nullptr};
	
	void setup()
	{
		comboWid = new ComboWid;
		QObject::connect(qApp, &QApplication::lastWindowClosed, &guitest::onLastWindowClosed);
	}
	
	void showComboWidWindow()
	{
		comboWid->show();
	}

	void onLastWindowClosed()
	{
		delete comboWid;
	}
}

It works fine now and my CustomWid is getting destroyed properly at the end, but if any of you still have any information as to how I can keep using smart pointer in my common.cpp that would be very helpful as I am trying to avoid any use of raw pointers in my application.

Christian Ehrlicher

You must not use shared pointers with QObjects when you set a parent to them.

CJha

@Christian-Ehrlicher But I am neither using a shared pointer nor setting a parent to my ComboWid, the pointer is std::unique_ptr and the parent of ComboWid is Q_NULLPTR.

Christian Ehrlicher

a std::unqiue_ptr is nothing really different from a normal shared_ptr
Since your pointer is destroyed after QApplication was destroyed you will get a crash. Don't use shared pointers for widgets - it's not needed.

SimonSchroeder

I don't see any point why your application crashes. You have to be careful with smart pointers and Qt as others have mentioned. As soon as your object has a parent it will be destroyed by it.

That being said: I don't see your widget to have a parent. So, this does not explain it. You should run a debugger, maybe set a breakpoint inside the destructor and see when it is getting called.

However, there are a few more things to consider: Don't use global variables! There is rarely any reason for this. These will get you into trouble eventually. It is commandable that you have put comboWid into a namespace. Still, it is not a good idea. (Global variables have problems with the order of initialization and destruction.)

Second: Don't use smart pointers with widgets! Qt predates smart pointers from C++11 and thus found its own way to control ownership. In much over a decade of using Qt I have never had any case where I needed a smart pointer to control the lifetime of widgets. If your ComboWid is only called from the MainWindow (from a single one, that is) make it a member of MainWindow and initialize it in the constructor of MainWindow. This is still not the best solution, but it is much better than what you have right now. (Better in the sense of more common.)

My solution would look a little different anyway. I am not sure what use case you want to simulate with your little toy example. But, usually I would expect ComboWid to be a dialog and not a widget. If you have ComboWid derive from QDialog you can easily do away with pointers to it altogether. Because then the following would work:

void MainWindow::on_pushButton_clicked()
{
    ComboWid comboWid; // local variable with local lifetime

    comboWid.exec(); // will only return after the dialog is closed
}

If, however, you insist on using a pointer (one could start a discussion about nested event loops because of comboWid.exec()), the normal way would be to have a signal emitted when the widget/dialog closes (I am not sure if there is already a suitable one for QWidget) and connect it to deleteLater().

What do others think about using QDialog::exec() instead?

Christian Ehrlicher

@SimonSchroeder said in QComboBox and smart pointer conflict?:

I don't see any point why your application crashes.

It's crashing at the end when the QWidget gets destroyed after QApplication was already destroyed which is not allowed as I already explained.

SimonSchroeder

@Christian-Ehrlicher said in QComboBox and smart pointer conflict?:

It's crashing at the end when the QWidget gets destroyed after QApplication was already destroyed which is not allowed as I already explained.

Alright, I missed that in your previous post. This makes the use of a smart pointer to a QWidget together with a global variable a problem.

CJha

@SimonSchroeder Hi! sorry for the late reply.

That being said: I don't see your widget to have a parent. So, this does not explain it. You should run a debugger, maybe set a breakpoint inside the destructor and see when it is getting called.

I set a breakpoint inside the destructor, it is getting called normally and it is printing the debug output, however, as soon as the end of the destructor is reached I get the exception thrown. I am unable to get the complete debug information if I set the breakpoint at closeEvent for my main window because I am using Visual Studio and it says "Qt5Widgets.pdb not loaded: Qt5Widgetsd.pdb contains the debug information required to find the source for the module Qt5Widgetsd.dll". I don't know how to fix this.

Regarding your other points, I would like to explain why I am doing it the way I am doing it:

My comboWid is actually called guiModule in my application, it is a subclass of QWidget. It defines the looks of the application (as you can guess by the name) and users can change quite a few options in there.

If you have ComboWid derive from QDialog you can easily do away with pointers to it altogether.

The guiModule itself manages a few other classes such guiStyle which is a subclass of QProxyStyle. The guiModule's UI is only shown when requested by the admin user. For normal users to change common settings I have provided a few public functions inside the guiModule to change those settings and these will be connected to some widgets inside the application depending on the requirement. So, even if the UI of the guiModule is not visible, it still works in the background and changes the looks of the application. This is the main reason I am not using a dialog.

make it a member of MainWindow and initialize it in the constructor of MainWindow

The guiModule is made in a way that I just have to write npi::setup(); (npi is my primary namespace, it contains gui namespace which contains guiModule) after creating the QApplication object in the main and it creates a bunch of variables and default values along with the guiModule which takes over the entire GUI looks part of the application. This makes it much easier for me to have a consistent UI across different applications that I am developing.

Only one instance of guiModule is made, and this instance is going to exist for the lifetime of the application and so it is not a big issue if it is destroyed properly or not. But in any case I would like to make sure that it is destroyed before the application exits.

SimonSchroeder

@CJha said in QComboBox and smart pointer conflict?:

But in any case I would like to make sure that it is destroyed before the application exits.

In general this is a good approach. When you write clean code all resources should be freed by your own application (typically RAII in C++). However, when your application closes the OS will free all memory belonging to the application and removes all file handles of the application. Especially in complex applications it can take quite a while to clean up everything in order. In those cases it is a lot more userfriendly to just exit without the clean up. So, don't worry too much about clean up. Still, you should understand why this happens and how it can be avoided.