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

[Chris Shoemaker]

On Fri, Jun 03, 2005 at 02:16:46PM -0400, David Hampton wrote:
> 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.

I can sympathize.

> 
> > 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.

That question is not really relevant.  "should result in" is correct,
but it means indirectly as a result of ref count dropping to 0.  Look
at the api docs for the gtk_object_destroy() function:

    Emits the "destroy" signal notifying all reference holders that
    they should release the GtkObject... The memory for the object
    itself won't be deleted until its reference count actually drops to 0;
    gtk_object_destroy() MERELY ASKS reference holders to RELEASE THEIR
    REFERENCES, IT DOES NOT FREE THE OBJECT.

Emphasis mine.  It's not very ambiguous.

> 
> > (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.

That's right, because we don't have to worry about finalization.  It
will happen automatically when refcount == 0.  All we have to do
correctly is call object_ref when we save a reference to the dialog,
(and unref when we release, of course.)

> 
> > 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.

I certainly don't mean to be unfair, and I apologize if Derek thinks I
was.  Derek didn't say it was a good design, but it's pretty hard to
get him to say anything in GC is a _bad_ design.  I really think that
he's just spent so long with GC code, that almost everything there
looks reasonable to him, even stuff that, IMHO, isn't very reasonable.

> 
> > 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.

Granted.

<snip>
> 
> 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.  

All good points.

> 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.

That's not really true.  "Offense" is too strong; It'd be more
accurate to say that I'm "concerned" that he doesn't agree with me
that this is a design issue _at_all_, and furthermore, (actually this
is more of the real concern:) that this is just one example
representative of a rather long list of (IMO) design "weaknesses"
about which we disagree.  


> 
> 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.  

That's a refreshing attitude.

<snip comments on ref holding>
> 
> 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.

Agreed on all counts.

-chris