Merge branch 'binding-threadsafe-2' into 'master'

Make GBinding thread-safe (alternative approach)

See merge request GNOME/glib!1745

docs/reference/gobject/gobject-sections.txt

@@ -978,8 +978,10 @@ g_io_condition_get_type
 GBinding
 GBindingFlags
 g_binding_get_source
+g_binding_dup_source
 g_binding_get_source_property
 g_binding_get_target
+g_binding_dup_target
 g_binding_get_target_property
 g_binding_get_flags
 g_binding_unbind

gobject/gbinding.c

@@ -139,6 +139,94 @@ g_binding_flags_get_type (void)
   return static_g_define_type_id;
 }
 
+/* Reference counted helper struct that is passed to all callbacks to ensure
+ * that they never work with already freed objects without having to store
+ * strong references for them.
+ *
+ * Using strong references anywhere is not possible because of the API
+ * requirements of GBinding, specifically that the initial reference of the
+ * GBinding is owned by the source/target and the caller and can be released
+ * either by the source/target being finalized or calling g_binding_unbind().
+ *
+ * As such, the only strong reference has to be owned by both weak notifies of
+ * the source and target and the first to be called has to release it.
+ */
+typedef struct {
+  GWeakRef binding;
+  GWeakRef source;
+  GWeakRef target;
+  gboolean binding_removed;
+} BindingContext;
+
+static BindingContext *
+binding_context_ref (BindingContext *context)
+{
+  return g_atomic_rc_box_acquire (context);
+}
+
+static void
+binding_context_clear (BindingContext *context)
+{
+  g_weak_ref_clear (&context->binding);
+  g_weak_ref_clear (&context->source);
+  g_weak_ref_clear (&context->target);
+}
+
+static void
+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.
+ */
+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))
@@ -150,8 +238,14 @@ struct _GBinding
   GObject parent_instance;
 
   /* no reference is held on the objects, to avoid cycles */
-  GObject *source;
-  GObject *target;
+  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;
@@ -160,16 +254,11 @@ struct _GBinding
   GParamSpec *source_pspec;
   GParamSpec *target_pspec;
 
-  GBindingTransformFunc transform_s2t;
-  GBindingTransformFunc transform_t2s;
-
   GBindingFlags flags;
 
-  guint source_notify;
-  guint target_notify;
-
-  gpointer transform_data;
-  GDestroyNotify notify;
+  guint source_notify; /* LOCK: unbind_lock */
+  guint target_notify; /* LOCK: unbind_lock */
+  gboolean target_weak_notify_installed; /* LOCK: unbind_lock */
 
   /* a guard, to avoid loops */
   guint is_frozen : 1;
@@ -195,49 +284,136 @@ static guint gobject_notify_signal_id;
 
 G_DEFINE_TYPE (GBinding, g_binding, G_TYPE_OBJECT)
 
