From a477921b80e7d8dc7bee07f36f130086a5bb7ee7 Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <m@mitchellh.com>
Date: Fri, 25 Jul 2025 11:44:09 -0700
Subject: [PATCH] apprt/gtk: fix double-free if quit action is used

This fixes a double-free that Valgrind found when the quit action was
used (the keybinding to quit or the menu item).

The issue stems from the fact that our quit action worked by traversing the
toplevels and destroying all windows. When all windows are destroyed,
GTK exits the main loop.

When fcitx is used as the input method editor (IME), it appears to hold
its own `gtk.Window` widget as a property (probably for the IME popup).
Unfortunately this does not react well to being destroyed externally and
triggers a double-free when the IME widget also tries to dispose itself.

I think this is probably a bug somewhere in the GTK IME widget because
it should be resilient to this kind of destruction. But, we can't
tolerate a double free in the mean time.

We can still quit by destroying only OUR windows (which cascades to
destroy everything else).
---
 src/apprt/gtk-ng/class/application.zig | 17 ++++++++++++++---
 src/apprt/gtk/App.zig                  | 17 ++++++++++++++++-
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/src/apprt/gtk-ng/class/application.zig b/src/apprt/gtk-ng/class/application.zig
index f82e91e77..385749c04 100644
--- a/src/apprt/gtk-ng/class/application.zig
+++ b/src/apprt/gtk-ng/class/application.zig
@@ -433,15 +433,26 @@ pub const Application = extern struct {
     }
 
     fn quitNow(self: *Self) void {
-        // Get all our windows and destroy them, forcing them to
-        // free their memory.
+        // Get all our windows and destroy them, forcing them to free.
         const list = gtk.Window.listToplevels();
         defer list.free();
         list.foreach(struct {
             fn callback(data: ?*anyopaque, _: ?*anyopaque) callconv(.c) void {
                 const ptr = data orelse return;
                 const window: *gtk.Window = @ptrCast(@alignCast(ptr));
-                window.destroy();
+
+                // We only want to destroy our windows. These windows own
+                // every other type of window that is possible so this will
+                // trigger a proper shutdown sequence.
+                //
+                // We previously just destroyed ALL windows but this leads to
+                // a double-free with the fcitx ime, because it has a nested
+                // gtk.Window as a property that we don't own and it later
+                // tries to free on its own. I think this is probably a bug in
+                // the fcitx ime widget but still, we don't want a double free!
+                if (gobject.ext.isA(window, Window)) {
+                    window.destroy();
+                }
             }
         }.callback, null);
 
diff --git a/src/apprt/gtk/App.zig b/src/apprt/gtk/App.zig
index e32d0c8ca..faa4781f6 100644
--- a/src/apprt/gtk/App.zig
+++ b/src/apprt/gtk/App.zig
@@ -1556,7 +1556,22 @@ pub fn quitNow(self: *App) void {
         fn callback(data: ?*anyopaque, _: ?*anyopaque) callconv(.c) void {
             const ptr = data orelse return;
             const window: *gtk.Window = @ptrCast(@alignCast(ptr));
-            window.destroy();
+
+            // We only want to destroy our windows. These windows own
+            // every other type of window that is possible so this will
+            // trigger a proper shutdown sequence.
+            //
+            // We previously just destroyed ALL windows but this leads to
+            // a double-free with the fcitx ime, because it has a nested
+            // gtk.Window as a property that we don't own and it later
+            // tries to free on its own. I think this is probably a bug in
+            // the fcitx ime widget but still, we don't want a double free!
+            //
+            // Since we don't use gobject directly we can't check class,
+            // so we use a heuristic based on CSS class.
+            if (window.as(gtk.Widget).hasCssClass("terminal-window") != 0) {
+                window.destroy();
+            }
         }
     }.callback, null);