Bug 31631

Summary: Serious issues with TpClientChannelFactoryInterface
Product: Telepathy Reporter: Guillaume Desmottes <guillaume.desmottes>
Component: tp-glibAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: major    
Priority: medium Keywords: patch
Version: unspecified   
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=shortlog;h=refs/heads/factory
Whiteboard: r+
i915 platform: i915 features:

Description Guillaume Desmottes 2010-11-15 03:20:33 UTC
While starting to use TpClientChannelFactoryInterface in Empathy I found 2 big issues with it.

A) tp_base_client_set_channel_factory() is broken. This is fixed in http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=commitdiff;h=9deb3dcad3a60f43d6367296ae32f2b26c4103a6

B) The 'self' pointer passed to method implementation is wrong.
-    return iface->dup_channel_features (iface, channel);
+    return iface->dup_channel_features (self, channel);
solves it but it seems that something is wrong.

This cast is suspicious, self is supposed to already be a TpClientChannelFactoryInterface; so why do we need to cast it?

 TpClientChannelFactoryInterface *iface = TP_CLIENT_CHANNEL_FACTORY_GET_IFACE (
      self);

And I'm wondering if the TP_CLIENT_CHANNEL_FACTORY macro really makes sense as that's an iface and not an object?
Comment 1 Simon McVittie 2010-11-16 06:32:31 UTC
(In reply to comment #0)
> A) tp_base_client_set_channel_factory() is broken. This is fixed in
> http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=commitdiff;h=9deb3dcad3a60f43d6367296ae32f2b26c4103a6

r+

> B) The 'self' pointer passed to method implementation is wrong.
> -    return iface->dup_channel_features (iface, channel);
> +    return iface->dup_channel_features (self, channel);
> solves it but it seems that something is wrong.
> 
> This cast is suspicious, self is supposed to already be a
> TpClientChannelFactoryInterface; so why do we need to cast it?

Ugh, I should have spotted this at review. The type of the method is wrong; we've screwed up the declarations.

TpClientChannelFactory* is a pointer to an object that implements the ClientChannelFactory interface, which would be useful for potentially-stateful instance methods.

TpClientChannelFactoryInterface* is a pointer to the interface's vtable (class-like struct), which isn't actually useful.

This means that you can't usefully implement any client channel factory whose methods look at their first argument.

Proposed solution: add obj_create_channel and obj_dup_channel_features methods that do what we intended; have their default implementations chain to the current (wrong) ones; in telepathy-glib 1.0, remove the current methods and remove the obj_ prefix from the corrected versions.

> And I'm wondering if the TP_CLIENT_CHANNEL_FACTORY macro really makes sense as
> that's an iface and not an object?

14:10 < cassidy> [...] According 
                 to the doc, G_TYPE_CHECK_INSTANCE_CAST should only be used in 
                 type implementation, so I think we shouldn't have used it here

I think a GInterface counts as a type: GAsyncResult has this usage, for instance. Our macros are analogous to GAsyncResult's, except that TP_CLIENT_CHANNEL_FACTORY is implemented wrong.

The semantics of TP_CLIENT_CHANNEL_FACTORY(obj) are (meant to be): if obj is non-NULL and its GType implements ("is-a") ClientChannelFactory, return it, cast to TpClientChannelFactory*; else critical and return NULL.

However, TP_CLIENT_CHANNEL_FACTORY actually casts the instance pointer to TpClientChannelFactoryInterface, which is invalid - the instance doesn't start with a struct _TpClientChannelFactoryInterface, so the fields in the returned struct will be meaningless (they'll just alias whatever's at the beginning of struct _GObject).

At some point I should enhance tools/gobject-foo.py to produce correct GInterfaces with the same names that G_DEFINE_INTERFACE would use.
Comment 2 Simon McVittie 2010-11-16 08:39:45 UTC
16:05 < smcv> we could probably fix it without API breaks, but only with a 
              liberal scattering of gpointer
16:06 < smcv> or we could just go "clearly nobody's using it yet, and 
              typesafety is more important" and break API while keeping ABI
16:07 < cassidy> I'd vote for that

Here's a branch to do that.
Comment 3 Guillaume Desmottes 2010-11-16 08:53:22 UTC
Looks good, sorry for this mess.

Maybe open a bug regarding the deprecated API so we won't forget about it for 1.0?
Comment 4 Simon McVittie 2010-11-17 02:43:16 UTC
Fixed in git for 0.13.6; does not affect 0.12.

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.