+static void weak_unbind (gpointer user_data, GObject *where_the_object_was);
+
+/* Must be called with the unbind lock held, context/binding != NULL and strong
+ * references to source/target or NULL.
+ * Return TRUE if the binding was actually removed and FALSE if it was already
+ * removed before. */
+static gboolean
+unbind_internal_locked (BindingContext *context, GBinding *binding, GObject *source, GObject *target)
+{
+  gboolean binding_was_removed = FALSE;
+
+  g_assert (context != NULL);
+  g_assert (binding != NULL);
+
+  /* If the target went away we still have a strong reference to the source
+   * here and can clear it from the binding. Otherwise if the source went away
+   * we can clear the target from the binding. Finalizing an object clears its
+   * signal handlers and all weak references pointing to it before calling
+   * weak notify callbacks.
+   *
+   * If both still exist we clean up everything set up by the binding.
+   */
+  if (source)
+    {
+      /* We always add/remove the source property notify and the weak notify
+       * of the source at the same time, and should only ever do that once. */
+      if (binding->source_notify != 0)
+        {
+          g_signal_handler_disconnect (source, binding->source_notify);
+
+          g_object_weak_unref (source, weak_unbind, context);
+          binding_context_unref (context);
+
+          binding->source_notify = 0;
+        }
+      g_weak_ref_set (&context->source, NULL);
+    }
+
+  /* As above, but with the target. If source==target then no weak notify was
+   * installed for the target, which is why that is stored as a separate
+   * boolean inside the binding. */
+  if (target)
+    {
+      /* There might be a target property notify without a weak notify on the
+       * target or the other way around, so these have to be handled
+       * independently here unlike for the source. */
+      if (binding->target_notify != 0)
+        {
+          g_signal_handler_disconnect (target, binding->target_notify);
+
+          binding->target_notify = 0;
+        }
+      g_weak_ref_set (&context->target, NULL);
+
+      /* Remove the weak notify from the target, at most once */
+      if (binding->target_weak_notify_installed)
+        {
+          g_object_weak_unref (target, weak_unbind, context);
+          binding_context_unref (context);
+          binding->target_weak_notify_installed = FALSE;
+        }
+    }
+
+  /* Make sure to remove the binding only once and return to the caller that
+   * this was the call that actually removed it. */
+  if (!context->binding_removed)
+    {
+      context->binding_removed = TRUE;
+      binding_was_removed = TRUE;
+    }
+
+  return binding_was_removed;
+}
+
 /* the basic assumption is that if either the source or the target
  * goes away then the binding does not exist any more and it should
- * be reaped as well
- */
+ * be reaped as well. Each weak notify owns a strong reference to the
+ * binding that should be dropped here. */
 static void
 weak_unbind (gpointer  user_data,
              GObject  *where_the_object_was)
 {
-  GBinding *binding = user_data;
+  BindingContext *context = user_data;
+  GBinding *binding;
+  GObject *source, *target;
+  gboolean binding_was_removed = FALSE;
+  TransformFunc *transform_func;
 
-  /* if what went away was the source, unset it so that GBinding::finalize
-   * does not try to access it; otherwise, disconnect everything and remove
-   * the GBinding instance from the object's qdata
-   */
-  if (binding->source == where_the_object_was)
-    binding->source = NULL;
-  else
+  binding = g_weak_ref_get (&context->binding);
+  if (!binding)
     {
-      if (binding->source_notify != 0)
-        g_signal_handler_disconnect (binding->source, binding->source_notify);
-
-      g_object_weak_unref (binding->source, weak_unbind, user_data);
-
-      binding->source_notify = 0;
-      binding->source = NULL;
+      /* The binding was already destroyed before so there's nothing to do */
+      binding_context_unref (context);
+      return;
     }
 
-  /* as above, but with the target */
-  if (binding->target == where_the_object_was)
-    binding->target = NULL;
-  else
-    {
-      if (binding->target_notify != 0)
-        g_signal_handler_disconnect (binding->target, binding->target_notify);
+  g_mutex_lock (&binding->unbind_lock);
 
-      g_object_weak_unref (binding->target, weak_unbind, user_data);
+  transform_func = g_steal_pointer (&binding->transform_func);
 
-      binding->target_notify = 0;
-      binding->target = NULL;
-    }
+  source = g_weak_ref_get (&context->source);
+  target = g_weak_ref_get (&context->target);
 
-  /* this will take care of the binding itself */
+  /* If this is called then either the source or target or both must be in the
+   * process of being finalized and their weak reference must be reset to NULL
+   * already.
+   *
+   * If source==target then both will always be NULL here. */
+  g_assert (source == NULL || target == NULL);
+
+  binding_was_removed = unbind_internal_locked (context, binding, source, target);
+
+  g_mutex_unlock (&binding->unbind_lock);
+
+  /* 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);
+
+  /* This releases the strong reference we got from the weak ref above */
   g_object_unref (binding);
+
+  /* This will take care of the binding itself. */
+  if (binding_was_removed)
+    g_object_unref (binding);
+
+  /* Each weak notify owns a reference to the binding context. */
+  binding_context_unref (context);
 }
 
 static gboolean
@@ -299,115 +475,172 @@ default_invert_boolean_transform (GBinding     *binding,
 }
 
 static void
