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

[Chris Shoemaker]

On Thu, Jun 02, 2005 at 04:01:27PM -0400, Derek Atkins wrote:
> Quoting Chris Shoemaker <c.shoemaker at cox.net>:
> 
> > On Thu, Jun 02, 2005 at 01:44:29PM -0400, Derek Atkins wrote:
> > > Quoting Chris Shoemaker <c.shoemaker at cox.net>:
> > > 
> > > > Thanks for explaining.  I haven't looked at the details, but shouldn't
> > > > destroy signal handler just generate the right CM event, and then the
> > > > CM close handler for the cw structure actually frees cw.  I thought
> > > > that was the intended use of the CM.
> > > 
> > > Yes, but all of that can happen before the gtk_widget_destroy() function
> > > returns.
> > > 
> > 
> > hmmm, I must be missing something.  Only a month without hacking GC
> > and I'm confused already.  Time to crack the source open again.  I've
> > been meaning to brush the dust off my budget patches anyway...
> 
> gtk_widget_destroy() "emits" the signal which calls the signal handler.  The
> signal handler sets emits the CM event which calls the dialog CM destroy
> handler which destroys the dialog context.   

Yes.  That's how it should work, but that's not what the code does at all.

1) The gtk_destroy signal handler (gnc_customer_window_destroy_cb) does
NOT emit the CM event.  
(It should be calling gnc_close_gui_component(cw->component_id);)

2) The CM destroy handler (gnc_customer_window_close_handler) does NOT
destroy the dialog context.  Instead, that's actually done inside the
gtk_destroy signal handler.


The biggest hint that this is an abuse of the CM api is the fact that
the close handler is not explicitly unregistering the component with
the CM, instead relying on it happening as a side-effect of the
gtk_destroy signal.  If the close handler wanted to do anything with
the component after it destroyed the dialog (like, say, read the
non-gui state) it would be screwed because the whole component has
been freed!

I think all the confusion goes away when you do something like this:
[warning: not even compile tested]

// This is the gtk_destroy signal handler
void
gnc_customer_window_destroy_cb (GtkWidget *widget, gpointer data)
{
  CustomerWindow *cw = data;

  gnc_close_gui_component(cw->component_id);

}

// This is the CM close handler
static void
gnc_customer_window_close_handler (gpointer user_data)
{
  CustomerWindow *cw = user_data;
  GncCustomer *customer = cw_get_customer (cw);

  gnc_suspend_gui_refresh ();

  if (cw->dialog_type == NEW_CUSTOMER && customer != NULL) {
    gncCustomerBeginEdit (customer);
    gncCustomerDestroy (customer);
    cw->customer_guid = *xaccGUIDNULL ();
  }

  gnc_unregister_gui_component (cw->component_id);

  gtk_widget_destroy (cw->dialog);
  cw->dialog = NULL;
  g_free (cw);

  gnc_resume_gui_refresh ();
}

Makes a heck of a lot more sense to me.  David?

-chris