Widget::destroy() should be marked unsafe

[sdroege]

Based on discussion on IRC

<slomo> i wonder if we should expose g_object_run_dispose() in the Rust bindings
<slomo> ebassi: the docs say "This function should only be called from object system implementations.", but that's not really true, is it? if it's directly called from gtk_widget_destroy() and that can be directly called
* osa1 has quit (Read error)
<ebassi> slomo: In general, yes: you should never call g_object_run_dispose() yourself
<ebassi> But gtk_widget_destroy() is really gtk_object_destroy(), which is really the previous object system
<ebassi> And had to maintain some "backward/historical" compatibility
<imperio> slomo: ?
<slomo> ebassi: ok, not going to expose it then and better teach people about using strong/weak references correctly :)
<ebassi> i.e. after gtk_widget_destroy() the widget is in an inert state, and only the pointer may be still valid
<ebassi> slomo: Yeah, definitely
<slomo> so arguably we should also mark gtk_widget_destroy() as unsafe in the Rust bindings because bad things might happen if using the widget afterwards?
<ebassi> If you're hiding the ref/unref mechanism, and you're using weak refs and correct transfer of ownership, at the end widgets should just not be lying around
<ebassi> slomo: Yes
<slomo> oh no :) ok
<ebassi> If something else is holding a strong reference to a widget, and something else calls gtk_widget_destroy(), the instance is still valid memory because otherwise you wouldn't be able to call g_object_unref() on it
<slomo> but calling any API on that widget might cause everything to explode?
<ebassi> But the widget is unparented and unrealized, and calling methods on it won't likely lead to any nice result—though I consider crashing a bug
<ebassi> Personally, I don't like stuff exploding because people forgot to NULLify stuff inside dispose()
<ebassi> And I consider that a bug
<ebassi> But other people may be more lenient because they haven't had to debug random crashes at 11pm
<slomo> there's also things potentially exploding because there's other code that does not expect any of the fields to be NULL while the object is still alive :)

Someone wants to create a PR and also update our examples to call Window::close() instead?

[GuillaumeGomez]

Starting to send the gtk PR first.

[EPashkin]

https://github.com/gtk-rs/examples/pull/296 is merged too. IMHO this can be closed

[GuillaumeGomez]

Done!

[owtaylor]

[edit]

I fully believe in what I wrote below. However, I should note (having looked around some more) that the current GTK maintainers removed gtk_widget_destroy() in GTK4. So, writing code that uses destroy() on a portion of a widget tree to break reference count cycles is not forward looking.

"You can't go back home again" - you leave a project, people take it in directions you don't necessarily agree with :shrug: - but they are doing the work and you aren't.

---

I don't fully agree with the conclusion here. (Speaking as one of the main designers of the GTK+ life cycle system as it exists now.)

What does gtk_window_close() do in the normal case (if delete event isn't handled)? it causes gtk_widget_destroy() to be run on the GtkWindow. So it doesn't really make much sense to consider close() to be safe, but destroy() to be unsafe. (*)

Using destroy() greatly reduces the need to reason properly about reference cycles, and reduces the chances of memory leaks from not reasoning properly about them.

Consider the snippet of code:

let parent = gtk::Box::new(gtk::Orientation::Vertical, 0);
win.add(&parent);

let button = gtk::Button::with_label("Click Me!");
parent.add(&button);

button.connect_clicked(clone!(@strong win, @strong parent => move |_| {
    // Leaks parent button, everything they reference
    win.remove(&parent);

    // Would be fine, because the closure is dropped at destroy time
    // unsafe { parent.destroy(); }
}));

For this reason, I've always recommended that if you mean to get rid of some part of the widget tree, you should destroy() it rather than removing it. Removing is a special case thing for when you want to preserve some widgetry for reuse.

I do like that the clone! macro explicitly requires weak or strong markings - this certainly makes it less likely to leak things this way than in a language that defaults to strong reference counts. Though closures are not the only way to create strong reference cycles.

(*) It's always been a little murky what you can do with a destroyed-but-still-referenced widget - some operations might work, some might no-op, some might produces Gtk-Criticals, some might segfault. But by marking destroy() unsafe - you haven't eliminated these edge cases case, because destroy() is the way that toplevels get torn down and discarded.

[sdroege]

Even in GTK4, e.g. closing a window will go through g_object_run_dispose() on all its children so that part is still true.

I agree that the current situation is not perfect yet because g_object_run_dispose() can still be run from safe code (gtk_window_closed() etc), and also in my interpretation of the GObject documentation all operations on a disposed object should not cause criticals or segfault (the operations might fail cleanly though!) so it should be fine to make it a safe operation. However the current GTK maintainers don't seem to agree with this because it would be too difficult to implement, and especially in multi-threaded situations it's almost impossible to implement this correctly unless you're using a language like Rust that forces you to do it (GStreamer's dispose implementations are almost all not thread-safe or otherwise wrong, but also note that GStreamer does not call g_object_run_dispose() itself anywhere by design so it's not a problem unless application code or bindings do that). Other libraries are equally buggy (or working as intended by the maintainers).

Considering this, the current situation is a compromise to put the landmines a bit further away (gtk_widget_destroy() / g_object_run_dispose()) while still keeping the required functionality (gtk_window_close()).

See also https://github.com/gtk-rs/gtk-rs/issues/333