Merge branch '1903-mimeapps-test-race' into 'master'

Fix thread unsafety in GFileMonitorSource and mimeapps test

Closes #1903

See merge request GNOME/glib!1164

gio/gdesktopappinfo.c

@@ -143,6 +143,7 @@ G_DEFINE_TYPE_WITH_CODE (GDesktopAppInfo, g_desktop_app_info, G_TYPE_OBJECT,
 
 typedef struct
 {
+  gatomicrefcount             ref_count;
   gchar                      *path;
   gchar                      *alternatively_watching;
   gboolean                    is_config;
@@ -154,17 +155,35 @@ typedef struct
   GHashTable                 *memory_implementations;
 } DesktopFileDir;
 
-static DesktopFileDir *desktop_file_dirs;
+static GPtrArray      *desktop_file_dirs = NULL;
-static guint           n_desktop_file_dirs;
 static const gchar    *desktop_file_dirs_config_dir = NULL;
-static const guint     desktop_file_dir_user_config_index = 0;
+static DesktopFileDir *desktop_file_dir_user_config = NULL;  /* (owned) */
-static guint           desktop_file_dir_user_data_index;
+static DesktopFileDir *desktop_file_dir_user_data = NULL;  /* (owned) */
 static GMutex          desktop_file_dir_lock;
 static const gchar    *gio_launch_desktop_path = NULL;
 
 /* Monitor 'changed' signal handler {{{2 */
 static void desktop_file_dir_reset (DesktopFileDir *dir);
 
+static DesktopFileDir *
+desktop_file_dir_ref (DesktopFileDir *dir)
+{
+  g_atomic_ref_count_inc (&dir->ref_count);
+
+  return dir;
+}
+
+static void
+desktop_file_dir_unref (DesktopFileDir *dir)
+{
+  if (g_atomic_ref_count_dec (&dir->ref_count))
+    {
+      desktop_file_dir_reset (dir);
+      g_free (dir->path);
+      g_free (dir);
+    }
+}
+
 /*< internal >
  * desktop_file_dir_get_alternative_dir:
  * @dir: a #DesktopFileDir
@@ -270,11 +289,15 @@ static gboolean
 desktop_file_dir_app_name_is_masked (DesktopFileDir *dir,
                                      const gchar    *app_name)
 {
-  while (dir > desktop_file_dirs)
+  guint i;
-    {
-      dir--;
 
-      if (dir->app_names && g_hash_table_contains (dir->app_names, app_name))
+  for (i = 0; i < desktop_file_dirs->len; i++)
+    {
+      DesktopFileDir *i_dir = g_ptr_array_index (desktop_file_dirs, i);
+
+      if (dir == i_dir)
+        return FALSE;
+      if (i_dir->app_names && g_hash_table_contains (i_dir->app_names, app_name))
         return TRUE;
     }
 
@@ -1251,44 +1274,41 @@ desktop_file_dir_unindexed_get_implementations (DesktopFileDir  *dir,
 /* DesktopFileDir "API" {{{2 */
 
 /*< internal >
- * desktop_file_dir_create:
+ * desktop_file_dir_new:
- * @array: the #GArray to add a new item to
  * @data_dir: an XDG_DATA_DIR
  *
- * Creates a #DesktopFileDir for the corresponding @data_dir, adding it
+ * Creates a #DesktopFileDir for the corresponding @data_dir.
- * to @array.
  */
-static void
+static DesktopFileDir *
-desktop_file_dir_create (GArray      *array,
+desktop_file_dir_new (const gchar *data_dir)
-                         const gchar *data_dir)
 {
-  DesktopFileDir dir = { 0, };
+  DesktopFileDir *dir = g_new0 (DesktopFileDir, 1);
 
-  dir.path = g_build_filename (data_dir, "applications", NULL);
+  g_atomic_ref_count_init (&dir->ref_count);
+  dir->path = g_build_filename (data_dir, "applications", NULL);
 
-  g_array_append_val (array, dir);
+  return g_steal_pointer (&dir);
 }
 
 /*< internal >
- * desktop_file_dir_create:
+ * desktop_file_dir_new_for_config:
- * @array: the #GArray to add a new item to
  * @config_dir: an XDG_CONFIG_DIR
  *
- * Just the same as desktop_file_dir_create() except that it does not
+ * Just the same as desktop_file_dir_new() except that it does not
  * add the "applications" directory.  It also marks the directory as
  * config-only, which prevents us from attempting to find desktop files
  * here.
  */
-static void
+static DesktopFileDir *
-desktop_file_dir_create_for_config (GArray      *array,
+desktop_file_dir_new_for_config (const gchar *config_dir)
-                                    const gchar *config_dir)
 {
-  DesktopFileDir dir = { 0, };
+  DesktopFileDir *dir = g_new0 (DesktopFileDir, 1);
 
-  dir.path = g_strdup (config_dir);
+  g_atomic_ref_count_init (&dir->ref_count);
-  dir.is_config = TRUE;
+  dir->path = g_strdup (config_dir);
+  dir->is_config = TRUE;
 
-  g_array_append_val (array, dir);
+  return g_steal_pointer (&dir);
 }
 
 /*< internal >
@@ -1309,6 +1329,7 @@ desktop_file_dir_reset (DesktopFileDir *dir)
   if (dir->monitor)
     {
       g_signal_handlers_disconnect_by_func (dir->monitor, desktop_file_dir_changed, dir);
+      g_file_monitor_cancel (dir->monitor);
       g_object_unref (dir->monitor);
       dir->monitor = NULL;
     }
@@ -1340,6 +1361,14 @@ desktop_file_dir_reset (DesktopFileDir *dir)
   dir->is_setup = FALSE;
 }
 
+static void
+closure_notify_cb (gpointer  data,
+                   GClosure *closure)
+{
+  DesktopFileDir *dir = data;
+  desktop_file_dir_unref (dir);
+}
+
 /*< internal >
  * desktop_file_dir_init:
  * @dir: a #DesktopFileDir
@@ -1368,7 +1397,9 @@ desktop_file_dir_init (DesktopFileDir *dir)
    * we will fall back to polling.
    */
   dir->monitor = g_local_file_monitor_new_in_worker (watch_dir, TRUE, G_FILE_MONITOR_NONE,
-                                                     desktop_file_dir_changed, dir, NULL);
+                                                     desktop_file_dir_changed,
+                                                     desktop_file_dir_ref (dir),
+                                                     closure_notify_cb, NULL);
 
   desktop_file_dir_unindexed_init (dir);
 
