From 51ee5cf1c2d0cb474f00d79972a1dae16b3e49b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Mon, 19 Oct 2020 13:55:12 +0300 Subject: [PATCH] Use GWeakRef in GBinding This makes GBinding slightly more thread-safe as the source/target can't simply disappear. --- gobject/gbinding.c | 137 +++++++++++++++++++++++++++++++-------------- 1 file changed, 94 insertions(+), 43 deletions(-) diff --git a/gobject/gbinding.c b/gobject/gbinding.c index 662d76b3c..1448adb36 100644 --- a/gobject/gbinding.c +++ b/gobject/gbinding.c @@ -150,8 +150,8 @@ struct _GBinding GObject parent_instance; /* no reference is held on the objects, to avoid cycles */ - GObject *source; - GObject *target; + GWeakRef source; + GWeakRef target; /* the property names are interned, so they should not be freed */ const gchar *source_property; @@ -204,36 +204,38 @@ weak_unbind (gpointer user_data, GObject *where_the_object_was) { GBinding *binding = user_data; + GObject *source, *target; + + source = g_weak_ref_get (&binding->source); + target = g_weak_ref_get (&binding->target); /* 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 + if (source) { if (binding->source_notify != 0) - g_signal_handler_disconnect (binding->source, binding->source_notify); + g_signal_handler_disconnect (source, binding->source_notify); - g_object_weak_unref (binding->source, weak_unbind, user_data); + g_object_weak_unref (source, weak_unbind, user_data); binding->source_notify = 0; - binding->source = NULL; + g_weak_ref_set (&binding->source, NULL); + g_object_unref (source); } /* as above, but with the target */ - if (binding->target == where_the_object_was) - binding->target = NULL; - else + if (target) { if (binding->target_notify != 0) - g_signal_handler_disconnect (binding->target, binding->target_notify); + g_signal_handler_disconnect (target, binding->target_notify); - g_object_weak_unref (binding->target, weak_unbind, user_data); + g_object_weak_unref (target, weak_unbind, user_data); binding->target_notify = 0; - binding->target = NULL; + g_weak_ref_set (&binding->target, NULL); + g_object_unref (target); } /* this will take care of the binding itself */ @@ -303,6 +305,7 @@ on_source_notify (GObject *gobject, GParamSpec *pspec, GBinding *binding) { + GObject *target; GValue from_value = G_VALUE_INIT; GValue to_value = G_VALUE_INIT; gboolean res; @@ -310,10 +313,14 @@ on_source_notify (GObject *gobject, if (binding->is_frozen) return; + target = g_weak_ref_get (&binding->target); + if (!target) + return; + 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 (gobject, binding->source_pspec->name, &from_value); res = binding->transform_s2t (binding, &from_value, @@ -324,13 +331,15 @@ on_source_notify (GObject *gobject, 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); } static void @@ -338,6 +347,7 @@ on_target_notify (GObject *gobject, GParamSpec *pspec, GBinding *binding) { + GObject *source; GValue from_value = G_VALUE_INIT; GValue to_value = G_VALUE_INIT; gboolean res; @@ -345,10 +355,14 @@ on_target_notify (GObject *gobject, if (binding->is_frozen) return; + source = g_weak_ref_get (&binding->source); + if (!source) + return; + 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 (gobject, binding->target_pspec->name, &from_value); res = binding->transform_t2s (binding, &from_value, @@ -359,22 +373,30 @@ on_target_notify (GObject *gobject, 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); } static inline void g_binding_unbind_internal (GBinding *binding, gboolean unref_binding) { - gboolean source_is_target = binding->source == binding->target; + GObject *source, *target; + gboolean source_is_target; gboolean binding_was_removed = FALSE; + source = g_weak_ref_get (&binding->source); + target = g_weak_ref_get (&binding->target); + + source_is_target = source == target; + /* dispose of the transformation data */ if (binding->notify != NULL) { @@ -384,29 +406,33 @@ g_binding_unbind_internal (GBinding *binding, binding->notify = NULL; } - if (binding->source != NULL) + if (source != NULL) { if (binding->source_notify != 0) - g_signal_handler_disconnect (binding->source, binding->source_notify); + g_signal_handler_disconnect (source, binding->source_notify); - g_object_weak_unref (binding->source, weak_unbind, binding); + g_object_weak_unref (source, weak_unbind, binding); binding->source_notify = 0; - binding->source = NULL; + g_weak_ref_set (&binding->source, NULL); binding_was_removed = TRUE; + + g_object_unref (source); } - if (binding->target != NULL) + if (target != NULL) { if (binding->target_notify != 0) - g_signal_handler_disconnect (binding->target, binding->target_notify); + g_signal_handler_disconnect (target, binding->target_notify); if (!source_is_target) - g_object_weak_unref (binding->target, weak_unbind, binding); + g_object_weak_unref (target, weak_unbind, binding); binding->target_notify = 0; - binding->target = NULL; + g_weak_ref_set (&binding->target, NULL); binding_was_removed = TRUE; + + g_object_unref (target); } if (binding_was_removed && unref_binding) @@ -420,6 +446,9 @@ g_binding_finalize (GObject *gobject) g_binding_unbind_internal (binding, FALSE); + g_weak_ref_clear (&binding->source); + g_weak_ref_clear (&binding->target); + G_OBJECT_CLASS (g_binding_parent_class)->finalize (gobject); } @@ -481,11 +510,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->source, g_value_get_object (value)); break; case PROP_TARGET: - binding->target = g_value_get_object (value); + g_weak_ref_set (&binding->target, g_value_get_object (value)); break; case PROP_SOURCE_PROPERTY: @@ -535,7 +564,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->source)); break; case PROP_SOURCE_PROPERTY: @@ -544,7 +573,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->target)); break; case PROP_TARGET_PROPERTY: @@ -567,12 +596,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->source); + target = g_weak_ref_get (&binding->target); + g_assert (source != NULL); + g_assert (target != NULL); g_assert (binding->source_property != NULL); g_assert (binding->target_property != NULL); @@ -580,8 +612,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); @@ -599,13 +631,13 @@ g_binding_constructed (GObject *gobject) 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->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); if (binding->flags & G_BINDING_BIDIRECTIONAL) { @@ -615,15 +647,18 @@ 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->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); + + g_object_unref (source); + g_object_unref (target); } static void @@ -766,9 +801,17 @@ g_binding_get_flags (GBinding *binding) 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->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; } /** @@ -789,9 +832,17 @@ g_binding_get_source (GBinding *binding) 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->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; } /** @@ -1043,7 +1094,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); return binding; }