After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 735166 - Python wrongly releases floating variants consumed by a refsink
Python wrongly releases floating variants consumed by a refsink
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: introspection
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
: 738653 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2014-08-21 14:11 UTC by Alban Crequy
Modified: 2014-10-18 02:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
test.py (783 bytes, text/x-python)
2014-08-21 14:11 UTC, Alban Crequy
  Details
Fix reference counting problems with GLib.Variant.new_tuple() (1.82 KB, patch)
2014-08-21 21:51 UTC, Simon Feltman
committed Details | Review

Description Alban Crequy 2014-08-21 14:11:15 UTC
Created attachment 284092 [details]
test.py

Python wrongly releases floating variants consumed by a g_variant_ref_sink. In the attached python code, the following warning will be printed:

sys:1: Warning: g_variant_unref: assertion 'value->ref_count > 0' failed

pygobject uses g_object_is_floating() to manage the reference counting of GObjects. But I don't find any references to g_variant_is_floating() in pygobject's source.
Comment 1 Simon Feltman 2014-08-21 20:52:30 UTC
Confirmed, I'm somewhat surprised this hasn't surfaced until now.

The g_object_is_floating() checks in use are generally hacks to work around bugs in underlying libraries or the oddity that g_object_new() is transfer-full but can return floating references. In the case of marshalling variants to Python, we always call g_variant_ref_sink() for transfer-none which handles the majority of cases (all the variant constructors) See: [1]

The problem here is new_tuple() is actually statically implemented and does not go through this code path [2]. It also doesn't sink the resulting ref after the it calls g_variant_new_tuple() which is the bug here. The fix should be as simple as calling g_variant_ref_sink().

Since g_variant_new_tuple() sinks its inputs, the bug can also be seen as:

t = GLib.Variant.new_tuple(GLib.Variant.new_tuple())
del t

As an aside, while reading the code I also notice the temporary array passed to g_variant_new_tuple() is leaked.

As another aside, we can probably delete this static binding as our array marshalling code path should handle all of this... but I'll save that for 3.16.

[1] https://git.gnome.org/browse/pygobject/tree/gi/pygi-struct-marshal.c?id=3.13.90#n403
[2] https://git.gnome.org/browse/pygobject/tree/gi/gimodule.c?id=3.13.90#n472
Comment 2 Simon Feltman 2014-08-21 21:15:23 UTC
(In reply to comment #1)
> As an aside, while reading the code I also notice the temporary array passed to
> g_variant_new_tuple() is leaked.

Never mind, I didn't notice the "a" at the end of "g_newa".
Comment 3 Simon Feltman 2014-08-21 21:51:54 UTC
A workaround is to use the Python constructor:

tuple_v = GLib.Variant('(s)', ('org.freedesktop.DBus',))

The following fix has been pushed:
c1d3875 Fix reference counting problems with GLib.Variant.new_tuple()
Comment 4 Simon Feltman 2014-08-21 21:51:57 UTC
Created attachment 284147 [details] [review]
Fix reference counting problems with GLib.Variant.new_tuple()

Always sink the results of g_variant_new_tuple() in the statically
bound wrapper. This matches the generic GI marshalling behavior
of passing GVariants to Python with transfer-none.
Comment 5 Simon Feltman 2014-10-18 02:45:21 UTC
*** Bug 738653 has been marked as a duplicate of this bug. ***