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

[Chris Shoemaker]

On Fri, Jun 03, 2005 at 09:14:19AM -0400, Derek Atkins wrote:
> Quoting Chris Shoemaker <c.shoemaker at cox.net>:
> 
<snip>
> 
> I really don't think there's a bug here...  There was a use-after-free but it
> was from the context, not the widget.  Hampton fixed that.  I really don't
> think we need to re-architect the rest of the code.  It works just fine, and
> all the rest of gnucash uses this same model:
> 
> CM close callback tells the widget to destroy itself
> Gtk Destroy callback frees the context and clears the references.
> 
> 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.  The existing
pattern is bad for many reasons.

1) It's an abuse of the documented gtk destroy api, which is only
suppose to break references, NOT free anything.  (Freeing is handled
by finalize, and only when refcount = 0.)

2) High coupling: currently, the destroy handler takes the cw as
user-data and can pillage all the fields of the cw struct before
finally freeing something that doesn't even depend on the dialog.  In
the correct usage of destroy, it should only receive the component_id
through userdata -- just enough to break external references, no more,
no less -- low coupling.

3) Low cohesion: currently, the destroy handler for the dialog has to
know tons about the cw struct and about the CM, including CM
unregistration.  Likewise, the close handler has to know about all the
side-effects of the destroy handler.  In a high cohesion design, the
destroy handler only has to know how to break the single external
reference, and the close callback rightly accesses the data fields for
the component that generated the event.  The close handler doesn't
have to know any more about the destroy signal than what I can read in
the GTK docs: it breaks refs.

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.  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, and especially when those proposals don't require
any action on your part.  

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.

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

Just a thought.

-chris