mirror of
https://github.com/ghostty-org/ghostty.git
synced 2026-09-14 18:01:58 +00:00
gtk: fix stale pointers to property bindings (#14065)
Fixes #14037 where dragging the surface from a tab with just a single surface to another tab causes a crash. The cause of the crash is a stale pointer to the property binding created in `Surface.bindIsSplit`. When the surface is moved, `SplitTree.moveSplit` first updates the two split tree data structures of the source/target tab and then calls `bindIsSplit` to bind the `is-split` property of the moved surface to the `SplitTree` widget in the target tab. When `bindIsSplit` is called, the `SplitTree` widget in the source tab has already been destroyed (because the source tab is now empty) which causes the old binding to be freed automatically and the pointer `Surface.is_split_binding` becomes stale. `bindIsSplit` then tries to run `is_split_binding.unbind()` which causes the crash. When you create a binding with `bindProperty`, the binding itself owns the initially created reference and it gets freed when the source or target object of the binding is finalized. To prevent this, we now create an extra reference to the binding object so that the Surface widget owns it and is responsible for freeing it. The binding can still get severed automatically, but the binding object itself will not be destroyed. This is the solution mentioned in the [GObject docs](https://docs.gtk.org/gobject/method.Object.bind_property.html). Alternatively, using a WeakRef for the pointer would have also worked. Updated the binding in `SurfaceScrolledWindow` to use the same pattern. That one was probably fine, because the binding should only be created once, but it doesn't hurt to be safe. I reproduced the crash on KDE, on Hyprland I just got a glib critical error message about the invalid pointer. That probably has to do with what exactly happens to the freed memory, or maybe differing versions. #### AI Disclosure Code and comments were written by myself, used GPT5.6 in researching gobject binding lifecycles.
This commit is contained in:
@@ -853,14 +853,24 @@ pub const Surface = extern struct {
|
||||
|
||||
pub fn bindIsSplit(self: *Self, tree: *SplitTree) void {
|
||||
const priv = self.private();
|
||||
if (priv.is_split_binding) |bind| bind.unbind();
|
||||
if (priv.is_split_binding) |binding| {
|
||||
binding.unbind();
|
||||
binding.unref();
|
||||
priv.is_split_binding = null;
|
||||
}
|
||||
|
||||
priv.is_split_binding = tree.as(gobject.Object).bindProperty(
|
||||
const binding = tree.as(gobject.Object).bindProperty(
|
||||
"is-split",
|
||||
self.as(gobject.Object),
|
||||
"is-split",
|
||||
.{ .sync_create = true },
|
||||
);
|
||||
// The ref created by bindProperty is owned by the binding itself.
|
||||
// We need another ref to prevent the binding object from being
|
||||
// freed if the source object (SplitTree) is finalized. Otherwise
|
||||
// our pointer to the binding could become stale.
|
||||
binding.ref();
|
||||
priv.is_split_binding = binding;
|
||||
}
|
||||
|
||||
/// Callback used to determine whether unfocused-split-fill / unfocused-split-opacity
|
||||
@@ -1881,6 +1891,12 @@ pub const Surface = extern struct {
|
||||
priv.config = null;
|
||||
}
|
||||
|
||||
if (priv.is_split_binding) |binding| {
|
||||
binding.unbind();
|
||||
binding.unref();
|
||||
priv.is_split_binding = null;
|
||||
}
|
||||
|
||||
if (priv.vadj_signal_group) |group| {
|
||||
group.setTarget(null);
|
||||
group.as(gobject.Object).unref();
|
||||
|
||||
@@ -89,7 +89,8 @@ pub const SurfaceScrolledWindow = extern struct {
|
||||
const priv = self.private();
|
||||
|
||||
if (priv.config_binding) |binding| {
|
||||
binding.as(gobject.Object).unref();
|
||||
binding.unbind();
|
||||
binding.unref();
|
||||
priv.config_binding = null;
|
||||
}
|
||||
|
||||
@@ -168,18 +169,23 @@ pub const SurfaceScrolledWindow = extern struct {
|
||||
|
||||
// Unbind old config binding if it exists
|
||||
if (priv.config_binding) |binding| {
|
||||
binding.as(gobject.Object).unref();
|
||||
binding.unbind();
|
||||
binding.unref();
|
||||
priv.config_binding = null;
|
||||
}
|
||||
|
||||
// Bind config from surface to our config property
|
||||
if (priv.surface) |surface| {
|
||||
priv.config_binding = surface.as(gobject.Object).bindProperty(
|
||||
const binding = surface.as(gobject.Object).bindProperty(
|
||||
properties.config.name,
|
||||
self.as(gobject.Object),
|
||||
properties.config.name,
|
||||
.{ .sync_create = true },
|
||||
);
|
||||
// Keep another ref, otherwise the binding would be freed and
|
||||
// our pointer become stale if the surface gets finalized.
|
||||
binding.ref();
|
||||
priv.config_binding = binding;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user