[Gnucash-changes] Eliminate a double free of memory.

[David Hampton]

On Fri, 2005-06-03 at 11:11 -0400, Chris Shoemaker wrote:
> On Fri, Jun 03, 2005 at 09:14:19AM -0400, Derek Atkins wrote:
> > So I don't think we need to do anything.  What we've got works just fine (modulo
> > the bug that hampton fixed already).
> 
> It's better to design out a bug than to patch it out. 

Normally I would agree with you.  In this particular case, I tried to do
that three years ago and ended up throwing up my hands and declaring
that I had better things to do.

> 1) It's an abuse of the documented gtk destroy api, which is only
> suppose to break references, NOT free anything.

We're reading the documentation differently.

    Because these references are released, calling gtk_object_destroy()
    should result in FREEING ALL MEMORY ASSOCIATED WITH AN OBJECT,
    unless some buggy code fails to release its references in response
    to the "destroy" signal.

Emphasis mine.  This begs the question of whether the gnucash data
structure that is intimately tied to the dialog is considered memory
associated with an object.  As an object implementor, I'd say no.  As a
gnucash developer, I'd say yes, since the majority of the fields in the
data structure point to widgets within the dialog, and all of these
pointers will be invalidated when the dialog is destroyed.

> (Freeing is handled by finalize, and only when refcount = 0.)

Finalize is not a signal, its a gobject class virtual function.  Its
only available to object implementors, not code that uses objects that
were implemented elsewhere.  If you want to use finalize here then
you'll need to subclass a GtkWindow.

> BTW, I'm _really_ not a design nazi.  I just _look_ like one next to
> you.  :) You seem to think that anything in GC that works must be a good
> design.

That's unfair Chris.  I don't think I've ever heard Derek say that its a
good design, only that it works so why change it.

> You might try being a bit more open-minded about proposals
> for design improvements, especially coming from people who've proven a
> willingness to code, 

Also unfair.  I'd say Derek's probably the main person who has kept this
project going for the last couple of years.

> and especially when those proposals don't require
> any action on your part.  

Not true.  It needs to be evaluated to see if its a good design, whether
it will work in gnucash, etc.   Derek's already pointed out a potential
timing issue neither of us saw.

> If you think a proposal is really bad, try to make _constructive_
> recommendations for improvement, (GC can always be improved, you know)
> and not be so disparaging.  I'm seeing that this "we've always done it
> that way; we do that everywhere; it works; we don't need to change
> anything... etc." attitude is a disturbing and recurring pattern.

Lets see.  Derek did the following:

1) Pointed out that events may be turned off in the CM so that the close
function is called long after the gtk_widget_destroy function has
finished.  I had completely forgotten it might do that and was assuming
all the calls were synchronous.

2) Pointed out that other windows/dialogs have the same code.  My guess
would be that they all do.

3) Stated that he doesn't think its a bug, and that we don't need to re-
architect the code.

The first is a crucial point for designing a solution.  The second is
pointing out the scale of the problem.  The third is his viewpoint on
the issue.  You've clearly taken offense that he doesn't agree with you
that this is a heinous design issue that needs to be fixed immediately.

While I agree that this component manager/gtk interaction code isn't the
best written code, its served it purpose over the last five years.  I
think there are more important things to fix and ideas to implement, but
if this is what you want to work on, more power to you.  Its not a
simple problem.  I don't know if your idea of hanging onto an extra ref
on the dialog will work, because I don't know how the window manager
tells gnucash to destroy the dialog.  If this communication results in a
call to g_object_unref, then you'll never get the destroy callback
because the refcount will now never go to zero.  If it results in a
direct call to gtk_widget_destroy, does the ref really give you
anything.  If the destroy function has already run to completion then
all the child widgets are gone, all you have left is a pointer to an
empty GtkWindow object, and all the other pointers in the data structure
are still invalid because the widgets they point to are gone.

I can't speak for Derek, but I'm willing to look at any changes you
propose for this area.  They just have to be thoroughly tested (e.g.
normal close, window manager close, CM close, application killed, etc.)
before I will spend much time on them.  I'm still overwhelmed with gtk2
changes/cleanups, and I've barely even cracked the new "Human Interface
Guidelines" document.

> Maybe being more honest about GC's design, ehm, weaknesses, would
> encourage more developer participation.

Sure, gnucash has design weaknesses.  I'm not sure this should be
considered a big one.  I'd be more concerned about the code paths that
have multiple nested calls from C to Scheme to C, or the fact that half
of the startup logic is in Scheme, or that the register is one giant
hairy mess of a Gnome Canvas studded with hand drawn boxes linked to an
off-screen GtkEntry with some of the most twisted and tortured logic
I've seen.  I'm sure I can come up with more.

> Just a thought.

My two bits.

David