[gimp] Issue #8102: crash when using the clone tool.



commit c06083158a0230d1d3fce240bda7a757e2f0152b
Author: Jehan <jehan girinstud io>
Date:   Thu Apr 21 18:57:06 2022 +0200

    Issue #8102: crash when using the clone tool.
    
    So it turns out that the "notify::src-drawables" property signal in
    particular can happen during gimp_paint_tool_paint_interpolate() called
    from gimp_paint_tool_paint_thread(). Though the function
    gimp_clone_options_gui_update_src_label() was run in the main thread in
    simple cases, being called this way through a paint thread happens when
    very quickly changing the source while painting, which is what Aryeom
    does (when I see her using the clone tool, she would sometimes change
    the source very quickly several times in barely a few seconds).
    
    Yet GTK and GDK calls must not happen in non-main threads. It used to be
    acceptable when protected with gdk_threads_enter|leave() calls but doing
    this is deprecated. The now sanctioned method is to call the GTK code in
    an idle function since these are guaranteed to run in the main thread.
    
    This was most likely explaining why crashes could quite randomly happen,
    though I'm not sure why I never had those (even though I could reproduce
    the GTK calls happening in non-main threads, but without crashing GIMP)
    and why Aryeom gets these much more often suddenly. Maybe some recent
    dependency change which would trigger these more easily or other
    context-dependant changes (as most non thread-safe code, bugs and crash
    often happen in race conditions, so are harder to reproduce reliably)?

 app/tools/gimpcloneoptions-gui.c | 40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)
---
diff --git a/app/tools/gimpcloneoptions-gui.c b/app/tools/gimpcloneoptions-gui.c
index 0375b7c813..f9051006ea 100644
--- a/app/tools/gimpcloneoptions-gui.c
+++ b/app/tools/gimpcloneoptions-gui.c
@@ -53,8 +53,7 @@ static void gimp_clone_options_gui_context_image_changed (GimpContext       *con
                                                           GimpImage         *image,
                                                           GimpSourceOptions *options);
 
-static void gimp_clone_options_gui_update_src_label       (GimpSourceOptions *options,
-                                                           GtkWidget         *label);
+static gboolean gimp_clone_options_gui_update_src_label  (GimpSourceOptions *options);
 
 
 static gboolean
@@ -84,7 +83,12 @@ gimp_clone_options_gui_drawables_changed (GimpImage         *image,
   gtk_widget_set_sensitive (button, (g_list_length (drawables) < 2));
   g_list_free (drawables);
 
-  gimp_clone_options_gui_update_src_label (options, NULL);
+  /* In case this was called several times in a row by threads. */
+  g_idle_remove_by_data (options);
+
+  g_idle_add_full (G_PRIORITY_DEFAULT_IDLE,
+                   (GSourceFunc) gimp_clone_options_gui_update_src_label,
+                   g_object_ref (options), (GDestroyNotify) g_object_unref);
 }
 
 static void
@@ -92,7 +96,18 @@ gimp_clone_options_gui_src_changed (GimpSourceOptions *options,
                                     GParamSpec        *pspec,
                                     GtkWidget         *label)
 {
-  gimp_clone_options_gui_update_src_label (options, label);
+  /* In case this was called several times in a row by threads. */
+  g_idle_remove_by_data (options);
+
+  /* Why we need absolutely to run this in a thread is that it updates
+   * the GUI. And sometimes this src_changed callback may be called by
+   * paint threads (see gimppainttool-paint.c). This may cause crashes
+   * as in recent GTK, all GTK/GDK calls should be main from the main
+   * thread. Idle functions are ensured to be run in this main thread.
+   */
+  g_idle_add_full (G_PRIORITY_DEFAULT_IDLE,
+                   (GSourceFunc) gimp_clone_options_gui_update_src_label,
+                   g_object_ref (options), (GDestroyNotify) g_object_unref);
 }
 
 static void
@@ -131,14 +146,13 @@ gimp_clone_options_gui_context_image_changed (GimpContext       *context,
     }
 }
 
-static void
-gimp_clone_options_gui_update_src_label (GimpSourceOptions *options,
-                                         GtkWidget         *label)
+static gboolean
+gimp_clone_options_gui_update_src_label (GimpSourceOptions *options)
 {
-  gchar *markup = NULL;
+  GtkWidget *label;
+  gchar     *markup = NULL;
 
-  if (! label)
-    label = g_object_get_data (G_OBJECT (options), "src-label");
+  label = g_object_get_data (G_OBJECT (options), "src-label");
 
   if (options->src_drawables == NULL)
     {
@@ -200,6 +214,8 @@ gimp_clone_options_gui_update_src_label (GimpSourceOptions *options,
 
   gtk_label_set_markup (GTK_LABEL (label), markup);
   g_free (markup);
+
+  return G_SOURCE_REMOVE;
 }
 
 
@@ -250,7 +266,9 @@ gimp_clone_options_gui (GimpToolOptions *tool_options)
   gtk_label_set_use_markup (GTK_LABEL (label), TRUE);
   str = g_strdup_printf ("<i>%s</i>", _("No source selected"));
   gtk_label_set_markup (GTK_LABEL (label), str);
-  g_object_set_data (G_OBJECT (tool_options), "src-label", label);
+  g_object_set_data_full (G_OBJECT (tool_options), "src-label",
+                          g_object_ref (label),
+                          (GDestroyNotify) g_object_unref);
   g_free (str);
 
   gtk_box_pack_start (GTK_BOX (source_vbox), label, FALSE, FALSE, 0);


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]