Memory leaks concern

If you are using the main C++ distribution of wxWidgets, Feel free to ask any question related to wxWidgets development here. This means questions regarding to C++ and wxWidgets, not compile problems.
Post Reply
DjSt3rios
Earned a small fee
Earned a small fee
Posts: 17
Joined: Thu Mar 11, 2021 4:41 pm

Memory leaks concern

Post by DjSt3rios »

Hi my friends, I am fairly new in C++ and I think I have memory leaks, although I do delete everything that's not an wx object, but I noticed that when I switch between 2 windows all the time, the memory is slowly going up. I have a login screen and a main window. The application starts with 6MB RAM of usage, I login and then logout 4-6 times, and the RAM usage is about 10MB, which is not much, but still if I am doing something wrong I'd like to know it. Is this normal to happen? I thought maybe there is some sort of Garbage collection that runs once in a while, but if I am not mistaken C++ does not even have such a system(not by default anyway). Here's an example of logging out(Main window closes, and login window is created):

Code: Select all

GeneralDialog dlg(nullptr,"Logout confirmation","Are you sure you want to logout?");
	if (dlg.ShowModal() == wxID_YES)
	{
		wxGetApp().user->Logout();
		wxGetApp().mainWindow->Close(true);
		wxGetApp().mainWindow->Destroy();
		wxGetApp().loginWindow = new LoginWindow(nullptr);
		wxGetApp().loginWindow->Show();
		delete wxGetApp().mainWindow;
	}
and once the user is logged in, the new window is created with this code:

Code: Select all

wxGetApp().mainWindow = new MainWindow(nullptr);
		wxGetApp().mainWindow->Show(true);
		wxGetApp().loginWindow->Close(true);
		wxGetApp().loginWindow->Destroy();
		delete wxGetApp().loginWindow;
Almost all the objects I use are derived from wx, like wxString, wxImage etc, which should be deleted automatically, right?
User avatar
doublemax
Moderator
Moderator
Posts: 19116
Joined: Fri Apr 21, 2006 8:03 pm
Location: $FCE2

Re: Memory leaks concern

Post by doublemax »

I don't see any obvious memory leaks in that code, but i'm surprised it doesn't crash.

What's the base class for MainWindow and LoginWindow?

In the following i'll assume it's wxFrame.

Code: Select all

wxGetApp().mainWindow->Close(true);
wxGetApp().mainWindow->Destroy();
delete wxGetApp().mainWindow;
The default behavior of a wxFrame is to destroy itself when it's closed. So this code should normally crash, as mainWindow gets destroyed twice. Usually just calling Close() should be enough. Unless it has a close event handler which prevents the automatic destruction.

Code: Select all

wxGetApp().loginWindow = new LoginWindow(nullptr);
wxGetApp().loginWindow->Show();
I'm not 100% sure whether all toplevel windows get automatically destroyed if the app terminates, but if not and loginWindow is not NULL, this will leak memory as the old instance will not get destroyed.

You should also ask yourself if it's really needed to destroy mainWindow and loginWindow, or if it's not enough to just create them once and then use Hide() and Show().
Almost all the objects I use are derived from wx, like wxString, wxImage etc, which should be deleted automatically, right?
Not necessarily. Generally you don't have to manually destroy any wxWindow, wxSizer and wxMenu, as they will be destroyed by their respective owner or they destroy themselves (wxFrame). (There are a few exceptions).

If if you create any other object like wxString, wxImage etc. on the heap (= with new()) you'll usually have to delete it yourself.
Use the source, Luke!
DjSt3rios
Earned a small fee
Earned a small fee
Posts: 17
Joined: Thu Mar 11, 2021 4:41 pm

Re: Memory leaks concern

Post by DjSt3rios »

doublemax wrote: Wed Mar 17, 2021 4:50 pm I don't see any obvious memory leaks in that code, but i'm surprised it doesn't crash.

What's the base class for MainWindow and LoginWindow?

In the following i'll assume it's wxFrame.

Code: Select all

wxGetApp().mainWindow->Close(true);
wxGetApp().mainWindow->Destroy();
delete wxGetApp().mainWindow;
The default behavior of a wxFrame is to destroy itself when it's closed. So this code should normally crash, as mainWindow gets destroyed twice. Usually just calling Close() should be enough. Unless it has a close event handler which prevents the automatic destruction.

Code: Select all

wxGetApp().loginWindow = new LoginWindow(nullptr);
wxGetApp().loginWindow->Show();
I'm not 100% sure whether all toplevel windows get automatically destroyed if the app terminates, but if not and loginWindow is not NULL, this will leak memory as the old instance will not get destroyed.

