Bug 25019 - tp_presence_mixin_emit_presence_update() segfaults if TP_TYPE_SVC_CONNECTION_INTERFACE_PRESENCE is not implemented
Summary: tp_presence_mixin_emit_presence_update() segfaults if TP_TYPE_SVC_CONNECTION_...
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/sm...
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2009-11-10 08:14 UTC by butch howard
Modified: 2010-08-17 06:16 UTC (History)
0 users

See Also:
i915 platform:
i915 features:

Attachments

Description butch howard 2009-11-10 08:14:10 UTC 
Since INTERFACE_PRESENCE is deprecated, it implies that it is not necessary to implement that interface. However, if it is not implemented (even though the TpPresenceMixin does all the work for us it is not clear that it should be used), then a call to tp_presence_mixin_emit_presence_update() will cause a segfault when the internals of tp_svc_connection_interface_presence_emit_presence_update() type check the GOBJECT for the requested interface.

A simple fix for this is to wrap the call in a test similar to that done a few lines below for the call for the Simple Presence emit so that the function is not called unless the Presence interface is implemented.

Updated Code:

void
tp_presence_mixin_emit_presence_update (GObject *obj,
                                        GHashTable *contact_statuses)
{
  TpPresenceMixinClass *mixin_cls =
    TP_PRESENCE_MIXIN_CLASS (G_OBJECT_GET_CLASS (obj));
  GHashTable *presence_hash;

  DEBUG ("called.");

  if (g_type_interface_peek (G_OBJECT_GET_CLASS (obj),
		  TP_TYPE_SVC_CONNECTION_INTERFACE_PRESENCE) != NULL)
    {
	  presence_hash = construct_presence_hash (mixin_cls->statuses,
		  contact_statuses);
	  tp_svc_connection_interface_presence_emit_presence_update (obj,
		  presence_hash);
	
	  g_hash_table_destroy (presence_hash);
    }

  if (g_type_interface_peek (G_OBJECT_GET_CLASS (obj),
      TP_TYPE_SVC_CONNECTION_INTERFACE_SIMPLE_PRESENCE) != NULL)
    {
      presence_hash = construct_simple_presence_hash (mixin_cls->statuses,
        contact_statuses);
      tp_svc_connection_interface_simple_presence_emit_presences_changed (obj,
        presence_hash);

      g_hash_table_destroy (presence_hash);
    }
}
Comment 1 Simon McVittie 2009-11-10 09:03:32 UTC 
My position on the deprecation is that CMs should still implement complex Presence (since the presence mixin makes it just as easy to do so), but clients are allowed to assume that only SimplePresence matters (telepathy-qt4 and telepathy-glib high-level APIs ignore complex Presence entirely).

It's certainly a bug that the presence mixin crashes "late", though; it should either crash out with an assertion failure immediately (in class_init), or cope gracefully. I'll add a check similar to the one you propose in a future version of telepathy-glib.

However, I suggest you work around this by adding the necessary 1 line of code to implement old-style Presence too!
Comment 2 butch howard 2009-11-10 10:45:16 UTC 
I can understand that position on the depreciation in this case. From someone new to this set of libraries, 'deprecated' was read in the usual sense of 'not needed at all' or 'to be avoided like the plague' or 'might go away in the near future' (even though there is an existing implementation available). 

The difference between the CMs and the clients should really be made clear in the places where the complex Presence is marked as deprecated to avoid this confusion for others.

To address the immediate need, I have made that one-line change to take advantage of the implementation in the mixin (and thanks for doing that work for us!).

Graceful failures with clear indicators of why are much better than abrupt with  hard to determine reasons.
Comment 3 Simon McVittie 2010-08-12 03:33:50 UTC 
(In reply to comment #0)
> Since INTERFACE_PRESENCE is deprecated, it implies that it is not necessary to
> implement that interface.

Revisiting this bug, I think it's time we made this true, so I've applied your patch - thanks! I've also rewritten the TpPresenceMixin documentation to make it describe all the necessary actions to wire it up to the Connection.
Comment 4 Simon McVittie 2010-08-17 06:16:00 UTC 
Fixed in 0.11.13

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.