Re: MPS: Crash while completing symbol with corfu

[Pip Cet <pipcet@protonmail.com>]

[-- Attachment #1: Type: text/plain, Size: 860 bytes --]

On Tuesday, July 2nd, 2024 at 15:14, Ihor Radchenko <yantar92@posteo.net> wrote:

> Pip Cet pipcet@protonmail.com writes:
> 
> > Oops, turns out you shouldn't double free just to be sure.
> > 
> > Can you try the attached patch? I was able to trigger your bug by running
> > 
> > ./emacs -Q --eval "(while t (menu-bar-mode 'toggle) (sit-for 0.1))"
> > 
> > The change is in menu_destroy_callback, which passed through the wrong pointer.
> 
> 
> No immediate crashes now.
> I will keep this session running, with your patches installed.

Thanks! Here's what I'm testing now, FWIW.

Eli, is this sufficiently isolated from the non-MPS case? We use some trivial wrapper macros which become non-trivial in the MPS case, and we always use a slightly more verbose glib function to register handlers (and, in the MPS case, a cleanup handler).

Pip

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-GTK-related-MPS-crashes.patch --]
[-- Type: text/x-patch; name=0001-Fix-GTK-related-MPS-crashes.patch, Size: 17701 bytes --]

From be3ed571e4e1d69d29e1e4f51cf80432b176eff0 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@protonmail.com>
Date: Wed, 3 Jul 2024 13:43:59 +0000
Subject: [PATCH] Fix GTK-related MPS crashes