-on_source_notify (GObject    *gobject,
+on_source_notify (GObject          *source,
                   GParamSpec       *pspec,
-                  GBinding   *binding)
+                  BindingContext   *context)
 {
+  GBinding *binding;
+  GObject *target;
+  TransformFunc *transform_func;
   GValue from_value = G_VALUE_INIT;
   GValue to_value = G_VALUE_INIT;
   gboolean res;
 
-  if (binding->is_frozen)
+  binding = g_weak_ref_get (&context->binding);
+  if (!binding)
     return;
 
+  if (binding->is_frozen)
+    {
+      g_object_unref (binding);
+      return;
+    }
+
+  target = g_weak_ref_get (&context->target);
+  if (!target)
+    {
+      g_object_unref (binding);
+      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 (binding->source, binding->source_pspec->name, &from_value);
+  g_object_get_property (source, binding->source_pspec->name, &from_value);
 
-  res = binding->transform_s2t (binding,
+  res = transform_func->transform_s2t (binding,
                                        &from_value,
                                        &to_value,
-                                binding->transform_data);
+                                       transform_func->transform_data);
+
+  transform_func_unref (transform_func);
+
   if (res)
     {
       binding->is_frozen = TRUE;
 
       g_param_value_validate (binding->target_pspec, &to_value);
-      g_object_set_property (binding->target, binding->target_pspec->name, &to_value);
+      g_object_set_property (target, binding->target_pspec->name, &to_value);
 
       binding->is_frozen = FALSE;
     }
 
   g_value_unset (&from_value);
   g_value_unset (&to_value);
+
+  g_object_unref (target);
+  g_object_unref (binding);
 }
 
 static void
-on_target_notify (GObject    *gobject,
+on_target_notify (GObject          *target,
                   GParamSpec       *pspec,
-                  GBinding   *binding)
+                  BindingContext   *context)
 {
+  GBinding *binding;
+  GObject *source;
+  TransformFunc *transform_func;
   GValue from_value = G_VALUE_INIT;
   GValue to_value = G_VALUE_INIT;
   gboolean res;
 
-  if (binding->is_frozen)
+  binding = g_weak_ref_get (&context->binding);
+  if (!binding)
     return;
 
+  if (binding->is_frozen)
+    {
+      g_object_unref (binding);
+      return;
+    }
+
+  source = g_weak_ref_get (&context->source);
+  if (!source)
+    {
+      g_object_unref (binding);
+      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 (binding->target, binding->target_pspec->name, &from_value);
+  g_object_get_property (target, binding->target_pspec->name, &from_value);
 
-  res = binding->transform_t2s (binding,
+  res = transform_func->transform_t2s (binding,
                                        &from_value,
                                        &to_value,
-                                binding->transform_data);
+                                       transform_func->transform_data);
+  transform_func_unref (transform_func);
+
   if (res)
     {
       binding->is_frozen = TRUE;
 
       g_param_value_validate (binding->source_pspec, &to_value);
-      g_object_set_property (binding->source, binding->source_pspec->name, &to_value);
+      g_object_set_property (source, binding->source_pspec->name, &to_value);
 
       binding->is_frozen = FALSE;
     }
 
   g_value_unset (&from_value);
   g_value_unset (&to_value);
+
+  g_object_unref (source);
+  g_object_unref (binding);
 }
 
 static inline void
 g_binding_unbind_internal (GBinding *binding,
                            gboolean  unref_binding)
 {
-  gboolean source_is_target = binding->source == binding->target;
+  BindingContext *context = binding->context;
+  GObject *source, *target;
   gboolean binding_was_removed = FALSE;
+  TransformFunc *transform_func;
 
-  /* dispose of the transformation data */
-  if (binding->notify != NULL)
-    {
-      binding->notify (binding->transform_data);
+  g_mutex_lock (&binding->unbind_lock);
 
-      binding->transform_data = NULL;
-      binding->notify = NULL;
-    }
+  transform_func = g_steal_pointer (&binding->transform_func);
 
-  if (binding->source != NULL)
-    {
-      if (binding->source_notify != 0)
-        g_signal_handler_disconnect (binding->source, binding->source_notify);
+  source = g_weak_ref_get (&context->source);
+  target = g_weak_ref_get (&context->target);
 
-      g_object_weak_unref (binding->source, weak_unbind, binding);
+  /* If the binding was removed previously, source and target are both NULL.
+   * Otherwise both will not be NULL. */
+  g_assert ((source == NULL && target == NULL) || (source != NULL && target != NULL));
 
-      binding->source_notify = 0;
-      binding->source = NULL;
-      binding_was_removed = TRUE;
-    }
+  binding_was_removed = unbind_internal_locked (context, binding, source, target);
 
-  if (binding->target != NULL)
-    {
-      if (binding->target_notify != 0)
-        g_signal_handler_disconnect (binding->target, binding->target_notify);
+  g_mutex_unlock (&binding->unbind_lock);
 
-      if (!source_is_target)
-        g_object_weak_unref (binding->target, weak_unbind, binding);
+  /* 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);
 
-      binding->target_notify = 0;
-      binding->target = NULL;
-      binding_was_removed = TRUE;
-    }
+  g_clear_pointer (&transform_func, transform_func_unref);
 
   if (binding_was_removed && unref_binding)
     g_object_unref (binding);
@@ -420,6 +653,10 @@ g_binding_finalize (GObject *gobject)
 
   g_binding_unbind_internal (binding, FALSE);
 
+  binding_context_unref (binding->context);
+
+  g_mutex_clear (&binding->unbind_lock);
+
   G_OBJECT_CLASS (g_binding_parent_class)->finalize (gobject);
 }
 
@@ -481,11 +718,11 @@ g_binding_set_property (GObject      *gobject,
   switch (prop_id)
     {
     case PROP_SOURCE:
-      binding->source = g_value_get_object (value);
+      g_weak_ref_set (&binding->context->source, g_value_get_object (value));
       break;
 
     case PROP_TARGET:
-      binding->target = g_value_get_object (value);
+      g_weak_ref_set (&binding->context->target, g_value_get_object (value));
       break;
 
     case PROP_SOURCE_PROPERTY:
@@ -535,7 +772,7 @@ g_binding_get_property (GObject    *gobject,
   switch (prop_id)
     {
     case PROP_SOURCE:
-      g_value_set_object (value, binding->source);
+      g_value_take_object (value, g_weak_ref_get (&binding->context->source));
       break;
 
     case PROP_SOURCE_PROPERTY:
@@ -544,7 +781,7 @@ g_binding_get_property (GObject    *gobject,
       break;
 
     case PROP_TARGET:
-      g_value_set_object (value, binding->target);
+      g_value_take_object (value, g_weak_ref_get (&binding->context->target));
       break;
 
     case PROP_TARGET_PROPERTY:
@@ -567,12 +804,15 @@ g_binding_constructed (GObject *gobject)
 {
   GBinding *binding = G_BINDING (gobject);
   GBindingTransformFunc transform_func = default_transform;
+  GObject *source, *target;
   GQuark source_property_detail;
   GClosure *source_notify_closure;
 
   /* assert that we were constructed correctly */
-  g_assert (binding->source != NULL);
-  g_assert (binding->target != NULL);
+  source = g_weak_ref_get (&binding->context->source);
+  target = g_weak_ref_get (&binding->context->target);
+  g_assert (source != NULL);
+  g_assert (target != NULL);
   g_assert (binding->source_property != NULL);
   g_assert (binding->target_property != NULL);
 
@@ -580,8 +820,8 @@ g_binding_constructed (GObject *gobject)
    * g_object_bind_property_full() does it; we cannot fail construction
    * anyway, so it would be hard for use to properly warn here
    */
-  binding->source_pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (binding->source), binding->source_property);
-  binding->target_pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (binding->target), binding->target_property);
+  binding->source_pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (source), binding->source_property);
+  binding->target_pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (target), binding->target_property);
   g_assert (binding->source_pspec != NULL);
   g_assert (binding->target_pspec != NULL);
 
