Merge branch 'th/weak-ref-release-one-by-one' into 'main'

[th/weak-ref-release-one-by-one] gobject: invoke g_object_weak_ref() one-by-one during destruction

Closes #1002

See merge request GNOME/glib!4584

gobject/gobject.c

@@ -207,6 +207,7 @@ static guint               object_floating_flag_handler (GObject        *object,
                                                          gint            job);
 static inline void object_set_optional_flags (GObject *object,
                                               guint flags);
+static void g_object_weak_release_all (GObject *object, gboolean release_all);
 
 static void object_interface_check_properties           (gpointer        check_data,
 							 gpointer        g_iface);
@@ -1827,7 +1828,7 @@ g_object_real_dispose (GObject *object)
   g_signal_handlers_destroy (object);
 
   /* GWeakNotify and GClosure can call into user code */
-  g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL);
+  g_object_weak_release_all (object, FALSE);
   closure_array_destroy_all (object);
 }
 
@@ -3715,56 +3716,134 @@ g_object_disconnect (gpointer     _object,
   va_end (var_args);
 }
 
-typedef struct {
+typedef struct
-  GObject *object;
+{
-  guint n_weak_refs;
-  struct {
   GWeakNotify notify;
   gpointer data;
-  } weak_refs[1];  /* flexible array */
+} WeakRefTuple;
+
+struct _WeakRefReleaseAllState;
+
+typedef struct _WeakRefReleaseAllState
+{
+  guint remaining_to_notify;
+  struct _WeakRefReleaseAllState *release_all_next;
+} WeakRefReleaseAllState;
+
+typedef struct
+{
+  guint n_weak_refs;
+  guint alloc_size;
+  WeakRefReleaseAllState *release_all_states;
+  WeakRefTuple weak_refs[1]; /* flexible array */
 } WeakRefStack;
 
-static void
+#define WEAK_REF_STACK_ALLOC_SIZE(alloc_size) (G_STRUCT_OFFSET (WeakRefStack, weak_refs) + sizeof (WeakRefTuple) * (alloc_size))
-weak_refs_notify (gpointer data)
-{
-  WeakRefStack *wstack = data;
-  guint i;
 
-  for (i = 0; i < wstack->n_weak_refs; i++)
+G_GNUC_UNUSED G_ALWAYS_INLINE static inline gboolean
-    wstack->weak_refs[i].notify (wstack->weak_refs[i].data, wstack->object);
+_weak_ref_release_all_state_contains (WeakRefReleaseAllState *release_all_state, WeakRefReleaseAllState *needle)
+{
+  for (; release_all_state; release_all_state = release_all_state->release_all_next)
+    {
+      if (release_all_state == needle)
+        return TRUE;
+    }
+  return FALSE;
+}
+
+G_ALWAYS_INLINE static inline void
+_weak_ref_stack_free (WeakRefStack *wstack)
+{
+#ifdef G_ENABLE_DEBUG
+  g_assert (!wstack->release_all_states);
+#endif
   g_free (wstack);
 }
 
+G_ALWAYS_INLINE static inline void
+_weak_ref_stack_update_release_all_state (WeakRefStack *wstack, guint idx)
+{
+  WeakRefReleaseAllState **previous_ptr;
+  WeakRefReleaseAllState *release_all_state;
+
+#ifdef G_ENABLE_DEBUG
+  g_assert (idx < wstack->n_weak_refs);
+#endif
+
+  previous_ptr = &wstack->release_all_states;
+
+  while (G_UNLIKELY ((release_all_state = *previous_ptr)))
+    {
+      if (idx >= release_all_state->remaining_to_notify)
+        {
+#ifdef G_ENABLE_DEBUG
+          g_assert (release_all_state->remaining_to_notify <= wstack->n_weak_refs);
+#endif
+          /* We removed an index higher than the "remaining_to_notify" count. */
+          goto next;
+        }
+
+      /* Lower the "remaining_to_notify" bar of the entries we consider, as we
+       * just removed an entry at index @idx (below that bar). */
+      release_all_state->remaining_to_notify--;
+
+      if (release_all_state->remaining_to_notify > 0)
+        goto next;
+
+      /* Remove the entry from the linked list. No need to reset
+       * release_all_state->release_all_next pointer to NULL as it has no
+       * purpose when not being linked. */
+      *previous_ptr = release_all_state->release_all_next;
+      continue;
+
+    next:
+      previous_ptr = &release_all_state->release_all_next;
+    }
+}
+
 static gpointer
 g_object_weak_ref_cb (gpointer *data,
                       GDestroyNotify *destroy_notify,
                       gpointer user_data)
 {
-  GObject *object = ((gpointer *) user_data)[0];
+  WeakRefTuple *tuple = user_data;
-  GWeakNotify notify = ((gpointer *) user_data)[1];
-  gpointer notify_data = ((gpointer *) user_data)[2];
   WeakRefStack *wstack = *data;
   guint i;
 
   if (!wstack)
     {
-      wstack = g_new (WeakRefStack, 1);
+      wstack = g_malloc (WEAK_REF_STACK_ALLOC_SIZE (1));
-      wstack->object = object;
+      wstack->alloc_size = 1;
       wstack->n_weak_refs = 1;
+      wstack->release_all_states = NULL;
       i = 0;
 
-      *destroy_notify = weak_refs_notify;
+      *data = wstack;
+      /* We don't set a @destroy_notify. Shortly before finalize(), we call
+       * g_object_weak_release_all(), which frees the WeakRefStack. At that
+       * point the ref-count is already at zero and g_object_weak_ref() will
+       * assert against being called. This means, we expect that there is
+       * never anything to destroy. */
+#ifdef G_ENABLE_DEBUG
+      *destroy_notify = g_destroy_notify_assert_not_reached;
+#endif
     }
   else
     {
       i = wstack->n_weak_refs++;
-      wstack = g_realloc (wstack, sizeof (*wstack) + sizeof (wstack->weak_refs[0]) * i);
+
+      if (G_UNLIKELY (wstack->n_weak_refs > wstack->alloc_size))
+        {
+          if (G_UNLIKELY (wstack->alloc_size >= (G_MAXUINT / 2u + 1u)))
+            g_error ("g_object_weak_ref(): cannot register more than 2^31 references");
+          wstack->alloc_size = wstack->alloc_size * 2u;
+
+          wstack = g_realloc (wstack, WEAK_REF_STACK_ALLOC_SIZE (wstack->alloc_size));
+          *data = wstack;
+        }
     }
 
-  *data = wstack;
+  wstack->weak_refs[i] = *tuple;
-
-  wstack->weak_refs[i].notify = notify;
-  wstack->weak_refs[i].data = notify_data;
 
   return NULL;
 }
@@ -3798,7 +3877,10 @@ g_object_weak_ref (GObject    *object,
   _g_datalist_id_update_atomic (&object->qdata,
                                 quark_weak_notifies,
                                 g_object_weak_ref_cb,
-                                ((gpointer[]){ object, notify, data }));
+                                &((WeakRefTuple){
+                                    .notify = notify,
+                                    .data = data,
+                                }));
 }
 
 static gpointer
@@ -3806,8 +3888,7 @@ g_object_weak_unref_cb (gpointer *data,
                         GDestroyNotify *destroy_notify,
                         gpointer user_data)
 {
-  GWeakNotify notify = ((gpointer *) user_data)[0];
+  WeakRefTuple *tuple = user_data;
-  gpointer notify_data = ((gpointer *) user_data)[1];
   WeakRefStack *wstack = *data;
   gboolean found_one = FALSE;
   guint i;
@@ -3816,18 +3897,34 @@ g_object_weak_unref_cb (gpointer *data,
     {
       for (i = 0; i < wstack->n_weak_refs; i++)
         {
-          if (wstack->weak_refs[i].notify != notify ||
+          if (wstack->weak_refs[i].notify != tuple->notify ||
-              wstack->weak_refs[i].data != notify_data)
+              wstack->weak_refs[i].data != tuple->data)
             continue;
 
+          _weak_ref_stack_update_release_all_state (wstack, i);
+
           wstack->n_weak_refs -= 1;
           if (wstack->n_weak_refs == 0)
             {
-              g_free (wstack);
+              _weak_ref_stack_free (wstack);
               *data = NULL;
             }
-          else if (i != wstack->n_weak_refs)
+          else
-            wstack->weak_refs[i] = wstack->weak_refs[wstack->n_weak_refs];
+            {
+              if (i != wstack->n_weak_refs)
+                {
+                  memmove (&wstack->weak_refs[i],
+                           &wstack->weak_refs[i + 1],
+                           sizeof (wstack->weak_refs[i]) * (wstack->n_weak_refs - i));
+                }
+
+              if (G_UNLIKELY (wstack->n_weak_refs <= wstack->alloc_size / 4u))
+                {
+                  wstack->alloc_size = wstack->alloc_size / 2u;
+                  wstack = g_realloc (wstack, WEAK_REF_STACK_ALLOC_SIZE (wstack->alloc_size));
+                  *data = wstack;
+                }
+            }
 
           found_one = TRUE;
           break;
@@ -3835,7 +3932,7 @@ g_object_weak_unref_cb (gpointer *data,
     }
 
   if (!found_one)
-    g_critical ("%s: couldn't find weak ref %p(%p)", G_STRFUNC, notify, notify_data);
+    g_critical ("%s: couldn't find weak ref %p(%p)", G_STRFUNC, tuple->notify, tuple->data);
 
   return NULL;
 }
@@ -3859,7 +3956,139 @@ g_object_weak_unref (GObject    *object,
   _g_datalist_id_update_atomic (&object->qdata,
                                 quark_weak_notifies,
                                 g_object_weak_unref_cb,
-                                ((gpointer[]){ notify, data }));
+                                &((WeakRefTuple){
+                                    .notify = notify,
+                                    .data = data,
+                                }));
+}
+
+typedef struct
+{
+  WeakRefReleaseAllState *const release_all_state;
+  WeakRefTuple tuple;
+  gboolean release_all_done;
+} WeakRefReleaseAllData;
+
+static gpointer
+g_object_weak_release_all_cb (gpointer *data,
+                              GDestroyNotify *destroy_notify,
+                              gpointer user_data)
+{
+  WeakRefStack *wstack = *data;
+  WeakRefReleaseAllData *wdata = user_data;
+  WeakRefReleaseAllState *release_all_state = wdata->release_all_state;
+
+  if (!wstack)
+    return NULL;
+
+#ifdef G_ENABLE_DEBUG
+  g_assert (wstack->n_weak_refs > 0);
+#endif
+
+  if (release_all_state)
+    {
+      if (release_all_state->remaining_to_notify == G_MAXUINT)
+        {
+          if (wstack->n_weak_refs == 1u)
+            {
+              /* We only pop the single entry. */
+              wdata->release_all_done = TRUE;
+              release_all_state = NULL;
+            }
+          else
+            {
+              release_all_state->remaining_to_notify = wstack->n_weak_refs;
+
+              /* Prepend to linked list. */
+              release_all_state->release_all_next = wstack->release_all_states;
+              wstack->release_all_states = release_all_state;
+            }
+        }
+      else
+        {
+          if (release_all_state->remaining_to_notify == 0u)
+            {
+#ifdef G_ENABLE_DEBUG
+              g_assert (!_weak_ref_release_all_state_contains (wstack->release_all_states, release_all_state));
+#endif
+              return NULL;
+            }
+#ifdef G_ENABLE_DEBUG
+          g_assert (release_all_state->remaining_to_notify <= wstack->n_weak_refs);
+          g_assert (_weak_ref_release_all_state_contains (wstack->release_all_states, release_all_state));
+#endif
+        }
+    }
+
+  _weak_ref_stack_update_release_all_state (wstack, 0);
+
+  if (release_all_state && release_all_state->remaining_to_notify == 0)
+    wdata->release_all_done = TRUE;
+
+  wstack->n_weak_refs--;
+
+  /* Emit the notifications in FIFO order. */
+  wdata->tuple = wstack->weak_refs[0];
+
+  if (wstack->n_weak_refs == 0)
+    {
+      _weak_ref_stack_free (wstack);
+      *data = NULL;
+
+      /* Also set release_all_done.
+       *
+       * If g_object_weak_release_all() was called during dispose (with
+       * release_all FALSE), we anyway have an upper limit of how many
+       * notifications we want to pop. We only pop the notifications that were
+       * registered when the loop initially starts. In that case, we surely
+       * don't want the caller to call back.
+       *
+       * g_object_weak_release_all() is also being called before finalize. At
+       * that point, the ref count is already at zero, and g_object_weak_ref()
+       * asserts against being called. So nobody can register a new weak ref
+       * anymore.
+       *
+       * In both cases, we don't require the calling loop to call back. This
+       * saves an additional GData lookup. */
+      wdata->release_all_done = TRUE;
+    }
+  else
+    {
+      memmove (&wstack->weak_refs[0],
+               &wstack->weak_refs[1],
+               sizeof (wstack->weak_refs[0]) * wstack->n_weak_refs);
+
+      /* Don't bother to shrink the buffer. Most likely the object gets
+       * destroyed soon after. */
+    }
+
+  return wdata;
+}
+
+static void
+g_object_weak_release_all (GObject *object, gboolean release_all)
+{
+  WeakRefReleaseAllState release_all_state = {
+    .remaining_to_notify = G_MAXUINT,
+  };
+  WeakRefReleaseAllData wdata = {
+    .release_all_state = release_all ? NULL : &release_all_state,
+    .release_all_done = FALSE,
+  };
+
+  while (TRUE)
+    {
+      if (!_g_datalist_id_update_atomic (&object->qdata,
+                                         quark_weak_notifies,
+                                         g_object_weak_release_all_cb,
+                                         &wdata))
+        break;
+
+      wdata.tuple.notify (wdata.tuple.data, object);
+
+      if (wdata.release_all_done)
+        break;
+    }
 }
 
 /**
@@ -4603,7 +4832,7 @@ retry_decrement:
 
   closure_array_destroy_all (object);
   g_signal_handlers_destroy (object);
-  g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL);
+  g_object_weak_release_all (object, TRUE);
 
   TRACE (GOBJECT_OBJECT_FINALIZE (object, G_TYPE_FROM_INSTANCE (object)));
   G_OBJECT_GET_CLASS (object)->finalize (object);

gobject/tests/references.c

@@ -282,6 +282,273 @@ test_references (void)
   g_assert_true (object_destroyed);
 }
 
+/*****************************************************************************/
+
+static guint weak_ref4_notified;
+static guint weak_ref4_finalizing_notified;
+static guint weak_ref4_finalizing_notified_skipped;
+static gint weak_ref4_finalizing_resurrected = -1;
+static guint weak_ref4_indexes_n;
+static const guint *weak_ref4_indexes;
+
+static void
+weak_ref4_finalizing (gpointer data,
+                      GObject *object)
+{
+  static gint last = -1;
+  gint idx;
+
+  g_assert_true (weak_ref4_finalizing_resurrected == FALSE || weak_ref4_finalizing_resurrected == 4);
+
+  idx = (gint) GPOINTER_TO_UINT (data);
+  g_assert_cmpint (last, <, idx);
+  last = idx;
+
+  weak_ref4_finalizing_notified++;
+}
+
+static void
+weak_ref4 (gpointer data,
+           GObject *object)
+{
+  static gint last = -1;
+  guint idx;
+
+  g_assert_cmpint (weak_ref4_finalizing_notified, ==, 0);
+
+  idx = GPOINTER_TO_UINT (data);
+  g_assert_cmpint (last, <, (gint) idx);
+  last = (gint) idx;
+
+  weak_ref4_notified++;
+
+  if (weak_ref4_finalizing_resurrected == -1)
+    {
+      if (g_random_boolean ())
+        {
+          /* Resurrect the object. */
+          weak_ref4_finalizing_resurrected = TRUE;
+          g_object_ref (object);
+        }
+      else
+        weak_ref4_finalizing_resurrected = FALSE;
+    }
+
+  g_object_weak_ref (object, weak_ref4_finalizing, GUINT_TO_POINTER (idx));
+
+  if (weak_ref4_indexes_n > 0 && (g_random_int () % 4 == 0))
+    {
+
+      while (weak_ref4_indexes_n > 0 && weak_ref4_indexes[weak_ref4_indexes_n - 1] <= idx)
+        {
+          /* already invoked. Skip. */
+          weak_ref4_indexes_n--;
+        }
+
+      /* From the callback, we unregister another callback that we know is
+       * still registered. */
+      if (weak_ref4_indexes_n > 0)
+        {
+          guint new_idx = weak_ref4_indexes[weak_ref4_indexes_n - 1];
+
+          weak_ref4_indexes_n--;
+          g_object_weak_unref (object, weak_ref4, GUINT_TO_POINTER (new_idx));
+
+          /* Behave as if we got this callback invoked still, so that the remainder matches up. */
+          weak_ref4_finalizing_notified_skipped++;
+          weak_ref4_notified++;
+        }
+    }
+}
+
+static void
+test_references_many (void)
+{
+  GObject *object;
+  guint *indexes;
+  guint i;
+  guint n;
+  guint m;
+
+  /* Test subscribing a (random) number of weak references. */
+  object = g_object_new (TEST_TYPE_OBJECT, NULL);
+  n = g_random_int () % 1000;
+  indexes = g_new (guint, MAX (n, 1));
+  for (i = 0; i < n; i++)
+    {
+      g_object_weak_ref (object, weak_ref4, GUINT_TO_POINTER (i));
+      indexes[i] = i;
+    }
+  for (i = 0; i < n; i++)
+    {
+      guint tmp, j;
+
+      j = (guint) g_random_int_range ((gint) i, (gint) n);
+      tmp = indexes[i];
+      indexes[i] = indexes[j];
+      indexes[j] = tmp;
+    }
+  m = g_random_int () % (n + 1u);
+  for (i = 0; i < m; i++)
+    g_object_weak_unref (object, weak_ref4, GUINT_TO_POINTER (indexes[i]));
+  weak_ref4_indexes_n = n - m;
+  weak_ref4_indexes = &indexes[m];
+  g_object_unref (object);
+  g_assert_cmpint (weak_ref4_notified, ==, n - m);
+  if (n - m == 0)
+    {
+      g_assert_cmpint (weak_ref4_finalizing_resurrected, ==, -1);
+      g_assert_cmpint (weak_ref4_finalizing_notified, ==, 0);
+      g_assert_cmpint (weak_ref4_finalizing_notified_skipped, ==, 0);
+    }
+  else if (weak_ref4_finalizing_resurrected == TRUE)
+    {
+      weak_ref4_finalizing_resurrected = 4;
+      g_assert_cmpint (weak_ref4_finalizing_notified, ==, 0);
+      g_object_unref (object);
+      g_assert_cmpint (weak_ref4_notified, ==, n - m);
+      g_assert_cmpint (weak_ref4_finalizing_notified + weak_ref4_finalizing_notified_skipped, ==, n - m);
+    }
+  else
+    {
+      g_assert_cmpint (weak_ref4_finalizing_resurrected, ==, FALSE);
+      g_assert_cmpint (weak_ref4_finalizing_notified + weak_ref4_finalizing_notified_skipped, ==, n - m);
+    }
+  g_free (indexes);
+}
+
+/*****************************************************************************/
+
+static void notify_two (gpointer data, GObject *former_object);
+
+static void
+notify_one (gpointer data, GObject *former_object)
+{
+  g_object_weak_unref (data, notify_two, former_object);
+}
+
+static void
+notify_two (gpointer data, GObject *former_object)
+{
+  g_assert_not_reached ();
+}
+
+static void
+test_references_two (void)
+{
+  GObject *obj;
+
+  /* https://gitlab.gnome.org/GNOME/gtk/-/issues/5542#note_1688809 */
+
+  obj = g_object_new (G_TYPE_OBJECT, NULL);
+
+  g_object_weak_ref (obj, notify_one, obj);
+  g_object_weak_ref (obj, notify_two, obj);
+
+  g_object_unref (obj);
+}
+
+/*****************************************************************************/
+
+#define RUN_DISPOSE_N_THREADS 5
+
+GThread *run_dispose_threads[RUN_DISPOSE_N_THREADS];
+GObject *run_dispose_obj;
+gint run_dispose_thread_ready_count;
+const guint RUN_DISPOSE_N_ITERATIONS = 5000;
+
+gint run_dispose_weak_notify_pending;
+gint run_dispose_weak_notify_pending_nested;
+
+static void
+_run_dispose_weak_notify_nested (gpointer data,
+                                 GObject *where_the_object_was)
+{
+  g_assert_true (run_dispose_obj == where_the_object_was);
+  g_assert_cmpint (g_atomic_int_add (&run_dispose_weak_notify_pending_nested, -1), >, 0);
+}
+
+static void
+_run_dispose_weak_notify (gpointer data,
+                          GObject *where_the_object_was)
+{
+  g_assert_true (run_dispose_obj == where_the_object_was);
+  g_assert_cmpint (g_atomic_int_add (&run_dispose_weak_notify_pending, -1), >, 0);
+
+  if (g_random_int () % 10 == 0)
+    {
+      g_object_run_dispose (run_dispose_obj);
+    }
+
+  if (g_random_int () % 10 == 0)
+    {
+      g_atomic_int_inc (&run_dispose_weak_notify_pending_nested);
+      g_object_weak_ref (run_dispose_obj, _run_dispose_weak_notify_nested, NULL);
+    }
+}
+
+static gpointer
+_run_dispose_thread_fcn (gpointer thread_data)
+{
+  guint i;
+
+  g_atomic_int_inc (&run_dispose_thread_ready_count);
+
+  while (g_atomic_int_get (&run_dispose_thread_ready_count) != RUN_DISPOSE_N_THREADS)
+    g_usleep (10);
+
+  for (i = 0; i < RUN_DISPOSE_N_ITERATIONS; i++)
+    {
+      g_atomic_int_inc (&run_dispose_weak_notify_pending);
+      g_object_weak_ref (run_dispose_obj, _run_dispose_weak_notify, NULL);
+      if (g_random_int () % 10 == 0)
+        g_object_run_dispose (run_dispose_obj);
+    }
+
+  g_object_run_dispose (run_dispose_obj);
+
+  return NULL;
+}
+
+static void
+test_references_run_dispose (void)
+{
+  GQuark quark_weak_notifies = g_quark_from_static_string ("GObject-weak-notifies");
+  guint i;
+
+  run_dispose_obj = g_object_new (G_TYPE_OBJECT, NULL);
+
+  for (i = 0; i < RUN_DISPOSE_N_THREADS; i++)
+    {
+      run_dispose_threads[i] = g_thread_new ("run-dispose", _run_dispose_thread_fcn, GINT_TO_POINTER (i));
+    }
+  for (i = 0; i < RUN_DISPOSE_N_THREADS; i++)
+    {
+      g_thread_join (run_dispose_threads[i]);
+    }
+
+  g_assert_cmpint (g_atomic_int_get (&run_dispose_weak_notify_pending), ==, 0);
+
+  if (g_atomic_int_get (&run_dispose_weak_notify_pending_nested) == 0)
+    {
+      g_assert_null (g_object_get_qdata (run_dispose_obj, quark_weak_notifies));
+    }
+  else
+    {
+      g_assert_nonnull (g_object_get_qdata (run_dispose_obj, quark_weak_notifies));
+    }
+
+  g_object_run_dispose (run_dispose_obj);
+
+  g_assert_cmpint (g_atomic_int_get (&run_dispose_weak_notify_pending), ==, 0);
+  g_assert_cmpint (g_atomic_int_get (&run_dispose_weak_notify_pending_nested), ==, 0);
+  g_assert_null (g_object_get_qdata (run_dispose_obj, quark_weak_notifies));
+
+  g_clear_object (&run_dispose_obj);
+}
+
+/*****************************************************************************/
+
 int
 main (int   argc,
       char *argv[])
@@ -293,6 +560,9 @@ main (int   argc,
   g_test_init (&argc, &argv, NULL);
 
   g_test_add_func ("/gobject/references", test_references);
+  g_test_add_func ("/gobject/references-many", test_references_many);
+  g_test_add_func ("/gobject/references_two", test_references_two);
+  g_test_add_func ("/gobject/references_run_dispose", test_references_run_dispose);
 
   return g_test_run ();
 }