You should also ask yourself if it's really needed to destroy mainWindow and loginWindow, or if it's not enough to just create them once and then use Hide() and Show().
Almost all the objects I use are derived from wx, like wxString, wxImage etc, which should be deleted automatically, right?
Not necessarily. Generally you don't have to manually destroy any wxWindow, wxSizer and wxMenu, as they will be destroyed by their respective owner or they destroy themselves (wxFrame). (There are a few exceptions).

If if you create any other object like wxString, wxImage etc. on the heap (= with new()) you'll usually have to delete it yourself.
Thank you. I am still trying to get the grasp of this. You are correct, my windows are based on wxFrame. Maybe I will indeed create them once and then just hide them, and reset some information shown in them. But I still feel a bit sad, trying to understand where my leaks come from 🤔 For now I will check all my objects such as wxString,wxImage and make sure to delete them manually, and see if it's any better.
User avatar
doublemax
Moderator
Moderator
Posts: 19116
Joined: Fri Apr 21, 2006 8:03 pm
Location: $FCE2

Re: Memory leaks concern

Post by doublemax »

First you check this part:

Code: Select all

wxGetApp().loginWindow = new LoginWindow(nullptr);
wxGetApp().loginWindow->Show();
I'm not 100% sure whether all toplevel windows get automatically destroyed if the app terminates, but if not and loginWindow is not NULL, this will leak memory as the old instance will not get destroyed.
Use the source, Luke!
ONEEYEMAN
Part Of The Furniture
Part Of The Furniture
Posts: 7459
Joined: Sat Apr 16, 2005 7:22 am
Location: USA, Ukraine

Re: Memory leaks concern

Post by ONEEYEMAN »

Hi,
For the login window I would use wxDialog-derived class.
Just have ID and password as wxTextCtrl and 2 buttons: "OK" and "Cancel".

Also, stylistically: you don't have to use wxGetApp() call every time something happens. It's weird and un-neccessary.
Look how tjhings is done in the "minimal" sample and ALWAYS, ALWAYS, ALWAYS look at the sample to see how to do what you want.

Thank you.
DjSt3rios
Earned a small fee
Earned a small fee
Posts: 17
Joined: Thu Mar 11, 2021 4:41 pm

Re: Memory leaks concern

Post by DjSt3rios »

doublemax wrote: Wed Mar 17, 2021 5:08 pm First you check this part:

Code: Select all

wxGetApp().loginWindow = new LoginWindow(nullptr);
wxGetApp().loginWindow->Show();
I'm not 100% sure whether all toplevel windows get automatically destroyed if the app terminates, but if not and loginWindow is not NULL, this will leak memory as the old instance will not get destroyed.
Okay so I added a check to check if it's a null pointer, and only then create a new window, otherwise restore the existing window, but I think I found my problem. Sometimes I have trouble calling functions from callbacks, I assume it was a multi-threading issue, so what I did was create an wxThreadEvent, like so:

Code: Select all

wxThreadEvent* tevent = new wxThreadEvent(wxEVT_THREAD, 100);
		if (tevent != nullptr)
		{
			wxQueueEvent(wxGetApp().mainWindow, tevent->Clone());
		}
and then I have this function:

Code: Select all

void MainWindow::ThreadMessage(wxThreadEvent& event)
{
	int id = event.GetId();
	if(id == 100) { /* do something */ }
}
	
but I just realized that this wxThreadEvent is most likely never destroyed, so that's a leak right there 😥
Is there a better way of handling this?
ONEEYEMAN wrote: Wed Mar 17, 2021 5:24 pm Hi,
For the login window I would use wxDialog-derived class.
Just have ID and password as wxTextCtrl and 2 buttons: "OK" and "Cancel".

Also, stylistically: you don't have to use wxGetApp() call every time something happens. It's weird and un-neccessary.
Look how tjhings is done in the "minimal" sample and ALWAYS, ALWAYS, ALWAYS look at the sample to see how to do what you want.

Thank you.
sorry just saw your message, I made it as a frame because I was using wxFormBuilder to make my life easier, I thought it would be more appropriate, but I guess you are right, a dialog makes more sense. Although I am not sure if I will be able to style it as I want, but I will give it a go nonetheless! As for the wxGetApp() I use it to as a way to access some global variables, like the main window and the login window
User avatar
doublemax
Moderator
Moderator
Posts: 19116
Joined: Fri Apr 21, 2006 8:03 pm
Location: $FCE2

Re: Memory leaks concern

Post by doublemax »

