diff --git a/gio/glocalfilemonitor.c b/gio/glocalfilemonitor.c index f5b0090ce..fde52193a 100644 --- a/gio/glocalfilemonitor.c +++ b/gio/glocalfilemonitor.c @@ -350,7 +350,6 @@ g_file_monitor_source_handle_event (GFileMonitorSource *fms, gint64 event_time) { gboolean interesting = TRUE; - GFileMonitor *instance = NULL; g_assert (!child || is_basename (child)); g_assert (!rename_to || is_basename (rename_to)); @@ -361,9 +360,24 @@ g_file_monitor_source_handle_event (GFileMonitorSource *fms, g_mutex_lock (&fms->lock); - /* monitor is already gone -- don't bother */ - instance = g_weak_ref_get (&fms->instance_ref); - if (instance == NULL) + /* NOTE: + * + * We process events even if the file monitor has already been disposed. + * The reason is that we must not take a reference to the instance here as + * destroying it from the event handling thread will lead to a deadlock when + * taking the lock in _ih_sub_cancel. + * + * This results in seemingly-unbounded growth of the `event_queue` with the + * calls to `g_file_monitor_source_queue_event()`. However, each of those sets + * the ready time on the #GSource, which means that it will be dispatched in + * a subsequent iteration of the #GMainContext it’s attached to. At that + * point, `g_file_monitor_source_dispatch()` will return %FALSE, and this will + * trigger finalisation of the source. That will clear the `event_queue`. + * + * If the source is no longer attached, this will return early to prevent + * unbounded queueing. + */ + if (g_source_is_destroyed ((GSource *) fms)) { g_mutex_unlock (&fms->lock); return TRUE; @@ -454,7 +468,6 @@ g_file_monitor_source_handle_event (GFileMonitorSource *fms, g_file_monitor_source_update_ready_time (fms); g_mutex_unlock (&fms->lock); - g_clear_object (&instance); return interesting; } @@ -601,9 +614,9 @@ g_file_monitor_source_dispose (GFileMonitorSource *fms) g_file_monitor_source_update_ready_time (fms); - g_mutex_unlock (&fms->lock); - g_source_destroy ((GSource *) fms); + + g_mutex_unlock (&fms->lock); } static void diff --git a/gio/tests/testfilemonitor.c b/gio/tests/testfilemonitor.c index bbc61f8b0..7274f792e 100644 --- a/gio/tests/testfilemonitor.c +++ b/gio/tests/testfilemonitor.c @@ -1036,6 +1036,57 @@ test_file_hard_links (Fixture *fixture, g_object_unref (data.output_stream); } +static void +test_finalize_in_callback (Fixture *fixture, + gconstpointer user_data) +{ + GFile *file = NULL; + guint i; + + g_test_summary ("Test that finalization of a GFileMonitor in one of its " + "callbacks doesn’t cause a deadlock."); + g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/1941"); + + file = g_file_get_child (fixture->tmp_dir, "race-file"); + + for (i = 0; i < 50; i++) + { + GFileMonitor *monitor = NULL; + GError *local_error = NULL; + + /* Monitor the file. */ + monitor = g_file_monitor_file (file, G_FILE_MONITOR_NONE, NULL, &local_error); + g_assert_no_error (local_error); + g_assert_nonnull (monitor); + + /* Create the file. */ + g_file_replace_contents (file, "hello", 5, NULL, FALSE, + G_FILE_CREATE_NONE, NULL, NULL, &local_error); + g_assert_no_error (local_error); + + /* Immediately drop the last ref to the monitor in the hope that this + * happens in the middle of the critical section in + * g_file_monitor_source_handle_event(), so that any cleanup at the end + * of that function is done with a now-finalised file monitor. */ + g_object_unref (monitor); + + /* Re-create the monitor and do the same again for deleting the file, to + * give a second chance at hitting the race condition. */ + monitor = g_file_monitor_file (file, G_FILE_MONITOR_NONE, NULL, &local_error); + g_assert_no_error (local_error); + g_assert_nonnull (monitor); + + /* Delete the file. */ + g_file_delete (file, NULL, &local_error); + g_assert_no_error (local_error); + + /* Drop the ref again. */ + g_object_unref (monitor); + } + + g_object_unref (file); +} + int main (int argc, char *argv[]) { @@ -1047,6 +1098,7 @@ main (int argc, char *argv[]) g_test_add ("/monitor/dir-not-existent", Fixture, NULL, setup, test_dir_non_existent, teardown); g_test_add ("/monitor/cross-dir-moves", Fixture, NULL, setup, test_cross_dir_moves, teardown); g_test_add ("/monitor/file/hard-links", Fixture, NULL, setup, test_file_hard_links, teardown); + g_test_add ("/monitor/finalize-in-callback", Fixture, NULL, setup, test_finalize_in_callback, teardown); return g_test_run (); }