@@ -590,22 +830,19 @@ 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),
-                                          binding, NULL);
-  binding->source_notify = g_signal_connect_closure_by_id (binding->source,
+                                          binding_context_ref (binding->context),
+                                          (GClosureNotify) binding_context_unref);
+  binding->source_notify = g_signal_connect_closure_by_id (source,
                                                            gobject_notify_signal_id,
                                                            source_property_detail,
                                                            source_notify_closure,
                                                            FALSE);
 
-  g_object_weak_ref (binding->source, weak_unbind, binding);
+  g_object_weak_ref (source, weak_unbind, binding_context_ref (binding->context));
 
   if (binding->flags & G_BINDING_BIDIRECTIONAL)
     {
@@ -614,16 +851,27 @@ g_binding_constructed (GObject *gobject)
 
       target_property_detail = g_quark_from_string (binding->target_property);
       target_notify_closure = g_cclosure_new (G_CALLBACK (on_target_notify),
-                                              binding, NULL);
-      binding->target_notify = g_signal_connect_closure_by_id (binding->target,
+                                              binding_context_ref (binding->context),
+                                              (GClosureNotify) binding_context_unref);
+      binding->target_notify = g_signal_connect_closure_by_id (target,
                                                                gobject_notify_signal_id,
                                                                target_property_detail,
                                                                target_notify_closure,
                                                                FALSE);
     }
 
