gobject: fix race and use g_datalist_id_update_atomic() for closure array

There are two calls to

  g_datalist_id_set_data (&object->qdata, quark_closure_array, NULL);

that don't take a OPTIONAL_BIT_LOCK_CLOSURE_ARRAY lock.

One is right before finalize(), at which point it's most likely safe, as
nobody else should have a reference to the object to do anything
concurrently.

The other one is in g_object_real_dispose(). Usually, at this point too
no other thread has have a reference to the object. However, it could
obtain one by dispose() passing on a reference. In that case, the
removal of the quark_closure_array data can race against
g_object_watch_closure() and object_remove_closure().

Fix that, by having all operations on the CArray performed under a lock.
In this case, using the GData lock via g_datalist_id_update_atomic().

This also saves the additional OPTIONAL_BIT_LOCK_CLOSURE_ARRAY lock.
At the expense of doing slightly more work while holding the GData lock.

Also free the empty closure data during object_remove_closure(). This
frees some unused memory.

gobject/gobject.c

@@ -126,7 +126,6 @@ enum {
  * (per-object!) is not worth it. */
 #define OPTIONAL_BIT_LOCK_WEAK_REFS      1
 #define OPTIONAL_BIT_LOCK_TOGGLE_REFS    2
-#define OPTIONAL_BIT_LOCK_CLOSURE_ARRAY  3
 
 #if SIZEOF_INT == 4 && GLIB_SIZEOF_VOID_P >= 8
 #define HAVE_OPTIONAL_FLAGS_IN_GOBJECT 1
@@ -5263,27 +5262,44 @@ typedef struct {
   GClosure *closures[1]; /* flexible array */
 } CArray;
 
+static gpointer
+object_remove_closure_cb (gpointer *data,
+                          GDestroyNotify *destroy_notify,
+                          gpointer user_data)
+{
+  GClosure *closure = user_data;
+  CArray *carray = *data;
+  guint i;
+
+  for (i = 0; i < carray->n_closures; i++)
+    {
+      if (carray->closures[i] == closure)
+        {
+          carray->n_closures--;
+          if (carray->n_closures == 0)
+            {
+              g_free (carray);
+              *data = NULL;
+            }
+          else if (i < carray->n_closures)
+            carray->closures[i] = carray->closures[carray->n_closures];
+          return NULL;
+        }
+    }
+
+  g_return_val_if_reached (NULL);
+}
+
 static void
-object_remove_closure (gpointer  data,
+object_remove_closure (gpointer data,
-		       GClosure *closure)
+                       GClosure *closure)
 {
   GObject *object = data;
-  CArray *carray;
+
-  guint i;
+  _g_datalist_id_update_atomic (&object->qdata,
-  
+                                quark_closure_array,
-  object_bit_lock (object, OPTIONAL_BIT_LOCK_CLOSURE_ARRAY);
+                                object_remove_closure_cb,
-  carray = g_object_get_qdata (object, quark_closure_array);
+                                closure);
-  for (i = 0; i < carray->n_closures; i++)
-    if (carray->closures[i] == closure)
-      {
-	carray->n_closures--;
-	if (i < carray->n_closures)
-	  carray->closures[i] = carray->closures[carray->n_closures];
-	object_bit_unlock (object, OPTIONAL_BIT_LOCK_CLOSURE_ARRAY);
-	return;
-      }
-  object_bit_unlock (object, OPTIONAL_BIT_LOCK_CLOSURE_ARRAY);
-  g_assert_not_reached ();
 }
 
 static void
@@ -5306,6 +5322,38 @@ destroy_closure_array (gpointer data)
   g_free (carray);
 }
 
+static gpointer
+g_object_watch_closure_cb (gpointer *data,
+                           GDestroyNotify *destroy_notify,
+                           gpointer user_data)
+{
+  GObject *object = ((gpointer *) user_data)[0];
+  GClosure *closure = ((gpointer *) user_data)[1];
+  CArray *carray = *data;
+  guint i;
+
+  if (!carray)
+    {
+      carray = g_new (CArray, 1);
+      carray->object = object;
+      carray->n_closures = 1;
+      i = 0;
+
+      *destroy_notify = destroy_closure_array;
+    }
+  else
+    {
+      i = carray->n_closures++;
+      carray = g_realloc (carray, sizeof (*carray) + sizeof (carray->closures[0]) * i);
+    }
+
+  *data = carray;
+
+  carray->closures[i] = closure;
+
+  return NULL;
+}
+
 /**
  * g_object_watch_closure:
  * @object: #GObject restricting lifetime of @closure
@@ -5322,39 +5370,24 @@ destroy_closure_array (gpointer data)
  * use this @object as closure data.
  */
 void
-g_object_watch_closure (GObject  *object,
+g_object_watch_closure (GObject *object,
-			GClosure *closure)
+                        GClosure *closure)
 {
-  CArray *carray;
-  guint i;
-  
   g_return_if_fail (G_IS_OBJECT (object));
   g_return_if_fail (closure != NULL);
   g_return_if_fail (closure->is_invalid == FALSE);
   g_return_if_fail (closure->in_marshal == FALSE);
-  g_return_if_fail (g_atomic_int_get (&object->ref_count) > 0);	/* this doesn't work on finalizing objects */
+  g_return_if_fail (g_atomic_int_get (&object->ref_count) > 0); /* this doesn't work on finalizing objects */
-  
+
   g_closure_add_invalidate_notifier (closure, object, object_remove_closure);
   g_closure_add_marshal_guards (closure,
-				object, (GClosureNotify) g_object_ref,
+                                object, (GClosureNotify) g_object_ref,
-				object, (GClosureNotify) g_object_unref);
+                                object, (GClosureNotify) g_object_unref);
-  object_bit_lock (object, OPTIONAL_BIT_LOCK_CLOSURE_ARRAY);
+
-  carray = g_datalist_id_remove_no_notify (&object->qdata, quark_closure_array);
+  _g_datalist_id_update_atomic (&object->qdata,
-  if (!carray)
+                                quark_closure_array,
-    {
+                                g_object_watch_closure_cb,
-      carray = g_renew (CArray, NULL, 1);
+                                ((gpointer[]){ object, closure }));
-      carray->object = object;
-      carray->n_closures = 1;
-      i = 0;
-    }
-  else
-    {
-      i = carray->n_closures++;
-      carray = g_realloc (carray, sizeof (*carray) + sizeof (carray->closures[0]) * i);
-    }
-  carray->closures[i] = closure;
-  g_datalist_id_set_data_full (&object->qdata, quark_closure_array, carray, destroy_closure_array);
-  object_bit_unlock (object, OPTIONAL_BIT_LOCK_CLOSURE_ARRAY);
 }
 
 /**