glib: add a proper weak-ref-like relationship between GSource and GMainContext

Like the GWeakRef's in GObject, there is a global lock that is consulted
whenever g_main_context_unref() or g_source_destroy() or
g_source_unref() is called to retrieve a reference to the associated
GMainContext.

There are a number of actual races that are fixed by this change.
1. Racing GSource destruction with g_main_context_unref() is solved
   by holding the global source_weak_locations lock while setting
   source->context = NULL and while g_source_destroy() attempts to
   retrieve source->context;
2. Same race as 1. but inside g_source_unref()
3. Theoretical race of double freeing the contents of
   context->pending_dispatches if both g_source_destroy() and
   g_main_context_unref() both free resources inside g_main_context_unref().

A couple of implementation notes:
1. Unlocking source_weak_locations too early in g_main_context_unref()
   (before g_source_destroy_internal() is called) may have a race of the
   G_HOOK_FLAG_ACTIVE state of the source and cause a leak of the source.
   This is why source_weak_locations is also held over the calls to
   g_source_destroy_internal() in g_main_context_unref().  So that
   either g_main_context_unref() or g_source_destroy() (but only 1) has
   the chance to free the resources associated with the GMainContext.
2. g_main_context_unref() now needs to be more of a dispose()
   implementation as it can be called multiple times with losing the
   last ref.

Fixes: https://gitlab.gnome.org/GNOME/glib/-/issues/803

glib/gmain.c

@@ -392,6 +392,15 @@ static gboolean g_idle_dispatch    (GSource     *source,
 				    gpointer     user_data);
 
 static void block_source (GSource *source);
+static GMainContext *source_dup_main_context (GSource *source);
+
+/* Lock for serializing access for safe execution of
+ * g_main_context_unref() with concurrent use of
+ * g_source_destroy() and g_source_unref().
+ *
+ * Locking order is source_destroy_lock, then context lock.
+ */
+static GRWLock source_destroy_lock;
 
 static GMainContext *glib_worker_context;
 
@@ -437,8 +446,6 @@ GSourceFuncs g_unix_signal_funcs =
   NULL, NULL
 };
 #endif /* !G_OS_WIN32 */
-G_LOCK_DEFINE_STATIC (main_context_list);
-static GSList *main_context_list = NULL;
 
 GSourceFuncs g_timeout_funcs =
 {
@@ -510,20 +517,42 @@ g_main_context_unref (GMainContext *context)
   GSList *s_iter, *remaining_sources = NULL;
   GSourceList *list;
   guint i;
+  guint old_ref;
+  GSource **pending_dispatches;
+  gsize pending_dispatches_len;
 
   g_return_if_fail (context != NULL);
   g_return_if_fail (g_atomic_int_get (&context->ref_count) > 0); 
 
-  if (!g_atomic_int_dec_and_test (&context->ref_count))
-    return;
+retry_decrement:
+  old_ref = g_atomic_int_get (&context->ref_count);
+  if (old_ref > 1)
+    {
+      if (!g_atomic_int_compare_and_exchange (&context->ref_count, old_ref, old_ref - 1))
+        goto retry_decrement;
 
-  G_LOCK (main_context_list);
-  main_context_list = g_slist_remove (main_context_list, context);
-  G_UNLOCK (main_context_list);
+      return;
+    }
+
+  g_rw_lock_writer_lock (&source_destroy_lock);
+
+  /* if a weak ref got to the source_destroy lock first, we need to retry */
+  old_ref = g_atomic_int_add (&context->ref_count, -1);
+  if (old_ref != 1)
+    {
+      g_rw_lock_writer_unlock (&source_destroy_lock);
+      return;
+    }
+
+  LOCK_CONTEXT (context);
+  pending_dispatches = (GSource **) g_ptr_array_steal (context->pending_dispatches, &pending_dispatches_len);
+  UNLOCK_CONTEXT (context);
 
   /* Free pending dispatches */
-  for (i = 0; i < context->pending_dispatches->len; i++)
-    g_source_unref_internal (context->pending_dispatches->pdata[i], context, FALSE);
+  for (i = 0; i < pending_dispatches_len; i++)
+    g_source_unref_internal (pending_dispatches[i], context, FALSE);
+
+  g_clear_pointer (&pending_dispatches, g_free);
 
   /* g_source_iter_next() assumes the context is locked. */
   LOCK_CONTEXT (context);
@@ -554,6 +583,11 @@ g_main_context_unref (GMainContext *context)
       g_source_destroy_internal (source, context, TRUE);
     }
 
