[glib: 4/9] Make transform function handling in GBinding thread-safe

[Philip Withnall <pwithnall src gnome org>]

commit 98bbe4f4d1fe7ca8daa2efaf250d779592c48867
Author: Sebastian Dröge <sebastian centricular com>
Date:   Wed Nov 11 13:03:11 2020 +0200

    Make transform function handling in GBinding thread-safe
    
    Unbinding can happen from one thread while a property notification is
    being handled concurrently in another one.
    
    To solve this, introduce a reference counter for the transform function
    that ensures that it always stays valid while in use and protect access
    to the one stored inside the binding with the unbind mutex.

 gobject/gbinding.c | 163 +++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 114 insertions(+), 49 deletions(-)
---
diff --git a/gobject/gbinding.c b/gobject/gbinding.c
index bd5f315bf..250b7d38d 100644
--- a/gobject/gbinding.c
+++ b/gobject/gbinding.c
@@ -178,6 +178,59 @@ binding_context_unref (BindingContext *context)
   g_atomic_rc_box_release_full (context, (GDestroyNotify) binding_context_clear);
 }
 
+/* Reference counting for the transform functions to ensure that they're always
+ * valid while making use of them in the property notify callbacks.
+ *
+ * The transform functions are released when unbinding but unbinding can happen
+ * while the transform functions are currently in use inside the notify callbacks.
+ *
+ * Note that the transform functions are only released from explicit unbinding
+ * or when the binding itself is finalized. Finalizing the source and target
+ * while the binding is still alive does not release the transform functions
+ * yet. */
+typedef struct {
+  GBindingTransformFunc transform_s2t;
+  GBindingTransformFunc transform_t2s;
+
+  gpointer transform_data;
+  GDestroyNotify destroy_notify;
+} TransformFunc;
+
+static TransformFunc *
+transform_func_new (GBindingTransformFunc transform_s2t,
+                    GBindingTransformFunc transform_t2s,
+                    gpointer              transform_data,
+                    GDestroyNotify        destroy_notify)
+{
+  TransformFunc *func = g_atomic_rc_box_new0 (TransformFunc);
+
+  func->transform_s2t = transform_s2t;
+  func->transform_t2s = transform_t2s;
+  func->transform_data = transform_data;
+  func->destroy_notify = destroy_notify;
+
+  return func;
+}
+
+static TransformFunc *
+transform_func_ref (TransformFunc *func)
+{
+  return g_atomic_rc_box_acquire (func);
+}
+
+static void
+transform_func_clear (TransformFunc *func)
+{
+  if (func->destroy_notify)
+    func->destroy_notify (func->transform_data);
+}
+
+static void
+transform_func_unref (TransformFunc *func)
+{
+  g_atomic_rc_box_release_full (func, (GDestroyNotify) transform_func_clear);
+}
+
 #define G_BINDING_CLASS(klass)          (G_TYPE_CHECK_CLASS_CAST ((klass), G_TYPE_BINDING, GBindingClass))
 #define G_IS_BINDING_CLASS(klass)       (G_TYPE_CHECK_CLASS_TYPE ((klass), G_TYPE_BINDING))
 #define G_BINDING_GET_CLASS(obj)        (G_TYPE_INSTANCE_GET_CLASS ((obj), G_TYPE_BINDING, GBindingClass))
@@ -191,6 +244,13 @@ struct _GBinding
   /* no reference is held on the objects, to avoid cycles */
   BindingContext *context;
 
+  /* protects transform_func, source, target property notify and
+   * target_weak_notify_installed for unbinding */
+  GMutex unbind_lock;
+
+  /* transform functions, only NULL after unbinding */
+  TransformFunc *transform_func; /* LOCK: unbind_lock */
+
   /* the property names are interned, so they should not be freed */
   const gchar *source_property;
   const gchar *target_property;
@@ -198,22 +258,12 @@ struct _GBinding
   GParamSpec *source_pspec;
   GParamSpec *target_pspec;
 
-  GBindingTransformFunc transform_s2t;
-  GBindingTransformFunc transform_t2s;
-
   GBindingFlags flags;
 
-  /* protects source and target property notify and
-   * target_weak_notify_installed for unbinding */
-  GMutex unbind_lock;
-
   guint source_notify; /* LOCK: unbind_lock */
   guint target_notify; /* LOCK: unbind_lock */
   gboolean target_weak_notify_installed; /* LOCK: unbind_lock */
 
-  gpointer transform_data;
-  GDestroyNotify notify;
-
   /* a guard, to avoid loops */
   guint is_frozen : 1;
 };