-  if (binding->target != binding->source)
-    g_object_weak_ref (binding->target, weak_unbind, binding);
+  if (target != source)
+    {
+      g_object_weak_ref (target, weak_unbind, binding_context_ref (binding->context));
+
+      /* Need to remember separately if a target weak notify was installed as
+       * unlike for the source it can exist independently of the property
+       * notification callback */
+      binding->target_weak_notify_installed = TRUE;
+    }
+
+  g_object_unref (source);
+  g_object_unref (target);
 }
 
 static void
@@ -728,6 +976,12 @@ g_binding_class_init (GBindingClass *klass)
 static void
 g_binding_init (GBinding *binding)
 {
+  g_mutex_init (&binding->unbind_lock);
+
+  binding->context = g_atomic_rc_box_new0 (BindingContext);
+  g_weak_ref_init (&binding->context->binding, binding);
+  g_weak_ref_init (&binding->context->source, NULL);
+  g_weak_ref_init (&binding->context->target, NULL);
 }
 
 /**
@@ -758,17 +1012,55 @@ g_binding_get_flags (GBinding *binding)
  * strong reference to the source. If the source is destroyed before the
  * binding then this function will return %NULL.
  *
+ * Use g_binding_dup_source() if the source or binding are used from different
+ * threads as otherwise the pointer returned from this function might become
+ * invalid if the source is finalized from another thread in the meantime.
+ *
  * Returns: (transfer none) (nullable): the source #GObject, or %NULL if the
  *     source does not exist any more.
  *
+ * Deprecated: 2.68: Use g_binding_dup_source() for a safer version of this
+ * function.
+ *
  * Since: 2.26
  */
 GObject *
 g_binding_get_source (GBinding *binding)
 {
+  GObject *source;
+
   g_return_val_if_fail (G_IS_BINDING (binding), NULL);
 
-  return binding->source;
+  source = g_weak_ref_get (&binding->context->source);
+  /* Unref here, this API is not thread-safe
+   * FIXME: Remove this API when we next break API */
+  if (source)
+    g_object_unref (source);
+
+  return source;
+}
+
+/**
+ * g_binding_dup_source:
+ * @binding: a #GBinding
+ *
+ * Retrieves the #GObject instance used as the source of the binding.
+ *
+ * A #GBinding can outlive the source #GObject as the binding does not hold a
+ * strong reference to the source. If the source is destroyed before the
+ * binding then this function will return %NULL.
+ *
+ * Returns: (transfer full) (nullable): the source #GObject, or %NULL if the
+ *     source does not exist any more.
+ *
+ * Since: 2.68
+ */
+GObject *
+g_binding_dup_source (GBinding *binding)
+{
+  g_return_val_if_fail (G_IS_BINDING (binding), NULL);
+
+  return g_weak_ref_get (&binding->context->source);
 }
 
 /**
@@ -781,17 +1073,55 @@ g_binding_get_source (GBinding *binding)
  * strong reference to the target. If the target is destroyed before the
  * binding then this function will return %NULL.
  *
+ * Use g_binding_dup_target() if the target or binding are used from different
+ * threads as otherwise the pointer returned from this function might become
+ * invalid if the target is finalized from another thread in the meantime.
+ *
  * Returns: (transfer none) (nullable): the target #GObject, or %NULL if the
  *     target does not exist any more.
  *
+ * Deprecated: 2.68: Use g_binding_dup_target() for a safer version of this
+ * function.
+ *
  * Since: 2.26
  */
 GObject *
 g_binding_get_target (GBinding *binding)
 {
+  GObject *target;
+
   g_return_val_if_fail (G_IS_BINDING (binding), NULL);
 
-  return binding->target;
+  target = g_weak_ref_get (&binding->context->target);
+  /* Unref here, this API is not thread-safe
+   * FIXME: Remove this API when we next break API */
+  if (target)
+    g_object_unref (target);
+
+  return target;
+}
+
+/**
+ * g_binding_dup_target:
+ * @binding: a #GBinding
+ *
+ * Retrieves the #GObject instance used as the target of the binding.
+ *
+ * A #GBinding can outlive the target #GObject as the binding does not hold a
+ * strong reference to the target. If the target is destroyed before the
+ * binding then this function will return %NULL.
+ *
+ * Returns: (transfer full) (nullable): the target #GObject, or %NULL if the
+ *     target does not exist any more.
+ *
+ * Since: 2.68
+ */
+GObject *
+g_binding_dup_target (GBinding *binding)
+{
+  g_return_val_if_fail (G_IS_BINDING (binding), NULL);
+
+  return g_weak_ref_get (&binding->context->target);
 }
 
 /**
@@ -840,9 +1170,13 @@ g_binding_get_target_property (GBinding *binding)
  * property expressed by @binding.
  *
  * This function will release the reference that is being held on
- * the @binding instance; if you want to hold on to the #GBinding instance
- * after calling g_binding_unbind(), you will need to hold a reference
- * to it.
+ * the @binding instance if the binding is still bound; if you want to hold on
+ * to the #GBinding instance after calling g_binding_unbind(), you will need
+ * to hold a reference to it.
+ *
+ * Note however that this function does not take ownership of @binding, it
+ * only unrefs the reference that was initially created by
+ * g_object_bind_property() and is owned by the binding.
  *
  * Since: 2.38
  */