@@ -1487,55 +1518,51 @@ desktop_file_dirs_lock (void)
 
   /* If the XDG dirs configuration has changed (expected only during tests),
    * clear and reload the state. */
-  if (g_strcmp0 (desktop_file_dirs_config_dir, user_config_dir) != 0)
+  if (desktop_file_dirs_config_dir != NULL &&
+      g_strcmp0 (desktop_file_dirs_config_dir, user_config_dir) != 0)
     {
       g_debug ("%s: Resetting desktop app info dirs from %s to %s",
                G_STRFUNC, desktop_file_dirs_config_dir, user_config_dir);
 
-      for (i = 0; i < n_desktop_file_dirs; i++)
+      g_ptr_array_set_size (desktop_file_dirs, 0);
-        desktop_file_dir_reset (&desktop_file_dirs[i]);
+      g_clear_pointer (&desktop_file_dir_user_config, desktop_file_dir_unref);
-      g_clear_pointer (&desktop_file_dirs, g_free);
+      g_clear_pointer (&desktop_file_dir_user_data, desktop_file_dir_unref);
-      n_desktop_file_dirs = 0;
-      desktop_file_dir_user_data_index = 0;
     }
 
-  if (desktop_file_dirs == NULL)
+  if (desktop_file_dirs == NULL || desktop_file_dirs->len == 0)
     {
       const char * const *dirs;
-      GArray *tmp;
       gint i;
 
-      tmp = g_array_new (FALSE, FALSE, sizeof (DesktopFileDir));
+      if (desktop_file_dirs == NULL)
+        desktop_file_dirs = g_ptr_array_new_with_free_func ((GDestroyNotify) desktop_file_dir_unref);
 
       /* First, the configs.  Highest priority: the user's ~/.config */
-      desktop_file_dir_create_for_config (tmp, user_config_dir);
+      desktop_file_dir_user_config = desktop_file_dir_new_for_config (user_config_dir);
+      g_ptr_array_add (desktop_file_dirs, desktop_file_dir_ref (desktop_file_dir_user_config));
 
       /* Next, the system configs (/etc/xdg, and so on). */
       dirs = g_get_system_config_dirs ();
       for (i = 0; dirs[i]; i++)
-        desktop_file_dir_create_for_config (tmp, dirs[i]);
+        g_ptr_array_add (desktop_file_dirs, desktop_file_dir_new_for_config (dirs[i]));
 
       /* Now the data.  Highest priority: the user's ~/.local/share/applications */
-      desktop_file_dir_user_data_index = tmp->len;
+      desktop_file_dir_user_data = desktop_file_dir_new (g_get_user_data_dir ());
-      desktop_file_dir_create (tmp, g_get_user_data_dir ());
+      g_ptr_array_add (desktop_file_dirs, desktop_file_dir_ref (desktop_file_dir_user_data));
 
       /* Following that, XDG_DATA_DIRS/applications, in order */
       dirs = g_get_system_data_dirs ();
       for (i = 0; dirs[i]; i++)
-        desktop_file_dir_create (tmp, dirs[i]);
+        g_ptr_array_add (desktop_file_dirs, desktop_file_dir_new (dirs[i]));
 
       /* The list of directories will never change after this, unless
        * g_get_user_config_dir() changes due to %G_TEST_OPTION_ISOLATE_DIRS. */
-      desktop_file_dirs = (DesktopFileDir *) tmp->data;
-      n_desktop_file_dirs = tmp->len;
       desktop_file_dirs_config_dir = user_config_dir;
-
-      g_array_free (tmp, FALSE);
     }
 