* src/gtkutil.c (free_glib_user_data): New function.
(xg_list_insert, xg_list_remove): Conditionalize on !HAVE_MPS.
(qttip_cb, xg_create_frame_widgets)
(xg_create_frame_outer_widgets, create_dialog)
(make_cl_data, unref_cl_data, xg_mark_data)
(menuitem_highlight_callback, xg_create_one_menuitem)
(create_menus, xg_update_menu_item, xg_modify_menubar_widgets)
(draw_page, xg_print_frames_dialog, xg_create_toolbar)
(xg_im_context_commit, xg_widget_style_updated)
(tb_size_cb): Adjust to use `get_glib_user_data' and
`g_signal_connect_data'.
(xg_initialize): Remove linked list if HAVE_MPS.
* src/gtkutil.h (glib_user_data, get_glib_user_data): New helper macros.
* src/xmenu.c (menubar_selection_callback)
(popup_selection_callback): Adjust to use `get_glib_user_data'.
---
 src/gtkutil.c | 147 +++++++++++++++++++++++++++++++++++++-------------
 src/gtkutil.h |  21 ++++++++
 src/xmenu.c   |   6 ++-
 3 files changed, 135 insertions(+), 39 deletions(-)

diff --git a/src/gtkutil.c b/src/gtkutil.c
index e0bae6bf1ae..a1707040e75 100644
--- a/src/gtkutil.c
+++ b/src/gtkutil.c
@@ -148,6 +148,17 @@ #define TB_INFO_KEY "xg_frame_tb_info"
 static GtkWidget *xg_get_widget_from_map (ptrdiff_t idx, Display *dpy);
 
 \f
+#ifdef HAVE_MPS
+void free_glib_user_data (gpointer data, GClosure *closure)
+{
+  igc_xfree (data);
+}
+#else
+void free_glib_user_data (gpointer data, GClosure *closure)
+{
+  return;
+}
+#endif
 
 #ifdef HAVE_GTK3
 static void
@@ -618,6 +629,7 @@ xg_set_cursor (GtkWidget *w, GdkCursor *cursor)
     gdk_window_set_cursor (GDK_WINDOW (children->data), cursor);
 }
 
+#ifndef HAVE_MPS
 /* Insert NODE into linked LIST.  */
 
 static void
@@ -648,6 +660,7 @@ xg_list_remove (xg_list_node *list, xg_list_node *node)
       if (node->next) node->next->prev = node->prev;
     }
 }
+#endif
 
 /* Allocate and return a utf8 version of STR.  If STR is already
    utf8 or NULL, just return a copy of STR.
@@ -828,7 +841,7 @@ hierarchy_ch_cb (GtkWidget *widget,
                  GtkWidget *previous_toplevel,
                  gpointer   user_data)
 {
-  struct frame *f = user_data;
+  struct frame *f = get_glib_user_data (user_data, struct frame);
   xp_output *x = f->output_data.xp;
   GtkWidget *top = gtk_widget_get_toplevel (x->ttip_lbl);
 
@@ -850,7 +863,7 @@ qttip_cb (GtkWidget  *widget,
           GtkTooltip *tooltip,
           gpointer    user_data)
 {
-  struct frame *f = user_data;
+  struct frame *f = get_glib_user_data (user_data, struct frame);
   xp_output *x = f->output_data.xp;
   if (x->ttip_widget == NULL)
     {
@@ -882,8 +895,11 @@ qttip_cb (GtkWidget  *widget,
       gtk_widget_realize (GTK_WIDGET (x->ttip_window));
       gtk_widget_realize (x->ttip_lbl);
 
-      g_signal_connect (x->ttip_lbl, "hierarchy-changed",
-                        G_CALLBACK (hierarchy_ch_cb), f);
+      g_signal_connect_data (x->ttip_lbl, "hierarchy-changed",
+			     G_CALLBACK (hierarchy_ch_cb),
+			     glib_user_data (f),
+			     free_glib_user_data,
+			     G_CONNECT_DEFAULT);
     }
 
   return FALSE;
@@ -1556,8 +1572,11 @@ xg_create_frame_widgets (struct frame *f)
 
   gtk_widget_set_app_paintable (wtop, f->alpha_background != 1.0);
 #if GTK_CHECK_VERSION (3, 10, 0)
-  g_signal_connect (G_OBJECT (wtop), "style-updated",
-		    G_CALLBACK (xg_widget_style_updated), f);
+  g_signal_connect_data (G_OBJECT (wtop), "style-updated",
+			 G_CALLBACK (xg_widget_style_updated),
+			 glib_user_data (f),
+			 free_glib_user_data,
+			 G_CONNECT_DEFAULT);
 #endif
 
   /* gtk_window_set_has_resize_grip is a Gtk+ 3.0 function but Ubuntu
@@ -1646,8 +1665,11 @@ xg_create_frame_widgets (struct frame *f)
 #ifndef HAVE_PGTK
   /* Add callback to do nothing on WM_DELETE_WINDOW.  The default in
      GTK is to destroy the widget.  We want Emacs to do that instead.  */
-  g_signal_connect (G_OBJECT (wtop), "delete-event",
-                    G_CALLBACK (delete_cb), f);
+  g_signal_connect_data (G_OBJECT (wtop), "delete-event",
+			 G_CALLBACK (delete_cb),
+			 glib_user_data (f),
+			 free_glib_user_data,
+			 G_CONNECT_DEFAULT);
 #endif
 
   /* Convert our geometry parameters into a geometry string
@@ -1735,14 +1757,18 @@ xg_create_frame_widgets (struct frame *f)
   f->output_data.xp->ttip_window = 0;
 #ifndef HAVE_PGTK
   gtk_widget_set_tooltip_text (wtop, "Dummy text");
-  g_signal_connect (wtop, "query-tooltip", G_CALLBACK (qttip_cb), f);
+  g_signal_connect_data (wtop, "query-tooltip", G_CALLBACK (qttip_cb),
+			 glib_user_data (f), free_glib_user_data,
+			 G_CONNECT_DEFAULT);
 
   imc = gtk_im_multicontext_new ();
   g_object_ref (imc);
   gtk_im_context_set_use_preedit (imc, TRUE);
 
-  g_signal_connect (G_OBJECT (imc), "commit",
-		    G_CALLBACK (xg_im_context_commit), f);
+  g_signal_connect_data (G_OBJECT (imc), "commit",
+			 G_CALLBACK (xg_im_context_commit),
+			 glib_user_data (f), free_glib_user_data,
+			 G_CONNECT_DEFAULT);
   g_signal_connect (G_OBJECT (imc), "preedit-changed",
 		    G_CALLBACK (xg_im_context_preedit_changed), NULL);
   g_signal_connect (G_OBJECT (imc), "preedit-end",
@@ -1853,7 +1879,9 @@ xg_create_frame_outer_widgets (struct frame *f)
   f->output_data.xp->ttip_window = 0;
 #ifndef HAVE_PGTK
   gtk_widget_set_tooltip_text (wtop, "Dummy text");
-  g_signal_connect (wtop, "query-tooltip", G_CALLBACK (qttip_cb), f);
+  g_signal_connect_data (wtop, "query-tooltip", G_CALLBACK (qttip_cb),
+			 glib_user_data (f), free_glib_user_data,
+			 G_CONNECT_DEFAULT);
 #endif
 
   {
@@ -2371,8 +2399,11 @@ create_dialog (widget_value *wv,
           if (! item->enabled)
             gtk_widget_set_sensitive (w, FALSE);
           if (select_cb)
-            g_signal_connect (G_OBJECT (w), "clicked",
-                              select_cb, item->call_data);
+            g_signal_connect_data (G_OBJECT (w), "clicked",
+				   select_cb,
+				   glib_user_data (item->call_data),
+				   free_glib_user_data,
+				   G_CONNECT_DEFAULT);
 
           gtk_box_pack_start (cur_box, w, TRUE, TRUE, button_spacing);
           if (++button_nr == left_buttons)
@@ -2946,6 +2977,7 @@ xg_get_font (struct frame *f, const char *default_name)
 #define MENU_ITEM_NAME "emacs-menuitem"
 
 
+#ifndef HAVE_MPS
 /* Linked list of all allocated struct xg_menu_cb_data.  Used for marking
    during GC.  The next member points to the items.  */
 static xg_list_node xg_menu_cb_list;
@@ -2953,6 +2985,7 @@ #define MENU_ITEM_NAME "emacs-menuitem"
 /* Linked list of all allocated struct xg_menu_item_cb_data.  Used for marking
    during GC.  The next member points to the items.  */
 static xg_list_node xg_menu_item_cb_list;
+#endif
 
 /* Allocate and initialize CL_DATA if NULL, otherwise increase ref_count.
    F is the frame CL_DATA will be initialized for.
@@ -2969,14 +3002,20 @@ make_cl_data (xg_menu_cb_data *cl_data, struct frame *f, GCallback highlight_cb)
 {
   if (! cl_data)
     {
+#ifndef HAVE_MPS
       cl_data = xmalloc (sizeof *cl_data);
+#else
+      cl_data = igc_xzalloc_ambig (sizeof *cl_data);
+#endif
       cl_data->f = f;
       cl_data->menu_bar_vector = f->menu_bar_vector;
       cl_data->menu_bar_items_used = f->menu_bar_items_used;
       cl_data->highlight_cb = highlight_cb;
       cl_data->ref_count = 0;
 
+#ifndef HAVE_MPS
       xg_list_insert (&xg_menu_cb_list, &cl_data->ptrs);
+#endif
     }
 
   cl_data->ref_count++;
@@ -3020,8 +3059,12 @@ unref_cl_data (xg_menu_cb_data *cl_data)
       cl_data->ref_count--;
       if (cl_data->ref_count == 0)
         {
+#ifndef HAVE_MPS
           xg_list_remove (&xg_menu_cb_list, &cl_data->ptrs);
           xfree (cl_data);
+#else
+	  igc_xfree (cl_data);
+#endif
         }
     }
 }
@@ -3082,11 +3125,16 @@ xg_mark_data (void)
 static void
 menuitem_destroy_callback (GtkWidget *w, gpointer client_data)
 {
-  if (client_data)
+  xg_menu_item_cb_data *data =
+    get_glib_user_data (client_data, xg_menu_item_cb_data);
+  if (data)
     {
-      xg_menu_item_cb_data *data = client_data;
+#ifndef HAVE_MPS
       xg_list_remove (&xg_menu_item_cb_list, &data->ptrs);
       xfree (data);
+#else
+      igc_xfree (data);
+#endif
     }
 }
 
@@ -3129,7 +3177,7 @@ menuitem_highlight_callback (GtkWidget *w,
 static void
 menu_destroy_callback (GtkWidget *w, gpointer client_data)
 {
-  unref_cl_data (client_data);
+  unref_cl_data (get_glib_user_data (client_data, xg_menu_cb_data));
 }
 
 /* Make a GTK widget that contains both UTF8_LABEL and UTF8_KEY (both
@@ -3268,19 +3316,25 @@ xg_create_one_menuitem (widget_value *item,
   if (utf8_label) g_free (utf8_label);
   if (utf8_key) g_free (utf8_key);
 
+#ifdef HAVE_MPS
+  cb_data = igc_xzalloc_ambig (sizeof *cb_data);
+#else
   cb_data = xmalloc (sizeof *cb_data);
 
   xg_list_insert (&xg_menu_item_cb_list, &cb_data->ptrs);
+#endif
 
   cb_data->select_id = 0;
   cb_data->help = item->help;
   cb_data->cl_data = cl_data;
   cb_data->call_data = item->call_data;
 
-  g_signal_connect (G_OBJECT (w),
-                    "destroy",
-                    G_CALLBACK (menuitem_destroy_callback),
-                    cb_data);
+  g_signal_connect_data (G_OBJECT (w),
+			 "destroy",
+			 G_CALLBACK (menuitem_destroy_callback),
+			 glib_user_data (cb_data),
+			 free_glib_user_data,
+			 G_CONNECT_DEFAULT);
 
   /* Put cb_data in widget, so we can get at it when modifying menubar  */
   g_object_set_data (G_OBJECT (w), XG_ITEM_DATA, cb_data);
@@ -3290,7 +3344,10 @@ xg_create_one_menuitem (widget_value *item,
     {
       if (select_cb)
         cb_data->select_id
-          = g_signal_connect (G_OBJECT (w), "activate", select_cb, cb_data);
+          = g_signal_connect_data (G_OBJECT (w), "activate", select_cb,
+				   glib_user_data (cb_data),
+				   free_glib_user_data,
+				   G_CONNECT_DEFAULT);
     }
 
   return w;
@@ -3378,8 +3435,10 @@ create_menus (widget_value *data,
 #endif
 
 #ifdef HAVE_PGTK
-	  g_signal_connect (G_OBJECT (wmenu), "button-press-event",
-			    G_CALLBACK (menu_bar_button_pressed_cb), f);
+	  g_signal_connect_data (G_OBJECT (wmenu), "button-press-event",
+			    G_CALLBACK (menu_bar_button_pressed_cb),
+			    glib_user_data (f), free_glib_user_data,
+			    G_CONNECT_DEFAULT);
 #endif
           /* Set width of menu bar to a small value so it doesn't enlarge
              a small initial frame size.  The width will be set to the
@@ -3391,8 +3450,10 @@ create_menus (widget_value *data,
       /* Put cl_data on the top menu for easier access.  */
       cl_data = make_cl_data (cl_data, f, highlight_cb);
       g_object_set_data (G_OBJECT (wmenu), XG_FRAME_DATA, (gpointer)cl_data);
-      g_signal_connect (G_OBJECT (wmenu), "destroy",
-                        G_CALLBACK (menu_destroy_callback), cl_data);
+      g_signal_connect_data (G_OBJECT (wmenu), "destroy",
+			     G_CALLBACK (menu_destroy_callback),
+			     glib_user_data (cl_data), free_glib_user_data,
+			     G_CONNECT_DEFAULT);
 
       if (name)
         gtk_widget_set_name (wmenu, name);
@@ -3864,8 +3925,11 @@ xg_update_menu_item (widget_value *val,
           /* This item shall have a select callback.  */
           if (! cb_data->select_id)
             cb_data->select_id
-              = g_signal_connect (G_OBJECT (w), "activate",
-                                  select_cb, cb_data);
+              = g_signal_connect_data (G_OBJECT (w), "activate",
+				       select_cb,
+				       glib_user_data (cb_data),
+				       free_glib_user_data,
+				       G_CONNECT_DEFAULT);
         }
       else if (cb_data->select_id)
         {
@@ -4111,7 +4175,7 @@ xg_modify_menubar_widgets (GtkWidget *menubar, struct frame *f,
 menubar_map_cb (GtkWidget *w, gpointer user_data)
 {
   GtkRequisition req;
-  struct frame *f = user_data;
+  struct frame *f = get_glib_user_data (user_data, struct frame);
   gtk_widget_get_preferred_size (w, NULL, &req);
   req.height *= xg_get_scale (f);
   if (FRAME_MENUBAR_HEIGHT (f) != req.height)
@@ -4143,7 +4207,9 @@ xg_update_frame_menubar (struct frame *f)
                       FALSE, FALSE, 0);
   gtk_box_reorder_child (GTK_BOX (x->vbox_widget), x->menubar_widget, 0);
 
-  g_signal_connect (x->menubar_widget, "map", G_CALLBACK (menubar_map_cb), f);
+  g_signal_connect_data (x->menubar_widget, "map", G_CALLBACK (menubar_map_cb),
+			 glib_user_data (f), free_glib_user_data,
+			 G_CONNECT_DEFAULT);
   gtk_widget_show_all (x->menubar_widget);
   gtk_widget_get_preferred_size (x->menubar_widget, NULL, &req);
   req.height *= scale;
@@ -5223,7 +5289,7 @@ #define GETSETUP(f) make_float (f (page_setup, GTK_UNIT_POINTS))
 draw_page (GtkPrintOperation *operation, GtkPrintContext *context,
 	   gint page_nr, gpointer user_data)
 {
-  Lisp_Object frames = *((Lisp_Object *) user_data);
+  Lisp_Object frames = *get_glib_user_data (user_data, Lisp_Object);
   struct frame *f = XFRAME (Fnth (make_fixnum (page_nr), frames));
   cairo_t *cr = gtk_print_context_get_cairo_context (context);
 
@@ -5246,7 +5312,10 @@ xg_print_frames_dialog (Lisp_Object frames)
   if (page_setup != NULL)
     gtk_print_operation_set_default_page_setup (print, page_setup);
   gtk_print_operation_set_n_pages (print, list_length (frames));
-  g_signal_connect (print, "draw-page", G_CALLBACK (draw_page), &frames);
+  Lisp_Object *frames_ptr = &frames;
+  g_signal_connect_data (print, "draw-page", G_CALLBACK (draw_page),
+			 glib_user_data (frames_ptr), free_glib_user_data,
+			 G_CONNECT_DEFAULT);
   res = gtk_print_operation_run (print, GTK_PRINT_OPERATION_ACTION_PRINT_DIALOG,
                                  NULL, NULL);
   if (res == GTK_PRINT_OPERATION_RESULT_APPLY)
@@ -5478,7 +5547,7 @@ tb_size_cb (GtkWidget    *widget,
   /* When tool bar is created it has one preferred size.  But when size is
      allocated between widgets, it may get another.  So we must update
      size hints if tool bar size changes.  Seen on Fedora 18 at least.  */
-  struct frame *f = user_data;
+  struct frame *f = get_glib_user_data (user_data, struct frame);
 
   if (xg_update_tool_bar_sizes (f))
     adjust_frame_size (f, -1, -1, 2, false, Qtool_bar_lines);
@@ -5521,8 +5590,10 @@ xg_create_tool_bar (struct frame *f)
   gtk_toolbar_set_style (GTK_TOOLBAR (x->toolbar_widget), GTK_TOOLBAR_ICONS);
   gtk_orientable_set_orientation (GTK_ORIENTABLE (x->toolbar_widget),
                                   GTK_ORIENTATION_HORIZONTAL);
-  g_signal_connect (x->toolbar_widget, "size-allocate",
-                    G_CALLBACK (tb_size_cb), f);
+  g_signal_connect_data (x->toolbar_widget, "size-allocate",
+			 G_CALLBACK (tb_size_cb),
+			 glib_user_data (f), free_glib_user_data,
+			 G_CONNECT_DEFAULT);
 #ifdef HAVE_GTK3
   gsty = gtk_widget_get_style_context (x->toolbar_widget);
   gtk_style_context_add_class (gsty, "primary-toolbar");
@@ -6234,8 +6305,10 @@ xg_initialize (void)
 
   gdpy_def = NULL;
   xg_ignore_gtk_scrollbar = 0;
+#ifndef HAVE_MPS
   xg_menu_cb_list.prev = xg_menu_cb_list.next =
     xg_menu_item_cb_list.prev = xg_menu_item_cb_list.next = 0;
+#endif
 
 #if defined HAVE_PGTK || !defined HAVE_GTK3
   id_to_widget.max_size = id_to_widget.used = 0;
@@ -6337,7 +6410,7 @@ xg_virtual_mods_to_x (struct x_display_info *dpyinfo, guint virtual)
 xg_im_context_commit (GtkIMContext *imc, gchar *str,
 		      gpointer user_data)
 {
-  struct frame *f = user_data;
+  struct frame *f = get_glib_user_data (user_data, struct frame);
   struct input_event ie;
 #ifdef HAVE_XINPUT2
   struct xi_device_t *source;
@@ -6708,7 +6781,7 @@ xg_filter_key (struct frame *frame, XEvent *xkey)
 static void
 xg_widget_style_updated (GtkWidget *widget, gpointer user_data)
 {
-  struct frame *f = user_data;
+  struct frame *f = get_glib_user_data (user_data, struct frame);
 
   if (f->alpha_background < 1.0)
     {
diff --git a/src/gtkutil.h b/src/gtkutil.h
index b689053b4b8..ac1b961e5a3 100644
--- a/src/gtkutil.h
+++ b/src/gtkutil.h
@@ -33,6 +33,27 @@ #define EVENT GdkEvent
 #define EVENT XEvent
 #endif
 
+/* Helper macros for glib callbacks.  On MPS builds, we need to pin
+   (pointers to) the objects, which requires an extra layer of
+   indirection. */
+
+#ifdef HAVE_MPS
+#define glib_user_data(o) ({				\
+      gpointer p = igc_xzalloc_ambig (sizeof (o));	\
+      memcpy (p, &(o), sizeof (o));			\
+      p;						\
+    })
+
+#define get_glib_user_data(user_data, type)	\
+  (*(type **)(user_data))
+
+#else
+#define glib_user_data(o) ((gpointer)(o))
+#define get_glib_user_data(user_data, type)	\
+  ((type *)(user_data))
+#endif
+extern void free_glib_user_data (gpointer data, GClosure *closure);
+
 /* Minimum and maximum values used for GTK scroll bars  */
 
 #define XG_SB_MIN 1
diff --git a/src/xmenu.c b/src/xmenu.c
index 6dd7b3f37a0..45def2fb1ea 100644
--- a/src/xmenu.c
+++ b/src/xmenu.c
@@ -793,7 +793,8 @@ menu_highlight_callback (Widget widget, LWLIB_ID id, void *call_data)
 static void
 menubar_selection_callback (GtkWidget *widget, gpointer client_data)
 {
-  xg_menu_item_cb_data *cb_data = client_data;
+  xg_menu_item_cb_data *cb_data =
+    get_glib_user_data (client_data, xg_menu_item_cb_data);
 
   if (xg_crazy_callback_abort)
     return;
@@ -1478,7 +1479,8 @@ menu_position_func (GtkMenu *menu, gint *x, gint *y, gboolean *push_in, gpointer
 static void
 popup_selection_callback (GtkWidget *widget, gpointer client_data)
 {
-  xg_menu_item_cb_data *cb_data = client_data;
+  xg_menu_item_cb_data *cb_data =
+    get_glib_user_data (client_data, xg_menu_item_cb_data);
 
   if (xg_crazy_callback_abort) return;
   if (cb_data) menu_item_selection = cb_data->call_data;
-- 
2.45.2