@@ -1028,14 +1362,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;
 
-  binding->transform_data = user_data;
-  binding->notify = notify;
+  if (transform_from == NULL)
+    transform_from = binding->transform_func->transform_t2s;
+
+  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
@@ -1043,7 +1380,7 @@ g_object_bind_property_full (gpointer               source,
    * will emit a notification on the target
    */
   if (flags & G_BINDING_SYNC_CREATE)
-    on_source_notify (binding->source, binding->source_pspec, binding);
+    on_source_notify (source, binding->source_pspec, binding->context);
 
   return binding;
 }
@@ -1077,6 +1414,13 @@ g_object_bind_property_full (gpointer               source,
  * @source and the @target you can just call g_object_unref() on the returned
  * #GBinding instance.
  *
+ * Removing the binding by calling g_object_unref() on it must only be done if
+ * the binding, @source and @target are only used from a single thread and it
+ * is clear that both @source and @target outlive the binding. Especially it
+ * is not safe to rely on this if the binding, @source or @target can be
+ * finalized from different threads. Keep another reference to the binding and
+ * use g_binding_unbind() instead to be on the safe side.
+ *
  * A #GObject can have multiple bindings.
  *
  * Returns: (transfer none): the #GBinding instance representing the

gobject/gbinding.h

@@ -108,10 +108,14 @@ GType                 g_binding_get_type            (void) G_GNUC_CONST;
 
 GLIB_AVAILABLE_IN_ALL
 GBindingFlags         g_binding_get_flags           (GBinding *binding);
-GLIB_AVAILABLE_IN_ALL
+GLIB_DEPRECATED_IN_2_68_FOR(g_binding_dup_source)
 GObject *             g_binding_get_source          (GBinding *binding);
-GLIB_AVAILABLE_IN_ALL
+GLIB_AVAILABLE_IN_2_68
+GObject *             g_binding_dup_source          (GBinding *binding);
+GLIB_DEPRECATED_IN_2_68_FOR(g_binding_dup_target)
 GObject *             g_binding_get_target          (GBinding *binding);
+GLIB_AVAILABLE_IN_2_68
+GObject *             g_binding_dup_target          (GBinding *binding);
 GLIB_AVAILABLE_IN_ALL
 const gchar *         g_binding_get_source_property (GBinding *binding);
 GLIB_AVAILABLE_IN_ALL

gobject/tests/binding.c

@@ -353,6 +353,7 @@ binding_default (void)
 {
   BindingSource *source = g_object_new (binding_source_get_type (), NULL);
   BindingTarget *target = g_object_new (binding_target_get_type (), NULL);
+  GObject *tmp;
   GBinding *binding;
 
   binding = g_object_bind_property (source, "foo",
@@ -360,8 +361,14 @@ binding_default (void)
                                     G_BINDING_DEFAULT);
 
   g_object_add_weak_pointer (G_OBJECT (binding), (gpointer *) &binding);
-  g_assert_true ((BindingSource *) g_binding_get_source (binding) == source);
-  g_assert_true ((BindingTarget *) g_binding_get_target (binding) == target);
+  tmp = g_binding_dup_source (binding);
+  g_assert_nonnull (tmp);
+  g_assert_true ((BindingSource *) tmp == source);
+  g_object_unref (tmp);
+  tmp = g_binding_dup_target (binding);
+  g_assert_nonnull (tmp);
+  g_assert_true ((BindingTarget *) tmp == target);
+  g_object_unref (tmp);
   g_assert_cmpstr (g_binding_get_source_property (binding), ==, "foo");
   g_assert_cmpstr (g_binding_get_target_property (binding), ==, "bar");
   g_assert_cmpint (g_binding_get_flags (binding), ==, G_BINDING_DEFAULT);
@@ -388,6 +395,7 @@ binding_canonicalisation (void)
   BindingSource *source = g_object_new (binding_source_get_type (), NULL);
   BindingTarget *target = g_object_new (binding_target_get_type (), NULL);
   GBinding *binding;
+  GObject *tmp;
 
   g_test_summary ("Test that bindings set up with non-canonical property names work");
 
@@ -396,8 +404,14 @@ binding_canonicalisation (void)
                                     G_BINDING_DEFAULT);
 
   g_object_add_weak_pointer (G_OBJECT (binding), (gpointer *) &binding);
-  g_assert_true ((BindingSource *) g_binding_get_source (binding) == source);
-  g_assert_true ((BindingTarget *) g_binding_get_target (binding) == target);
+  tmp = g_binding_dup_source (binding);
+  g_assert_nonnull (tmp);
+  g_assert_true ((BindingSource *) tmp == source);
+  g_object_unref (tmp);
+  tmp = g_binding_dup_target (binding);
+  g_assert_nonnull (tmp);
+  g_assert_true ((BindingTarget *) tmp == target);
+  g_object_unref (tmp);
   g_assert_cmpstr (g_binding_get_source_property (binding), ==, "double-value");
   g_assert_cmpstr (g_binding_get_target_property (binding), ==, "double-value");
   g_assert_cmpint (g_binding_get_flags (binding), ==, G_BINDING_DEFAULT);
@@ -886,6 +900,182 @@ binding_interface (void)
   g_object_unref (target);
 }
 
