nautilus-throbber / BonoboEventSource refcounting

[Owen Taylor <otaylor redhat com>]

I spent a chunk of time last night debugging why the throbber process
was often crashing for me when I closed windows; I have the problem
pretty well tracked down, but the best solution(s) is somewhat 
less clear.


Here's what I found:

When a Nautilus window is closed, it is first unrealized, which
results in GTK+ peer of the embedded control being sent a
WM_DELETE_EVENT, and thus, destroyed. (GtkWidget::destroy)

When the GTK+ peer is destroyed, nautilus_throbber_destroy() calls

 nautilus_bonobo_object_force_destroy_at_idle (throbber->details->control);

Which, results in the refcount the control being forced to zero when
control returns to the main loop; the resulting call to
bonobo_control_destroy() unrefs the control's property bag, which as a
consequence is also destroyed along with the aggregate EventSource
object.

bonobo_event_source_destroy() makes the assumption that no more
incoming calls will occur at that point - it walks the listener list
in a non-reentrant fashion and leaves the object in a junk state
(event_source->priv pointing to garbage.)

However, at this point, the event source object has NOT been
deactivated - this does not occur until the event source object
is finalized, which is a subsequent stage, so incoming 
calls CAN happen. (with the forced destroy)

In fact, at the same time, the nautilus process has run along
to nautilus_window_destroy(), which does:

===
        if (window->throbber != CORBA_OBJECT_NIL) {
                CORBA_exception_init (&ev);
                property_bag = Bonobo_Control_getProperties (window->throbber, &ev);
                if (!BONOBO_EX (&ev) && property_bag != CORBA_OBJECT_NIL) {     
                        bonobo_event_source_client_remove_listener
                                (property_bag,
                                 window->details->throbber_location_change_request_listener_id,
                                 &ev);
                        bonobo_object_release_unref (property_bag, NULL);       
                }
                CORBA_exception_free (&ev);

                bonobo_object_release_unref (window->throbber, NULL);
        }
===

Which, in my tests usually gets serviced by the throbber in call to
Bonobo::Unknown::unref() in bonobo-event-source.c:desc_free().
resulting in a segfault when the above code calls
Bonobo::EventSource::removeListener.


So, that's the bug; what can be done to fix it?

 1) Remove the call to nautilus_bonobo_force_destroy_at_idle(); the
    root cause here is that the control thinks that nobody has a
    reference to it, while on the other hand, Nautilus thinks it has a
    reference to the control.

    Yet, I'm not sure this is right. Distributed reference counting is
    distinctly imperfect, and using the firm knowledge that the GTK+
    peer has been destroyed to decide that the control should be
    destroyed is not an unreasonable measure, despite the obvious and
    clear evil-ness of releasing someone else's refcount.

 1a) Change nautilus_bonobo_force_destroy_at_idle() to
    nautilus_bonobo_force_destroy_later() with a timeout of say, 1
    minute. This should still catch stale controls without being
    nearly as vulnerable to race conditions.

 2) Deactivate objects as the first step in destroying them, not when
    they are finally destroyed. (In fact, you'd probably 
    want to deactivate all objects in an aggregate before starting
    to destroy the aggregate)

    I don't quite get how Bonobo is handling the distinction between
    destroy and finalize in Bonobo, but there seems to be a general
    idea that destruction is singular and results in an inoperable
    object, so deactivation should generally be OK.

    Deactivating doesn't work if destroy functions are allowed to
    intentionally pass references to themselves back out again, and
    then take incoming calls, but that seems like a dubious practice
    at best.

 3) Move the call to destroy the throbber in NautilusWindow to an
    unrealize function so it gets executed prior to the child widgets
    being unrealized. While this fixes the problem in a simple way,
    it's not an obvious change, and I tend to suspect similar problems
    occur elsewhere. So some combination of the above steps is
    probably a good idea.

Regards,
                                        Owen


[ There are various additional solutions involving changing
  the destroy functions for Bonobo::Control and Bonobo::EventSource
  to be more reentrant, but unless subsequent incoming calls are 
  really intended to happen, such changes don't seem appropriate. ]