[glib: 5/9] Factor out common GBinding unbind code into a separate function

[Philip Withnall <pwithnall src gnome org>]

commit d296ad435d3d6a2822ae94507f27217d67240b48
Author: Sebastian Dröge <sebastian centricular com>
Date:   Tue Nov 24 14:31:21 2020 +0200

    Factor out common GBinding unbind code into a separate function
    
    This was previously duplicated in two places.

 gobject/gbinding.c | 140 ++++++++++++++++++++++-------------------------------
 1 file changed, 59 insertions(+), 81 deletions(-)
---
diff --git a/gobject/gbinding.c b/gobject/gbinding.c
index 250b7d38d..6e8d95e4b 100644
--- a/gobject/gbinding.c
+++ b/gobject/gbinding.c
@@ -288,44 +288,27 @@ static guint gobject_notify_signal_id;
 
 G_DEFINE_TYPE (GBinding, g_binding, G_TYPE_OBJECT)
 
-/* 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. 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)
+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)
 {
-  BindingContext *context = user_data;
-  GBinding *binding;
-  GObject *source, *target;
   gboolean binding_was_removed = FALSE;
 
-  binding = g_weak_ref_get (&context->binding);
-  if (!binding)
-    {
-      /* The binding was already destroyed before so there's nothing to do */
-      binding_context_unref (context);
-      return;
-    }
-
-  g_mutex_lock (&binding->unbind_lock);
-
-  source = g_weak_ref_get (&context->source);
-  target = g_weak_ref_get (&context->target);
-
-  /* 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. */
-  g_assert (source == NULL || target == NULL);
+  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.
+   * 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.
    *
-   * The property notification has already been removed for the object for
-   * which this function was called and does not have to be removed manually.
+   * If both still exist we clean up everything set up by the binding.
    */
   if (source)
     {
@@ -343,14 +326,14 @@ weak_unbind (gpointer  user_data,
       g_weak_ref_set (&context->source, NULL);
     }
 
-  /* As above, but with the target. If source == target then both will be NULL
-   * inside this function so there's no risk of removing the weak notify
-   * twice. */
+  /* 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 */
+       * independently here unlike for the source. */
       if (binding->target_notify != 0)
         {
           g_signal_handler_disconnect (target, binding->target_notify);
@@ -368,13 +351,52 @@ weak_unbind (gpointer  user_data,
         }
     }
 
-  /* Make sure to remove the binding only once */
+  /* 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. 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)
+{
+  BindingContext *context = user_data;
+  GBinding *binding;
+  GObject *source, *target;
+  gboolean binding_was_removed = FALSE;
+
+  binding = g_weak_ref_get (&context->binding);
+  if (!binding)
+    {
+      /* The binding was already destroyed before so there's nothing to do */
+      binding_context_unref (context);
+      return;
+    }
+
+  g_mutex_lock (&binding->unbind_lock);
+
+  source = g_weak_ref_get (&context->source);
+  target = g_weak_ref_get (&context->target);
+
+  /* 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 and target after the mutex is unlocked as it might
@@ -608,51 +630,7 @@ g_binding_unbind_internal (GBinding *binding,
    * Otherwise both will not be NULL. */
   g_assert ((source == NULL && target == NULL) || (source != NULL && target != NULL));
 
-  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 (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 */
-  if (!context->binding_removed)
-    {
-      context->binding_removed = TRUE;
-      binding_was_removed = TRUE;
-    }
+  binding_was_removed = unbind_internal_locked (context, binding, source, target);
 
   g_mutex_unlock (&binding->unbind_lock);