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

[Derek Atkins]

Quoting Chris Shoemaker <c.shoemaker at cox.net>:

> On Thu, Jun 02, 2005 at 05:29:06PM -0400, David Hampton wrote:
> > On Thu, 2005-06-02 at 16:59 -0400, Chris Shoemaker wrote:
> > > 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?
> > 
> > I think this will work because gtk_widget_destroy correctly handles
> > recursive invocation.  If the widget is already in the process of being
> > destroyed then the call to gtk_widget_destroy from
> > gnc_customer_window_close_handler is a noop, otherwise it will actually
> > destroy the dialog.
> 
> That makes sense since "gtk_widget_destroy" is supposed to mean "break
> all your references".  If they're already broken, re-calling does
> nothing.  
> 
> I'm not sure if the destroy-signal will get re-delivered in the
> recursive case, but I would hope that it wouldn't matter since I would
> expect the CM to ignore any events after the un-registration, whether
> the events are cached and then discarded because they arrived after
> the unregistration, or were discarded immediately because the
> component had already been unregistered.

It all depends which API gets called first.  If the user clicks the window
manager close button, it winds up calling gtk_widget_destroy() first, which
partially destroys the widget and calls the callback, which would call the CM,
which calls the CM callback, which would re-call the gtk_widget_destroy().

Basically, we need to handle re-entrancy in BOTH apis.  They are necessarily
tied together, which is why I don't consider it bad that we're assuming a
side-effect.

> -chris

-derek

-- 
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       warlord at MIT.EDU                        PGP key available