Opened 13 years ago

Closed 13 years ago

Last modified 13 months ago

#8617 closed patch (fixed)

Patch for minor memory leak

Reported by: SF/youngelf Owned by: fingolfin
Priority: low Component: GUI
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)

ListWidget.cpp.patch (545 bytes ) - added by SF/youngelf 13 years ago.
Patch against the latest 0.9.1 source tree.
ListWidget-patch.cpp (615 bytes ) - added by SF/youngelf 13 years ago.
Patch for gui/ListWidget.cpp against the recent svn release

Download all attachments as: .zip

Change History (15)

by SF/youngelf, 13 years ago

Attachment: ListWidget.cpp.patch added

Patch against the latest 0.9.1 source tree.

comment:1 by SF/youngelf, 13 years ago

Priority: normallow

comment:2 by sev-, 13 years ago

Owner: set to sev-
Status: newpending

comment:3 by sev-, 13 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 SF/youngelf, 13 years ago

Attachment: ListWidget-patch.cpp added

Patch for gui/ListWidget.cpp against the recent svn release

comment:4 by SF/youngelf, 13 years ago

Status: pendingnew

comment:5 by SF/youngelf, 13 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 fingolfin, 13 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 fingolfin, 13 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 SF/youngelf, 13 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 fingolfin, 13 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 fingolfin, 13 years ago

Owner: changed from sev- to fingolfin

comment:11 by fingolfin, 13 years ago

Resolution: fixed
Status: newclosed

comment:12 by fingolfin, 13 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 digitall, 13 months ago

Component: GUI
Note: See TracTickets for help on using tickets.