ut I just realized that this wxThreadEvent is most likely never destroyed, so that's a leak right there 😥
Is there a better way of handling this?
Yes, don't clone the event, QueueEvent takes ownership of the pointer and will destroy it later.
Use the source, Luke!
DjSt3rios
Earned a small fee
Earned a small fee
Posts: 17
Joined: Thu Mar 11, 2021 4:41 pm

Re: Memory leaks concern

Post by DjSt3rios »

doublemax wrote: Wed Mar 17, 2021 5:41 pm
ut I just realized that this wxThreadEvent is most likely never destroyed, so that's a leak right there 😥
Is there a better way of handling this?
Yes, don't clone the event, QueueEvent takes ownership of the pointer and will destroy it later.
Oh I see, thank you! I found 17 of these cases, so 17 leaks gone 😌 I will keep digging to see what I can find. In your opinion, should switching between the 2 windows have a constant amount of memory usage? (Now I just hide/show these windows, I do not recreate them)
User avatar
doublemax
Moderator
Moderator
Posts: 19116
Joined: Fri Apr 21, 2006 8:03 pm
Location: $FCE2

Re: Memory leaks concern

Post by doublemax »

should switching between the 2 windows have a constant amount of memory usage?
The memory usage should not change.
Use the source, Luke!
DjSt3rios
Earned a small fee
Earned a small fee
Posts: 17
Joined: Thu Mar 11, 2021 4:41 pm

Re: Memory leaks concern

Post by DjSt3rios »

doublemax wrote: Wed Mar 17, 2021 6:42 pm
should switching between the 2 windows have a constant amount of memory usage?
The memory usage should not change.
Thank you very much. I will try to find a tool to help me find these leaks
sparhawk
Experienced Solver
Experienced Solver
Posts: 81
Joined: Tue May 21, 2013 8:08 am

Re: Memory leaks concern

Post by sparhawk »

doublemax wrote: Wed Mar 17, 2021 4:50 pm I'm not 100% sure whether all toplevel windows get automatically destroyed if the app terminates, but if not and loginWindow is not NULL, this will leak memory as the old instance will not get destroyed.
In my OnExit handler, I just close the mainframe and I don't get any memory leaks reported, so this should be enough. However, in the OPs code the loginwindow has a nullptr as the parent, so it will not automatically destroyed. So if the app closes, and the login window is open, then it could create a memory leak, but this should not really be a problem, because the OS would clean up anyway if the app exits.
ONEEYEMAN
Part Of The Furniture
Part Of The Furniture
Posts: 7459
Joined: Sat Apr 16, 2005 7:22 am
Location: USA, Ukraine

Re: Memory leaks concern

Post by ONEEYEMAN »

Hi,
DjSt3rios wrote: Wed Mar 17, 2021 5:28 pm
doublemax wrote: Wed Mar 17, 2021 5:08 pm First you check this part:

Code: Select all

wxGetApp().loginWindow = new LoginWindow(nullptr);
wxGetApp().loginWindow->Show();
I'm not 100% sure whether all toplevel windows get automatically destroyed if the app terminates, but if not and loginWindow is not NULL, this will leak memory as the old instance will not get destroyed.
Okay so I added a check to check if it's a null pointer, and only then create a new window, otherwise restore the existing window, but I think I found my problem. Sometimes I have trouble calling functions from callbacks, I assume it was a multi-threading issue, so what I did was create an wxThreadEvent, like so:

Code: Select all

wxThreadEvent* tevent = new wxThreadEvent(wxEVT_THREAD, 100);
		if (tevent != nullptr)
		{
			wxQueueEvent(wxGetApp().mainWindow, tevent->Clone());
		}
and then I have this function:

Code: Select all

void MainWindow::ThreadMessage(wxThreadEvent& event)
{
	int id = event.GetId();
	if(id == 100) { /* do something */ }
}
	
but I just realized that this wxThreadEvent is most likely never destroyed, so that's a leak right there 😥
Is there a better way of handling this?
ONEEYEMAN wrote: Wed Mar 17, 2021 5:24 pm Hi,
For the login window I would use wxDialog-derived class.
Just have ID and password as wxTextCtrl and 2 buttons: "OK" and "Cancel".

Also, stylistically: you don't have to use wxGetApp() call every time something happens. It's weird and un-neccessary.
Look how tjhings is done in the "minimal" sample and ALWAYS, ALWAYS, ALWAYS look at the sample to see how to do what you want.