@@ -408,6 +458,7 @@ on_source_notify (GObject          *source,
 {
   GBinding *binding;
   GObject *target;
+  TransformFunc *transform_func;
   GValue from_value = G_VALUE_INIT;
   GValue to_value = G_VALUE_INIT;
   gboolean res;
@@ -429,15 +480,29 @@ on_source_notify (GObject          *source,
       return;
     }
 
+  /* Get the transform function safely */
+  g_mutex_lock (&binding->unbind_lock);
+  if (!binding->transform_func)
+    {
+      /* it was released already during unbinding, nothing to do here */
+      g_mutex_unlock (&binding->unbind_lock);
+      return;
+    }
+  transform_func = transform_func_ref (binding->transform_func);
+  g_mutex_unlock (&binding->unbind_lock);
+
   g_value_init (&from_value, G_PARAM_SPEC_VALUE_TYPE (binding->source_pspec));
   g_value_init (&to_value, G_PARAM_SPEC_VALUE_TYPE (binding->target_pspec));
 
   g_object_get_property (source, binding->source_pspec->name, &from_value);
 
-  res = binding->transform_s2t (binding,
-                                &from_value,
-                                &to_value,
-                                binding->transform_data);
+  res = transform_func->transform_s2t (binding,
+                                       &from_value,
+                                       &to_value,
+                                       transform_func->transform_data);
+
+  transform_func_unref (transform_func);
+
   if (res)
     {
       binding->is_frozen = TRUE;
@@ -462,6 +527,7 @@ on_target_notify (GObject          *target,
 {
   GBinding *binding;
   GObject *source;
+  TransformFunc *transform_func;
   GValue from_value = G_VALUE_INIT;
   GValue to_value = G_VALUE_INIT;
   gboolean res;
@@ -483,15 +549,28 @@ on_target_notify (GObject          *target,
       return;
     }
 
+  /* Get the transform function safely */
+  g_mutex_lock (&binding->unbind_lock);
+  if (!binding->transform_func)
+    {
+      /* it was released already during unbinding, nothing to do here */
+      g_mutex_unlock (&binding->unbind_lock);
+      return;
+    }
+  transform_func = transform_func_ref (binding->transform_func);
+  g_mutex_unlock (&binding->unbind_lock);
+
   g_value_init (&from_value, G_PARAM_SPEC_VALUE_TYPE (binding->target_pspec));
   g_value_init (&to_value, G_PARAM_SPEC_VALUE_TYPE (binding->source_pspec));
 
   g_object_get_property (target, binding->target_pspec->name, &from_value);
 
-  res = binding->transform_t2s (binding,
-                                &from_value,
-                                &to_value,
-                                binding->transform_data);
+  res = transform_func->transform_t2s (binding,
+                                       &from_value,
+                                       &to_value,
+                                       transform_func->transform_data);
+  transform_func_unref (transform_func);
+
   if (res)
     {
       binding->is_frozen = TRUE;
@@ -516,18 +595,12 @@ g_binding_unbind_internal (GBinding *binding,
   BindingContext *context = binding->context;
   GObject *source, *target;
   gboolean binding_was_removed = FALSE;
-
-  /* dispose of the transformation data */
-  if (binding->notify != NULL)
-    {
-      binding->notify (binding->transform_data);
-
-      binding->transform_data = NULL;
-      binding->notify = NULL;
-    }
+  TransformFunc *transform_func;
 
   g_mutex_lock (&binding->unbind_lock);
 
+  transform_func = g_steal_pointer (&binding->transform_func);
+
   source = g_weak_ref_get (&context->source);
   target = g_weak_ref_get (&context->target);
 
@@ -583,11 +656,13 @@ g_binding_unbind_internal (GBinding *binding,
 
   g_mutex_unlock (&binding->unbind_lock);
 
-  /* Unref source and target after the mutex is unlocked as it might
-   * release the last reference, which then accesses the mutex again */
+  /* Unref source, target and transform_func after the mutex is unlocked as it
+   * might release the last reference, which then accesses the mutex again */
   g_clear_object (&target);
   g_clear_object (&source);
 
+  g_clear_pointer (&transform_func, transform_func_unref);
+
   if (binding_was_removed && unref_binding)
     g_object_unref (binding);
 }
@@ -599,15 +674,6 @@ g_binding_finalize (GObject *gobject)
 
   g_binding_unbind_internal (binding, FALSE);
 
-  /* dispose of the transformation data */
-  if (binding->notify != NULL)
-    {
-      binding->notify (binding->transform_data);
-
-      binding->transform_data = NULL;
-      binding->notify = NULL;
-    }
-
   binding_context_unref (binding->context);
 
   g_mutex_clear (&binding->unbind_lock);
@@ -785,11 +851,7 @@ g_binding_constructed (GObject *gobject)
     transform_func = default_invert_boolean_transform;
 
   /* set the default transformation functions here */
-  binding->transform_s2t = transform_func;
-  binding->transform_t2s = transform_func;
-
-  binding->transform_data = NULL;
-  binding->notify = NULL;
+  binding->transform_func = transform_func_new (transform_func, transform_func, NULL, NULL);
 
   source_property_detail = g_quark_from_string (binding->source_property);
   source_notify_closure = g_cclosure_new (G_CALLBACK (on_source_notify),
@@ -1315,14 +1377,17 @@ g_object_bind_property_full (gpointer               source,
                           "flags", flags,
                           NULL);
 
-  if (transform_to != NULL)
-    binding->transform_s2t = transform_to;
+  g_assert (binding->transform_func != NULL);
 
-  if (transform_from != NULL)
-    binding->transform_t2s = transform_from;
+  /* Use default functions if not provided here */
+  if (transform_to == NULL)
+    transform_to = binding->transform_func->transform_s2t;
+
+  if (transform_from == NULL)
+    transform_from = binding->transform_func->transform_t2s;
 
-  binding->transform_data = user_data;
-  binding->notify = notify;
+  g_clear_pointer (&binding->transform_func, transform_func_unref);
+  binding->transform_func = transform_func_new (transform_to, transform_from, user_data, notify);
 
   /* synchronize the target with the source by faking an emission of
    * the ::notify signal for the source property; this will also take