Skip to content

Commit

Permalink
Fixes random crashes caused by bad wrapper instance cache.
Browse files Browse the repository at this point in the history
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 😅.
  • Loading branch information
hugopl committed Nov 21, 2023
1 parent 0795560 commit da061f7
Show file tree
Hide file tree
Showing 14 changed files with 91 additions and 38 deletions.
4 changes: 2 additions & 2 deletions ecr/interface.ecr
Original file line number Diff line number Diff line change
Expand Up @@ -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
12 changes: 12 additions & 0 deletions ecr/interface_g_type_method.ecr
Original file line number Diff line number Diff line change
@@ -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 %>
4 changes: 3 additions & 1 deletion ecr/object.ecr
Original file line number Diff line number Diff line change
Expand Up @@ -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
12 changes: 12 additions & 0 deletions ecr/object_g_type_method.ecr
Original file line number Diff line number Diff line change
@@ -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 %>
17 changes: 15 additions & 2 deletions spec/basic_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions spec/inheritance_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion spec/libtest/test_subject.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
12 changes: 6 additions & 6 deletions spec/list_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 7 additions & 6 deletions spec/s_list_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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

Expand Down
4 changes: 2 additions & 2 deletions spec/vfunc_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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

Expand Down
3 changes: 3 additions & 0 deletions src/bindings/g_object/lib_g_object.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
10 changes: 2 additions & 8 deletions src/bindings/g_object/object.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
2 changes: 1 addition & 1 deletion src/generator/object_gen.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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
29 changes: 22 additions & 7 deletions src/gi-crystal.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit da061f7

Please sign in to comment.