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 546802 - Pygtk destroys cycle too early
Pygtk destroys cycle too early
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: gobject
2.14.x
Other All
: Normal minor
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Blocks: 693111
 
 
Reported: 2008-08-07 13:37 UTC by Mark Seaborn
Modified: 2017-10-11 09:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (pygobject-2-14 branch) (2.32 KB, patch)
2008-08-08 17:04 UTC, Gustavo Carneiro
none Details | Review
another test (592 bytes, text/plain)
2008-08-29 19:22 UTC, Paul Pogonyshev
  Details
better now? (2.42 KB, patch)
2008-08-30 14:22 UTC, Paul Pogonyshev
none Details | Review

Description Mark Seaborn 2008-08-07 13:37:49 UTC
Please describe the problem:
The following test case creates a reference cycle between a Gtk widget and an event handler.  Python's GC believes the cycle is no longer externally referenced and destroys it.  However, it is still referenced, and the event handler fails when Gtk invokes it.

This occurs on Ubuntu hardy, which has Pygtk 2.12.1.

Steps to reproduce:
The following program reproduces the problem:

import gc
import gtk
import gobject


def make_hyperlink():
    event_box = gtk.EventBox()
    def on_hyperlink_mapped(*args):
        event_box.window.set_cursor(gtk.gdk.Cursor(gtk.gdk.HAND2))
    event_box.connect("map-event", on_hyperlink_mapped)
    return event_box


window = gtk.Window()
window.add(make_hyperlink())
gc.collect()
window.show_all()
# Need to enter the Gtk main loop to trigger this.
# Can do gtk.main(), but the following will exit after triggering the exception.
for i in range(10):
    gobject.main_context_default().iteration()


Actual results:
It gives the following output:

Traceback (most recent call last):
  • File "example.py", line 10 in on_hyperlink_mapped
    event_box.window.set_cursor(gtk.gdk.Cursor(gtk.gdk.HAND2))
NameError: free variable 'event_box' referenced before assignment in enclosing scope


Expected results:
No traceback is expected.

Does this happen every time?
Yes.

Other information:
The traceback does not happen on Ubuntu feisty, which has Pygtk 2.10.4.
Comment 1 Gustavo Carneiro 2008-08-07 14:21:15 UTC
This is interesting, but I am more inclined to believe it to be a Python bug rather than a PyGObject one, pending further investigation.
Comment 2 Mark Seaborn 2008-08-08 15:18:39 UTC
My question was going to be: How does the C GObject keep a refcount on the Python GObject wrapper so that that wrapper (and the cycle it is involved in with the Python callback) does not get freed?

I found that the answer is: It uses g_object_add_toggle_ref() so that PyGObject gets notified when the only remaining refcount to the GObject is the one it holds.

However, g_object_add_toggle_ref() is only used conditionally.  See the calls to pygobject_switch_to_toggle_ref(): the calls are only made if the PyGObject's instance dictionary is used, presumably on the grounds that if the PyGObject does not contain any state it can be discarded.  (The GObject docs say "you should only ever use a toggle reference if there is important state in the proxy object".)  This logic does not take into account the Python callbacks, whose refcounts are owned by the PyGObject rather than by the underlying GObject.

I can trigger a call to pygobject_switch_to_toggle_ref() by adding the line:
  event_box.x = 1
(which calls pygobject_setattro())
or
  event_box.__dict__
(which calls pygobject_get_dict())

So if I change make_hyperlink() to the following, the problem goes away:

def make_hyperlink():
    event_box = gtk.EventBox()
    event_box.__dict__
    def on_hyperlink_mapped(*args):
        event_box.window.set_cursor(gtk.gdk.Cursor(gtk.gdk.HAND2))
    event_box.connect("map-event", on_hyperlink_mapped)
    return event_box
Comment 3 Gustavo Carneiro 2008-08-08 16:11:49 UTC
So you discovered that the problem does not happen if the wrapper is using a toggle reference.  This means only means that the GObject keeps a reference back to the wrapper.

Without the toggle ref, the wrapper may die while the GObject stays alive, in case the wrapper refcount reaches zero.  However, in this particular case the python function closure (fun_closure attribute in the function object) should be holding a reference to the wrapper, thus ensuring the wrapper stays alive even though the GObject does not reference it anymore.

In my tests I printed the func_closure before and during invocation.  A closure contains a list of "cells", each cell is a variable to bind in the function namespace.  Before the invocation, the cells are all there and correct.  But during invocation the problematic cell containing a reference to our object appears 'empty'.

Why was the python function closure cell cleared I have no idea, but I know it shouldn't have happened.  That's why I suspect a Python bug.  I could be wrong, but it's the only explanation I have for the time being...
Comment 4 Gustavo Carneiro 2008-08-08 17:02:10 UTC
Ah.. I think I know where the problem is: pygobject_traverse.