+  g_rw_lock_writer_unlock (&source_destroy_lock);
+
+  /* the context is going to die now */
+  g_return_if_fail (old_ref > 0);
+
   sl_iter = context->source_lists.head;
   while (sl_iter != NULL)
     {
@@ -562,20 +596,29 @@ g_main_context_unref (GMainContext *context)
       g_slice_free (GSourceList, list);
     }
 
-  g_hash_table_destroy (context->sources);
+  g_hash_table_remove_all (context->sources);
 
   UNLOCK_CONTEXT (context);
-  g_mutex_clear (&context->mutex);
 
-  g_ptr_array_free (context->pending_dispatches, TRUE);
-  g_free (context->cached_poll_array);
+  /* if the object has been reffed meanwhile by an internal weak ref, keep the
+   * resources alive until the last reference is gone.
+   */
+  if (old_ref == 1)
+    {
+      g_mutex_clear (&context->mutex);
 
-  poll_rec_list_free (context, context->poll_records);
+      g_ptr_array_free (context->pending_dispatches, TRUE);
+      g_free (context->cached_poll_array);
 
-  g_wakeup_free (context->wakeup);
-  g_cond_clear (&context->cond);
+      poll_rec_list_free (context, context->poll_records);
 
-  g_free (context);
+      g_wakeup_free (context->wakeup);
+      g_cond_clear (&context->cond);
+
+      g_hash_table_unref (context->sources);
+
+      g_free (context);
+    }
 
   /* And now finally get rid of our references to the sources. This will cause
    * them to be freed unless something else still has a reference to them. Due
@@ -671,16 +714,11 @@ g_main_context_new_with_flags (GMainContextFlags flags)
   g_wakeup_get_pollfd (context->wakeup, &context->wake_up_rec);
   g_main_context_add_poll_unlocked (context, 0, &context->wake_up_rec);
 
-  G_LOCK (main_context_list);
-  main_context_list = g_slist_append (main_context_list, context);
-
 #ifdef G_MAIN_POLL_DEBUG
   if (_g_main_poll_debug)
     g_print ("created context=%p\n", context);
 #endif
 
-  G_UNLOCK (main_context_list);
-
   return context;
 }
 
@@ -1333,6 +1371,22 @@ g_source_destroy_internal (GSource      *source,
     UNLOCK_CONTEXT (context);
 }
 
+static GMainContext *
+source_dup_main_context (GSource *source)
+{
+  GMainContext *ret = NULL;
+
+  g_rw_lock_reader_lock (&source_destroy_lock);
+
+  ret = source->context;
+  if (ret)
+    g_atomic_int_inc (&ret->ref_count);
+
+  g_rw_lock_reader_unlock (&source_destroy_lock);
+
+  return ret;
+}
+
 /**
  * g_source_destroy:
  * @source: a #GSource
@@ -1361,10 +1415,13 @@ g_source_destroy (GSource *source)
   g_return_if_fail (source != NULL);
   g_return_if_fail (g_atomic_int_get (&source->ref_count) > 0);
   
-  context = source->context;
-  
+  context = source_dup_main_context (source);
+
   if (context)
-    g_source_destroy_internal (source, context, FALSE);
+    {
+      g_source_destroy_internal (source, context, FALSE);
+      g_main_context_unref (context);
+    }
   else
     g_atomic_int_and (&source->flags, ~G_HOOK_FLAG_ACTIVE);
 }
@@ -2369,10 +2426,17 @@ retry_beginning:
 void
 g_source_unref (GSource *source)
 {
+  GMainContext *context;
+
   g_return_if_fail (source != NULL);
   /* refcount is checked inside g_source_unref_internal() */
 
