Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes random crashes caused by bad wrapper instance cache. #133

Merged
merged 1 commit into from
Nov 21, 2023
Merged

Conversation

hugopl
Copy link
Owner

@hugopl hugopl commented Nov 21, 2023

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 😅.

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 😅.
@hugopl hugopl merged commit 0e0e04e into main Nov 21, 2023
1 check passed
@hugopl hugopl deleted the bad-cast branch December 10, 2023 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant