From da061f7caacc86d0f900d06a3f9b3b150ade05a7 Mon Sep 17 00:00:00 2001 From: Hugo Parente Lima Date: Tue, 21 Nov 2023 11:11:47 -0300 Subject: [PATCH] Fixes random crashes caused by bad wrapper instance cache. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When you create a GObject, GI-Crystal adds a qdata to it with a pointer to the Crystal object, so if this object goes into C world and appear later in e.g. a signal callback, the Crystal object can be restored and all Crystal code there, including memory for the isntance variables will work as expected. To avoid this object to be collected by GC is another subject. This behavior is needed by user types, because we must be able to access the instance variables, etc. But for wrappers this isn't needed since it only holds a pointer to the C object, but GICrystal was doing this anyway to save some memory allocations, as we know: _Early optimization is the root of all evil_ The problem is: If you a signal with a GObject parameter that is really a `Gtk::ListView` the signal code will do `GObject::Object.new(pointer, :none)`. This call will create the Crystal instance and save it in the qdata, so later when you do a cast like: `Gtk::ListView.cast` it seems to work, but it is returning the Crystal object for the `GObject::Object` memory layout, due to unsafe code the compiler will believe us and think it's a `Gtk::ListView`. When you try to use some specific `Gtk::ListView` method a crash can happen depending on how the compiler decided to do such method call, since the type id is for a different type it will just crash, I'm not sure if Crystal uses a vtable for that, if so this also explains the error. *Solution* Remove the cache for non-user types fixes the problem. But to make wrapper objects more Crystal-like the `self.new` was modified to identify the GType and call the right wrapper `.new` method for it. This means that is possible now to use Crystal casts for wrapper objects! This opens the possibility to avoid the Abstract classes created for each GObject interface, since we could simple do: `GObject::Object.new(ptr, :none).as(Interface)` And I tried, but crashed the compiler 😅. --- ecr/interface.ecr | 4 ++-- ecr/interface_g_type_method.ecr | 12 +++++++++++ ecr/object.ecr | 4 +++- ecr/object_g_type_method.ecr | 12 +++++++++++ spec/basic_spec.cr | 17 ++++++++++++++-- spec/inheritance_spec.cr | 5 +++-- spec/libtest/test_subject.c | 2 +- spec/list_spec.cr | 12 +++++------ spec/s_list_spec.cr | 13 ++++++------ spec/vfunc_spec.cr | 4 ++-- src/bindings/g_object/lib_g_object.cr | 3 +++ src/bindings/g_object/object.cr | 10 ++------- src/generator/object_gen.cr | 2 +- src/gi-crystal.cr | 29 ++++++++++++++++++++------- 14 files changed, 91 insertions(+), 38 deletions(-) create mode 100644 ecr/interface_g_type_method.ecr create mode 100644 ecr/object_g_type_method.ecr diff --git a/ecr/interface.ecr b/ecr/interface.ecr index e90e818..7b36edc 100644 --- a/ecr/interface.ecr +++ b/ecr/interface.ecr @@ -32,12 +32,12 @@ module <%= namespace_name %> class <%= abstract_interface_name(object, false) %> < GObject::Object include <%= type_name %> - GICrystal.declare_new_method(<%= abstract_interface_name(object) %>, g_object_get_qdata, g_object_set_qdata) + GICrystal.declare_new_method(<%= abstract_interface_name(object) %>, g_object_get_qdata) # Forbid users to create instances of this. private def initialize end - <% render "ecr/g_type_method.ecr" %> + <% render "ecr/interface_g_type_method.ecr" %> end end diff --git a/ecr/interface_g_type_method.ecr b/ecr/interface_g_type_method.ecr new file mode 100644 index 0000000..a75b6d5 --- /dev/null +++ b/ecr/interface_g_type_method.ecr @@ -0,0 +1,12 @@ +<% if object.type_init -%> + @@g_type : UInt64? + + # Returns the type id (GType) registered in GLib type system. + def self.g_type : UInt64 + @@g_type ||= <%= to_lib_namespace(namespace) %>.<%= object.type_init %>.tap do |g_type| + # Set the Crystal constructor on type qdata + ctor = -><%= abstract_interface_name(object) %>.new(Void*, GICrystal::Transfer) + LibGObject.g_type_set_qdata(g_type, GICrystal::INSTANCE_FACTORY, ctor.pointer) + end + end +<% end %> diff --git a/ecr/object.ecr b/ecr/object.ecr index 444510d..bd20e0e 100644 --- a/ecr/object.ecr +++ b/ecr/object.ecr @@ -77,10 +77,12 @@ module <%= namespace_name %> end <% end %> - <% render "ecr/g_type_method.ecr" %> + <% render "ecr/object_g_type_method.ecr" %> <% render_properties %> <% render_methods %> <% render_signals %> <% render_vfuncs %> + + def_equals_and_hash @pointer end end diff --git a/ecr/object_g_type_method.ecr b/ecr/object_g_type_method.ecr new file mode 100644 index 0000000..b1486bb --- /dev/null +++ b/ecr/object_g_type_method.ecr @@ -0,0 +1,12 @@ +<% if object.type_init -%> + @@g_type : UInt64? + + # Returns the type id (GType) registered in GLib type system. + def self.g_type : UInt64 + @@g_type ||= <%= to_lib_namespace(namespace) %>.<%= object.type_init %>.tap do |g_type| + # Set the Crystal constructor on type qdata + ctor = -><%= namespace_name %>::<%= type_name %>.new(Void*, GICrystal::Transfer) + LibGObject.g_type_set_qdata(g_type, GICrystal::INSTANCE_FACTORY, ctor.pointer) + end + end +<% end %> diff --git a/spec/basic_spec.cr b/spec/basic_spec.cr index 6441ccc..c56441e 100644 --- a/spec/basic_spec.cr +++ b/spec/basic_spec.cr @@ -113,14 +113,27 @@ describe "GObject Binding" do end describe "casts" do + it "don't use wrapper cache" do + ptr = LibGObject.g_object_new(Test::SubjectChild.g_type, "string", "hey", "int32", 42, Pointer(Void).null) + obj = GObject::Object.new(ptr, :full) + subject = Test::SubjectChild.cast(obj) + # If these pointers are equal, they are the same Crystal object, with luck it may not crash, but + # it can crash because the crystal type id is for GObject::Object instead of Test::SubjectChild. + # + # Note that if the type is an user type, these pointers must be the same! + obj.as(Void*).should_not eq(subject.as(Void*)) + end + it "can downcast objects" do child = Test::SubjectChild.new(string: "hey") gobj = child.me_as_gobject - gobj.ref_count.should eq(1) + gobj.ref_count.should eq(2) # we can't reuse the crystal wrapper of non-user objects gobj.class.should eq(Test::SubjectChild) cast = Test::Subject.cast(gobj) - cast.ref_count.should eq(1) + cast.ref_count.should eq(3) cast.string.should eq("hey") + child.should eq(gobj) + cast.should eq(gobj) end it "thrown an exception on bad casts" do diff --git a/spec/inheritance_spec.cr b/spec/inheritance_spec.cr index b54a71f..64e047d 100644 --- a/spec/inheritance_spec.cr +++ b/spec/inheritance_spec.cr @@ -66,7 +66,8 @@ describe "Classes inheriting GObject::Object" do casted_obj.object_id.should eq(obj.object_id) end - it "create a crystal instance if the object was born on C world" do + # Crystal types created in C world are not supported yet + pending "creates crystal object for Crystal type born in C world" do raw_gobj = LibGObject.g_object_newv(UserObject.g_type, 0, nil) wrapper = GObject::Object.new(raw_gobj, :full) obj = UserObject.cast(wrapper) @@ -78,7 +79,7 @@ describe "Classes inheriting GObject::Object" do obj.ref_count.should eq(1) end - it "create a crystal instance if the object with float ref was born on C world" do + pending "creates crystal object for Crystal type with float ref born in C world" do raw_gobj = LibGObject.g_object_newv(UserFloatRefObject.g_type, 0, nil) wrapper = GObject::Object.new(raw_gobj, :full) obj = UserFloatRefObject.cast(wrapper) diff --git a/spec/libtest/test_subject.c b/spec/libtest/test_subject.c index 98393fc..a503e22 100644 --- a/spec/libtest/test_subject.c +++ b/spec/libtest/test_subject.c @@ -519,7 +519,7 @@ GList* test_subject_return_list_of_iface_transfer_full(TestSubject* self) { GList* list = NULL; g_object_ref(self); list = g_list_append(list, self); - list = g_list_append(list, test_subject_new_from_string("Subject from C")); + list = g_list_append(list, test_subject_new_from_string("Born in C")); return list; } diff --git a/spec/list_spec.cr b/spec/list_spec.cr index 6f33d97..9347ccd 100644 --- a/spec/list_spec.cr +++ b/spec/list_spec.cr @@ -2,27 +2,27 @@ require "./spec_helper" describe "GList" do it "works with interfaces" do - subject = Test::Subject.new(string: "Subject from Crystal") + subject = Test::Subject.new(string: "Born in Crystal") list = subject.return_list_of_iface_transfer_full list.size.should eq(2) subject_from_c = list[0] subject_from_c.object_id.should eq(subject.object_id) - Test::Subject.cast(subject_from_c).string.should eq("Subject from Crystal") - Test::Subject.cast(list[1]).string.should eq("Subject from C") + Test::Subject.cast(subject_from_c).string.should eq("Born in Crystal") + Test::Subject.cast(list[1]).string.should eq("Born in C") end it "works with GObjects" do - subject = Test::Subject.new(string: "Subject from Crystal") + subject = Test::Subject.new(string: "Born in Crystal") list = subject.return_list_of_gobject_transfer_full list.size.should eq(2) subject_from_c = list[0] subject_from_c.object_id.should eq(subject.object_id) - Test::Subject.cast(subject_from_c).string.should eq("Subject from Crystal") - Test::Subject.cast(list[1]).string.should eq("Subject from C") + Test::Subject.cast(subject_from_c).string.should eq("Born in Crystal") + Test::Subject.cast(list[1]).string.should eq("Born in C") end it "works on transfer full" do diff --git a/spec/s_list_spec.cr b/spec/s_list_spec.cr index e98fdc1..df50168 100644 --- a/spec/s_list_spec.cr +++ b/spec/s_list_spec.cr @@ -7,9 +7,8 @@ describe "GSList" do list = subject.return_slist_of_iface_transfer_full list.size.should eq(2) - subject_from_c = list[0] - subject_from_c.object_id.should eq(subject.object_id) - Test::Subject.cast(subject_from_c).string.should eq("Subject from Crystal") + list[0].object_id.should eq(subject.object_id) + Test::Subject.cast(list[0]).string.should eq("Subject from Crystal") Test::Subject.cast(list[1]).string.should eq("Subject from C") end @@ -19,9 +18,11 @@ describe "GSList" do list = subject.return_slist_of_gobject_transfer_full list.size.should eq(2) - subject_from_c = list[0] - subject_from_c.object_id.should eq(subject.object_id) - Test::Subject.cast(subject_from_c).string.should eq("Subject from Crystal") + list[0].object_id.should eq(subject.object_id) + list[0].as(Test::Subject).string.should eq("Subject from Crystal") + Test::Subject.cast(list[0]).string.should eq("Subject from Crystal") + + list[1].as(Test::Subject).string.should eq("Subject from C") Test::Subject.cast(list[1]).string.should eq("Subject from C") end diff --git a/spec/vfunc_spec.cr b/spec/vfunc_spec.cr index cec6561..ecf3f61 100644 --- a/spec/vfunc_spec.cr +++ b/spec/vfunc_spec.cr @@ -86,7 +86,7 @@ describe "GObject vfuncs" do obj.float64.should eq(3.3) obj.string.should eq("string") subject = Test::Subject.cast(obj.obj) - subject.ref_count.should eq(1) + subject.ref_count.should eq(2) # now 2 wrappers for the same C obj subject.string.should eq("hey") end @@ -98,7 +98,7 @@ describe "GObject vfuncs" do obj.float64.should eq(3.3) obj.string.should eq("string") subject = Test::Subject.cast(obj.obj) - subject.ref_count.should eq(1) + subject.ref_count.should eq(2) # now 2 wrappers for the same C obj subject.string.should eq("hey") end diff --git a/src/bindings/g_object/lib_g_object.cr b/src/bindings/g_object/lib_g_object.cr index 429bb53..19788bf 100644 --- a/src/bindings/g_object/lib_g_object.cr +++ b/src/bindings/g_object/lib_g_object.cr @@ -5,6 +5,9 @@ lib LibGObject # So we can re-use the Crystal objects saving some memory allocations. fun g_object_set_qdata(object : Void*, quark : UInt32, data : Void*) + # This is used only tests + fun g_object_new(type : UInt64, first_property_name : Pointer(LibC::Char), ...) : Void* + fun g_signal_new(signal_name : LibC::Char*, itype : UInt64, signal_flags : UInt32, diff --git a/src/bindings/g_object/object.cr b/src/bindings/g_object/object.cr index 237172a..35ea2e0 100644 --- a/src/bindings/g_object/object.cr +++ b/src/bindings/g_object/object.cr @@ -444,13 +444,7 @@ module GObject # Cast a `GObject::Object` to this type, returns nil if cast can't be made. def self.cast?(obj : GObject::Object) : self? - return if LibGObject.g_type_check_instance_is_a(obj, g_type).zero? - - instance = GICrystal.instance_pointer(obj) - # This should never happen with GC resistant objects - raise GICrystal::ObjectCollectedError.new if instance.null? - - instance.as(self) + new(obj.to_unsafe, :none) unless LibGObject.g_type_check_instance_is_a(obj, g_type).zero? end # A hook to be executed after the underlying gobject has been initialized. @@ -634,7 +628,7 @@ module GObject # Cast a `GObject::Object` to `self`, returns nil if the cast can't be made. def self.cast?(obj : GObject::Object) : self? - new(obj.to_unsafe, GICrystal::Transfer::None) unless LibGObject.g_type_check_instance_is_a(obj, g_type).zero? + new(obj.to_unsafe, :none) unless LibGObject.g_type_check_instance_is_a(obj, g_type).zero? end end end diff --git a/src/generator/object_gen.cr b/src/generator/object_gen.cr index 7227b47..fcb7e0b 100644 --- a/src/generator/object_gen.cr +++ b/src/generator/object_gen.cr @@ -80,7 +80,7 @@ module Generator def render_qdata_optimized_new_method(io : IO) return if !object.inherits?("GObject") && !object.inherits?("GParam") - io << "GICrystal.declare_new_method(" << type_name << ',' << object.qdata_get_func << ',' << object.qdata_set_func << ")\n" + io << "GICrystal.declare_new_method(" << type_name << ',' << object.qdata_get_func << ")\n" end end end diff --git a/src/gi-crystal.cr b/src/gi-crystal.cr index 689fc6a..c3affe2 100644 --- a/src/gi-crystal.cr +++ b/src/gi-crystal.cr @@ -29,6 +29,8 @@ module GICrystal # See `declare_new_method`. INSTANCE_QDATA_KEY = LibGLib.g_quark_from_static_string("gi-crystal::instance") + # See `declare_new_method`. + INSTANCE_FACTORY = LibGLib.g_quark_from_static_string("gi-crystal::factory") # Raised when trying to cast an object that was already collected by GC. class ObjectCollectedError < RuntimeError @@ -117,23 +119,36 @@ module GICrystal end end - # This declare the `new` method on a instance of type *type*, *qdata_get_func* and *qdata_set_func* are functions used - # to set/get qdata on objects, e.g. `g_object_get_qdata`/`g_object_set_qdata` for GObjects. + # This declare the `new` method on a instance of type *type*, *qdata_get_func* (g_object_get_qdata) is used + # to fetch a possible already existing Crystal object. # - # GICrystal stores the Crystal object instance in the GObject, so when it appears from C world we try to re-use the - # old Crystal instance if it wasn't garbage collected. The key used for this is `INSTANCE_QDATA_KEY`. + # GICrystal stores the User defined Crystal objects instances in the GObject, so when it appears from C + # world we can retrieve the Crystal instance back. # # This is mainly used for `GObject::Object`, since `GObject::ParamSpec` doesn't support casts on GICrystal. - macro declare_new_method(type, qdata_get_func, qdata_set_func) + macro declare_new_method(type, qdata_get_func) # :nodoc: def self.new(pointer : Pointer(Void), transfer : GICrystal::Transfer) : self # Try to recover the Crystal instance if any instance = LibGObject.{{ qdata_get_func }}(pointer, GICrystal::INSTANCE_QDATA_KEY) return instance.as({{ type }}) if instance - # This object never meet Crystal land, so we allocate memory and initialize it. + # Try to construct the right wrapper for the matching GObject using the function pointer + # to `{{ type }}.new` stored in GType qdata. + # + # This pointer is stored at type initialization, i.e. the `g_type` class method. + instance_g_type = pointer.as(LibGObject::TypeInstance*).value.g_class.value.g_type + if instance_g_type != g_type + ctor_ptr = LibGObject.g_type_get_qdata(instance_g_type, GICrystal::INSTANCE_FACTORY) + if ctor_ptr + ctor = Proc(Void*, GICrystal::Transfer, {{ type }}).new(ctor_ptr, Pointer(Void).null) + return ctor.call(pointer, transfer) + end + end + + # This object never meet Crystal land and there's no Crystal wrapper for the exact GType, + # so we allocate memory and initialize it for the type we know. instance = {{ type }}.allocate - LibGObject.{{ qdata_set_func }}(pointer, GICrystal::INSTANCE_QDATA_KEY, Pointer(Void).new(instance.object_id)) instance.initialize(pointer, transfer) GC.add_finalizer(instance) instance