-  for (i = 0; i < n_desktop_file_dirs; i++)
+  for (i = 0; i < desktop_file_dirs->len; i++)
-    if (!desktop_file_dirs[i].is_setup)
+    if (!((DesktopFileDir *) g_ptr_array_index (desktop_file_dirs, i))->is_setup)
-      desktop_file_dir_init (&desktop_file_dirs[i]);
+      desktop_file_dir_init (g_ptr_array_index (desktop_file_dirs, i));
 }
 
 static void
@@ -1549,8 +1576,8 @@ desktop_file_dirs_invalidate_user_config (void)
 {
   g_mutex_lock (&desktop_file_dir_lock);
 
-  if (n_desktop_file_dirs)
+  if (desktop_file_dir_user_config != NULL)
-    desktop_file_dir_reset (&desktop_file_dirs[desktop_file_dir_user_config_index]);
+    desktop_file_dir_reset (desktop_file_dir_user_config);
 
   g_mutex_unlock (&desktop_file_dir_lock);
 }
@@ -1560,8 +1587,8 @@ desktop_file_dirs_invalidate_user_data (void)
 {
   g_mutex_lock (&desktop_file_dir_lock);
 
-  if (n_desktop_file_dirs)
+  if (desktop_file_dir_user_data != NULL)
-    desktop_file_dir_reset (&desktop_file_dirs[desktop_file_dir_user_data_index]);
+    desktop_file_dir_reset (desktop_file_dir_user_data);
 
   g_mutex_unlock (&desktop_file_dir_lock);
 }
@@ -1950,9 +1977,9 @@ g_desktop_app_info_new (const char *desktop_id)
 
   desktop_file_dirs_lock ();
 
