GNOME Bugzilla – Bug 736175
Regression: Gtk.TextBuffer.insert-text signal needs pass-by-ref for location argument
Last modified: 2014-09-12 06:43:26 UTC
This is a similar edge case to bug 734465 in that the signal is incorrectly annotated as input instead of inout. The difference being signal marshalling in pygi ignores argument direction with the exception of a pass-by-reference hack for out arguments [1]. This can potentially be solved with the following ideas: 1) Change the signal annotation to inout and allows the hack for both out and inout directions. This will make pygi backwards incompatible with older versions of GTK+ for this particular signal argument when attempts to modify the location argument are made. 2) Don't do any direction checks with regards to the hack and always pass structures by reference for signal args. This should maintain back compatibility but will continue to suffer from bug 722899 in the context of signals. [1] https://git.gnome.org/browse/pygobject/commit/?id=28d0337f0e3
A third slightly more insane solution is to copy the struct back to Python if the arguments ref count is more than 1 after the callback finishes.
I tried 2) but I think there is too much existing code that keeps references to signal arguments well after the callback, which then causes fatal crashes. It is quite strange also that you wouldn't be able to hold on to arguments that are supposed to be just (in). 1) seems reasonable, although it does rely on annotations being correct (which currently for gtk+ they aren't). The insane solution is indeed pretty hacky, but the upside is that you won't get _any_ crashes (which would still happen with 1) and 2)).
The following fix has been pushed: 4ebb1f5 tests: Add failing regression test for Gtk.TextBuffer.insert-text signal
Created attachment 285595 [details] [review] tests: Add failing regression test for Gtk.TextBuffer.insert-text signal
Note that gjs uses G_SIGNAL_TYPE_STATIC_SCOPE to determine if it should copy structs or not: https://git.gnome.org/browse/gjs/tree/gi/value.cpp?id=GJS_1_41_91#n141 While this might work as an initial fix for the problem here (instead of trying to use transfer direction which may or may not be valid). It will still suffer use after free errors if the callback stores the wrapped struct.
Created attachment 285606 [details] [review] overrides: Add comparison operators to Gtk.TextIter Use functools.total_ordering to ensure we have a complete set of comparison operators for Gtk.TextIter.
Created attachment 285607 [details] [review] Fix memory management problems with struct arguments to signals Replicate struct marshaling logic for determining if struct arguments to signals should be passed by reference to callbacks. Maintain a list of these structs and apply an in-place copy of the struct pointer if the struct wrapper is held longer than the duration of the Python callback. This allows for both mutation of struct arguments from callbacks as well as memory safety incase a callbacks holds onto the struct.
Review of attachment 285606 [details] [review]: This was an addition I thought I was going to need for testing. But it turned out text iters don't keep their validity after the modification callbacks anyhow, so I didn't end up using it. However, it still seems useful in general.
Review of attachment 285607 [details] [review]: I ended up trying out the more insane solution and it ended up being not that bad. Seems to be the most robust in terms of compatibility, functionality, and safety. Review and testing welcome.
Review of attachment 285607 [details] [review]: Additionally verified valgrind doesn't show anything new and the problem reported in bug 735486 continues to be fixed.
Review of attachment 285607 [details] [review]: ::: gi/pygi-signal-closure.c @@ +164,3 @@ + if (item && PyObject_IsInstance (item, (PyObject *) &PyGIBoxed_Type)) { + ((PyGBoxed *)item)->free_on_dealloc = FALSE; + pass_by_ref_structs = g_slist_append (pass_by_ref_structs, item); Minor nitpick, maybe use g_slist_prepend here?
Created attachment 285608 [details] [review] Fix memory management problems with struct arguments to signals Use g_slist_prepend and add foreign struct check.
Review of attachment 285608 [details] [review]: ::: gi/pygi-signal-closure.c @@ +152,3 @@ GType gtype = g_registered_type_info_get_g_type ((GIRegisteredTypeInfo *) info); + if (!g_type_is_a (gtype, G_TYPE_VALUE) && !g_struct_info_is_foreign (info) && + g_type_is_a (gtype, G_TYPE_BOXED)) { I don't know too much about this stuff, but shouldn't g_struct_info_is_foreign only be called when info_type == GI_INFO_TYPE_STRUCT, and shouldn't it be cast to (GIStructInfo *)?
(In reply to comment #19) > Review of attachment 285608 [details] [review]: > > ::: gi/pygi-signal-closure.c > @@ +152,3 @@ > GType gtype = g_registered_type_info_get_g_type > ((GIRegisteredTypeInfo *) info); > + if (!g_type_is_a (gtype, G_TYPE_VALUE) && > !g_struct_info_is_foreign (info) && > + g_type_is_a (gtype, G_TYPE_BOXED)) { > > I don't know too much about this stuff, but shouldn't g_struct_info_is_foreign > only be called when info_type == GI_INFO_TYPE_STRUCT, and shouldn't it be cast > to (GIStructInfo *)? Yeah, that is definitely an error. This was copied from the marshaling code path which is also wrong. Although I don't think GI_INFO_TYPE_BOXED is actually used and it's docs aren't very helpful either: "boxed, see GIStructInfo or GIUnionInfo" Luckily the foreign field on StructBlob maps to a reserved bit on UnionBlob which is probably why this hasn't failed yet: https://git.gnome.org/browse/gobject-introspection/tree/girepository/gitypelib-internal.h?id=GOBJECT_INTROSPECTION_1_41_91#n790
Created attachment 285622 [details] [review] Limit foreign struct checks to GI_INFO_TYPE_STRUCT Add struct type check before calling g_struct_info_is_foreign().
Created attachment 285623 [details] [review] Fix memory management problems with struct arguments to signals Replicate struct marshaling logic for determining if struct arguments to signals should be passed by reference to callbacks. Maintain a list of these structs and apply an in-place copy of the struct pointer if the struct wrapper is held longer than the duration of the Python callback. This allows for both mutation of struct arguments from callbacks as well as memory safety incase a callbacks holds onto the struct.
This looks great, thanks for looking into this. This works perfectly fine for at least gedit now.
Review of attachment 285622 [details] [review]: ::: gi/pygi-argument.c @@ +1384,3 @@ GType g_type = g_registered_type_info_get_g_type ( (GIRegisteredTypeInfo *) info); + gboolean is_foreign = (info_type == GI_INFO_TYPE_STRUCT) && + (g_struct_info_is_foreign (info)); Maybe cast info?
Comment on attachment 285606 [details] [review] overrides: Add comparison operators to Gtk.TextIter Moved patch to new bug 736287
Created attachment 285683 [details] [review] Limit foreign struct checks to GI_INFO_TYPE_STRUCT Added cast and fixed a few more locations which also had this error.
Attachment 285623 [details] pushed as d70b300 - Fix memory management problems with struct arguments to signals Attachment 285683 [details] pushed as 09161ff - Limit foreign struct checks to GI_INFO_TYPE_STRUCT
*** Bug 736525 has been marked as a duplicate of this bug. ***