[gnome-software: 1/3] gs-app: Return a reference from gs_app_dup_addons()

[Milan Crha <mcrha src gnome org>]

commit 94bd8862f4be6de797bb77f128ef4c44f6afc51a
Author: Philip Withnall <pwithnall endlessos org>
Date:   Wed Apr 13 15:53:12 2022 +0100

    gs-app: Return a reference from gs_app_dup_addons()
    
    Rename `gs_app_get_addons()` to `gs_app_dup_addons()`, access the
    `GsApp`’s private data under lock, and atomically return a strong
    reference to the list of addons.
    
    This is one step to avoiding a race where the list of addons is modified
    by one thread while being read by another. The second step (which will
    happen in a following commit) is to change `gs_app_add_addon()` to
    guarantee that the `GsAppList` returned by `gs_app_dup_addons()` is
    immutable.
    
    Signed-off-by: Philip Withnall <pwithnall endlessos org>
    
    Helps: #1721

 lib/gs-app-collation.h                    |  2 +-
 lib/gs-app-list.c                         |  5 +++--
 lib/gs-app.c                              | 14 ++++++++------
 lib/gs-plugin-job-refine.c                |  9 +++++----
 lib/gs-plugin-loader.c                    | 22 +++++++++++-----------
 plugins/dummy/gs-self-test.c              |  5 +++--
 plugins/flatpak/gs-flatpak.c              | 10 +++++-----
 plugins/flatpak/gs-plugin-flatpak.c       |  4 ++--
 plugins/packagekit/gs-plugin-packagekit.c | 16 +++++++++-------
 src/gs-details-page.c                     | 12 ++++++------
 10 files changed, 53 insertions(+), 46 deletions(-)
---
diff --git a/lib/gs-app-collation.h b/lib/gs-app-collation.h
index 892c75518..876ae5443 100644
--- a/lib/gs-app-collation.h
+++ b/lib/gs-app-collation.h
@@ -15,7 +15,7 @@
 G_BEGIN_DECLS
 
 GsAppList      *gs_app_get_related             (GsApp          *app);
-GsAppList      *gs_app_get_addons              (GsApp          *app);
+GsAppList      *gs_app_dup_addons              (GsApp          *app);
 GsAppList      *gs_app_get_history             (GsApp          *app);
 
 G_END_DECLS
