Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
On 2/8/2013 at 9:43 AM, Simon Feltman s.felt...@gmail.com wrote: Ticket and patch for the transient argument to the GtkCellRendererText editing-started signal: https://bugzilla.gnome.org/show_bug.cgi?id=693400 This is what I was proposing when I started this thread. If this kind of fix is ok, I have no problem looking for more cases in the GTK+ code base. Isn't this also a problem for GtkCellRendererCombo? I know especially with callbacks on the GtkEntry GtkBin child if 'has-entry' are enabled with Perl there are issues ... Possibly the other's as well, but I normally do not use them. Regards, Martin On Thu, Feb 7, 2013 at 7:08 PM, Simon Feltman s.felt...@gmail.com wrote: I've created a ticket and proposed patch for GTK+: https://bugzilla.gnome.org/show_bug.cgi?id=693393 I am looking for feedback to see if this type of thing is even acceptable and if it's worthy of further pursuit. Thanks, -Simon On Thu, Feb 7, 2013 at 1:16 PM, Torsten Schoenfeld kaffeeti...@gmx.dewrote: On 07.02.2013 01:12, Simon Feltman wrote: Unfortunately there are a few more of these: Gtk.Action.create_menu Gtk.Action.create_menu_item Gtk.Action.create_tool_item Gtk.PrintOperation.create_**custom_widget Gtk.PrintOperation.create-**custom-widget (signal) Gladeui.EditorProperty.create_**input Gladeui.BaseEditor.build_**child? Gladeui.BaseEditor.build-child (signal) Also: Gtk.CellRenderer.start_**editing. __**_ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/**mailman/listinfo/gtk-devel-**listhttps://mail.gnome.or g/mailman/listinfo/gtk-devel-list Vrywaringsklousule / Disclaimer: http://www.nwu.ac.za/it/gov-man/disclaimer.html ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
On 08.02.2013 04:08, Simon Feltman wrote: I've created a ticket and proposed patch for GTK+: https://bugzilla.gnome.org/show_bug.cgi?id=693393 I am looking for feedback to see if this type of thing is even acceptable and if it's worthy of further pursuit. Won't gobject-introspection silently turn the (transfer full) annotation into (transfer none) due to the special handling for GInitiallyUnonwed descendants? Also, the list in the bug is missing Gtk.CellRenderer.start_editing (the vfunc, not the method). ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
On Fri, Feb 8, 2013 at 3:05 AM, Torsten Schoenfeld kaffeeti...@gmx.dewrote: Won't gobject-introspection silently turn the (transfer full) annotation into (transfer none) due to the special handling for GInitiallyUnonwed descendants? It seems to pick it up correctly. The problem was actually the rename annotation doesn't work for vfuncs. Also, the list in the bug is missing Gtk.CellRenderer.start_editing (the vfunc, not the method). Ack, sorry, I will add an additional note. -Simon ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
On Wed, Feb 6, 2013 at 12:57 PM, Owen Taylor otay...@redhat.com wrote: class ToolMenuAction(Gtk.Action): def do_create_tool_item(self): return Gtk.MenuToolButton() This is basically broken API at the GTK+ level :-( ... a virtual function can't return (transfer none) unless it's a getter for an existing field. It is expected that language bindings will have no way to create a floating object, so a virtual function cannot expect to be returned a floating object. The only thing I can see at the GTK+ level would be to add a make_tool_item replacement vfunc and use that instead if non-null. I don't know GTK+ internals but taking a quick look are you saying to use the _gtk_reserved pointers for new vfuncs and annotate them as transfer full? A potential issue is the API which wraps this is also marked as transfer none (gtk_action_create_tool_item) and the code that calls this is all setup to receive a floating ref that it sinks, changing it seems like it would be an API break. In any case if gtk_action_create_tool_item starts returning an already sunk object, it will leak. This could be hacked around by doing a force_floating on the results of the vfunc and then gtk_action_create_tool_item will continue to maintain the same expected floating results. But is this kind of thing acceptable? If it is we might not even need to use the reserved pointers but just start mark the existing vfunc as transfer full and then force the result as floating? -Simon ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
On 07.02.2013 01:12, Simon Feltman wrote: Unfortunately there are a few more of these: Gtk.Action.create_menu Gtk.Action.create_menu_item Gtk.Action.create_tool_item Gtk.PrintOperation.create_custom_widget Gtk.PrintOperation.create-custom-widget (signal) Gladeui.EditorProperty.create_input Gladeui.BaseEditor.build_child? Gladeui.BaseEditor.build-child (signal) Also: Gtk.CellRenderer.start_editing. ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
I've created a ticket and proposed patch for GTK+: https://bugzilla.gnome.org/show_bug.cgi?id=693393 I am looking for feedback to see if this type of thing is even acceptable and if it's worthy of further pursuit. Thanks, -Simon On Thu, Feb 7, 2013 at 1:16 PM, Torsten Schoenfeld kaffeeti...@gmx.dewrote: On 07.02.2013 01:12, Simon Feltman wrote: Unfortunately there are a few more of these: Gtk.Action.create_menu Gtk.Action.create_menu_item Gtk.Action.create_tool_item Gtk.PrintOperation.create_**custom_widget Gtk.PrintOperation.create-**custom-widget (signal) Gladeui.EditorProperty.create_**input Gladeui.BaseEditor.build_**child? Gladeui.BaseEditor.build-child (signal) Also: Gtk.CellRenderer.start_**editing. __**_ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/**mailman/listinfo/gtk-devel-**listhttps://mail.gnome.org/mailman/listinfo/gtk-devel-list ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
Ticket and patch for the transient argument to the GtkCellRendererText editing-started signal: https://bugzilla.gnome.org/show_bug.cgi?id=693400 This is what I was proposing when I started this thread. If this kind of fix is ok, I have no problem looking for more cases in the GTK+ code base. Thanks, -Simon On Thu, Feb 7, 2013 at 7:08 PM, Simon Feltman s.felt...@gmail.com wrote: I've created a ticket and proposed patch for GTK+: https://bugzilla.gnome.org/show_bug.cgi?id=693393 I am looking for feedback to see if this type of thing is even acceptable and if it's worthy of further pursuit. Thanks, -Simon On Thu, Feb 7, 2013 at 1:16 PM, Torsten Schoenfeld kaffeeti...@gmx.dewrote: On 07.02.2013 01:12, Simon Feltman wrote: Unfortunately there are a few more of these: Gtk.Action.create_menu Gtk.Action.create_menu_item Gtk.Action.create_tool_item Gtk.PrintOperation.create_**custom_widget Gtk.PrintOperation.create-**custom-widget (signal) Gladeui.EditorProperty.create_**input Gladeui.BaseEditor.build_**child? Gladeui.BaseEditor.build-child (signal) Also: Gtk.CellRenderer.start_**editing. __**_ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/**mailman/listinfo/gtk-devel-**listhttps://mail.gnome.org/mailman/listinfo/gtk-devel-list ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
Hi Simon,
I didn't see this thread earlier. I wanted to chime in to strongly
support the view that:
* Floating references are C-convenience only
* Languages can and should sink floating references when they
first touch them.
* Any interface or code in libraries that create problems with
this are buggy.
This has been the view from day 1, and I think you'll create
considerably bigger problems trying to futz around with it and treat
floating references specially in a binding than you have now.
On Tue, 2013-02-05 at 05:33 -0800, Simon Feltman wrote:
For completeness, the two major problems are as follows
https://bugzilla.gnome.org/show_bug.cgi?id=687522
This is a vfunc implementation which the gtk internals are basically
expecting a floating ref from. Using the standard scheme just listed,
we sink and own the created MenuToolButton. The held widget is then
finalized at the end of the vfunc, returning an invalid object back to
the caller. If we add an extra ref we get a leak because the method is
marked as transfer-none. Example:
class ToolMenuAction(Gtk.Action):
def do_create_tool_item(self):
return Gtk.MenuToolButton()
This is basically broken API at the GTK+ level :-( ... a virtual
function can't return (transfer none) unless it's a getter for an
existing field. It is expected that language bindings will have no way
to create a floating object, so a virtual function cannot expect to be
returned a floating object.
The only thing I can see at the GTK+ level would be to add a
make_tool_item replacement vfunc and use that instead if non-null.
There's a workaround at the application level, which is something like:
def do_create_tool_item(self):
button = Gtk.MenuToolButton()
self.buttons.append(button)
button.connect('destroy', self.remove_from_buttons)
return button
we can document this bug in the gtk-doc and suggest the workaround
there. But I'd strongly suggest *not* doing wholesale changes to the
Python memory management based on this bug.
https://bugzilla.gnome.org/show_bug.cgi?id=661359
This is a very simple case of a widget as a parameter being marshaled
as an in arg to a callback. But because the gtk internals have not yet
sunk the floating ref for the editable parameter, PyGObject will do
so. By the time the callback is finished, the editable will be
finalized leaving gtk with a bad object. It should really just be
adding a safety ref during the lifetime of the wrapper and not mess
with the floating flag.
def on_view_label_cell_editing_started(renderer, editable, path):
print path
renderer = Gtk.CellRendererText()
renderer.connect('editing-started',
on_view_label_cell_editing_started)
This one is is simple. GTK+ needs to sink the arg before calling the
function. There should be no compatibility problems. The pygi patch on
the bug appears simply wrong and probably is creating the leak you
noticed.
- Owen
(I apologize for any duplication of earlier discussion on the thread)
___
gtk-devel-list mailing list
gtk-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
Owen,
Thank you, this will definitely make things easier. Comments below:
* Floating references are C-convenience only
It might be a convenience for writing code, not necessarily reading or
understanding it. Something more reasonable would be explicitly
named convenience functions which steal a ref instead of adding one. But
that may only be an alternate reality at this point.
* Languages can and should sink floating references when they
first touch them.
* Any interface or code in libraries that create problems with
this are buggy.
I will submit bug reports and patches where applicable.
This has been the view from day 1, and I think you'll create
considerably bigger problems trying to futz around with it and treat
floating references specially in a binding than you have now.
Agreed.
The only thing I can see at the GTK+ level would be to add a
make_tool_item replacement vfunc and use that instead if non-null.
There's a workaround at the application level, which is something like:
def do_create_tool_item(self):
button = Gtk.MenuToolButton()
self.buttons.append(button)
button.connect('destroy', self.remove_from_buttons)
return button
we can document this bug in the gtk-doc and suggest the workaround
there. But I'd strongly suggest *not* doing wholesale changes to the
Python memory management based on this bug.
Docs will help, we can also detect the bad situation and assert giving some
explanation to minimize support load and keep future maintainers from
trying to fix it again. Would something like the following be safe in
regards to testing the GObject ref count?
assert !(transfer == nothing pyobject-ref_count == 1
gobject-ref_count ==1)
This means the Python wrapper is only held in the out args tuple and will
be free'd when the closure is finalized, deleting the gobject along with it
and giving back a bad object. We could also add an additional ref to the
gobject and throw up a nasty warning.
Unfortunately there are a few more of these:
Gtk.Action.create_menu
Gtk.Action.create_menu_item
Gtk.Action.create_tool_item
Gtk.PrintOperation.create_custom_widget
Gtk.PrintOperation.create-custom-widget (signal)
Gladeui.EditorProperty.create_input
Gladeui.BaseEditor.build_child?
Gladeui.BaseEditor.build-child (signal)
def on_view_label_cell_editing_started(renderer, editable, path):
print path
renderer = Gtk.CellRendererText()
renderer.connect('editing-started',
on_view_label_cell_editing_started)
This one is is simple. GTK+ needs to sink the arg before calling the
function. There should be no compatibility problems. The pygi patch on
the bug appears simply wrong and probably is creating the leak you
noticed.
Yes, definitely a leak. However, fixing it will break people again. So the
gtk bug needs to be fixed first. There is also no telling how many of these
there are without looking through all vfuncs/signals which take a widget as
an input arg with transfer=none.
Thanks,
-Simon
___
gtk-devel-list mailing list
gtk-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
The side affect of GtkWindow is actually fine in this case because the annotation of transfer none for gtk_window_new makes sense here. The function is specifying it returns an internal borrowed reference and Python will add an additional ref during the wrappers lifetime. However, there is trouble when using g_object_new(GTK_TYPE_WINDOW) because it is annotated as transfer full which means the constructor is giving us full ownership of that internal reference and without some type of special case, the language binding wrappers will free that internal reference. The cleanest fix would be for gtk_window_new to specify transfer full along with sinking and adding an additional ref before returning, this would at least give consistency between g_object_new(GTK_TYPE_WINDOW) and gtk_window_new in terms of how introspection based bindings see it. But this is most likely out of the question. Instead it will take some combination of tracking and testing things like: is derived from InitiallyUnowned, is it floating, are we calling a constructor, and what is the ownership transfer. And then try to make a best guess as to what we are supposed to do. -Simon On Mon, Feb 4, 2013 at 11:05 PM, Tristan Van Berkom t...@gnome.org wrote: On Tue, Feb 5, 2013 at 12:08 PM, Simon Feltman s.felt...@gmail.com wrote: I could easily be misunderstanding the internals, but at some point isn't a call to something like gtk_widget_set_parent on the children needed for widgets to ever be displayed or useful? (which sinks the children) One of the more gross internal details of GTK+ is that GtkWindows (any toplevel widgets) get added to an internal 'list of toplevels'. So a GtkWindow is an odd subclass that (like someone else pointed out), sinks it's own floating reference at initialization time. The ownership of the window is virtually given to GTK+ and then disposed of automatically at gtk_widget_destory() time. I suppose that strictly speaking, an object constructor can indeed have side effects (but I can't think of any case where it would be anywhere close to 'sane' to intentionally use object constructors for their side effects and ignore the results). Best, -Tristan If it really might be a problem we could work around the leak by tracking if the instance was created within python and if the instance has ever been marshaled to C. At which point we could rely on the GC cleanup of the wrapper to sink and unref the extra ref in cases the GObject was never passed on to C at any point. This sucks but it seems a little better than checking GObject ref counts during marshaling and floating sunk objects based on if it was initially floating and the GObject ref count is only 1, which might be unsafe. -Simon On Mon, Feb 4, 2013 at 4:56 AM, Torsten Schoenfeld kaffeeti...@gmx.de wrote: On 04.02.2013 03:39, Simon Feltman wrote: I am starting to warm up to an idea where we simply never sink objects and always follow the rules entailed by ownership transference annotations already in place, with one caveat: g_object_new is annotated as transfer full but can also return floating references. In this case, we must check the returned type and not believe the annotation when it returns InitiallyUnowned instances, but instead treat it like transfer none and add a new ref. What about custom implementations of classes that are supposed to take over floating refs? For example, how would you write a custom GtkContainer subclass in Python with your scheme? Wouldn't you then need a way to explicitly sink the floating ref? ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
On 29/01/13 11:44, Simon Feltman wrote: It seems like the problem at hand can be solved by maintaining the floating ref and adding our own safety ref for the wrapper. My impression was that floating references were purely for C convenience, and that interpreted languages with their own refcounting should be turning them into normal refs that can be reasoned about more easily. Shouldn't PyGObject rather be sinking *every* floating ref it sees? (Effectively, the Python wrapper would behave like a container, I suppose.) Then you'd have: (transfer none) function returning a pointer to a non-floating object: g_object_ref_sink() refs it; the wrapper now owns a normal ref. If you gtk_container_add() the wrapped object, the ref is no longer floating, so the container takes a new ref and the wrapper still owns the old ref. If the Python wrapper is destroyed, the C container still keeps the wrapped object alive. Good? (transfer none) function returning a pointer to a floating object: g_object_ref_sink() turns the floating ref into a normal ref. Proceed as above. Good? (transfer full) function returning a normal ref (as detected by g_object_is_floating() == FALSE): do nothing, the wrapper takes ownership of that ref. Proceed as above. Good? (transfer full) function returning a floating ref (as detected by g_object_is_floating() == TRUE): g_object_ref_sink() refs it, the wrapper now owns a normal ref. Proceed as above. Good? Pseudocode: def give_me_a_real_ref(function): if function.transfer_none: return g_object_ref_sink(function()) else: tmp = function() if g_object_is_floating(tmp): g_object_ref_sink(tmp) return tmp Or is that wrong? Regards, S ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
This is basically how PyGObject works now. There are no problems with this during casual usage when Python is always in the position of the caller. The problem is this scheme does not work with the marshaling of floating widgets passed into Python vfuncs/closures as arguments or intended as return values from them. I just added a bunch of commentary to the following bug about why it is failing: https://bugzilla.gnome.org/show_bug.cgi?id=657202#c21 ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
For completeness, the two major problems are as follows:
https://bugzilla.gnome.org/show_bug.cgi?id=687522
This is a vfunc implementation which the gtk internals are basically
expecting a floating ref from. Using the standard scheme just listed, we
sink and own the created MenuToolButton. The held widget is then finalized
at the end of the vfunc, returning an invalid object back to the caller. If
we add an extra ref we get a leak because the method is marked as
transfer-none. Example:
class ToolMenuAction(Gtk.Action):
def do_create_tool_item(self):
return Gtk.MenuToolButton()
https://bugzilla.gnome.org/show_bug.cgi?id=661359
This is a very simple case of a widget as a parameter being marshaled as an
in arg to a callback. But because the gtk internals have not yet sunk the
floating ref for the editable parameter, PyGObject will do so. By the
time the callback is finished, the editable will be finalized leaving gtk
with a bad object. It should really just be adding a safety ref during the
lifetime of the wrapper and not mess with the floating flag.
def on_view_label_cell_editing_started(renderer, editable, path):
print path
renderer = Gtk.CellRendererText()
renderer.connect('editing-started', on_view_label_cell_editing_started)
-Simon
On Tue, Feb 5, 2013 at 5:09 AM, Simon Feltman s.felt...@gmail.com wrote:
This is basically how PyGObject works now. There are no problems with this
during casual usage when Python is always in the position of the caller.
The problem is this scheme does not work with the marshaling of floating
widgets passed into Python vfuncs/closures as arguments or intended as
return values from them. I just added a bunch of commentary to the
following bug about why it is failing:
https://bugzilla.gnome.org/show_bug.cgi?id=657202#c21
___
gtk-devel-list mailing list
gtk-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
On Tue, 2013-02-05 at 05:33 -0800, Simon Feltman wrote: For completeness, the two major problems are as follows: https://bugzilla.gnome.org/show_bug.cgi?id=687522 This is a vfunc implementation which the gtk internals are basically expecting a floating ref from. Using the standard scheme just listed, we sink and own the created MenuToolButton. The held widget is then finalized at the end of the vfunc, returning an invalid object back to the caller. If we add an extra ref we get a leak because the method is marked as transfer-none. Giovanni's diagnosis sounds right to me; it's pretty unfortunate that there's a transient state where the object is unreferenced. Besides the timeout approach, you could also use a thread? But I guess that's also racy since there's a window in which the python mutex is not held, but the native code has not yet added a ref. Possibly an approach where on (transfer full) input parameters, and we're recursing (i.e. we have a chain of python - native - python), defer the unrefs until the next outermost python is reentered. ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
On Mon, Feb 04, 2013 at 04:44:02PM +0900, Tristan Van Berkom wrote: On Mon, Feb 4, 2013 at 11:39 AM, Simon Feltman s.felt...@gmail.com wrote: [...] However, it also adds a leak for the most basic (and useless) case: for i in range(10): Gtk.Button() This could arguably trigger a compiler warning, or even an error. This is nonsense. Since Gtk.Button() is not guaranteed *not* to have any side effects it is perfectly valid to run it without doing anything with the return value. In fact, since we talk about a dynamic language, the interpreter does not know, in general, what Gtk.Button will mean at the time the code is actually executed. Yeti ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
On 04.02.2013 03:39, Simon Feltman wrote: I am starting to warm up to an idea where we simply never sink objects and always follow the rules entailed by ownership transference annotations already in place, with one caveat: g_object_new is annotated as transfer full but can also return floating references. In this case, we must check the returned type and not believe the annotation when it returns InitiallyUnowned instances, but instead treat it like transfer none and add a new ref. What about custom implementations of classes that are supposed to take over floating refs? For example, how would you write a custom GtkContainer subclass in Python with your scheme? Wouldn't you then need a way to explicitly sink the floating ref? ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
I could easily be misunderstanding the internals, but at some point isn't a call to something like gtk_widget_set_parent on the children needed for widgets to ever be displayed or useful? (which sinks the children) If it really might be a problem we could work around the leak by tracking if the instance was created within python and if the instance has ever been marshaled to C. At which point we could rely on the GC cleanup of the wrapper to sink and unref the extra ref in cases the GObject was never passed on to C at any point. This sucks but it seems a little better than checking GObject ref counts during marshaling and floating sunk objects based on if it was initially floating and the GObject ref count is only 1, which might be unsafe. -Simon On Mon, Feb 4, 2013 at 4:56 AM, Torsten Schoenfeld kaffeeti...@gmx.dewrote: On 04.02.2013 03:39, Simon Feltman wrote: I am starting to warm up to an idea where we simply never sink objects and always follow the rules entailed by ownership transference annotations already in place, with one caveat: g_object_new is annotated as transfer full but can also return floating references. In this case, we must check the returned type and not believe the annotation when it returns InitiallyUnowned instances, but instead treat it like transfer none and add a new ref. What about custom implementations of classes that are supposed to take over floating refs? For example, how would you write a custom GtkContainer subclass in Python with your scheme? Wouldn't you then need a way to explicitly sink the floating ref? __**_ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/**mailman/listinfo/gtk-devel-**listhttps://mail.gnome.org/mailman/listinfo/gtk-devel-list ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
On Tue, Feb 5, 2013 at 12:08 PM, Simon Feltman s.felt...@gmail.com wrote: I could easily be misunderstanding the internals, but at some point isn't a call to something like gtk_widget_set_parent on the children needed for widgets to ever be displayed or useful? (which sinks the children) One of the more gross internal details of GTK+ is that GtkWindows (any toplevel widgets) get added to an internal 'list of toplevels'. So a GtkWindow is an odd subclass that (like someone else pointed out), sinks it's own floating reference at initialization time. The ownership of the window is virtually given to GTK+ and then disposed of automatically at gtk_widget_destory() time. I suppose that strictly speaking, an object constructor can indeed have side effects (but I can't think of any case where it would be anywhere close to 'sane' to intentionally use object constructors for their side effects and ignore the results). Best, -Tristan If it really might be a problem we could work around the leak by tracking if the instance was created within python and if the instance has ever been marshaled to C. At which point we could rely on the GC cleanup of the wrapper to sink and unref the extra ref in cases the GObject was never passed on to C at any point. This sucks but it seems a little better than checking GObject ref counts during marshaling and floating sunk objects based on if it was initially floating and the GObject ref count is only 1, which might be unsafe. -Simon On Mon, Feb 4, 2013 at 4:56 AM, Torsten Schoenfeld kaffeeti...@gmx.de wrote: On 04.02.2013 03:39, Simon Feltman wrote: I am starting to warm up to an idea where we simply never sink objects and always follow the rules entailed by ownership transference annotations already in place, with one caveat: g_object_new is annotated as transfer full but can also return floating references. In this case, we must check the returned type and not believe the annotation when it returns InitiallyUnowned instances, but instead treat it like transfer none and add a new ref. What about custom implementations of classes that are supposed to take over floating refs? For example, how would you write a custom GtkContainer subclass in Python with your scheme? Wouldn't you then need a way to explicitly sink the floating ref? ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
After some deliberation and writing a bunch of tests: http://git.gnome.org/browse/pygobject/commit/?id=97f48f I am starting to warm up to an idea where we simply never sink objects and always follow the rules entailed by ownership transference annotations already in place, with one caveat: g_object_new is annotated as transfer full but can also return floating references. In this case, we must check the returned type and not believe the annotation when it returns InitiallyUnowned instances, but instead treat it like transfer none and add a new ref. The reason I think this will work is because libraries that make use of InitiallyUnowned are designed to deal with the floating refs already. Python is basically a thin wrapper used to tie objects together within these systems and never needs to maintain ownership beyond making sure the underlying object is kept alive while the wrapper is alive. From this, we can rely on the internals of the libraries to do the right thing and sink floating references where they normally would in C usage. If they don't, it is most likely a bug in C as well (by convention). This seems like it will solve all of the current problems and special casing during marshaling. However, it also adds a leak for the most basic (and useless) case: for i in range(10): Gtk.Button() This would leak the initial floating ref and the memory would be lost. However, I can't think of a real use case where something like that would ever be needed. The alternatives to can become grossly convoluted: https://bugzilla.gnome.org/show_bug.cgi?id=687522#c15 Thoughts? -Simon On Tue, Jan 29, 2013 at 3:44 AM, Simon Feltman s.felt...@gmail.com wrote: I tend to agree we should be avoiding reliance on main loops (or GC timing) to get the ref counts right if possible. PyGObject also uses toggle refs similarly to gjs for keeping the wrappers alive. However, in PyGObject this only happens if a Python instance attribute is set. Whereas with gjs it seems to use a toggle ref all the time just in case an attribute is set? It seems like the problem at hand can be solved by maintaining the floating ref and adding our own safety ref for the wrapper. With one caveat: upon completion of the python callback we may consider sinking the GObject if the ref is floating and the Python wrapper has a reference count greater than one. This basically means code in the callback made an assignment of the object to something outside of its scope and that should be considered a strong reference. But that might not even be necessary. I've attempted to describe this along with all the other problematic reference counting situations in a separate document: https://live.gnome.org/PyGObject/Analysis/ObjectReferenceCountingForVFuncsAndClosures The biggest concern at this point is how to properly deal with vfunc implementations which return objects and are annotated as transfer none. Review, corrections, and feedback is very welcome. Thanks, -Simon On Fri, Jan 18, 2013 at 12:19 AM, Tristan Van Berkom t...@gnome.orgwrote: On Fri, Jan 18, 2013 at 5:49 AM, Giovanni Campagna scampa.giova...@gmail.com wrote: [...] I know that Python doesn't have a GC in the traditional sense, but you could still send finalization for GObject wrappers to a idle callback so there is no risk of finalizing objects that C code assumes are still alive. That doesn't sound like a very safe workaround to me. There are situations where a lot of code can run without the mainloop ever becoming idle, while running a ClutterTimeline is one of those cases (or at least I've observed that idle callbacks dont generally get called while a ClutterTimeline is playing, perhaps they do with an ultra high priority). Another thing to consider is that not all code written with the glib stack is actually reactive event based, code that does not run a mainloop will risk blowing up in size quickly, possibly attaining out of memory conditions unnecessarily if the code happens to be highly recursive. Cheers, -Tristan ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
On Mon, Feb 4, 2013 at 11:39 AM, Simon Feltman s.felt...@gmail.com wrote: [...] However, it also adds a leak for the most basic (and useless) case: for i in range(10): Gtk.Button() This could arguably trigger a compiler warning, or even an error. In any case, since there is no variable assignment, I don't think it makes sense to make an effort to manage the returned memory (so I think your reasoning here is perfectly sound). I only wonder if it can be done using the G_IS_INITIALLY_UNOWNED() and g_object_is_floating() APIs instead of GIR annotations. Aside from the low-level platform stack I don't think the gtk-doc annotations are widely used, or obeyed to the letter (so it feels a bit scary to trust those annotations)... not sure if that is wildly impossible, but I was under the impression that language bindings have been using the 'floating' indicators for a long time before GIRs started to exist. Best of luck, -Tristan This would leak the initial floating ref and the memory would be lost. However, I can't think of a real use case where something like that would ever be needed. The alternatives to can become grossly convoluted: https://bugzilla.gnome.org/show_bug.cgi?id=687522#c15 Thoughts? -Simon On Tue, Jan 29, 2013 at 3:44 AM, Simon Feltman s.felt...@gmail.com wrote: I tend to agree we should be avoiding reliance on main loops (or GC timing) to get the ref counts right if possible. PyGObject also uses toggle refs similarly to gjs for keeping the wrappers alive. However, in PyGObject this only happens if a Python instance attribute is set. Whereas with gjs it seems to use a toggle ref all the time just in case an attribute is set? It seems like the problem at hand can be solved by maintaining the floating ref and adding our own safety ref for the wrapper. With one caveat: upon completion of the python callback we may consider sinking the GObject if the ref is floating and the Python wrapper has a reference count greater than one. This basically means code in the callback made an assignment of the object to something outside of its scope and that should be considered a strong reference. But that might not even be necessary. I've attempted to describe this along with all the other problematic reference counting situations in a separate document: https://live.gnome.org/PyGObject/Analysis/ObjectReferenceCountingForVFuncsAndClosures The biggest concern at this point is how to properly deal with vfunc implementations which return objects and are annotated as transfer none. Review, corrections, and feedback is very welcome. Thanks, -Simon On Fri, Jan 18, 2013 at 12:19 AM, Tristan Van Berkom t...@gnome.org wrote: On Fri, Jan 18, 2013 at 5:49 AM, Giovanni Campagna scampa.giova...@gmail.com wrote: [...] I know that Python doesn't have a GC in the traditional sense, but you could still send finalization for GObject wrappers to a idle callback so there is no risk of finalizing objects that C code assumes are still alive. That doesn't sound like a very safe workaround to me. There are situations where a lot of code can run without the mainloop ever becoming idle, while running a ClutterTimeline is one of those cases (or at least I've observed that idle callbacks dont generally get called while a ClutterTimeline is playing, perhaps they do with an ultra high priority). Another thing to consider is that not all code written with the glib stack is actually reactive event based, code that does not run a mainloop will risk blowing up in size quickly, possibly attaining out of memory conditions unnecessarily if the code happens to be highly recursive. Cheers, -Tristan ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
On Fri, Jan 18, 2013 at 5:49 AM, Giovanni Campagna scampa.giova...@gmail.com wrote: [...] I know that Python doesn't have a GC in the traditional sense, but you could still send finalization for GObject wrappers to a idle callback so there is no risk of finalizing objects that C code assumes are still alive. That doesn't sound like a very safe workaround to me. There are situations where a lot of code can run without the mainloop ever becoming idle, while running a ClutterTimeline is one of those cases (or at least I've observed that idle callbacks dont generally get called while a ClutterTimeline is playing, perhaps they do with an ultra high priority). Another thing to consider is that not all code written with the glib stack is actually reactive event based, code that does not run a mainloop will risk blowing up in size quickly, possibly attaining out of memory conditions unnecessarily if the code happens to be highly recursive. Cheers, -Tristan ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
I cannot offer a solution, just a grumpy observation: The problem occurring with this kind of ref sequence is due to gobject-introspection's decision to always set transfer-ownership=none for GInitiallyUnowned descendants and the resulting habit of language bindings to always sink floating refs. For previous discussion: https://bugzilla.gnome.org/show_bug.cgi?id=657202. On 17.01.2013 01:42, Simon Feltman wrote: The pygobject specific problems I mention could be solved by tracking incoming floating refs and re-floating them upon closure exit if the python ref is not stored anywhere (or by attempting to just keep it floating). The approach in parentheses is used by the Perl bindings: • For constructors which return GInitiallyUnowned instances, we internally force transfer-ownership=full. • We generally only sink floating refs if transfer-ownership=full. Thus, there is no need to add and remove an additional ref during closure marshalling and the GtkEntry in the example keeps its floating flag. ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
2013/1/17 Simon Feltman s.felt...@gmail.com: Hi Everyone, I'm trying to figure out a complete solution for proper reference management of gobjects passed into python vfuncs and signal closures as in arguments. Currently pygobject will add a ref to gobject arguments marked as transfer-none during marshaling and additionally sink/ref the gobject when the python object wrapper is created around it. The initial added ref during marshaling is then leaked upon the closure finishing in most cases. The fix could simply be to not add the initial ref during marshaling (or we need to make sure it is released when the closure finishes). The problem is this behavior is relied upon when a floating ref is passed and is the explicit fix described in the following ticket (which un-fortunately causes a leak in the non-floating case): https://bugzilla.gnome.org/show_bug.cgi?id=661359 The specific problem described in bug 661359 occurs when a python closure is connected to a GtkCellRendererTexts editing-started signal. During the start of editing, a GtkEntry is created and passed almost immediately to signal emission before any ownership takes place: http://git.gnome.org/browse/gtk+/tree/gtk/gtkcellrenderer.c#n864 This results in the following ref counting behavior of the GtkEntry as it is used by gtk_cell_area_activate_cell and gtk_cell_renderer_start_editing: 1 (floating) - gtk_cell_renderer_text_start_editing creates the GtkEntry 2 (floating) - g_signal_emit argument marshaling adds an additional ref during signal emission 3 (floating) - python closure marshaling adds an additional ref described above 3 (owned) - A PyGObject wraps the GtkEntry and sinks the floating ref * python callback is called and passed the wrapper 2 (owned) - The PyGObject wrapper is destroyed after the python callback finishes additionally freeing a gobject ref 1 (owned) - g_signal_emit argument marshaling completes and frees the ref it added 2 (owned) - gtk_cell_area_activate_cell sets the CellAreas edit widget which adds a ref 1 (owned) - cell editing finishes and removes the widget from the CellArea I don't know the internals of pygobject, but to me it looks the leak comes from not releasing the reference you add inside the callback marshaller. The way it works in gjs is: 1 (floating) - gtk_cell_renderer_text_start_editing creates the GtkEntry 2 (floating) - g_signal_emit argument marshaling adds an additional ref during signal emission 2 (owned) - gjs closure marshaling creates a JS object wrapper, calling ref_sink on the object 2 (owned, toggle, GC root) gjs adds a toggle reference and then immediately unrefs the object 1 (owned, toggle, not rooted) - g_signal_emit argument marshaling completes and frees the ref it added 2 (owned, toggle, GC root) - gtk_cell_area_activate_cell sets the CellAreas edit widget which adds a ref 1 (owned, toggle, not rooted) - cell editing finishes and removes the widget from the CellArea ... time passes ... 0 - GC triggers and the JS object finalizer removes the last ref I know that Python doesn't have a GC in the traditional sense, but you could still send finalization for GObject wrappers to a idle callback so there is no risk of finalizing objects that C code assumes are still alive. Giovanni ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
Hi Everyone, I'm trying to figure out a complete solution for proper reference management of gobjects passed into python vfuncs and signal closures as in arguments. Currently pygobject will add a ref to gobject arguments marked as transfer-none during marshaling and additionally sink/ref the gobject when the python object wrapper is created around it. The initial added ref during marshaling is then leaked upon the closure finishing in most cases. The fix could simply be to not add the initial ref during marshaling (or we need to make sure it is released when the closure finishes). The problem is this behavior is relied upon when a floating ref is passed and is the explicit fix described in the following ticket (which un-fortunately causes a leak in the non-floating case): https://bugzilla.gnome.org/show_bug.cgi?id=661359 The specific problem described in bug 661359 occurs when a python closure is connected to a GtkCellRendererTexts editing-started signal. During the start of editing, a GtkEntry is created and passed almost immediately to signal emission before any ownership takes place: http://git.gnome.org/browse/gtk+/tree/gtk/gtkcellrenderer.c#n864 This results in the following ref counting behavior of the GtkEntry as it is used by gtk_cell_area_activate_cell and gtk_cell_renderer_start_editing: 1 (floating) - gtk_cell_renderer_text_start_editing creates the GtkEntry 2 (floating) - g_signal_emit argument marshaling adds an additional ref during signal emission 3 (floating) - python closure marshaling adds an additional ref described above 3 (owned) - A PyGObject wraps the GtkEntry and sinks the floating ref * python callback is called and passed the wrapper 2 (owned) - The PyGObject wrapper is destroyed after the python callback finishes additionally freeing a gobject ref 1 (owned) - g_signal_emit argument marshaling completes and frees the ref it added 2 (owned) - gtk_cell_area_activate_cell sets the CellAreas edit widget which adds a ref 1 (owned) - cell editing finishes and removes the widget from the CellArea Given the above it seems cell editing will leak a new editor each time editing occurs even if no closures are connected to the editing-started signal (python or otherwise). The pygobject specific problems I mention could be solved by tracking incoming floating refs and re-floating them upon closure exit if the python ref is not stored anywhere (or by attempting to just keep it floating). However, the root cause of all this could simply be that cell editing needs better reference management of the editors being created. For instance, gtk_cell_renderer_text_start_editing creates the GtkEntry and stores it as an attribute in GtkCellRendererText.priv.entry without sinking the ref: http://git.gnome.org/browse/gtk+/tree/gtk/gtkcellrenderertext.c#n2049 I think this should be changed to immediately sink the ref and unref it during gtk_cell_renderer_text_editing_done. This would solve the python problems and fix the reference leak for the editor (and bug 661359 can be backed out). Additionally I think gtk_cell_area_set_edit_widget should be sinking the incoming editable widget instead of simply adding a ref: http://git.gnome.org/browse/gtk+/tree/gtk/gtkcellarea.c#n3309 As a general practice shouldn't refs be sunk when they are being assigned to a structured attribute especially when dealing with GInitiallyUnowned based objects? Insight into any of this is greatly appreciated. -Simon ___ gtk-devel-list mailing list gtk-devel-list@gnome.org https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)
On Thu, Jan 17, 2013 at 9:42 AM, Simon Feltman s.felt...@gmail.com wrote: Hi Everyone, I'm trying to figure out a complete solution for proper reference management of gobjects passed into python vfuncs and signal closures as in arguments. Currently pygobject will add a ref to gobject arguments marked as transfer-none during marshaling and additionally sink/ref the gobject when the python object wrapper is created around it. The initial added ref during marshaling is then leaked upon the closure finishing in most cases. The fix could simply be to not add the initial ref during marshaling (or we need to make sure it is released when the closure finishes). The problem is this behavior is relied upon when a floating ref is passed and is the explicit fix described in the following ticket (which un-fortunately causes a leak in the non-floating case): https://bugzilla.gnome.org/show_bug.cgi?id=661359 The specific problem described in bug 661359 occurs when a python closure is connected to a GtkCellRendererTexts editing-started signal. During the start of editing, a GtkEntry is created and passed almost immediately to signal emission before any ownership takes place: http://git.gnome.org/browse/gtk+/tree/gtk/gtkcellrenderer.c#n864 This results in the following ref counting behavior of the GtkEntry as it is used by gtk_cell_area_activate_cell and gtk_cell_renderer_start_editing: 1 (floating) - gtk_cell_renderer_text_start_editing creates the GtkEntry 2 (floating) - g_signal_emit argument marshaling adds an additional ref during signal emission 3 (floating) - python closure marshaling adds an additional ref described above 3 (owned) - A PyGObject wraps the GtkEntry and sinks the floating ref * python callback is called and passed the wrapper 2 (owned) - The PyGObject wrapper is destroyed after the python callback finishes additionally freeing a gobject ref 1 (owned) - g_signal_emit argument marshaling completes and frees the ref it added 2 (owned) - gtk_cell_area_activate_cell sets the CellAreas edit widget which adds a ref 1 (owned) - cell editing finishes and removes the widget from the CellArea Given the above it seems cell editing will leak a new editor each time editing occurs even if no closures are connected to the editing-started signal (python or otherwise). The pygobject specific problems I mention could be solved by tracking incoming floating refs and re-floating them upon closure exit if the python ref is not stored anywhere (or by attempting to just keep it floating). However, the root cause of all this could simply be that cell editing needs better reference management of the editors being created. For instance, gtk_cell_renderer_text_start_editing creates the GtkEntry and stores it as an attribute in GtkCellRendererText.priv.entry without sinking the ref: http://git.gnome.org/browse/gtk+/tree/gtk/gtkcellrenderertext.c#n2049 I think this should be changed to immediately sink the ref and unref it during gtk_cell_renderer_text_editing_done. This would solve the python problems and fix the reference leak for the editor (and bug 661359 can be backed out). Additionally I think gtk_cell_area_set_edit_widget should be sinking the incoming editable widget instead of simply adding a ref: http://git.gnome.org/browse/gtk+/tree/gtk/gtkcellarea.c#n3309 As a general practice shouldn't refs be sunk when they are being assigned to a structured attribute especially when dealing with GInitiallyUnowned based objects? Perhaps, but this would not solve your problem as I understand it. o Working around the problem by changing the way GTK+ code is written is not a viable long term solution afaict o Your issue as far as I can see is that a floating object gets passed as a signal argument, this can happen in a variety of situations, consider a construct where a code segment creates an object and passes it to another object immediately: -- BaseBall *ball = g_object_new (BASE_TYPE_BALL, pitch-type, PITCH_TYPE_CURVE, NULL); pitcher_throw_ball (pitcher, ball); -- In this case the pitcher might own the ball for some time, or it might in turn relinquish ownership and give the ball to the global Stadium * object. o Forcing the object to become floating again after the PyGObject is finalized feels rather hacky, it sounds like you will leak the underlying object in cases where ownership was actually intended (i.e. assignment to a PyObject member) Rather, it sounds like PyGObject should not mandate sinking the floating reference until ownership is actually implied, i.e. by tying the lifecycle of the said object to a member of another PyGObject (a strong reference). Stack variables on the other hand, such as declared and assigned in PyGobject functions, or passed to
