sdroege
commented
2 years ago cc @jf2048
Open YaLTeR opened 2 years ago
sdroege
commented
2 years ago cc @jf2048
jf2048
commented
2 years ago AdwTabView is emitting signals from dispose which should probably not happen? I guess we could also not panic there, and instead return None, but I think that is probably not safe with signals using the C marshallers.
Edit: Probably we can not use a weak ref and just move a pointer, since watch_closure guarantees the object has a ref.
jf2048
commented
2 years ago Here is a hacky workaround, if this is a blocking bug:
<?xml version="1.0" encoding="UTF-8"?>
<interface>
<requires lib="gtk" version="4.0"/>
<object class="GObject" id="workaround"/>
<template class="Window" parent="GtkApplicationWindow">
<property name="child">
<object class="AdwTabView" id="view">
<signal name="page-detached" handler="on_page_detached" object="workaround"/>
</object>
</property>
</template>
</interface>
YaLTeR
commented
2 years ago I just tried it again on 0.4.7 and this fix might have a soundness issue: seems like some of the objects are already destroyed at the point when the callback is called. I have a TemplateChild<gtk::Stack> in my composite template, which I call self.stack.set_visible_child() on in the page-detached callback, and it gives me a gtk_stack_set_visible_child: assertion 'GTK_IS_STACK (stack)' failed when closing the window.
YaLTeR
commented
2 years ago I cannot yet make a reproducer for the assertion, but this shows that a child Stack is already missing some of its child widgets when the callback is called, which is not entirely expected behavior:
use gtk::prelude::*;
use gtk::glib;
mod imp {
use gtk::{subclass::prelude::*, CompositeTemplate};
use super::*;
#[derive(Debug, Default, CompositeTemplate)]
#[template(file = "window.ui")]
pub struct Window {
#[template_child]
stack: TemplateChild<gtk::Stack>,
#[template_child]
label: TemplateChild<gtk::Label>,
}
#[glib::object_subclass]
impl ObjectSubclass for Window {
const NAME: &'static str = "Window";
type Type = super::Window;
type ParentType = gtk::ApplicationWindow;
fn class_init(klass: &mut Self::Class) {
Self::bind_template(klass);
Self::bind_template_callbacks(klass);
}
fn instance_init(obj: &glib::subclass::InitializingObject<Self>) {
obj.init_template();
}
}
impl ObjectImpl for Window {}
impl WidgetImpl for Window {}
impl WindowImpl for Window {}
impl ApplicationWindowImpl for Window {}
#[gtk::template_callbacks]
impl Window {
#[template_callback]
fn on_page_detached(&self) {
println!("page-detached");
self.stack.set_visible_child(&*self.label);
}
}
}
glib::wrapper! {
pub struct Window(ObjectSubclass<imp::Window>)
@extends gtk::ApplicationWindow, gtk::Window, gtk::Widget;
}
fn build_ui(app: &adw::Application) {
let window: Window = glib::Object::new(&[("application", app)]).unwrap();
window.present();
// Trigger the issue automatically, but it also works when closing with the "X" button.
window.close();
}
fn main() {
let application = adw::Application::new(None, Default::default());
application.connect_activate(build_ui);
application.run();
}<?xml version="1.0" encoding="UTF-8"?>
<interface>
<requires lib="gtk" version="4.0"/>
<template class="Window" parent="GtkApplicationWindow">
<property name="child">
<object class="GtkStack" id="stack">
<child>
<object class="GtkLabel" id="label">
<property name="label">Second Stack Page</property>
</object>
</child>
<child>
<object class="AdwTabView">
<signal name="page-detached" handler="on_page_detached" swapped="true"/>
<child>
<object class="GtkLabel">
<property name="label">Hello World</property>
</object>
</child>
</object>
</child>
</object>
</property>
</template>
</interface> Running `target/debug/gtk-test`
page-detached
(gtk-test:133772): Gtk-WARNING **: 12:52:28.341: Given child of type 'GtkLabel' not found in GtkStackFigured out how to reproduce the assertion:
use gtk::prelude::*;
use gtk::glib;
mod imp {
use gtk::{subclass::prelude::*, CompositeTemplate};
use super::*;
#[derive(Debug, Default, CompositeTemplate)]
#[template(file = "window.ui")]
pub struct Window {
#[template_child]
stack: TemplateChild<gtk::Stack>,
#[template_child]
label: TemplateChild<gtk::Label>,
#[template_child]
tab_view: TemplateChild<adw::TabView>,
}
#[glib::object_subclass]
impl ObjectSubclass for Window {
const NAME: &'static str = "Window";
type Type = super::Window;
type ParentType = gtk::ApplicationWindow;
fn class_init(klass: &mut Self::Class) {
Self::bind_template(klass);
Self::bind_template_callbacks(klass);
}
fn instance_init(obj: &glib::subclass::InitializingObject<Self>) {
obj.init_template();
}
}
impl ObjectImpl for Window {
fn constructed(&self, obj: &Self::Type) {
self.parent_constructed(obj);
self.stack.set_visible_child(&*self.tab_view);
}
}
impl WidgetImpl for Window {}
impl WindowImpl for Window {}
impl ApplicationWindowImpl for Window {}
#[gtk::template_callbacks]
impl Window {
#[template_callback]
fn on_page_detached(&self) {
println!("page-detached");
self.stack.set_visible_child(&*self.label);
}
}
}
glib::wrapper! {
pub struct Window(ObjectSubclass<imp::Window>)
@extends gtk::ApplicationWindow, gtk::Window, gtk::Widget;
}
fn build_ui(app: &adw::Application) {
let window: Window = glib::Object::new(&[("application", app)]).unwrap();
window.present();
// Trigger the assertion automatically, but it also works when closing with the "X" button.
window.close();
}
fn main() {
let application = adw::Application::new(None, Default::default());
application.connect_activate(build_ui);
application.run();
}<?xml version="1.0" encoding="UTF-8"?>
<interface>
<requires lib="gtk" version="4.0"/>
<template class="Window" parent="GtkApplicationWindow">
<property name="child">
<object class="GtkStack" id="stack">
<child>
<object class="GtkLabel" id="label">
<property name="label">Second Stack Page</property>
</object>
</child>
<child>
<object class="AdwTabView" id="tab_view">
<signal name="page-detached" handler="on_page_detached" swapped="true"/>
<child>
<object class="GtkLabel">
<property name="label">Hello World</property>
</object>
</child>
</object>
</child>
</object>
</property>
</template>
</interface>Looks like it's not just a problem with this particular signal. Tab view's notify::selected-page is also fired during the dispose as the pages are detached. This triggers the Rust callback, from where it's possible to access objects which have already been freed.
The expected behavior for this particular case would be to avoid the handler call when the object is already being disposed, so the same behavior as you currently get when manually connecting with clone!(@weak obj.
jf2048
commented
2 years ago Can you post a stack trace from where that warning is printed, with the C symbols? The child widgets should still be around during dispose
YaLTeR
commented
2 years ago bt
* thread #1, name = 'gtk-test', stop reason = step over
* frame #0: 0x00007ffff78adf72 libgtk-4.so.1`gtk_stack_set_visible_child(stack=0x000055555595d1e0, child=0x0000555555678380) at gtkstack.c:2026:3
frame #1: 0x00005555555774c4 gtk-test`set_visible_child<gtk4::auto::label::Label>(self=0x0000555555a08020, child=0x0000555555a08028) at stack.rs:214:13
frame #2: 0x000055555556ae4a gtk-test`on_page_detached(self=0x0000555555a08020) at main.rs:53:13
frame #3: 0x000055555556af04 gtk-test`{closure#0}((null)=0x00007fffffffc430, values=(4) &[{...}, {...}, {...}, {...}]) at main.rs:48:5
frame #4: 0x000055555557646a gtk-test`call_once<gtk_test::imp::{impl#11}::CALLBACKS::{closure#0}, (&[glib::value::Value])>((null)=gtk_test::imp::{impl#11}::CALLBACKS::{closure#0} @ 0x00007fffffffc430, (null)=((4) &[{...}, {...}, {...}, {...}])) at function.rs:227:5
frame #5: 0x000055555557635a gtk-test`call<fn(&[glib::value::Value]) -> core::option::Option<glib::value::Value>, ((null)=0x000055555561b450, (null)=((4) &[{...}, {...}, {...}, {...}]))> at function.rs:70:5
frame #6: 0x000055555556f53d gtk-test`call<(&[glib::value::Value]), fn(&[glib::value::Value]) -> core::option::Option<glib::value::Value>>(self=0x0000555555965710, args=((4) &[{...}, {...}, {...}, {...}])) at function.rs:237:13
frame #7: 0x000055555557d201 gtk-test`{closure#1}(args=(3) &[{...}, {...}, {...}]) at builder_rust_scope.rs:137:33
frame #8: 0x00005555555844ba gtk-test`{closure#0}<gtk4::builder_rust_scope::imp::{impl#1}::create_closure::{closure#1}::{closure#1}>(values=(3) &[{...}, {...}, {...}]) at closure.rs:203:49
frame #9: 0x0000555555585a09 gtk-test`marshal<glib::closure::{impl#4}::new_local::{closure#0}>(_closure=0x00005555558b15b0, return_value=0x0000000000000000, n_param_values=3, param_values=0x00007fffffffcb10, _invocation_hint=0x00007fffffffca90, marshal_data=0x00005555558b5bd0) at closure.rs:232:26
frame #10: 0x00007ffff7246c7f libgobject-2.0.so.0`g_closure_invoke + 367
frame #11: 0x00007ffff7263126 libgobject-2.0.so.0`___lldb_unnamed_symbol1042 + 2662
frame #12: 0x00007ffff72649ea libgobject-2.0.so.0`g_signal_emit_valist + 4218
frame #13: 0x00007ffff7264c03 libgobject-2.0.so.0`g_signal_emit + 147
frame #14: 0x00007ffff7edad66 libadwaita-1.so.0`detach_page(self=0x00005555559221c0, page=0x0000555555923340, notify_pages=0) at adw-tab-view.c:927:3
frame #15: 0x00007ffff7edaf17 libadwaita-1.so.0`adw_tab_view_dispose(object=0x00005555559221c0) at adw-tab-view.c:1303:5
frame #16: 0x00007ffff7253bc4 libgobject-2.0.so.0`g_object_unref + 292
frame #17: 0x00007ffff7138292 libglib-2.0.so.0`___lldb_unnamed_symbol2378 + 242
frame #18: 0x00007ffff7138ef3 libglib-2.0.so.0`g_hash_table_remove_all + 99
frame #19: 0x00007ffff713c392 libglib-2.0.so.0`g_hash_table_destroy + 18
frame #20: 0x00007ffff7138292 libglib-2.0.so.0`___lldb_unnamed_symbol2378 + 242
frame #21: 0x00007ffff7138ef3 libglib-2.0.so.0`g_hash_table_remove_all + 99
frame #22: 0x00007ffff713c392 libglib-2.0.so.0`g_hash_table_destroy + 18
frame #23: 0x00007ffff712e84e libglib-2.0.so.0`___lldb_unnamed_symbol2353 + 670
frame #24: 0x00007ffff7941553 libgtk-4.so.1`gtk_widget_dispose at gtkwidget.c:7534:7
frame #25: 0x00007ffff79414fc libgtk-4.so.1`gtk_widget_dispose(object=0x0000555555a082e0) at gtkwidget.c:7428:7
frame #26: 0x00007ffff771a93c libgtk-4.so.1`gtk_application_window_dispose(object=0x0000555555a082e0) at gtkapplicationwindow.c:624:3
frame #27: 0x0000555555577d1b gtk-test`dispose<gtk_test::imp::Window>(obj=0x0000555555a082e0) at object.rs:132:9
frame #28: 0x00007ffff7253bc4 libgobject-2.0.so.0`g_object_unref + 292
frame #29: 0x00005555555b1612 gtk-test`drop(self=0x00007fffffffd118) at object.rs:316:13
frame #30: 0x000055555559781b gtk-test`drop_in_place<glib::object::ObjectRef>((null)=0x00007fffffffd118) at mod.rs:188:1
frame #31: 0x000055555557661b gtk-test`drop_in_place<gtk_test::Window>((null)=0x00007fffffffd118) at mod.rs:188:1
frame #32: 0x000055555556d313 gtk-test`build_ui(app=0x00007fffffffd218) at main.rs:70:1
frame #33: 0x0000555555576328 gtk-test`call<fn(&libadwaita::auto::application::Application), (&libadwaita::auto::application::Application)>((null)=0x0000000000000001, (null)=(0x00007fffffffd218)) at function.rs:70:5
frame #34: 0x000055555557088b gtk-test`activate_trampoline<libadwaita::auto::application::Application, fn(&libadwaita::auto::application::Application)>(this=0x000055555563f100, f=0x0000000000000001) at application.rs:604:13
frame #35: 0x00007ffff7246c7f libgobject-2.0.so.0`g_closure_invoke + 367
frame #36: 0x00007ffff7263126 libgobject-2.0.so.0`___lldb_unnamed_symbol1042 + 2662
frame #37: 0x00007ffff72649ea libgobject-2.0.so.0`g_signal_emit_valist + 4218
frame #38: 0x00007ffff7264c03 libgobject-2.0.so.0`g_signal_emit + 147
frame #39: 0x00007ffff736e188 libgio-2.0.so.0`___lldb_unnamed_symbol5770 + 600
frame #40: 0x00007ffff736e366 libgio-2.0.so.0`g_application_run + 310
frame #41: 0x00005555555755d7 gtk-test`run_with_args<libadwaita::auto::application::Application, alloc::string::String>(self=0x00007fffffffd8b0, args=(1) &["/var/home/yalter/source/rs/gtk-test/target/debug/gtk-test", ...]) at application.rs:30:13
frame #42: 0x0000555555575446 gtk-test`run<libadwaita::auto::application::Application>(self=0x00007fffffffd8b0) at application.rs:23:9
frame #43: 0x000055555556d376 gtk-test`main at main.rs:77:5
frame #44: 0x00005555555764fb gtk-test`call_once<fn(), ()>((null)=0x000055555556d330, (null)=()) at function.rs:227:5
frame #45: 0x000055555557738e gtk-test`__rust_begin_short_backtrace<fn(), ()>(f=0x000055555556d330) at backtrace.rs:123:18
frame #46: 0x0000555555577311 gtk-test`{closure#0}<()> at rt.rs:145:18
frame #47: 0x00005555555cd7a0 gtk-test`lang_start_internal [inlined] core::ops::function::impls::_$LT$impl$u20$core..ops..function..FnOnce$LT$A$GT$$u20$for$u20$$RF$F$GT$::call_once::h443f738a8e9f947a at function.rs:259:13
frame #48: 0x00005555555cd79d gtk-test`lang_start_internal [inlined] std::panicking::try::do_call::h1e21ba261ba489ec at panicking.rs:406:40
frame #49: 0x00005555555cd79d gtk-test`lang_start_internal [inlined] std::panicking::try::h6afd48af8b6c96ac at panicking.rs:370:19
frame #50: 0x00005555555cd79d gtk-test`lang_start_internal [inlined] std::panic::catch_unwind::h85dd95e0bab7fb60 at panic.rs:133:14
frame #51: 0x00005555555cd79d gtk-test`lang_start_internal [inlined] std::rt::lang_start_internal::_$u7b$$u7b$closure$u7d$$u7d$::h038455e697c8b03e at rt.rs:128:48
frame #52: 0x00005555555cd79d gtk-test`lang_start_internal [inlined] std::panicking::try::do_call::h6b0ad65979f3077a at panicking.rs:406:40
frame #53: 0x00005555555cd79d gtk-test`lang_start_internal [inlined] std::panicking::try::h010108d314169ac6 at panicking.rs:370:19
frame #54: 0x00005555555cd79d gtk-test`lang_start_internal [inlined] std::panic::catch_unwind::hff397f912b1535c2 at panic.rs:133:14
frame #55: 0x00005555555cd79d gtk-test`lang_start_internal at rt.rs:128:20
frame #56: 0x00005555555772e0 gtk-test`lang_start<()>(main=0x000055555556d330, argc=1, argv=0x00007fffffffdc68) at rt.rs:144:17
frame #57: 0x000055555556dfbc gtk-test`main + 28
frame #58: 0x00007ffff6f00560 libc.so.6`__libc_start_call_main + 128
frame #59: 0x00007ffff6f0060c libc.so.6`__libc_start_main@@GLIBC_2.34 + 124
frame #60: 0x0000555555567a55 gtk-test`_start + 37Yeah I see now, it's not safe to access any TemplateChild objects from those signals or from dispose. This is also a problem if you try to use those signals from C
jf2048
commented
2 years ago A "solution" could be to automatically set all those TemplateChild objects to null during dispose. I'm not sure how we would do go about doing that. And then you would have to always use TemplateChild::try_get on them in anything that can be called from those signals or from dispose.
As a workaround, I think you can set a flag during dispose and exit on_page_detached early if it's set
YaLTeR
commented
2 years ago How bad would it be to make .ui signals disconnect on dispose? And if you need a signal during dispose then you need to .connect() from the code. I have a feeling this would match the average expected behavior better.
jf2048
commented
2 years ago We can't do that, the signal ids are internal to the GtkBuilder and are not stored anywhere. Now I'm fairly certain this is a gtk bug since they do get set to null there, but this happens after the child objects are disposed: https://gitlab.gnome.org/GNOME/gtk/-/blob/3b50f2e8b996f92f49d2974ccff2b160905e8e73/gtk/gtkwidget.c#L7533-7534
I would be curious to know if anyone else is hitting this and how it gets handled from C applications though
YaLTeR
commented
2 years ago I'm not sure I understand it correctly, but the PR for the original fix to this issue changed a weak reference to a strong reference, wouldn't changing that back and returning instead of panicking on upgrade failure result in the "disconnect .ui signals on dispose" effective behavior?
jf2048
commented
2 years ago This is a different issue from the original one and not related to the signal handlers BTW. This is because of the order of dispose on the child widgets which we have no control over. I think this patch will do as best we can: https://gitlab.gnome.org/GNOME/gtk/-/merge_requests/4611
That at least changes it to a crash instead of a dangling pointer bug. If you don't want it to panic and don't want to use TemplateChild::try_get, then you could do this:
<signal name="page-detached" handler="on_page_detached" swapped="true" object="stack"/>That will disconnect the handler when stack is disposed regardless of the order it happens in the window. You can also do this now without that patch to avoid the bug
Bug description
Backtrace