Memory leaks in the code base

[AdelKS]

Hello,

I wanted to try this project, and since I develop with address sanitizer on, it reported some memory leaks in the code base. You can reproduce the leaks with the demo project by adding these lines to the root CMakeLists.txt

diff --git a/CMakeLists.txt b/CMakeLists.txt
index cc265fa..5e1777b 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -8,6 +8,9 @@ project(
     HOMEPAGE_URL https://github.com/simulton/qschematic
 )

+add_compile_options(-fsanitize=address)
+add_link_options(-fsanitize=address)
+
 # User options
 option(QSCHEMATIC_BUILD_DEMO "Whether to build the demo project" ON)
 option(QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD "Whether to pull the GPDS dependency via FetchContent" ON)

then opening and closing the demo app. I will not copy paste the leak error messages here as I think it is not useful.

Given that the demo app is rather complex, the following simple code already showcases leaks:

mainwindow.h

#pragma once

#include <QMainWindow>
#include <QWidget>
#include <qschematic/scene.h>
#include <qschematic/view.h>

class MainWindow : public QMainWindow
{
    Q_OBJECT

public:
    MainWindow(QWidget *parent = nullptr)
        : QMainWindow(parent)
    {
         scene = new QSchematic::Scene;
         view = new QSchematic::View;

         view->setScene(scene);
         setCentralWidget(view);
    }

protected:
    QSchematic::Scene *scene;
    QSchematic::View *view;
};

main.cpp

#include <QApplication>

#include "mainwindow.h"

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

Good luck memory leak hunting !

Adel.

[Tectu]

We have the 1.3.1 release coming up. I'd really like to tackle this before that but I'm not sure whether I have the time necessary for that.

Feel free to submit a PR if you already have some fixes available!

[AdelKS]

Feel free to submit a PR if you already have some fixes available!

I can try to help out, I will keep you updated if I take the time to look into it and find some fixes

[Tectu]

Well, the release is scheduled for tomorrow so... :D

We can always follow up with a patch release 1.3.2 afterwards.

[AdelKS]

Okay, I may have a look at it later today. Otherwise yeah, a patch release sounds like a good plan!

[AdelKS]

I tried to look into it, and unfortunately I think it would take me way more time than someone who knows the code base. I do not think I can help any further on this x(

[Tectu]

That's completely fine - thanks a lot for your efforts in reporting this & looking into it!

We'll proceed with the 1.3.1 release and handle put this into the pipeline for the next release.

[Tectu]

Forgot to reference this issue on this commit: 7d00a75e88a31c1ad6f9ff2856637eab5d2963f1

[Tectu]

@AdelKS Seems like most leaks were located in the demo itself.

Regarding the reproducible example you showed in your initial post: There is a ton of memory leaks because both QSchematic::Scene and QSchematic::View destructors are not being called. This can be easily fixed by passing the corresponding parent to the respective constructors:

mainwindow.h:

#pragma once

#include <QMainWindow>
#include <QWidget>
#include <qschematic/scene.h>
#include <qschematic/view.h>

class MainWindow : public QMainWindow
{
    Q_OBJECT

public:
    MainWindow(QWidget *parent = nullptr)
        : QMainWindow(parent)
    {
         scene = new QSchematic::Scene(this);     // <--- Pass parent!
         view = new QSchematic::View(this);       // <--- Pass parent!

         view->setScene(scene);
         setCentralWidget(view);
    }

protected:
    QSchematic::Scene *scene;
    QSchematic::View *view;
};

[AdelKS]

Seems like most leaks were located in the demo itself.

Great to hear that.

There is a ton of memory leaks because both QSchematic::Scene and QSchematic::View destructors are not being called. This can be easily fixed by passing the corresponding parent to the respective constructors:

Ah ! I thought this part would automatically bind to parent

         view->setScene(scene);
         setCentralWidget(view);

And yeah, the same leaks happen with regular QGraphicsScene and QGraphicsView. Super good that it was this simple. I guess there are still some leaks within the code ? probably due to similar reasons.

[AdelKS]

Small off-topic remark: the demo has a better default style than the library's default style. Although I very much understand that everything is customizable, but it's good to be able to do less code for a first iteration and still have a nice render.

[Tectu]

I guess there are still some leaks within the code ? probably due to similar reasons.

Did you spot any further memory leaks within the QSchematic code base from current master? The last run I did showed none. If you do, please provide some minimal reproducible example and the corresponding sanitizer output so we can hunt this down.

Small off-topic remark: the demo has a better default style than the library's default style. Although I very much understand that everything is customizable, but it's good to be able to do less code for a first iteration and still have a nice render.

This will need some consideration but in general I agree. Feel free to open a separate issue on this so we can handle it appropriately.

[AdelKS]

Did you spot any further memory leaks within the QSchematic code base from current master?

No no, I assumed it since you didn't close the issue. I just tried again the example I attached to this issue and indeed no memory leak reported. Apologies for the wrong alarm.

This will need some consideration but in general I agree. Feel free to open a separate issue on this so we can handle it appropriately.

It's fine, I won't take any more of your time. Unfortunately, at the current state, I do not need to use QSchematic any longer. So I will leave that to someone else when he really needs it. Thanks!

[Tectu]

It's fine, I won't take any more of your time.

Any improvements are welcomed - whether contributed or suggested :)