+typedef struct {
+  GThread *thread;
+  GBinding *binding;
+  GMutex *lock;
+  GCond *cond;
+  gboolean *wait;
+  gint *count; /* (atomic) */
+} ConcurrentUnbindData;
+
+static gpointer
+concurrent_unbind_func (gpointer data)
+{
+  ConcurrentUnbindData *unbind_data = data;
+
+  g_mutex_lock (unbind_data->lock);
+  g_atomic_int_inc (unbind_data->count);
+  while (*unbind_data->wait)
+    g_cond_wait (unbind_data->cond, unbind_data->lock);
+  g_mutex_unlock (unbind_data->lock);
+  g_binding_unbind (unbind_data->binding);
+  g_object_unref (unbind_data->binding);
+
+  return NULL;
+}
+
+static void
+binding_concurrent_unbind (void)
+{
+  guint i, j;
+
+  g_test_summary ("Test that unbinding from multiple threads concurrently works correctly");
+
+  for (i = 0; i < 50; i++)
+    {
+      BindingSource *source = g_object_new (binding_source_get_type (), NULL);
+      BindingTarget *target = g_object_new (binding_target_get_type (), NULL);
+      GBinding *binding;
+      GQueue threads = G_QUEUE_INIT;
+      GMutex lock;
+      GCond cond;
+      gboolean wait = TRUE;
+      gint count = 0; /* (atomic) */
+      ConcurrentUnbindData *data;
+
+      g_mutex_init (&lock);
+      g_cond_init (&cond);
+
+      binding = g_object_bind_property (source, "foo",
+                                        target, "bar",
+                                        G_BINDING_BIDIRECTIONAL);
+      g_object_ref (binding);
+
+      for (j = 0; j < 10; j++)
+        {
+          data = g_new0 (ConcurrentUnbindData, 1);
+
+          data->binding = g_object_ref (binding);
+          data->lock = &lock;
+          data->cond = &cond;
+          data->wait = &wait;
+          data->count = &count;
+
+          data->thread = g_thread_new ("binding-concurrent", concurrent_unbind_func, data);
+          g_queue_push_tail (&threads, data);
+        }
+
+      /* wait until all threads are started */
+      while (g_atomic_int_get (&count) < 10)
+        g_thread_yield ();
+
+      g_mutex_lock (&lock);
+      wait = FALSE;
+      g_cond_broadcast (&cond);
+      g_mutex_unlock (&lock);
+
+      while ((data = g_queue_pop_head (&threads)))
+        {
+          g_thread_join (data->thread);
+          g_free (data);
+        }
+
+      g_mutex_clear (&lock);
+      g_cond_clear (&cond);
+
+      g_object_unref (binding);
+      g_object_unref (source);
+      g_object_unref (target);
+    }
+}
+
+typedef struct {
+  GObject *object;
+  GMutex *lock;
+  GCond *cond;
+  gint *count; /* (atomic) */
+  gboolean *wait;
+} ConcurrentFinalizeData;
+
+static gpointer
+concurrent_finalize_func (gpointer data)
+{
+  ConcurrentFinalizeData *finalize_data = data;
+
+  g_mutex_lock (finalize_data->lock);
+  g_atomic_int_inc (finalize_data->count);
+  while (*finalize_data->wait)
+    g_cond_wait (finalize_data->cond, finalize_data->lock);
+  g_mutex_unlock (finalize_data->lock);
+  g_object_unref (finalize_data->object);
+  g_free (finalize_data);
+
+  return NULL;
+}
+
+static void
+binding_concurrent_finalizing (void)
+{
+  guint i;
+
+  g_test_summary ("Test that finalizing source/target from multiple threads concurrently works correctly");
+
+  for (i = 0; i < 50; i++)
+    {
+      BindingSource *source = g_object_new (binding_source_get_type (), NULL);
+      BindingTarget *target = g_object_new (binding_target_get_type (), NULL);
+      GBinding *binding;
+      GMutex lock;
+      GCond cond;
+      gboolean wait = TRUE;
+      ConcurrentFinalizeData *data;
+      GThread *source_thread, *target_thread;
+      gint count = 0; /* (atomic) */
+
+      g_mutex_init (&lock);
+      g_cond_init (&cond);
+
+      binding = g_object_bind_property (source, "foo",
+                                        target, "bar",
+                                        G_BINDING_BIDIRECTIONAL);
+      g_object_ref (binding);
+
+      data = g_new0 (ConcurrentFinalizeData, 1);
+      data->object = (GObject *) source;
+      data->wait = &wait;
+      data->lock = &lock;
+      data->cond = &cond;
+      data->count = &count;
+      source_thread = g_thread_new ("binding-concurrent", concurrent_finalize_func, data);
+
+      data = g_new0 (ConcurrentFinalizeData, 1);
+      data->object = (GObject *) target;
+      data->wait = &wait;
+      data->lock = &lock;
+      data->cond = &cond;
+      data->count = &count;
+      target_thread = g_thread_new ("binding-concurrent", concurrent_finalize_func, data);
+
+      /* wait until all threads are started */
+      while (g_atomic_int_get (&count) < 2)
+        g_thread_yield ();
+
+      g_mutex_lock (&lock);
+      wait = FALSE;
+      g_cond_broadcast (&cond);
+      g_mutex_unlock (&lock);
+
+      g_thread_join (source_thread);
+      g_thread_join (target_thread);
+
+      g_mutex_clear (&lock);
+      g_cond_clear (&cond);
+
+      g_object_unref (binding);
+    }
+}
+
 int
 main (int argc, char *argv[])
 {
@@ -908,6 +1098,8 @@ main (int argc, char *argv[])
   g_test_add_func ("/binding/unbind-multiple", binding_unbind_multiple);
   g_test_add_func ("/binding/fail", binding_fail);
   g_test_add_func ("/binding/interface", binding_interface);
+  g_test_add_func ("/binding/concurrent-unbind", binding_concurrent_unbind);
+  g_test_add_func ("/binding/concurrent-finalizing", binding_concurrent_finalizing);
 
   return g_test_run ();
 }