[glib] gmain: handle child sources being destroyed before parent

[Dan Winship <danw src gnome org>]

commit 2357f67b1b7a9448d78e8606f10b472c595c7c90
Author: Dan Winship <danw gnome org>
Date:   Wed Jul 18 15:08:44 2012 -0400

    gmain: handle child sources being destroyed before parent
    
    Fix a crash when a child source is destroyed before its parent. Also,
    add a test case for this and the previous fix.

 glib/gmain.c          |   23 ++++++++++-------
 glib/tests/mainloop.c |   65 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 10 deletions(-)
---
diff --git a/glib/gmain.c b/glib/gmain.c
index 443d423..d2f42b0 100644
--- a/glib/gmain.c
+++ b/glib/gmain.c
@@ -1091,7 +1091,7 @@ g_source_destroy_internal (GSource      *source,
   
   if (!SOURCE_DESTROYED (source))
     {
-      GSList *tmp_list;
+      GSList *sources, *tmp_list;
       gpointer old_cb_data;
       GSourceCallbackFuncs *old_cb_funcs;
       
@@ -1122,20 +1122,24 @@ g_source_destroy_internal (GSource      *source,
 
       if (source->priv->child_sources)
 	{
-	  /* This is safe because even if a child_source finalizer or
-	   * closure notify tried to modify source->priv->child_sources
-	   * from outside the lock, it would fail since
-	   * SOURCE_DESTROYED(source) is now TRUE.
-	   */
-	  tmp_list = source->priv->child_sources;
+	  sources = tmp_list = source->priv->child_sources;
+	  source->priv->child_sources = NULL;
 	  while (tmp_list)
 	    {
 	      g_source_destroy_internal (tmp_list->data, context, TRUE);
 	      g_source_unref_internal (tmp_list->data, context, TRUE);
 	      tmp_list = tmp_list->next;
 	    }
-	  g_slist_free (source->priv->child_sources);
-	  source->priv->child_sources = NULL;
+	  g_slist_free (sources);
+	}
+
+      if (source->priv->parent_source)
+	{
+	  GSource *parent = source->priv->parent_source;
+
+	  parent->priv->child_sources =
+	    g_slist_remove (parent->priv->child_sources, source);
+	  source->priv->parent_source = NULL;
 	}
 	  
       g_source_unref_internal (source, context, TRUE);
@@ -1364,7 +1368,6 @@ g_source_remove_child_source (GSource *source,
   if (context)
     LOCK_CONTEXT (context);
 
-  source->priv->child_sources = g_slist_remove (source->priv->child_sources, child_source);
   g_source_destroy_internal (child_source, context, TRUE);
   g_source_unref_internal (child_source, context, TRUE);
 
diff --git a/glib/tests/mainloop.c b/glib/tests/mainloop.c
index d064492..2fdcfcb 100644
--- a/glib/tests/mainloop.c
+++ b/glib/tests/mainloop.c
@@ -481,6 +481,70 @@ test_recursive_child_sources (void)
   g_main_context_unref (ctx);
 }
 
+typedef struct {
+  GSource *parent, *old_child, *new_child;
+  GMainLoop *loop;
+} SwappingTestData;
+
+static gboolean
+swap_sources (gpointer user_data)
+{
+  SwappingTestData *data = user_data;
+
+  if (data->old_child)
+    {
+      g_source_remove_child_source (data->parent, data->old_child);
+      g_clear_pointer (&data->old_child, g_source_unref);
+    }
+
+  if (!data->new_child)
+    {
+      data->new_child = g_timeout_source_new (0);
+      g_source_set_callback (data->new_child, quit_loop, data->loop, NULL);
+      g_source_add_child_source (data->parent, data->new_child);
+    }
+
+  return G_SOURCE_CONTINUE;
+}
+
+static gboolean
+assert_not_reached_callback (gpointer user_data)
+{
+  g_assert_not_reached ();
+
+  return G_SOURCE_REMOVE;
+}
+
+static void
+test_swapping_child_sources (void)
+{
+  GMainContext *ctx;
+  GMainLoop *loop;
+  SwappingTestData data;
+
+  ctx = g_main_context_new ();
+  loop = g_main_loop_new (ctx, FALSE);
+
+  data.parent = g_timeout_source_new (50);
+  data.loop = loop;
+  g_source_set_callback (data.parent, swap_sources, &data, NULL);
+  g_source_attach (data.parent, ctx);
+
+  data.old_child = g_timeout_source_new (100);
+  g_source_add_child_source (data.parent, data.old_child);
+  g_source_set_callback (data.old_child, assert_not_reached_callback, NULL, NULL);
+
+  data.new_child = NULL;
+  g_main_loop_run (loop);
+
+  g_source_destroy (data.parent);
+  g_source_unref (data.parent);
+  g_source_unref (data.new_child);
+
+  g_main_loop_unref (loop);
+  g_main_context_unref (ctx);
+}
+
 int
 main (int argc, char *argv[])
 {
@@ -493,6 +557,7 @@ main (int argc, char *argv[])
   g_test_add_func ("/mainloop/invoke", test_invoke);
   g_test_add_func ("/mainloop/child_sources", test_child_sources);
   g_test_add_func ("/mainloop/recursive_child_sources", test_recursive_child_sources);
+  g_test_add_func ("/mainloop/swapping_child_sources", test_swapping_child_sources);
 
   return g_test_run ();
 }