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 640868 - Wrong gobject refcount when calling introspected constructors for widgets
Wrong gobject refcount when calling introspected constructors for widgets
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: introspection
Git master
Other Linux
: Normal normal
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Blocks: 693111
 
 
Reported: 2011-01-28 23:09 UTC by Steve Frécinaux
Modified: 2013-02-03 21:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix wrong refcount when calling introspected widget constructors (1.46 KB, patch)
2011-01-28 23:29 UTC, Steve Frécinaux
none Details | Review
Fix wrong refcount when calling introspected widget constructors (2.15 KB, patch)
2011-02-02 13:02 UTC, Steve Frécinaux
committed Details | Review

Description Steve Frécinaux 2011-01-28 23:09:44 UTC
Here is an example below.

>>> l = Gtk.Button.new()
>>> l.__grefcount__
2
>>> l = Gtk.Button()
>>> l.__grefcount__
1

The __grefcount__ should be the same in both cases. I think the issue is that Gtk.Button.new() returns an object with a floating reference, which is reffed by pygobject (so that makes 2 refs, despite the object is not owned by anyone). We should probably use ref_sink() instead for objects returned with transfer full.

The reference count is correct for Gtk.Window.new() (which has a "sunk floating ref", transfer none) and for GdkPixbuf.Pixbuf.new() (which return a regular ref, transfer full)
Comment 1 Steve Frécinaux 2011-01-28 23:28:20 UTC
I made a mistake in the above comment: gtk_button_new() is actually (transfer none) because there are 'zero refs' on it when it's returned.
Comment 2 Steve Frécinaux 2011-01-28 23:29:14 UTC
Created attachment 179559 [details] [review]
Fix wrong refcount when calling introspected widget constructors

Introspected widget constructors, like Gtk.Button.new(), can return
objects with a floating reference, which was then reffed by pygobject,
resulting in two references, despite the object is not owned by anyone.

This patch uses ref_sink() when pygobject takes its own reference, to
avoid adding that extra reference. Hence we now claim ownership on
objects returned by constructors with transfer=none (which is the case
for nearly all the widget constructors, despite the floating ref).
Comment 3 Steve Frécinaux 2011-02-02 13:02:57 UTC
Created attachment 179876 [details] [review]
Fix wrong refcount when calling introspected widget constructors

Added a test case. Without the patch, the new test case failed with this error:

======================================================================
FAIL: test_floating (test_everything.TestEverything)
----------------------------------------------------------------------
Traceback (most recent call last):
  • File "/home/sf/src/gnome2/pygobject/tests/test_everything.py", line 77 in test_floating
    self.assertEquals(e.__grefcount__, 1)       AssertionError: 2 != 1

Comment 4 Tomeu Vizoso 2011-02-02 13:04:54 UTC
Review of attachment 179876 [details] [review]:

This one is great, thanks!
Comment 5 Jonathan Matthew 2011-02-05 14:26:40 UTC
After this change, this now crashes:

from gi.repository import Gtk, GObject

class Thing(Gtk.HBox):
    pass

GObject.new(Thing)
Comment 6 Steve Frécinaux 2011-02-07 12:14:58 UTC
It works if you do Thing() instead. That makes me think the issue is this:

- the object is created (refcount 1, floating)
- __init__ is called: the wrapper is created, and takes ownership (refcount 1)
- __init__ is finished, and the wrapper is discarded (refcount 0)
- g_object_new returns, and a new wrapper is created for an object which was already finalized.

So we need to somehow figure out this wrapper is created within a constructor, and not sink the ref in this case specifically.

I suppose the previous patch might be reverted in the meantime to avoid such crashes and have leaks instead.
Comment 7 Martin Pitt 2012-02-10 08:31:29 UTC
This patch was committed over a year ago, closing. Thanks!