-  g_source_unref_internal (source, source->context, FALSE);
+  context = source_dup_main_context (source);
+
+  g_source_unref_internal (source, context, FALSE);
+
+  if (context)
+    g_main_context_unref (context);
 }
 
 /**
@@ -5582,8 +5646,8 @@ wake_source (GSource *source)
    *    - the GMainContext will either be NULL or point to a live
    *      GMainContext
    *
-   *    - the GMainContext will remain valid since we hold the
-   *      main_context_list lock
+   *    - the GMainContext will remain valid since source_dup_main_context()
+   *      gave us a ref or NULL
    *
    *  Since we are holding a lot of locks here, don't try to enter any
    *  more GMainContext functions for fear of dealock -- just hit the
@@ -5591,11 +5655,12 @@ wake_source (GSource *source)
    *  unsafe with some very minor changes in the future, and signal
    *  handling is not the most well-tested codepath.
    */
-  G_LOCK(main_context_list);
-  context = source->context;
+  context = source_dup_main_context (source);
   if (context)
     g_wakeup_signal (context->wakeup);
-  G_UNLOCK(main_context_list);
+
+  if (context)
+    g_main_context_unref (context);
 }
 
 static void

glib/tests/mainloop.c

@@ -2481,6 +2481,166 @@ test_steal_fd (void)
   g_free (tmpfile);
 }
 
