[Bug 40283] Crash when a client needs recovery

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=40283

--- Comment #9 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2011-09-20 03:44:07 PDT ---
> By reading the code, it could also fail at
> presenting the channel if the handler claimed it from observer/approver

Regression test or it didn't happen :-)

> +static McdClientProxy *
> +_mcd_dispatcher_lookup_handler (McdDispatcher *self,
> +                                TpChannel *channel,
> +                                McdRequest *request)

What this function does is subtle enough that it deserves a doc-comment,
something like:

    /*
     * @channel: a non-null Channel which has already been dispatched
     * @request: (allow-none): if not NULL, a request that resulted in
     *     @channel
     *
     * Return a Handler to which @channel could be re-dispatched,
     * for instance as a result of a re-request or a PresentChannel call.
     *
     * If @channel was dispatched to a Handler, return that Handler.
     * Otherwise, if it was Claimed by a process, and that process
     * has a Handler to which the channel could have been dispatched,
     * return that Handler. Otherwise return NULL.
     */

I think it'd be clearer what this function does if you inlined
mcd_dispatcher_dup_possible_handlers into it, at which point you'd discover
that going from a list of Handlers to a list of well-known names and back to
the first Handler was pointless, and you could just use the first element of
the list returned by _mcd_client_registry_list_possible_handlers.

I think the doc-comment I suggested makes it clear that this is the right
function to call from dispatcher_present_channel(), and the right function to
call from _mcd_dispatcher_reinvoke_handler(). So making it into its own
function is a good bit of refactoring.

I'm still not at all sure that it's the right solution to
mcd_dispatcher_client_needs_recovery_cb() - see Comment #5 and Bug #40305 - but
that's not a regression. Please at least add a FIXME comment pointing to Bug
#40305 (and perhaps also Bug #30043 to get the desired semantics pinned down).

> + /* Failing that, maybe the Handler it was dispatched to was temporary;

Failing what? For it to make sense, you need another comment for this to
continue from, something like the one you deleted from reinvoke_handler:

> if (well_known_name != NULL)
>   {
>     /* We know which Handler well-known name was responsible: if it
>      * still exists, we want to call HandleChannels on it */
>     handler = _mcd_client_registry_lookup (self->priv->clients,
>                                            well_known_name);
>   }

I think it might deserve a DEBUG(), either here or on successful exit from the
function - I think it'd be good if each route through the function was
guaranteed to DEBUG() at least once. Be careful that it can't printf ("%s",
NULL), which crashes on some platforms.

I don't like the "goto out" where out is within a block; perhaps you could
refactor that so "out" is either unnecessary, or at the top level?

In _mcd_dispatcher_reinvoke_handler() you've deleted calls to
mcd_dispatcher_finish_reinvocation(). Why? If I understand the code correctly,
you should mcd_dispatcher_finish_reinvocation() before you goto finally. Is
this case covered by the tests?

In dispatcher_present_channel, because you use _mcd_channel_get_request(), what
you're doing is particularly subtle: you're basing the search for a suitable
Handler on the handler that was preferred by the request that initially created
the (Tp)Channel, if any[1]. I think this is right (or at least, better than not
looking at the request at all), but deserves a comment!

[1] Actually you're not, but only because
_mcd_client_registry_list_possible_handlers() is misleading, for which I'll
clone a bug.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.