gbinding: use g_object_weak_ref_full for bindings
The pattern here is that we acquire strong references via GWeakRef.
So you might think that g_object_weak_unref() is safe to call.
However, it is not safe, in case where another thread might run
g_object_run_dispose(). Which is clear, because GWeakRef only ensures we
hold a strong reference, but g_object_run_dispose() is anyway run on an
object where we already hold a reference.
In weak_unbind(), we obtain strong references, but (after) that
point, another thead might call g_object_run_dispose(). Then,
inside unbind_internal_locked() we do:
g_object_weak_unref (source, weak_unbind, context);
binding_context_unref (context);
Note that here weak_unbind might have already be unrefed, and
g_object_weak_unref() fails an assertion. But worse, the
weak_unbind callback will also be called and we issue two
binding_context_unref() and crash.
This is fixed by using g_object_weak_ref_full() (which handles the case
that the weak notification might have already be emitted) and a separate
GDestroyNotify (that is guaranteed to run exactly once).
This still doesn't make it fully work. Note that we also call
g_signal_handler_disconnect (source, binding->source_notify);
this has exactly the same problem. A concurrent g_object_run_dispose()
will already disconnect all signal handlers, and calling disconnect
fails an assertion. I think the solution for that is a new API
g_signal_handler_try_disconnect(), which does not assert. After all, the
gulong signal ID is unique (the gulong is large enough to never wrap and
there is even a g_error() check against that).
diff --git a/gobject/gbinding.c b/gobject/gbinding.c
index 193bd5c..10739df 100644
--- a/gobject/gbinding.c
+++ b/gobject/gbinding.c
@@ -314,8 +314,7 @@
{
g_signal_handler_disconnect (source, binding->source_notify);
- g_object_weak_unref (source, weak_unbind, context);
- binding_context_unref (context);
+ g_object_weak_unref_full (source, weak_unbind, context, FALSE, FALSE);
binding->source_notify = 0;
}
@@ -341,8 +340,7 @@
/* 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);
+ g_object_weak_unref_full (target, weak_unbind, context, FALSE, FALSE);
binding->target_weak_notify_installed = FALSE;
}
}
@@ -376,7 +374,6 @@
if (!binding)
{
/* The binding was already destroyed before so there's nothing to do */
- binding_context_unref (context);
return;
}
@@ -430,9 +427,6 @@
/* 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
@@ -833,7 +827,11 @@
source_notify_closure,
FALSE);
- g_object_weak_ref (source, weak_unbind, binding_context_ref (binding->context));
+ g_object_weak_ref_full (source,
+ weak_unbind,
+ binding_context_ref (binding->context),
+ (GDestroyNotify) binding_context_unref,
+ FALSE);
if (binding->flags & G_BINDING_BIDIRECTIONAL)
{
@@ -853,7 +851,11 @@
if (target != source)
{
- g_object_weak_ref (target, weak_unbind, binding_context_ref (binding->context));
+ g_object_weak_ref_full (target,
+ weak_unbind,
+ binding_context_ref (binding->context),
+ (GDestroyNotify) binding_context_unref,
+ FALSE);
/* Need to remember separately if a target weak notify was installed as
* unlike for the source it can exist independently of the property