Обсуждение: PGA3 Memory Leaks

Andreas,

You made this change around line 347 in frmMain.cpp:

/*
    // Keith 2003.03.05
    // Fixed memory leak -- These are not destroyed automatically
       // Andreas 2003-04-08 yes they are, cascaded through the splitter!
        // GTK won't like explicit deletes.
    delete treeContextMenu;
    delete browserImages;
    delete statisticsImages;
    delete propertiesImages;
    delete statistics;
*/

I'm sorry, but this is incorrect.

1) Treecontextmenu does not get deleted automatically. It is a context menu,
and it never gets attached to the window. See the docs for wxWindow:

"Just before the menu is popped up, wxMenu::UpdateUI is called to ensure
that the menu items are in the correct state. The menu does not get deleted
by the window."

2) Imagelists do not get deleted automatically. It says that specifically in
the documentation for wxTreeCtrl.

"Sets the normal image list. Image list assigned with this method will not
be deleted by wxTreeCtrl's destructor, you must delete it yourself."

3) Statistics does not appear to need to be deleted.

Therefore, I've undone the change.

-Keith

efesar wrote:

>Therefore, I've undone the change.
>
>-Keith
>
>
Keith,
PLEASE undo your undos. The code will CRASH on GTK before the settings
are written. I didn't go into detail which line is the offending one.
Memory leaks on program exit is a waste of brain in my opinion. The
operating system will do all cleanup for us.
Since there are no trees, menues and so on destroyed and dynamically
recreated, we don't need to care what's happening to those at the time
of program end. And at last: I wouldn't have touched that code if it worked!

Regards,
Andreas

> PLEASE undo your undos. The code will CRASH on GTK before the settings
> are written. I didn't go into detail which line is the offending one.
> Memory leaks on program exit is a waste of brain in my opinion. The
> operating system will do all cleanup for us.
> Since there are no trees, menues and so on destroyed and dynamically
> recreated, we don't need to care what's happening to those at the time
> of program end. And at last: I wouldn't have touched that code if
> it worked!

Andreas,

The wxWindows documentation specifically states those objects must be
deleted.

The one line I didn't uncomment was "delete statistics." Since statistics is
a wxListCtrl, it probably doesn't need to be deleted. That's why I left it
commented. Please test and document. It's possible your crash was caused by
that.

In the meantime, I think the change should stay. IMHO, memory leaks are not
wastes of time.

Dave -- Opinion? I'll leave this one up to you, it that's okay.

-Keith

efesar wrote:

>Andreas,
>
>The wxWindows documentation specifically states those objects must be
>deleted.
>
>The one line I didn't uncomment was "delete statistics." Since statistics is
>a wxListCtrl, it probably doesn't need to be deleted. That's why I left it
>commented. Please test and document. It's possible your crash was caused by
>that.
>
>In the meantime, I think the change should stay. IMHO, memory leaks are not
>wastes of time.
>
>Dave -- Opinion? I'll leave this one up to you, it that's okay.
>
>
>
Keith,

I had the crash on delete statistics. If you like to trace that on-exit
leaks, I don't mind as long as they don't cause double deletion as in
the case of statistics.
Don't get me wrong, I appreciate that we have somebody who's eager to
locate memory leaks, these things happen when producing code and should
be eliminated. It's just a difference whether that orphaned memory is a
db connection created all over and over again, or some bytes that get
dumped by the os anyway.

Regards,
Andreas

> -----Original Message-----
> From: Andreas Pflug [mailto:Andreas.Pflug@web.de]
> Sent: 16 April 2003 22:07
> To: efesar; pgadmin-hackers@postgresql.org
> Subject: Re: [pgadmin-hackers] PGA3 Memory Leaks
>
>
> efesar wrote:
>
> >Andreas,
> >
> >The wxWindows documentation specifically states those
> objects must be
> >deleted.
> >
> >The one line I didn't uncomment was "delete statistics." Since
> >statistics is a wxListCtrl, it probably doesn't need to be deleted.
> >That's why I left it commented. Please test and document.
> It's possible
> >your crash was caused by that.
> >
> >In the meantime, I think the change should stay. IMHO,
> memory leaks are
> >not wastes of time.
> >
> >Dave -- Opinion? I'll leave this one up to you, it that's okay.
> >
> >
> >
> Keith,
>
> I had the crash on delete statistics.

I'm all for fixing memory leaks whereever possible - particularly when
the docs tell us we should delete things. If there is a specific problem
because the docs are wrong, or something doesn't work quite right, then
we can take the time to figure out exactly the problem rather than just
removing blocks of code.

pgAdmin II, particularly the 1.2 release was amazingly (to me at least)
bug free because I took the time to design, plan and write it properly.
You guys are both doing an excellent job of keeping up that tradition
with pgAdmin III so far - Keith because you've auditted code on at least
a couple of occasions to check for leaks and other potential problems,
and Andreas because you've take the time to properly consider the design
and operation of the schema objects which, frankly, I hadn't.

So, let's keep this way of working up and we should have a killer app
that will hopefully end up being bundled with some of the binary
PostgreSQL distributions.

:-)

Regards, Dave.