diff --git a/lib/gs-app-list.c b/lib/gs-app-list.c
index 3122aa41d..1de573e1c 100644
--- a/lib/gs-app-list.c
+++ b/lib/gs-app-list.c
@@ -137,8 +137,9 @@ gs_app_list_add_watched_for_app (GsAppList *list, GPtrArray *apps, GsApp *app)
        if (list->flags & GS_APP_LIST_FLAG_WATCH_APPS)
                g_ptr_array_add (apps, app);
        if (list->flags & GS_APP_LIST_FLAG_WATCH_APPS_ADDONS) {
-               GsAppList *list2 = gs_app_get_addons (app);
-               for (guint i = 0; i < gs_app_list_length (list2); i++) {
+               g_autoptr(GsAppList) list2 = gs_app_dup_addons (app);
+
+               for (guint i = 0; list2 != NULL && i < gs_app_list_length (list2); i++) {
                        GsApp *app2 = gs_app_list_index (list2, i);
                        g_ptr_array_add (apps, app2);
                }
diff --git a/lib/gs-app.c b/lib/gs-app.c
index 6a30d0aef..52153c3e9 100644
--- a/lib/gs-app.c
+++ b/lib/gs-app.c
@@ -3866,21 +3866,23 @@ gs_app_set_metadata_variant (GsApp *app, const gchar *key, GVariant *value)
 }
 
 /**
- * gs_app_get_addons:
+ * gs_app_dup_addons:
  * @app: a #GsApp
  *
  * Gets the list of addons for the application.
  *
- * Returns: (transfer none): a list of addons
+ * Returns: (transfer full) (nullable): a list of addons, or %NULL if there are none
  *
- * Since: 3.22
- **/
+ * Since: 43
+ */
 GsAppList *
-gs_app_get_addons (GsApp *app)
+gs_app_dup_addons (GsApp *app)
 {
        GsAppPrivate *priv = gs_app_get_instance_private (app);
+       g_autoptr(GMutexLocker) locker = NULL;
        g_return_val_if_fail (GS_IS_APP (app), NULL);
-       return priv->addons;
+       locker = g_mutex_locker_new (&priv->mutex);
+       return (priv->addons != NULL) ? g_object_ref (priv->addons) : NULL;
 }
 
 /**
diff --git a/lib/gs-plugin-job-refine.c b/lib/gs-plugin-job-refine.c
index dabc8de42..f9618cd47 100644
--- a/lib/gs-plugin-job-refine.c
+++ b/lib/gs-plugin-job-refine.c
@@ -472,8 +472,9 @@ finish_refine_internal_op (GTask  *task,
 
                for (guint i = 0; i < gs_app_list_length (list); i++) {
                        GsApp *app = gs_app_list_index (list, i);
-                       GsAppList *addons = gs_app_get_addons (app);
-                       for (guint j = 0; j < gs_app_list_length (addons); j++) {
+                       g_autoptr(GsAppList) addons = gs_app_dup_addons (app);
+
+                       for (guint j = 0; addons != NULL && j < gs_app_list_length (addons); j++) {
                                GsApp *addon = gs_app_list_index (addons, j);
                                g_debug ("refining app %s addon %s",
                                         gs_app_get_id (app),
@@ -665,13 +666,13 @@ run_cb (GObject      *source_object,
                for (guint i = 0; i < gs_app_list_length (result_list); i++) {
                        g_autoptr(GPtrArray) to_remove = g_ptr_array_new ();
                        GsApp *app = gs_app_list_index (result_list, i);
-                       GsAppList *addons = gs_app_get_addons (app);
+                       g_autoptr(GsAppList) addons = gs_app_dup_addons (app);
 
                        /* find any apps with the same source */
                        const gchar *pkgname_parent = gs_app_get_source_default (app);
                        if (pkgname_parent == NULL)
                                continue;
-                       for (guint j = 0; j < gs_app_list_length (addons); j++) {
+                       for (guint j = 0; addons != NULL && j < gs_app_list_length (addons); j++) {
                                GsApp *addon = gs_app_list_index (addons, j);
                                if (g_strcmp0 (gs_app_get_source_default (addon),
                                               pkgname_parent) == 0) {
diff --git a/lib/gs-plugin-loader.c b/lib/gs-plugin-loader.c
index ab912992a..bb10a63c5 100644
--- a/lib/gs-plugin-loader.c
+++ b/lib/gs-plugin-loader.c
@@ -206,12 +206,12 @@ gs_plugin_loader_helper_new (GsPluginLoader *plugin_loader, GsPluginJob *plugin_
 static void
 reset_app_progress (GsApp *app)
 {
-       GsAppList *addons = gs_app_get_addons (app);
+       g_autoptr(GsAppList) addons = gs_app_dup_addons (app);
        GsAppList *related = gs_app_get_related (app);
 
        gs_app_set_progress (app, GS_APP_PROGRESS_UNKNOWN);
 
-       for (guint i = 0; i < gs_app_list_length (addons); i++) {
+       for (guint i = 0; addons != NULL && i < gs_app_list_length (addons); i++) {
                GsApp *app_addons = gs_app_list_index (addons, i);
                gs_app_set_progress (app_addons, GS_APP_PROGRESS_UNKNOWN);
        }
@@ -1516,7 +1516,7 @@ save_install_queue (GsPluginLoader *plugin_loader)
 static void
 add_app_to_install_queue (GsPluginLoader *plugin_loader, GsApp *app)
 {
-       GsAppList *addons;
+       g_autoptr(GsAppList) addons = NULL;
        guint i;
        guint id;
 
@@ -1531,8 +1531,8 @@ add_app_to_install_queue (GsPluginLoader *plugin_loader, GsApp *app)
        save_install_queue (plugin_loader);
 
        /* recursively queue any addons */
-       addons = gs_app_get_addons (app);
-       for (i = 0; i < gs_app_list_length (addons); i++) {
+       addons = gs_app_dup_addons (app);
+       for (i = 0; addons != NULL && i < gs_app_list_length (addons); i++) {
                GsApp *addon = gs_app_list_index (addons, i);
                if (gs_app_get_to_be_installed (addon))
                        add_app_to_install_queue (plugin_loader, addon);
@@ -1542,7 +1542,7 @@ add_app_to_install_queue (GsPluginLoader *plugin_loader, GsApp *app)
 static gboolean
 remove_app_from_install_queue (GsPluginLoader *plugin_loader, GsApp *app)
 {
-       GsAppList *addons;
+       g_autoptr(GsAppList) addons = NULL;
        gboolean ret;
        guint i;
        guint id;
@@ -1558,8 +1558,8 @@ remove_app_from_install_queue (GsPluginLoader *plugin_loader, GsApp *app)
                save_install_queue (plugin_loader);
 
                /* recursively remove any queued addons */
-               addons = gs_app_get_addons (app);
-               for (i = 0; i < gs_app_list_length (addons); i++) {
+               addons = gs_app_dup_addons (app);
+               for (i = 0; addons != NULL && i < gs_app_list_length (addons); i++) {
                        GsApp *addon = gs_app_list_index (addons, i);
                        remove_app_from_install_queue (plugin_loader, addon);
                }
@@ -3280,9 +3280,9 @@ gs_plugin_loader_process_thread_cb (GTask *task,
 
        /* unstage addons */
        if (add_to_pending_array) {
-               GsAppList *addons;
-               addons = gs_app_get_addons (gs_plugin_job_get_app (helper->plugin_job));
-               for (guint i = 0; i < gs_app_list_length (addons); i++) {
+               g_autoptr(GsAppList) addons = gs_app_dup_addons (gs_plugin_job_get_app (helper->plugin_job));
+
+               for (guint i = 0; addons != NULL && i < gs_app_list_length (addons); i++) {
                        GsApp *addon = gs_app_list_index (addons, i);
                        if (gs_app_get_to_be_installed (addon))
                                gs_app_set_to_be_installed (addon, FALSE);
diff --git a/plugins/dummy/gs-self-test.c b/plugins/dummy/gs-self-test.c
index 9964f166c..121fac35f 100644
--- a/plugins/dummy/gs-self-test.c
+++ b/plugins/dummy/gs-self-test.c
@@ -352,7 +352,7 @@ gs_plugins_dummy_installed_func (GsPluginLoader *plugin_loader)
 {
        GsApp *app;
        GsApp *addon;
-       GsAppList *addons;
+       g_autoptr(GsAppList) addons = NULL;
        g_autofree gchar *menu_path = NULL;
        g_autoptr(GError) error = NULL;
        g_autoptr(GsAppList) list = NULL;
@@ -405,7 +405,8 @@ gs_plugins_dummy_installed_func (GsPluginLoader *plugin_loader)
        g_assert_cmpstr (menu_path, ==, "Create->Music Players");
 
        /* check addon */
-       addons = gs_app_get_addons (app);
+       addons = gs_app_dup_addons (app);
+       g_assert_nonnull (addons);
        g_assert_cmpint (gs_app_list_length (addons), ==, 1);
        addon = gs_app_list_index (addons, 0);
        g_assert_cmpstr (gs_app_get_id (addon), ==, "zeus-spell.addon");
diff --git a/plugins/flatpak/gs-flatpak.c b/plugins/flatpak/gs-flatpak.c
index 71d628035..4a113a005 100644
--- a/plugins/flatpak/gs-flatpak.c
+++ b/plugins/flatpak/gs-flatpak.c
@@ -2562,15 +2562,15 @@ gs_flatpak_prune_addons_list (GsFlatpak *self,
                              GCancellable *cancellable,
                              GError **error)
 {
-       GsAppList *addons_list;
+       g_autoptr(GsAppList) addons_list = NULL;
        g_autoptr(GPtrArray) installed_related_refs = NULL;
        g_autoptr(GPtrArray) remote_related_refs = NULL;
        g_autofree gchar *ref = NULL;
        FlatpakInstallation *installation = gs_flatpak_get_installation (self, interactive);
        g_autoptr(GError) error_local = NULL;
 
-       addons_list = gs_app_get_addons (app);
-       if (gs_app_list_length (addons_list) == 0)
+       addons_list = gs_app_dup_addons (app);
+       if (addons_list == NULL || gs_app_list_length (addons_list) == 0)
                return TRUE;
 
        if (gs_app_get_origin (app) == NULL)
@@ -3282,11 +3282,11 @@ gs_flatpak_refine_addons (GsFlatpak *self,
                          gboolean interactive,
                          GCancellable *cancellable)
 {
-       GsAppList *addons;
+       g_autoptr(GsAppList) addons = NULL;
        g_autoptr(GString) errors = NULL;
        guint ii, sz;
 
-       addons = gs_app_get_addons (parent_app);
+       addons = gs_app_dup_addons (parent_app);
        sz = addons ? gs_app_list_length (addons) : 0;
 
        for (ii = 0; ii < sz; ii++) {
diff --git a/plugins/flatpak/gs-plugin-flatpak.c b/plugins/flatpak/gs-plugin-flatpak.c
index 78b08b9c6..05849482a 100644
--- a/plugins/flatpak/gs-plugin-flatpak.c
+++ b/plugins/flatpak/gs-plugin-flatpak.c
@@ -1065,14 +1065,14 @@ gs_flatpak_cover_addons_in_transaction (GsPlugin *plugin,
                                        GsApp *parent_app,
                                        GsAppState state)
 {
-       GsAppList *addons;
+       g_autoptr(GsAppList) addons = NULL;
        g_autoptr(GString) errors = NULL;
        guint ii, sz;
 
        g_return_if_fail (transaction != NULL);
        g_return_if_fail (GS_IS_APP (parent_app));
 
-       addons = gs_app_get_addons (parent_app);
+       addons = gs_app_dup_addons (parent_app);
        sz = addons ? gs_app_list_length (addons) : 0;
 
        for (ii = 0; ii < sz; ii++) {
diff --git a/plugins/packagekit/gs-plugin-packagekit.c b/plugins/packagekit/gs-plugin-packagekit.c
index 6ad23e988..0a80bfc9e 100644
--- a/plugins/packagekit/gs-plugin-packagekit.c
+++ b/plugins/packagekit/gs-plugin-packagekit.c
@@ -289,7 +289,9 @@ typedef gboolean (*GsAppFilterFunc) (GsApp *app);
 
 /* The elements in the returned #GPtrArray reference memory from within the
  * @apps list, so the array is only valid as long as @apps is not modified or
- * freed. The array is not NULL-terminated. */
+ * freed. The array is not NULL-terminated.
+ *
+ * If @apps is %NULL, that’s considered equivalent to an empty list. */
 static GPtrArray *
 app_list_get_package_ids (GsAppList       *apps,
                           GsAppFilterFunc  app_filter,
@@ -297,7 +299,7 @@ app_list_get_package_ids (GsAppList       *apps,
 {
        g_autoptr(GPtrArray) list_package_ids = g_ptr_array_new_with_free_func (NULL);
 
-       for (guint i = 0; i < gs_app_list_length (apps); i++) {
+       for (guint i = 0; apps != NULL && i < gs_app_list_length (apps); i++) {
                GsApp *app = gs_app_list_index (apps, i);
                GPtrArray *app_source_ids;
 
@@ -511,7 +513,7 @@ gs_plugin_app_install (GsPlugin *plugin,
                       GError **error)
 {
        GsPluginPackagekit *self = GS_PLUGIN_PACKAGEKIT (plugin);
-       GsAppList *addons;
+       g_autoptr(GsAppList) addons = NULL;
        GPtrArray *source_ids;
        g_autoptr(GsPackagekitHelper) helper = gs_packagekit_helper_new (plugin);
        const gchar *package_id;
@@ -598,7 +600,7 @@ gs_plugin_app_install (GsPlugin *plugin,
                        return FALSE;
                }
 
-               addons = gs_app_get_addons (app);
+               addons = gs_app_dup_addons (app);
                array_package_ids = app_list_get_package_ids (addons,
                                                              gs_app_get_to_be_installed,
                                                              TRUE);
@@ -715,7 +717,7 @@ gs_plugin_app_remove (GsPlugin *plugin,
        GsPluginPackagekit *self = GS_PLUGIN_PACKAGEKIT (plugin);
        const gchar *package_id;
        GPtrArray *source_ids;
-       GsAppList *addons;
+       g_autoptr(GsAppList) addons = NULL;
        g_autoptr(GsPackagekitHelper) helper = gs_packagekit_helper_new (plugin);
        guint i;
        guint cnt = 0;
@@ -771,8 +773,8 @@ gs_plugin_app_remove (GsPlugin *plugin,
        }
 
        /* Make sure addons' state is updated as well */
-       addons = gs_app_get_addons (app);
-       for (i = 0; i < gs_app_list_length (addons); i++) {
+       addons = gs_app_dup_addons (app);
+       for (i = 0; addons != NULL && i < gs_app_list_length (addons); i++) {
                GsApp *addon = gs_app_list_index (addons, i);
                if (gs_app_get_state (addon) == GS_APP_STATE_INSTALLED) {
                        gs_app_set_state (addon, GS_APP_STATE_UNKNOWN);
diff --git a/src/gs-details-page.c b/src/gs-details-page.c
index dd1591087..413756348 100644
--- a/src/gs-details-page.c
+++ b/src/gs-details-page.c
@@ -1209,13 +1209,13 @@ static void gs_details_page_addon_remove_cb (GsAppAddonRow *row, gpointer user_d
 static void
 gs_details_page_refresh_addons (GsDetailsPage *self)
 {
-       GsAppList *addons;
+       g_autoptr(GsAppList) addons = NULL;
        guint i, rows = 0;
 
        gs_widget_remove_all (self->list_box_addons, (GsRemoveFunc) gtk_list_box_remove);
 
-       addons = gs_app_get_addons (self->app);
-       for (i = 0; i < gs_app_list_length (addons); i++) {
+       addons = gs_app_dup_addons (self->app);
+       for (i = 0; addons != NULL && i < gs_app_list_length (addons); i++) {
                GsApp *addon;
                GtkWidget *row;
 
@@ -1978,12 +1978,12 @@ static void
 gs_details_page_app_installed (GsPage *page, GsApp *app)
 {
        GsDetailsPage *self = GS_DETAILS_PAGE (page);
-       GsAppList *addons;
+       g_autoptr(GsAppList) addons = NULL;
        guint i;
 
        /* if the app is just an addon, no need for a full refresh */
-       addons = gs_app_get_addons (self->app);
-       for (i = 0; i < gs_app_list_length (addons); i++) {
+       addons = gs_app_dup_addons (self->app);
+       for (i = 0; addons != NULL && i < gs_app_list_length (addons); i++) {
                GsApp *addon;
                addon = gs_app_list_index (addons, i);
                if (addon == app)