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

[Chris Shoemaker]

On Thu, Jun 02, 2005 at 10:11:11PM -0400, Derek Atkins wrote:
> 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().
                                                       ^^^^^^^^^^^^^^^^^^

Um, yeah, but my paragraph above was pondering what happens after all
that.  David said that gtk_widget_destroy would block the reentance,
but I didn't know if that also means block the re-delivery of the
signal after the last step in the sequence you just described.

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

Well, hopefully we can leave worrying about gtk_destroy re-entrency to
the gtk developers.  What we need to do is use the signal as it's
meant to be used.  The "destroy" signal handler should break external
references.

IMO, making the GTK destroy signal mean "free my component struct",
while making the ComponentManager's close signal mean "drop the
dialog's references" _is_ bad.  I recognize that it probably doesn't
bother you, but that's just because you're so intelligent.  Average
developers like me really benefit from semantic consistency.  :)

-chris