Thank you.
sorry just saw your message, I made it as a frame because I was using wxFormBuilder to make my life easier, I thought it would be more appropriate, but I guess you are right, a dialog makes more sense. Although I am not sure if I will be able to style it as I want, but I will give it a go nonetheless! As for the wxGetApp() I use it to as a way to access some global variables, like the main window and the login window
As I said - you should look at the minimal sample and see how everything is done.

You just have an extra code there.

Also - VERY IMPORTANT! - youj shouldn't have any global variables besides wxApp-based object in your code (ig its wx one).

Thank you.
DjSt3rios
Earned a small fee
Earned a small fee
Posts: 17
Joined: Thu Mar 11, 2021 4:41 pm

Re: Memory leaks concern

Post by DjSt3rios »

ONEEYEMAN wrote: Wed Mar 17, 2021 10:16 pm Hi,
DjSt3rios wrote: Wed Mar 17, 2021 5:28 pm
doublemax wrote: Wed Mar 17, 2021 5:08 pm First you check this part:

Okay so I added a check to check if it's a null pointer, and only then create a new window, otherwise restore the existing window, but I think I found my problem. Sometimes I have trouble calling functions from callbacks, I assume it was a multi-threading issue, so what I did was create an wxThreadEvent, like so:

Code: Select all

wxThreadEvent* tevent = new wxThreadEvent(wxEVT_THREAD, 100);
		if (tevent != nullptr)
		{
			wxQueueEvent(wxGetApp().mainWindow, tevent->Clone());
		}
and then I have this function:

Code: Select all

void MainWindow::ThreadMessage(wxThreadEvent& event)
{
	int id = event.GetId();
	if(id == 100) { /* do something */ }
}
	
but I just realized that this wxThreadEvent is most likely never destroyed, so that's a leak right there 😥
Is there a better way of handling this?
ONEEYEMAN wrote: Wed Mar 17, 2021 5:24 pm Hi,
For the login window I would use wxDialog-derived class.
Just have ID and password as wxTextCtrl and 2 buttons: "OK" and "Cancel".

Also, stylistically: you don't have to use wxGetApp() call every time something happens. It's weird and un-neccessary.
Look how tjhings is done in the "minimal" sample and ALWAYS, ALWAYS, ALWAYS look at the sample to see how to do what you want.

Thank you.
sorry just saw your message, I made it as a frame because I was using wxFormBuilder to make my life easier, I thought it would be more appropriate, but I guess you are right, a dialog makes more sense. Although I am not sure if I will be able to style it as I want, but I will give it a go nonetheless! As for the wxGetApp() I use it to as a way to access some global variables, like the main window and the login window
As I said - you should look at the minimal sample and see how everything is done.

You just have an extra code there.

Also - VERY IMPORTANT! - youj shouldn't have any global variables besides wxApp-based object in your code (ig its wx one).

Thank you.
Thank you for your input. So should I pass a pointer for my windows to each other and interact with them that way? One thing I noticed in the minimal sample is that there is no deconstructor, I guess because when the window closes, the application terminates and frees all memory
ONEEYEMAN
Part Of The Furniture
Part Of The Furniture
Posts: 7459
Joined: Sat Apr 16, 2005 7:22 am
Location: USA, Ukraine

Re: Memory leaks concern

Post by ONEEYEMAN »

Hi,
Yes, no destructor is needed.
However, I'm not sure what you are asking.

Every window/control has to have a parent, whether its a Desktop (in which case the parent window would be NULL) or another window (in which case you pass its pointer to a constructor).

Check both minimal and widgets samples for an example how to work with different controls.

And so when the parent window is deleted - all children of that window will be deleted as well. (It will work just like smart pointers).
This is the reason why wx don't use smart pointers in their code and do not recommends to use them in the user code. It is already done.

Also, I'm not sure what do you mean by saying "interact with them that way".

Can you explain in layman terms what you program should do?

Thank you.
DjSt3rios
Earned a small fee
Earned a small fee
Posts: 17
Joined: Thu Mar 11, 2021 4:41 pm

Re: Memory leaks concern

Post by DjSt3rios »

So I have a class containing user information, such as access token, refresh token (I use them for API calls), name etc. I also have a class with functions that makes call to the API server, and what I did was to store a pointer in the WxApp for easy access from all my windows, those being my Login window and the Main window. For example my API class is needed in both windows, should I make 2 separate objects? or make them static? I also interact between the Login window and main window, in case of a logout or login, I hide one window and show another, and I was checking in the wxApp if login window is valid, if not I created a new one. I guess I should pass a reference to the login window once I create the main window, so I don't have any variables at all in wxApp. I also handled the window closure, if one closes, the application exits, even if there is a hidden one, so it works ok for now, but I guess I need to optimize it more
Post Reply