[Bug 40283] Crash when a client needs recovery
bugzilla-daemon at freedesktop.org
bugzilla-daemon at freedesktop.org
Tue Sep 20 12:44:12 CEST 2011
--- 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,
* @channel: a non-null Channel which has already been dispatched
* @request: (allow-none): if not NULL, a request that resulted in
* 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,
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. I think this is right (or at least, better than not
looking at the request at all), but deserves a comment!
 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.
More information about the telepathy-bugs