-  for (i = 0; i < n_desktop_file_dirs; i++)
+  for (i = 0; i < desktop_file_dirs->len; i++)
     {
-      appinfo = desktop_file_dir_get_app (&desktop_file_dirs[i], desktop_id);
+      appinfo = desktop_file_dir_get_app (g_ptr_array_index (desktop_file_dirs, i), desktop_id);
 
       if (appinfo)
         break;
@@ -4109,8 +4136,8 @@ g_desktop_app_info_get_desktop_ids_for_content_type (const gchar *content_type,
   desktop_file_dirs_lock ();
 
   for (i = 0; types[i]; i++)
-    for (j = 0; j < n_desktop_file_dirs; j++)
+    for (j = 0; j < desktop_file_dirs->len; j++)
-      desktop_file_dir_mime_lookup (&desktop_file_dirs[j], types[i], hits, blacklist);
+      desktop_file_dir_mime_lookup (g_ptr_array_index (desktop_file_dirs, j), types[i], hits, blacklist);
 
   /* We will keep the hits past unlocking, so we must dup them */
   for (i = 0; i < hits->len; i++)
@@ -4312,21 +4339,21 @@ g_app_info_get_default_for_type (const char *content_type,
   for (i = 0; types[i]; i++)
     {
       /* Collect all the default apps for this type */
-      for (j = 0; j < n_desktop_file_dirs; j++)
+      for (j = 0; j < desktop_file_dirs->len; j++)
-        desktop_file_dir_default_lookup (&desktop_file_dirs[j], types[i], results);
+        desktop_file_dir_default_lookup (g_ptr_array_index (desktop_file_dirs, j), types[i], results);
 
       /* Consider the associations as well... */
-      for (j = 0; j < n_desktop_file_dirs; j++)
+      for (j = 0; j < desktop_file_dirs->len; j++)
-        desktop_file_dir_mime_lookup (&desktop_file_dirs[j], types[i], results, blacklist);
+        desktop_file_dir_mime_lookup (g_ptr_array_index (desktop_file_dirs, j), types[i], results, blacklist);
 
       /* (If any), see if one of those apps is installed... */
       for (j = 0; j < results->len; j++)
         {
           const gchar *desktop_id = g_ptr_array_index (results, j);
 
-          for (k = 0; k < n_desktop_file_dirs; k++)
+          for (k = 0; k < desktop_file_dirs->len; k++)
             {
-              info = (GAppInfo *) desktop_file_dir_get_app (&desktop_file_dirs[k], desktop_id);
+              info = (GAppInfo *) desktop_file_dir_get_app (g_ptr_array_index (desktop_file_dirs, k), desktop_id);
 
               if (info)
                 {
@@ -4405,8 +4432,8 @@ g_desktop_app_info_get_implementations (const gchar *interface)
 
   desktop_file_dirs_lock ();
 
-  for (i = 0; i < n_desktop_file_dirs; i++)
+  for (i = 0; i < desktop_file_dirs->len; i++)
-    desktop_file_dir_get_implementations (&desktop_file_dirs[i], &result, interface);
+    desktop_file_dir_get_implementations (g_ptr_array_index (desktop_file_dirs, i), &result, interface);
 
   desktop_file_dirs_unlock ();
 
@@ -4464,11 +4491,11 @@ g_desktop_app_info_search (const gchar *search_string)
 
   reset_total_search_results ();
 
-  for (i = 0; i < n_desktop_file_dirs; i++)
+  for (i = 0; i < desktop_file_dirs->len; i++)
     {
       for (j = 0; search_tokens[j]; j++)
         {
-          desktop_file_dir_search (&desktop_file_dirs[i], search_tokens[j]);
+          desktop_file_dir_search (g_ptr_array_index (desktop_file_dirs, i), search_tokens[j]);
           merge_token_results (j == 0);
         }
       merge_directory_results ();
@@ -4543,8 +4570,8 @@ g_app_info_get_all (void)
 
   desktop_file_dirs_lock ();
 
-  for (i = 0; i < n_desktop_file_dirs; i++)
+  for (i = 0; i < desktop_file_dirs->len; i++)
-    desktop_file_dir_get_all (&desktop_file_dirs[i], apps);
+    desktop_file_dir_get_all (g_ptr_array_index (desktop_file_dirs, i), apps);
 
   desktop_file_dirs_unlock ();
 

gio/glocalfilemonitor.c

@@ -46,7 +46,7 @@ struct _GFileMonitorSource {
   GSource       source;
 
   GMutex        lock;
-  gpointer      instance;
+  GWeakRef      instance_ref;
   GFileMonitorFlags flags;
   gchar        *dirname;
   gchar        *basename;
@@ -348,6 +348,7 @@ 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));
@@ -359,7 +360,8 @@ g_file_monitor_source_handle_event (GFileMonitorSource *fms,
   g_mutex_lock (&fms->lock);
 
   /* monitor is already gone -- don't bother */
-  if (!fms->instance)
+  instance = g_weak_ref_get (&fms->instance_ref);
+  if (instance == NULL)
     {
       g_mutex_unlock (&fms->lock);
       return TRUE;
@@ -450,6 +452,7 @@ 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;
 }
@@ -500,9 +503,11 @@ g_file_monitor_source_dispatch (GSource     *source,
   QueuedEvent *event;
   GQueue event_queue;
   gint64 now;
+  GFileMonitor *instance = NULL;
 
   /* make sure the monitor still exists */
-  if (!fms->instance)
+  instance = g_weak_ref_get (&fms->instance_ref);
+  if (instance == NULL)
     return FALSE;
 
   now = g_source_get_time (source);
@@ -550,15 +555,18 @@ g_file_monitor_source_dispatch (GSource     *source,
 
   g_file_monitor_source_update_ready_time (fms);
 
+  g_clear_object (&instance);
   g_mutex_unlock (&fms->lock);
 
   /* We now have our list of events to deliver */
   while ((event = g_queue_pop_head (&event_queue)))
     {
       /* an event handler could destroy 'instance', so check each time */
-      if (fms->instance)
+      instance = g_weak_ref_get (&fms->instance_ref);
-        g_file_monitor_emit_event (fms->instance, event->child, event->other, event->event_type);
+      if (instance != NULL)
+        g_file_monitor_emit_event (instance, event->child, event->other, event->event_type);
 
+      g_clear_object (&instance);
       queued_event_free (event);
     }
 
@@ -568,32 +576,29 @@ g_file_monitor_source_dispatch (GSource     *source,
 static void
 g_file_monitor_source_dispose (GFileMonitorSource *fms)
 {
+  GHashTableIter iter;
+  gpointer seqiter;
+  QueuedEvent *event;
+
   g_mutex_lock (&fms->lock);
 
-  if (fms->instance)
+  g_hash_table_iter_init (&iter, fms->pending_changes_table);
+  while (g_hash_table_iter_next (&iter, NULL, &seqiter))
     {
-      GHashTableIter iter;
+      g_hash_table_iter_remove (&iter);
-      gpointer seqiter;
+      g_sequence_remove (seqiter);
-      QueuedEvent *event;
-
-      g_hash_table_iter_init (&iter, fms->pending_changes_table);
-      while (g_hash_table_iter_next (&iter, NULL, &seqiter))
-        {
-          g_hash_table_iter_remove (&iter);
-          g_sequence_remove (seqiter);
-        }
-
-      while ((event = g_queue_pop_head (&fms->event_queue)))
-        queued_event_free (event);
-
-      g_assert (g_sequence_is_empty (fms->pending_changes));
-      g_assert (g_hash_table_size (fms->pending_changes_table) == 0);
-      g_assert (fms->event_queue.length == 0);
-      fms->instance = NULL;
-
-      g_file_monitor_source_update_ready_time (fms);
     }
 
+  while ((event = g_queue_pop_head (&fms->event_queue)))
+    queued_event_free (event);
+
+  g_assert (g_sequence_is_empty (fms->pending_changes));
+  g_assert (g_hash_table_size (fms->pending_changes_table) == 0);
+  g_assert (fms->event_queue.length == 0);
+  g_weak_ref_set (&fms->instance_ref, NULL);
+
+  g_file_monitor_source_update_ready_time (fms);
+
   g_mutex_unlock (&fms->lock);
 
   g_source_destroy ((GSource *) fms);
@@ -605,7 +610,9 @@ g_file_monitor_source_finalize (GSource *source)
   GFileMonitorSource *fms = (GFileMonitorSource *) source;
 
   /* should already have been cleared in dispose of the monitor */
-  g_assert (fms->instance == NULL);
+  g_assert (g_weak_ref_get (&fms->instance_ref) == NULL);
+  g_weak_ref_clear (&fms->instance_ref);
+
   g_assert (g_sequence_is_empty (fms->pending_changes));
   g_assert (g_hash_table_size (fms->pending_changes_table) == 0);
   g_assert (fms->event_queue.length == 0);
@@ -653,7 +660,7 @@ g_file_monitor_source_new (gpointer           instance,
   g_source_set_name (source, "GFileMonitorSource");
 
   g_mutex_init (&fms->lock);
-  fms->instance = instance;
+  g_weak_ref_init (&fms->instance_ref, instance);
   fms->pending_changes = g_sequence_new (pending_change_free);
   fms->pending_changes_table = g_hash_table_new (str_hash0, str_equal0);
   fms->rate_limit = DEFAULT_RATE_LIMIT;
@@ -887,6 +894,7 @@ g_local_file_monitor_new_in_worker (const gchar           *pathname,
                                     GFileMonitorFlags      flags,
                                     GFileMonitorCallback   callback,
                                     gpointer               user_data,
+                                    GClosureNotify         destroy_user_data,
                                     GError               **error)
 {
   GLocalFileMonitor *monitor;
@@ -899,7 +907,8 @@ g_local_file_monitor_new_in_worker (const gchar           *pathname,
   if (monitor)
     {
       if (callback)
-        g_signal_connect (monitor, "changed", G_CALLBACK (callback), user_data);
+        g_signal_connect_data (monitor, "changed", G_CALLBACK (callback),
+                               user_data, destroy_user_data, 0  /* flags */);
 
       g_local_file_monitor_start (monitor, pathname, is_directory, flags, GLIB_PRIVATE_CALL(g_get_worker_context) ());
     }

gio/glocalfilemonitor.h

@@ -87,6 +87,7 @@ g_local_file_monitor_new_in_worker (const gchar           *pathname,
                                     GFileMonitorFlags      flags,
                                     GFileMonitorCallback   callback,
                                     gpointer               user_data,
+                                    GClosureNotify         destroy_user_data,
                                     GError               **error);
 
 /* for implementations of GLocalFileMonitor */