diff --git a/glib/gmain.c b/glib/gmain.c index 1b4260b1132e46d927947f88c473a39e5dfe988a..ee23a6519f34c8434046b1a12928055717a403ff 100644 --- a/glib/gmain.c +++ b/glib/gmain.c @@ -539,6 +539,7 @@ g_main_context_unref (GMainContext *context) GSourceIter iter; GSource *source; GList *sl_iter; + GSList *s_iter, *remaining_sources = NULL; GSourceList *list; guint i; @@ -558,13 +559,32 @@ g_main_context_unref (GMainContext *context) /* g_source_iter_next() assumes the context is locked. */ LOCK_CONTEXT (context); - g_source_iter_init (&iter, context, TRUE); + + /* First collect all remaining sources from the sources lists and store a + * new reference in a separate list. Also set the context of the sources + * to NULL so that they can't access a partially destroyed context anymore. + * + * We have to do this first so that we have a strong reference to all + * sources and destroying them below does not also free them, and so that + * none of the sources can access the context from their finalize/dispose + * functions. */ + g_source_iter_init (&iter, context, FALSE); while (g_source_iter_next (&iter, &source)) { source->context = NULL; + remaining_sources = g_slist_prepend (remaining_sources, g_source_ref (source)); + } + g_source_iter_clear (&iter); + + /* Next destroy all sources. As we still hold a reference to all of them, + * this won't cause any of them to be freed yet and especially prevents any + * source that unrefs another source from its finalize function to be freed. + */ + for (s_iter = remaining_sources; s_iter; s_iter = s_iter->next) + { + source = s_iter->data; g_source_destroy_internal (source, context, TRUE); } - UNLOCK_CONTEXT (context); for (sl_iter = context->source_lists; sl_iter; sl_iter = sl_iter->next) { @@ -575,6 +595,7 @@ g_main_context_unref (GMainContext *context) g_hash_table_destroy (context->sources); + UNLOCK_CONTEXT (context); g_mutex_clear (&context->mutex); g_ptr_array_free (context->pending_dispatches, TRUE); @@ -586,6 +607,18 @@ g_main_context_unref (GMainContext *context) g_cond_clear (&context->cond); 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 + * to setting the context pointers in the sources to NULL above, this won't + * ever access the context or the internal linked list inside the GSource. + * We already removed the sources completely from the context above. */ + for (s_iter = remaining_sources; s_iter; s_iter = s_iter->next) + { + source = s_iter->data; + g_source_unref_internal (source, NULL, FALSE); + } + g_slist_free (remaining_sources); } /* Helper function used by mainloop/overflow test. @@ -1004,13 +1037,17 @@ g_source_iter_next (GSourceIter *iter, GSource **source) * GSourceList to be removed from source_lists (if iter->source is * the only source in its list, and it is destroyed), so we have to * keep it reffed until after we advance iter->current_list, above. + * + * Also we first have to ref the next source before unreffing the + * previous one as unreffing the previous source can potentially + * free the next one. */ + if (next_source && iter->may_modify) + g_source_ref (next_source); if (iter->source && iter->may_modify) g_source_unref_internal (iter->source, iter->context, TRUE); iter->source = next_source; - if (iter->source && iter->may_modify) - g_source_ref (iter->source); *source = iter->source; return *source != NULL; diff --git a/glib/tests/mainloop.c b/glib/tests/mainloop.c index a6dcc794fe45523c3e5891a0e438e05a59ff48b6..1368e9bc053ffce3f0ce9419653c0f01f1631657 100644 --- a/glib/tests/mainloop.c +++ b/glib/tests/mainloop.c @@ -1832,14 +1832,188 @@ test_maincontext_source_finalization (void) g_assert_true (source_finalize_called); } +/* GSource implementation which optionally keeps a strong reference to another + * GSource until finalization, when it destroys and unrefs the other source. + */ +typedef struct { + GSource source; + + GSource *other_source; +} SourceWithSource; + +static void +finalize_source_with_source (GSource *source) +{ + SourceWithSource *s = (SourceWithSource *) source; + + if (s->other_source) + { + g_source_destroy (s->other_source); + g_source_unref (s->other_source); + s->other_source = NULL; + } +} + +static GSourceFuncs source_with_source_funcs = { + NULL, + NULL, + NULL, + finalize_source_with_source +}; + +static void +test_maincontext_source_finalization_from_source (gconstpointer user_data) +{ + GMainContext *c = g_main_context_new (); + GSource *s1, *s2; + + g_test_summary ("Tests if freeing a GSource as part of another GSource " + "during main context destruction works."); + g_test_bug ("https://gitlab.gnome.org/GNOME/glib/merge_requests/1353"); + + s1 = g_source_new (&source_with_source_funcs, sizeof (SourceWithSource)); + s2 = g_source_new (&source_with_source_funcs, sizeof (SourceWithSource)); + ((SourceWithSource *)s1)->other_source = g_source_ref (s2); + + /* Attach sources in a different order so they end up being destroyed + * in a different order by the main context */ + if (GPOINTER_TO_INT (user_data) < 5) + { + g_source_attach (s1, c); + g_source_attach (s2, c); + } + else + { + g_source_attach (s2, c); + g_source_attach (s1, c); + } + + /* Test a few different permutations here */ + if (GPOINTER_TO_INT (user_data) % 5 == 0) + { + /* Get rid of our references so that destroying the context + * will unref them immediately */ + g_source_unref (s1); + g_source_unref (s2); + g_main_context_unref (c); + } + else if (GPOINTER_TO_INT (user_data) % 5 == 1) + { + /* Destroy and free the sources before the context */ + g_source_destroy (s1); + g_source_unref (s1); + g_source_destroy (s2); + g_source_unref (s2); + g_main_context_unref (c); + } + else if (GPOINTER_TO_INT (user_data) % 5 == 2) + { + /* Destroy and free the sources before the context */ + g_source_destroy (s2); + g_source_unref (s2); + g_source_destroy (s1); + g_source_unref (s1); + g_main_context_unref (c); + } + else if (GPOINTER_TO_INT (user_data) % 5 == 3) + { + /* Destroy and free the context before the sources */ + g_main_context_unref (c); + g_source_unref (s2); + g_source_unref (s1); + } + else if (GPOINTER_TO_INT (user_data) % 5 == 4) + { + /* Destroy and free the context before the sources */ + g_main_context_unref (c); + g_source_unref (s1); + g_source_unref (s2); + } +} + +static gboolean +dispatch_source_with_source (GSource *source, GSourceFunc callback, gpointer user_data) +{ + return G_SOURCE_REMOVE; +} + +static GSourceFuncs source_with_source_funcs_dispatch = { + NULL, + NULL, + dispatch_source_with_source, + finalize_source_with_source +}; + +static void +test_maincontext_source_finalization_from_dispatch (gconstpointer user_data) +{ + GMainContext *c = g_main_context_new (); + GSource *s1, *s2; + + g_test_summary ("Tests if freeing a GSource as part of another GSource " + "during main context iteration works."); + + s1 = g_source_new (&source_with_source_funcs_dispatch, sizeof (SourceWithSource)); + s2 = g_source_new (&source_with_source_funcs_dispatch, sizeof (SourceWithSource)); + ((SourceWithSource *)s1)->other_source = g_source_ref (s2); + + g_source_attach (s1, c); + g_source_attach (s2, c); + + if (GPOINTER_TO_INT (user_data) == 0) + { + /* This finalizes s1 as part of the iteration, which then destroys and + * frees s2 too */ + g_source_set_ready_time (s1, 0); + } + else if (GPOINTER_TO_INT (user_data) == 1) + { + /* This destroys s2 as part of the iteration but does not free it as + * it's still referenced by s1 */ + g_source_set_ready_time (s2, 0); + } + else if (GPOINTER_TO_INT (user_data) == 2) + { + /* This destroys both s1 and s2 as part of the iteration */ + g_source_set_ready_time (s1, 0); + g_source_set_ready_time (s2, 0); + } + + /* Get rid of our references so only the main context has one now */ + g_source_unref (s1); + g_source_unref (s2); + + /* Iterate as long as there are sources to dispatch */ + while (g_main_context_iteration (c, FALSE)) + { + /* Do nothing here */ + } + + g_main_context_unref (c); +} + int main (int argc, char *argv[]) { + gint i; + g_test_init (&argc, &argv, NULL); g_test_bug_base ("http://bugzilla.gnome.org/"); g_test_add_func ("/maincontext/basic", test_maincontext_basic); g_test_add_func ("/maincontext/source_finalization", test_maincontext_source_finalization); + for (i = 0; i < 10; i++) + { + gchar *name = g_strdup_printf ("/maincontext/source_finalization_from_source/%d", i); + g_test_add_data_func (name, GINT_TO_POINTER (i), test_maincontext_source_finalization_from_source); + g_free (name); + } + for (i = 0; i < 3; i++) + { + gchar *name = g_strdup_printf ("/maincontext/source_finalization_from_dispatch/%d", i); + g_test_add_data_func (name, GINT_TO_POINTER (i), test_maincontext_source_finalization_from_dispatch); + g_free (name); + } g_test_add_func ("/mainloop/basic", test_mainloop_basic); g_test_add_func ("/mainloop/timeouts", test_timeouts); g_test_add_func ("/mainloop/priorities", test_priorities);