#8617 closed patch (fixed)
Patch for minor memory leak
| Reported by: | SF/youngelf | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | low | Component: | GUI |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
Hi, I'm running ScummVM under valgrind, primarily to learn how to fix memory errors. This is the first of my finds and fixes. I would like to continue fixing memory leaks, so please accept this patch. (Copyright signed over to the ScummVM project.)
If this is too much trouble, please let me know.
Patch for gui/ListWidget.cpp included.
Vikram Aggarwal vikram@mayin.org
Ticket imported from: #1681843. Ticket imported from: patches/722.
Attachments (2)
Change History (15)
by , 18 years ago
| Attachment: | ListWidget.cpp.patch added |
|---|
comment:1 by , 18 years ago
| Priority: | normal → low |
|---|
comment:2 by , 18 years ago
| Owner: | set to |
|---|---|
| Status: | new → pending |
comment:3 by , 18 years ago
Unfortunately there is no way to accept your patch since we do not work with 0.9.1. 0.9.1 is based on a branch which was forked in June 2006 last year, so that is a very old codebase.
So, please, get latest SVN source and check with that. If the leak is still present there then we will gladly accept new patch.
by , 18 years ago
| Attachment: | ListWidget-patch.cpp added |
|---|
Patch for gui/ListWidget.cpp against the recent svn release
comment:4 by , 18 years ago
| Status: | pending → new |
|---|
comment:5 by , 18 years ago
Hi, I just checked out the latest svn trunk. This bug is still around. New patch attached. Thanks a lot. File Added: ListWidget-patch.cpp
comment:6 by , 18 years ago
The proper fix for this issue is to modify the GuiObject class, by adding a destructor like this:
~GuiObject() { delete _firstWidget; _firstWidget = 0; }
At least in theory (and removing similar code from the Dialog destructor).
comment:7 by , 18 years ago
To clarify: Adding this delete to the constructor just like that won't work, it'll cause double frees. But it would be the "more natural" solution, considering that we use the chained approach for widgets which are directly contained in a dialog (as children of that dialog, too. It would make sense to use it for widgets which are children of other widgets, too.
But this needs careful investigation. Maybe you are interested in looking into this? (for starters, I just added the destructor to a new file gui/object.cpp, but commented out the delete).
And finally: Fixing memory leaks is *highly* welcome. Please do not think we dislike your work just because I always want the "best" fix (whatever that means :). I guess until we have a "proper" fix we should just commit your patches, as they clearly fix bugs in the code.
comment:8 by , 18 years ago
Hi,
I understand your concern, and it is worth putting in extra effort to do it right, the first time around. Yes, I'm definitely interested in looking into this more thoroughly. I was also interested in the memory accesses by the different game engines, so it was just a matter of time before I started reading the source code.
Considering that I'm new to the codebase, any pointers would be welcome. I'll start by getting a hang of GuiObject, and all the widgets that are its children, to understand whats involved.
Thanks a lot.
comment:9 by , 18 years ago
Feel free to drop by on our IRC channel (#scummvm on irc.freenode.org) to discuss details or ask questions.
GuiObject is the base for the classes Dialog and Widget, see also here: <http://scummvm.org/docs/doxygen/html/classGUI_1_1GuiObject.php>.
Originally, only Dialogs could have Children, but at some point it turned out to be useful to let Widgets be children of other widgets (e.g. the TabWidget and the ListWidget make use of this feature). But when I did that, I apparently forgot about the memory managment part *sigh*. Bad bad fingolfin :-(
comment:10 by , 18 years ago
| Owner: | changed from to |
|---|
comment:11 by , 18 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
comment:12 by , 18 years ago
I fixed the GUI code to properly delete all children of all GuiObjects (I hope), this should in particular fix the leak described here.
comment:13 by , 6 years ago
| Component: | → GUI |
|---|
Patch against the latest 0.9.1 source tree.