Created attachment 225682 [details]
Test GObject.signal_handler_disconnect()

Created attachment 225683 [details]
Test GObject signal_handler_disconnect()

Confirmed. g_signal_handler_is_connected() receives an "instance" pointer to something which is just garbage:

(gdb) p *((GObject *) instance)
$2 = {g_type_instance = {g_class = 0x4}, ref_count = 13169824, qdata = 
    0xd5f070}

It crashes trying to deref the g_class pointer.

For the record, this is the smallest reproducer I found, running under gdb in built trunk:

PYTHONPATH=. gdb --args python3 -c 'from gi.repository import GObject; o=GObject.Object(); print(GObject.signal_handler_is_connected(o, 1))'

This is a bit tricky to fix, I'm afraid. The functions expect a generic gpointer, not a GObject* (which kind of makes sense in C as signals are not limited to GObject, but should work with any fundamental).

So we end up using _pygi_marshal_from_py_void() for the object argument, which just passes on a pointer to the PyObject, not the underlying object. However, for a C function passing a PyObject is almost never going to be what you want, unless you are actually talking to something like an introspected libpython API.

So we can gradually make this better by passing the wrapped GObject in case the argument is a GObject, and otherwise do what we currently do.

Created attachment 225820 [details]
broken patch attemtp

Hm, I was going to return the wrapped GObject for the case that py_arg is a GObject, but that doesn't seem to work. PyObject_TypeCheck(py_arg, &PyGObject_Type) doesn't succeed, although it's being used in a lot of other places (but in gi/_gobject/, not in gi/*.c). I guess I'm missing something here.

If I replace the condition with if(1), it works fine.

Does anyone have an idea about the type check here?

This is similar to bug 640671.

Comment on attachment 225820 [details]
broken patch attemtp

So this is bogus. We do need to keep PyObjects for gpointers, as they are commonly used to pass around data as black boxes ("userdata" arguments).

I asked John Palmieri, and he suggested that we annotate these functions.

So if we do this, it definitively works, but it limits the functions to GObjects, whereas they really work with any fundamental. glib maintainers, would you be open to a patch like this, which would change the function from "broken for everything" (through GI) to "broken for non-GObjects"?

--- a/gir/gobject-2.0.c
+++ b/gir/gobject-2.0.c
@@ -3697,7 +3697,7 @@

 /**
  * g_signal_handler_disconnect:
- * @instance: The instance to remove the signal handler from.
+ * @instance: (type GObject.Object): The instance to remove the signal handler from.
  * @handler_id: Handler id of the handler to be disconnected.
  *
  * Disconnects a handler from an instance so it will not be called during
@@ -3731,7 +3731,7 @@

 /**
  * g_signal_handler_is_connected:
- * @instance: The instance where a signal handler is sought.
+ * @instance: (type GObject.Object): The instance where a signal handler is sought.
  * @handler_id: the handler id.
  *
  * Returns whether @handler_id is the id of a handler connected to @instance.

That was my test patch in g-i; a real patch would of course go into gobject/gsignal.c.

Something like (type GObject.TypeInstance) would be ideal here; that doesn't work with PyGObject, though.

it's safe to assume that basically all GSignal users are in face GObject sub-classes. the whole "we can use GSignal with any GTypeInstance" is an artefact of the type system that has caused more issues than anything.

Created attachment 253235 [details] [review]
gobject-2.0: Annotate GSignal functions taking instances

Unbreak the GSignal API at least for GObject sub-classes.

(In reply to comment #11)
> it's safe to assume that basically all GSignal users are in face GObject
> sub-classes. the whole "we can use GSignal with any GTypeInstance" is an
> artefact of the type system that has caused more issues than anything.

Thanks for clarifying. But even if not, annotating it wouldn't make things any worse for non-GOBjects, as it currently doesn't work with anything.

Wrt. your patch, that's against gobject-introspection. But I think that actually needs to be done in glib proper (gobject/gsignal.c), as g-i merely is extracting annotations from there?

(In reply to comment #13)
> (In reply to comment #11)
> > it's safe to assume that basically all GSignal users are in face GObject
> > sub-classes. the whole "we can use GSignal with any GTypeInstance" is an
> > artefact of the type system that has caused more issues than anything.
> 
> Thanks for clarifying. But even if not, annotating it wouldn't make things any
> worse for non-GOBjects, as it currently doesn't work with anything.
> 
> Wrt. your patch, that's against gobject-introspection. But I think that
> actually needs to be done in glib proper (gobject/gsignal.c), as g-i merely is
> extracting annotations from there?

it was mostly a proof-of-concept for review: do the annotations seem okay to you? if they are, I'll patch GSignal directly and re-generate the file in g-i.

Created attachment 253240 [details] [review]
gobject-2.0: Annotate GSignal functions taking instances

That's Emmanuele's patch adjusted to glib. I also took the liberty to (skip) g_signal_chain_from_overridden_handler() in addition, as it's a varargs method.

(In reply to comment #14)
>
> it was mostly a proof-of-concept for review: do the annotations seem okay to
> you? if they are, I'll patch GSignal directly and re-generate the file in g-i.

Ah sorry; yes, they are. It's pretty much what I tried in comment 9.

Thanks!

There have also been a couple dups of this ticket, bug #698382 and bug #633999 (both of them have patches for GLib). I closed out bug #633999 because we ended up working around it in PyGObject. But getting these annotations in GLib would be a welcome change so we can remove our work arounds.

*** Bug 698382 has been marked as a duplicate of this bug. ***

Review of attachment 253240 [details] [review]:

looks good to me.

Thanks for the review! Pushed.

Updated annotations in gobject-introspection as well.

*** Bug 633999 has been marked as a duplicate of this bug. ***

[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]