We are always declaring to the Python GC that the wrapper holds a reference to the GClosures.  Then, when tp_traverse is being called on the wrapper, Python sees a reference cycle:

  wrapper => closure->callback => func_closures => wrapper

However, the mistake here is that Python GC believes that by clearing wrapper, the python function object is unreffed as a consequence, but that is only true if the wrapper is holding the last reference to the object.
Comment 5 Gustavo Carneiro 2008-08-08 17:04:22 UTC
Created attachment 116163 [details] [review]
patch (pygobject-2-14 branch)

This should fix the problem.
Comment 6 Johan (not receiving bugmail) Dahlin 2008-08-08 20:29:44 UTC
This only goes into trunk right?
Or should we really do a follow up 2.14 release?
Comment 7 Gustavo Carneiro 2008-08-09 14:27:03 UTC
I think it's far from critical bug.  Not really worth making a 2.14 release for it IMHO.
Comment 8 Mark Seaborn 2008-08-13 10:47:40 UTC
I notice that your patch appears to be equivalent to a patch that Arjan J. Molenaar proposed for fixing Bug 92955 (see comments 6 and 7: http://bugzilla.gnome.org/show_bug.cgi?id=92955#c7).  I was going to propose this myself until I discovered the use of the toggle refs mechanism, which appeared to be a replacement for looking at the GObject's refcount.  As I understand it now (based on Bug 320428), the reason for not using toggle refs all the time is that it would prevent the object from being freed if it were wrapped by two sets of language bindings.
Comment 9 Paul Pogonyshev 2008-08-27 19:29:02 UTC
Gustavo: are you confident enough about the patch to include it into the trunk (would-be-2.16)?
Comment 10 Gustavo Carneiro 2008-08-29 00:17:08 UTC
I am confident about the patch core.  The unit testing part could use more love though, as it opens a window which is temporarily visible.
Comment 11 Paul Pogonyshev 2008-08-29 19:22:11 UTC
Created attachment 117599 [details]
another test

I don't really understand this, but here is my attempt at a test.  However, it fails weirdly both without and with your fix.  Is that another bug?
Comment 12 Gustavo Carneiro 2008-08-30 00:07:06 UTC
(In reply to comment #11)
> Created an attachment (id=117599) [edit]
> another test
> 
> I don't really understand this, but here is my attempt at a test.  However, it
> fails weirdly both without and with your fix.  Is that another bug?
> 

Please.  First this isn't a unit test, it is not clear what conditions should be evaluated to check if it "passes" or not.  Also you are using gio which had reference counting errors last time (a few weeks ago) I checked (python complains if you have a python built with --with-pydebug --without-pymalloc).
Comment 13 Paul Pogonyshev 2008-08-30 14:22:20 UTC
Created attachment 117640 [details] [review]
better now?
Comment 14 Paul Pogonyshev 2008-08-30 14:24:46 UTC
Also, if cancellable (push|pop|get)_current are inherently buggy in our wrappers they must be either fixed or removed altogether.
Comment 15 Paul Pogonyshev 2008-08-30 14:31:51 UTC
About the patch itself: looks like the condition cannot change inside the loop, but I doubt compiler will spot this.  Won't it be better to move it outside the loop then?  Also, a comment would help, doesn't look like an obvious fix.  

Finally, a test for memory leaks (to be sure this patch doesn't suddenly make PyGObject leak lots of stuff) would be really helpful.
Comment 16 Sean Luke 2008-09-11 21:42:35 UTC
> This should fix the problem.

Sadly no.

I got Nokia to move this patch into the recent version of pymaemo.  Here's what I'm getting though:

If you create a closure with a lambda expression over 'self', the patch works great now.  But if you create a closure with a lambda expression over a local variable, pygtk is still deleting the variable early, causing Bad Things to happen.  This didn't happen in much earlier revisions.

Example:

<in a method>

        self.split.connect("destroy", lambda widget, data=None : setPersistenceData(name, self.split.get_position()))

'name' is a local variable holding a string.  Inside the lambda, 'self' is now not getting released, but 'name' is still getting released, converting into <none>.




Comment 17 Gustavo Carneiro 2008-09-12 09:27:47 UTC
That's weird; the patch should be fixing both cases.
Any chance you can come up with a minimal test program to reproduce the problem?  Just to make sure we don't misunderstand the issue...
Comment 18 Gustavo Carneiro 2008-09-12 09:54:55 UTC
In fact the minimal test is fundamental because I at least have been unable to reproduce the problem based on your description.
Comment 19 Sean Luke 2008-09-14 22:13:28 UTC
(In reply to comment #18)
> In fact the minimal test is fundamental because I at least have been unable to
> reproduce the problem based on your description.

A minimal test may be difficult to construct.  I do have a two-file python application which exhibits the behavior but it'd be too long to post here.  It's about 1800 lines.  Too big to rummage through?
Comment 20 Dave Peticolas 2009-05-21 03:10:27 UTC
This bug has been causing problems for me as well.
Did a fix ever go into pygobject? I seem to be hitting
the bug on at least pygobject 2.14.
Comment 21 Dave Peticolas 2009-05-21 03:13:05 UTC
Also, I think this rates as higher than 'minor'.

I'm looking at porting a lot of code that ran
fine on an older pygobject and it uses closures
freely.
Comment 22 gbowman2 2009-07-15 20:39:44 UTC
I'm having the same issue in gobject 2.14-2 using the original syntax from Mark Seaborn.

NameError: free variable 'event_box' referenced before assignment in enclosing
scope
Comment 23 johnp 2010-09-23 15:56:39 UTC
Is this still an issue?  I tried the test code and it works fine here.
Comment 24 gbowman2 2010-09-23 16:25:16 UTC
yes it is still an issue... I tested it today on a computer at work.
 - (debian, stable)
 - python 2.5.2
 - gtk 2.14.2

Traceback (most recent call last):
  • File "/tmp/test.py", line 9 in on_hyperlink_mapped
    event_box.window.set_cursor(gtk.gdk.Cursor(gtk.gdk.HAND2))
NameError: free variable 'event_box' referenced before assignment in enclosing scope

Comment 25 johnp 2010-09-23 17:16:23 UTC
Thanks for testing.  What version of pygobject is used?  I have 2.21.1.  Might have been fixed in later versions.
Comment 26 gbowman2 2010-09-23 20:02:50 UTC
(In reply to comment #25)
> Thanks for testing.  What version of pygobject is used?  I have 2.21.1.  Might
> have been fixed in later versions.

That seems like the case. I am using an older version than yourself.
In order to upgrade in debian, I believe I would need to move to unstable, it would be nice to have a patch instead as upgrading is just not possible with the current situation.

debian: python-gobject 2.14.2-2

-python-
In [1]: import gobject
In [2]: gobject.pygobject_version
Out[2]: (2, 14, 2)
Comment 27 johnp 2010-09-23 20:30:13 UTC
If it is important to you I would file this downstream with Debian.  They can add the patch to their packages and push to stable.  It seems to be fixed in the current code base so I'm going to close the bug.
Comment 28 Benjamin Gilbert 2014-06-05 02:40:57 UTC
The problem still exists in pygobject 2.28.6 and 3.13.1.  I don't seem to have access to reopen this bug; could someone please do it?

The reproducer above is not quite correct: the map-event never fires at all, because it has not been added to the window's event mask.  If

window.add_events(gtk.gdk.SUBSTRUCTURE_MASK)

is called immediately after creating the gtk.Window, the error will be triggered:

Traceback (most recent call last):
  • File "example.py", line 9 in on_hyperlink_mapped
    event_box.window.set_cursor(gtk.gdk.Cursor(gtk.gdk.HAND2))
NameError: free variable 'event_box' referenced before assignment in enclosing scope

This thread from 2008 has more discussion and another reproducer:

http://www.daa.com.au/pipermail/pygtk/2008-August/015767.html

The patch in comment 13 fixes the problem for me.



Here is a port of the reproducer to pygobject 3:

===
import gc
from gi.repository import Gdk, GLib, Gtk

def make_hyperlink():
    event_box = Gtk.EventBox()
    def on_hyperlink_mapped(*args):
        event_box.get_window().set_cursor(Gdk.Cursor(Gdk.CursorType.HAND2))
    event_box.connect("map-event", on_hyperlink_mapped)
    return event_box

window = Gtk.Window()
window.add_events(Gdk.EventMask.SUBSTRUCTURE_MASK)
window.add(make_hyperlink())
gc.collect()
window.show_all()
# Need to enter the Gtk main loop to trigger this.
# Can do Gtk.main(), but the following will exit after triggering the
# exception.
for i in range(10):
    GLib.main_context_default().iteration()
===


And here is a reproducer that doesn't use Python closures (based on the patch in comment 13):

===
import gc
from gi.repository import Gio, GLib, Gtk

def on_add(container, widget):
    print container
    print 'refcount', container.__grefcount__

def setup_frame(loop):
    frame = Gtk.Frame()
    # This works
    #frame.connect('add', on_add)
    # This should be equivalent, but fails
    frame.connect_object('add', on_add, frame)

    # This is a dummy object to avoid keeping a Python reference to the
    # frame (the only reference will be held from C).
    x = Gio.FileInfo()
    x.set_attribute_object('frame', frame)

    GLib.idle_add(lambda: x.get_attribute_object('frame').add(Gtk.Label(label='foo')))
    GLib.idle_add(lambda: loop.quit())

#gc.set_debug(gc.DEBUG_OBJECTS | gc.DEBUG_COLLECTABLE)
loop = GLib.MainLoop()
setup_frame(loop)
gc.collect()
loop.run()
===