+typedef enum
+{
+  INITIAL = 0,
+  MAIN_CONTEXT_READY = (1 << 0),
+  SOURCE_READY = (1 << 1),
+} SimultaneousDestructionTestState;
+
+typedef struct
+{
+  SimultaneousDestructionTestState state; /* (mutex lock) */
+  GMainContext *main_context; /* (mutex lock) */
+  GSource *source; /* (mutex lock) */
+  GMutex lock;
+  GCond cond;
+  GThread *main_context_thread;
+  GThread *source_thread; /* (mutex lock) */
+} SimultaneousDestructionTest;
+
+static SimultaneousDestructionTest *
+create_simultaneous_destruction_test (void)
+{
+  SimultaneousDestructionTest *test;
+
+  test = g_new0 (SimultaneousDestructionTest, 1);
+
+  g_mutex_init (&test->lock);
+  g_cond_init (&test->cond);
+
+  return test;
+}
+
+static void
+free_simultaneous_destruction_test (SimultaneousDestructionTest * test)
+{
+  g_mutex_clear (&test->lock);
+  g_cond_clear (&test->cond);
+
+  /* Should have already been cleared in wait_simultaneous_destruction_test() */
+  g_assert (test->main_context == NULL);
+  g_assert (test->source == NULL);
+  g_assert (test->main_context_thread == NULL);
+  g_assert (test->source_thread == NULL);
+
+  g_free (test);
+}
+
+static gpointer
+source_create_unref_thread_func (gpointer data)
+{
+  SimultaneousDestructionTest *test = data;
+
+  g_mutex_lock (&test->lock);
+  test->source = g_timeout_source_new_seconds (100);
+  g_source_attach (test->source, test->main_context);
+  test->state |= SOURCE_READY;
+  g_cond_broadcast (&test->cond);
+  while ((test->state & MAIN_CONTEXT_READY) == 0)
+    g_cond_wait (&test->cond, &test->lock);
+  g_mutex_unlock (&test->lock);
+
+  g_thread_yield ();
+  g_source_destroy (test->source);
+
+  g_mutex_lock (&test->lock);
+  g_source_unref (test->source);
+  test->source = NULL;
+  g_cond_broadcast (&test->cond);
+  g_mutex_unlock (&test->lock);
+
+  return NULL;
+}
+
+static gpointer
+main_context_create_unref_thread_func (gpointer data)
+{
+  SimultaneousDestructionTest *test = data;
+
+  g_mutex_lock (&test->lock);
+  test->main_context = g_main_context_new ();
+  test->source_thread = g_thread_new (NULL, source_create_unref_thread_func, test);
+
+  test->state |= MAIN_CONTEXT_READY;
+  while ((test->state & SOURCE_READY) == 0)
+    g_cond_wait (&test->cond, &test->lock);
+  g_mutex_unlock (&test->lock);
+
+  g_thread_yield ();
+  g_main_context_unref (test->main_context);
+
+  g_mutex_lock (&test->lock);
+  test->main_context = NULL;
+  g_cond_broadcast (&test->cond);
+  g_mutex_unlock (&test->lock);
+
+  return NULL;
+}
+
+static void
+start_simultaneous_destruction_test (SimultaneousDestructionTest * test)
+{
+  test->main_context_thread = g_thread_new (NULL, main_context_create_unref_thread_func, test);
+
+  g_mutex_lock (&test->lock);
+  while (test->main_context)
+    g_cond_wait (&test->cond, &test->lock);
+  g_mutex_unlock (&test->lock);
+}
+
+static void
+wait_simultaneous_destruction_test (SimultaneousDestructionTest * test)
+{
+  g_mutex_lock (&test->lock);
+  while (test->main_context || test->source)
+    g_cond_wait (&test->cond, &test->lock);
+  g_mutex_unlock (&test->lock);
+
+  g_thread_join (g_steal_pointer (&test->main_context_thread));
+  g_thread_join (g_steal_pointer (&test->source_thread));
+}
+
+static void
+test_simultaneous_source_context_destruction (void)
+{
+  guint64 n_concurrent = 128, n_iterations = 100;
+  SimultaneousDestructionTest **test;
+  guint64 i;
+
+  if (g_test_thorough ())
+    {
+      n_concurrent = 512;
+      n_iterations = 2000;
+    }
+
+  test = g_new0 (SimultaneousDestructionTest *, n_concurrent);
+
+  for (i = 0; i < n_iterations; i++)
+    {
+      gsize j = 0;
+      for (j = 0; j < n_concurrent; j++)
+        test[j] = create_simultaneous_destruction_test ();
+
+      for (j = 0; j < n_concurrent; j++)
+        start_simultaneous_destruction_test (test[j]);
+
+      for (j = 0; j < n_concurrent; j++)
+        {
+          wait_simultaneous_destruction_test (test[j]);
+          free_simultaneous_destruction_test (test[j]);
+        }
+
+      if (g_test_verbose() && i % 100 == 0)
+        g_test_message ("# %" G_GUINT64_FORMAT " / %" G_GUINT64_FORMAT, i, n_iterations);
+    }
+
+  if (g_test_verbose ())
+    g_test_message ("%" G_GUINT64_FORMAT " / %" G_GUINT64_FORMAT, n_iterations, n_iterations);
+
+  g_free (test);
+}
+
 int
 main (int argc, char *argv[])
 {
@@ -2535,6 +2695,7 @@ main (int argc, char *argv[])
   g_test_add_func ("/mainloop/steal-fd", test_steal_fd);
   g_test_add_data_func ("/mainloop/ownerless-polling/attach-first", GINT_TO_POINTER (TRUE), test_ownerless_polling);
   g_test_add_data_func ("/mainloop/ownerless-polling/pop-first", GINT_TO_POINTER (FALSE), test_ownerless_polling);
+  g_test_add_func ("/mainloop/simultaneous-source-context-destruction", test_simultaneous_source_context_destruction);
 
   return g_test_run ();
 }