[gnome-software: 11/20] gs-plugin-loader: Make job_process_async() wait until setup is complete

[Philip Withnall <pwithnall src gnome org>]

commit 16c6a89de6b3c9cb4365aac6ccaea1d69ea0399a
Author: Philip Withnall <pwithnall endlessos org>
Date:   Mon Mar 7 17:34:32 2022 +0000

    gs-plugin-loader: Make job_process_async() wait until setup is complete
    
    During early startup of `GsApplication`, `setup_async()` is called on
    the `GsPluginLoader`, and is allowed to complete asynchronously after
    `GsApplication.startup()` returns.
    
    At this point, `GsApplication.activate()` or any of the action map
    functions can be called. Some of them call `GsPluginLoader` methods like
    `job_process_async()`. These could be called before the asynchronous
    `setup_async()` operation has completed, and might crash or return
    incorrect results because of this.
    
    Avoid that by delaying the work in `job_process_async()` until setup has
    completed.
    
    Signed-off-by: Philip Withnall <pwithnall endlessos org>
    
    Helps: #1670

 lib/gs-plugin-loader.c | 181 ++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 149 insertions(+), 32 deletions(-)
---
diff --git a/lib/gs-plugin-loader.c b/lib/gs-plugin-loader.c
index cdccf0388..06fd4f744 100644
--- a/lib/gs-plugin-loader.c
+++ b/lib/gs-plugin-loader.c
@@ -40,6 +40,9 @@ struct _GsPluginLoader
 {
        GObject                  parent;
 
+       gboolean                 setup_complete;
+       GCancellable            *setup_complete_cancellable;  /* (nullable) (owned) */
+
        GPtrArray               *plugins;
        GPtrArray               *locations;
        gchar                   *language;
@@ -179,9 +182,6 @@ typedef gboolean     (*GsPluginGetLangPacksFunc)    (GsPlugin       *plugin,
 /* async helper */
 typedef struct {
        GsPluginLoader                  *plugin_loader;
-       GCancellable                    *cancellable;
-       GCancellable                    *cancellable_caller;
-       gulong                           cancellable_id;
        const gchar                     *function_name;
        const gchar                     *function_name_parent;
        GPtrArray                       *catlist;
@@ -253,20 +253,11 @@ gs_plugin_loader_helper_free (GsPluginLoaderHelper *helper)
                break;
        }
 
-       if (helper->cancellable_id > 0) {
-               g_debug ("Disconnecting cancellable %p", helper->cancellable_caller);
-               g_cancellable_disconnect (helper->cancellable_caller,
-                                         helper->cancellable_id);
-       }
        g_object_unref (helper->plugin_loader);
        if (helper->timeout_id != 0)
                g_source_remove (helper->timeout_id);
        if (helper->plugin_job != NULL)
                g_object_unref (helper->plugin_job);
-       if (helper->cancellable != NULL)
-               g_object_unref (helper->cancellable);
-       if (helper->cancellable_caller != NULL)
-               g_object_unref (helper->cancellable_caller);
        if (helper->catlist != NULL)
                g_ptr_array_unref (helper->catlist);
        g_strfreev (helper->tokens);
@@ -2113,6 +2104,9 @@ gs_plugin_loader_shutdown (GsPluginLoader *plugin_loader,
        /* Clear some internal data structures. */
        gs_plugin_loader_remove_all_plugins (plugin_loader);
        gs_plugin_loader_remove_all_file_monitors (plugin_loader);
+       plugin_loader->setup_complete = FALSE;
+       g_clear_object (&plugin_loader->setup_complete_cancellable);
+       plugin_loader->setup_complete_cancellable = g_cancellable_new ();
 }
 
 static void
@@ -2184,6 +2178,17 @@ static void plugin_setup_cb (GObject      *source_object,
                              gpointer      user_data);
 static void finish_setup_op (GTask *task);
 
+/* Mark the asynchronous setup operation as complete. This will notify any
+ * waiting tasks by cancelling the #GCancellable. It’s safe to clear the
+ * #GCancellable as each waiting task holds its own reference. */
+static void
+notify_setup_complete (GsPluginLoader *plugin_loader)
+{
+       plugin_loader->setup_complete = TRUE;
+       g_cancellable_cancel (plugin_loader->setup_complete_cancellable);
+       g_clear_object (&plugin_loader->setup_complete_cancellable);
+}
+
 /**
  * gs_plugin_loader_setup_async:
  * @plugin_loader: a #GsPluginLoader
@@ -2227,6 +2232,12 @@ gs_plugin_loader_setup_async (GsPluginLoader      *plugin_loader,
        task = g_task_new (plugin_loader, cancellable, callback, user_data);
        g_task_set_source_tag (task, gs_plugin_loader_setup_async);
 
+       /* If setup is already complete, return immediately. */
+       if (plugin_loader->setup_complete) {
+               g_task_return_boolean (task, TRUE);
+               return;
+       }
+
        /* use the default, but this requires a 'make install' */
        if (plugin_loader->locations->len == 0) {
                g_autofree gchar *filename = NULL;
@@ -2247,6 +2258,7 @@ gs_plugin_loader_setup_async (GsPluginLoader      *plugin_loader,
                                                    cancellable,
                                                    &local_error);
                if (monitor == NULL) {
+                       notify_setup_complete (plugin_loader);
                        g_task_return_error (task, g_steal_pointer (&local_error));
                        return;
                }
@@ -2265,6 +2277,7 @@ gs_plugin_loader_setup_async (GsPluginLoader      *plugin_loader,
                g_debug ("searching for plugins in %s", location);
                fns = gs_plugin_loader_find_plugins (location, &local_error);
                if (fns == NULL) {
+                       notify_setup_complete (plugin_loader);
                        g_task_return_error (task, g_steal_pointer (&local_error));
                        return;
                }
@@ -2356,6 +2369,7 @@ gs_plugin_loader_setup_async (GsPluginLoader      *plugin_loader,
 
                /* check we're not stuck */
                if (dep_loop_check++ > 100) {
+                       notify_setup_complete (plugin_loader);
                        g_task_return_new_error (task,
                                                 GS_PLUGIN_ERROR,
                                                 GS_PLUGIN_ERROR_PLUGIN_DEPSOLVE_FAILED,
@@ -2417,6 +2431,7 @@ gs_plugin_loader_setup_async (GsPluginLoader      *plugin_loader,
 
                /* check we're not stuck */
                if (dep_loop_check++ > 100) {
+                       notify_setup_complete (plugin_loader);
                        g_task_return_new_error (task,
                                                 GS_PLUGIN_ERROR,
                                                 GS_PLUGIN_ERROR_PLUGIN_DEPSOLVE_FAILED,
@@ -2503,6 +2518,7 @@ finish_setup_op (GTask *task)
 
        /* now we can load the install-queue */
        if (!load_install_queue (plugin_loader, &local_error)) {
+               notify_setup_complete (plugin_loader);
                g_task_return_error (task, g_steal_pointer (&local_error));
                return;
        }
@@ -2520,6 +2536,7 @@ finish_setup_op (GTask *task)
        }
 #endif  /* HAVE_SYSPROF */
 
+       notify_setup_complete (plugin_loader);
        g_task_return_boolean (task, TRUE);
 }
 
@@ -2648,6 +2665,7 @@ gs_plugin_loader_dispose (GObject *object)
        g_clear_pointer (&plugin_loader->pending_apps, g_ptr_array_unref);
        g_clear_object (&plugin_loader->category_manager);
        g_clear_object (&plugin_loader->odrs_provider);
+       g_clear_object (&plugin_loader->setup_complete_cancellable);
 
 #ifdef HAVE_SYSPROF
        g_clear_pointer (&plugin_loader->sysprof_writer, sysprof_capture_writer_unref);
@@ -2787,6 +2805,7 @@ gs_plugin_loader_init (GsPluginLoader *plugin_loader)
        plugin_loader->sysprof_writer = sysprof_capture_writer_new_from_env (0);
 #endif  /* HAVE_SYSPROF */
 
+       plugin_loader->setup_complete_cancellable = g_cancellable_new ();
        plugin_loader->scale = 1;
        plugin_loader->plugins = g_ptr_array_new_with_free_func (g_object_unref);
        plugin_loader->pending_apps = g_ptr_array_new_with_free_func (g_object_unref);
@@ -3522,7 +3541,8 @@ is_running_under_gdb (void)
 static gboolean
 gs_plugin_loader_job_timeout_cb (gpointer user_data)
 {
-       GsPluginLoaderHelper *helper = (GsPluginLoaderHelper *) user_data;
+       GTask *task = G_TASK (user_data);
+       GsPluginLoaderHelper *helper = g_task_get_task_data (task);
 
        /* Don’t impose timeouts if running under gdb. */
        if (is_running_under_gdb ()) {
@@ -3538,7 +3558,7 @@ gs_plugin_loader_job_timeout_cb (gpointer user_data)
        g_debug ("cancelling job %s as it took longer than %u seconds",
                 helper->function_name,
                 gs_plugin_job_get_timeout (helper->plugin_job));
-       g_cancellable_cancel (helper->cancellable);
+       g_cancellable_cancel (g_task_get_cancellable (task));
 
        /* failed */
        helper->timeout_triggered = TRUE;
@@ -3547,11 +3567,14 @@ gs_plugin_loader_job_timeout_cb (gpointer user_data)
 }
 
 static void
-gs_plugin_loader_cancelled_cb (GCancellable *cancellable, GsPluginLoaderHelper *helper)
+gs_plugin_loader_cancelled_cb (GCancellable *cancellable,
+                               gpointer      user_data)
 {
+       GCancellable *child_cancellable = G_CANCELLABLE (user_data);
+
        /* just proxy this forward */
-       g_debug ("Cancelling job with cancellable %p", helper->cancellable);
-       g_cancellable_cancel (helper->cancellable);
+       g_debug ("Cancelling job with cancellable %p", child_cancellable);
+       g_cancellable_cancel (child_cancellable);
 }
 
 static void
@@ -3637,6 +3660,32 @@ run_job_cb (GObject      *source_object,
        g_assert_not_reached ();
 }
 
+typedef struct {
+       GWeakRef parent_cancellable_weak;
+       gulong handler_id;
+} CancellableData;
+
+static void
+cancellable_data_free (CancellableData *data)
+{
+       g_autoptr(GCancellable) parent_cancellable = g_weak_ref_get (&data->parent_cancellable_weak);
+
+       if (parent_cancellable != NULL)
+               g_cancellable_disconnect (parent_cancellable, data->handler_id);
+
+       g_weak_ref_clear (&data->parent_cancellable_weak);
+       g_free (data);
+}
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC (CancellableData, cancellable_data_free)
+
+static gboolean job_process_setup_complete_cb (GCancellable *cancellable,
+                                               gpointer      user_data);
+static void job_process_cb (GTask *task);
+static void job_process_refine_cb (GObject      *source_object,
+                                   GAsyncResult *result,
+                                   gpointer      user_data);
+
 /**
  * gs_plugin_loader_job_process_async:
  * @plugin_loader: A #GsPluginLoader
@@ -3646,6 +3695,9 @@ run_job_cb (GObject      *source_object,
  * @user_data: user data to pass to @callback
  *
  * This method calls all plugins.
+ *
+ * If the #GsPluginLoader is still being set up, this function will wait until
+ * setup is complete before running.
  **/
 void
 gs_plugin_loader_job_process_async (GsPluginLoader *plugin_loader,
@@ -3656,7 +3708,6 @@ gs_plugin_loader_job_process_async (GsPluginLoader *plugin_loader,
 {
        GsPluginJobClass *job_class;
        GsPluginAction action;
-       GsPluginLoaderHelper *helper;
        g_autoptr(GTask) task = NULL;
        g_autoptr(GCancellable) cancellable_job = NULL;
        g_autofree gchar *task_name = NULL;
@@ -3674,10 +3725,66 @@ gs_plugin_loader_job_process_async (GsPluginLoader *plugin_loader,
        } else {
                task_name = g_strdup_printf ("%s %s", G_STRFUNC, gs_plugin_action_to_string (action));
                cancellable_job = g_cancellable_new ();
+
+               /* Old-style jobs always have a valid cancellable, so proxy the caller */
+               g_debug ("Chaining cancellation from %p to %p", cancellable, cancellable_job);
+               if (cancellable != NULL) {
+                       g_autoptr(CancellableData) cancellable_data = NULL;
+
+                       cancellable_data = g_new0 (CancellableData, 1);
+                       g_weak_ref_init (&cancellable_data->parent_cancellable_weak, cancellable);
+                       cancellable_data->handler_id = g_cancellable_connect (cancellable,
+                                                                             G_CALLBACK 
(gs_plugin_loader_cancelled_cb),
+                                                                             cancellable_job, NULL);
+
+                       g_object_set_data_full (G_OBJECT (cancellable_job),
+                                               "gs-cancellable-chain",
+                                               g_steal_pointer (&cancellable_data),
+                                               (GDestroyNotify) cancellable_data_free);
+               }
        }
 
        task = g_task_new (plugin_loader, cancellable_job, callback, user_data);
        g_task_set_name (task, task_name);
+       g_task_set_task_data (task, g_object_ref (plugin_job), (GDestroyNotify) g_object_unref);
+
+       /* Wait until the plugin has finished setting up.
+        *
+        * Do this using a #GCancellable. While we’re not using the #GCancellable
+        * to cancel anything, it is a reliable way to signal between threads
+        * without polling, waking up all waiting #GMainContexts when it’s
+        * ‘cancelled’. */
+       if (plugin_loader->setup_complete) {
+               job_process_cb (task);
+       } else {
+               g_autoptr(GSource) cancellable_source = g_cancellable_source_new 
(plugin_loader->setup_complete_cancellable);
+               g_task_attach_source (task, cancellable_source, G_SOURCE_FUNC 
(job_process_setup_complete_cb));
+       }
+}
+
+static gboolean
+job_process_setup_complete_cb (GCancellable *cancellable,
+                               gpointer      user_data)
+{
+       GTask *task = G_TASK (user_data);
+
+       job_process_cb (task);
+
+       return G_SOURCE_REMOVE;
+}
+
+static void
+job_process_cb (GTask *task)
+{
+       g_autoptr(GsPluginJob) plugin_job = g_object_ref (g_task_get_task_data (task));
+       GsPluginLoader *plugin_loader = g_task_get_source_object (task);
+       GCancellable *cancellable = g_task_get_cancellable (task);
+       GsPluginJobClass *job_class;
+       GsPluginAction action;
+       GsPluginLoaderHelper *helper;
+
+       job_class = GS_PLUGIN_JOB_GET_CLASS (plugin_job);
+       action = gs_plugin_job_get_action (plugin_job);
 
        /* If the job provides a more specific async run function, use that.
         *
@@ -3692,7 +3799,7 @@ gs_plugin_loader_job_process_async (GsPluginLoader *plugin_loader,
 #endif
 
                job_class->run_async (plugin_job, plugin_loader, cancellable,
-                                     run_job_cb, g_steal_pointer (&task));
+                                     run_job_cb, g_object_ref (task));
                return;
        }
 
@@ -3738,7 +3845,7 @@ gs_plugin_loader_job_process_async (GsPluginLoader *plugin_loader,
                        refine_job = gs_plugin_job_refine_new (list, GS_PLUGIN_REFINE_FLAGS_REQUIRE_ID | 
GS_PLUGIN_REFINE_FLAGS_DISABLE_FILTERING);
                        gs_plugin_loader_job_process_async (plugin_loader, refine_job,
                                                            cancellable,
-                                                           callback, user_data);
+                                                           job_process_refine_cb, g_object_ref (task));
                        return;
                }
        }
@@ -3846,17 +3953,6 @@ gs_plugin_loader_job_process_async (GsPluginLoader *plugin_loader,
                }
        }
 
-       /* jobs always have a valid cancellable, so proxy the caller */
-       helper->cancellable = g_object_ref (cancellable_job);
-       g_debug ("Chaining cancellation from %p to %p", cancellable, cancellable_job);
-       if (cancellable != NULL) {
-               helper->cancellable_caller = g_object_ref (cancellable);
-               helper->cancellable_id =
-                       g_cancellable_connect (helper->cancellable_caller,
-                                              G_CALLBACK (gs_plugin_loader_cancelled_cb),
-                                              helper, NULL);
-       }
-
        /* set up a hang handler */
        switch (action) {
        case GS_PLUGIN_ACTION_GET_ALTERNATES:
@@ -3871,7 +3967,7 @@ gs_plugin_loader_job_process_async (GsPluginLoader *plugin_loader,
                        helper->timeout_id =
                                g_timeout_add_seconds (gs_plugin_job_get_timeout (plugin_job),
                                                       gs_plugin_loader_job_timeout_cb,
-                                                      helper);
+                                                      task);
                }
                break;
        default:
@@ -3895,6 +3991,24 @@ gs_plugin_loader_job_process_async (GsPluginLoader *plugin_loader,
        g_task_run_in_thread (task, gs_plugin_loader_process_thread_cb);
 }
 
+static void
+job_process_refine_cb (GObject      *source_object,
+                       GAsyncResult *result,
+                       gpointer      user_data)
+{
+       GsPluginLoader *plugin_loader = GS_PLUGIN_LOADER (source_object);
+       g_autoptr(GsAppList) results = NULL;
+       g_autoptr(GTask) task = g_steal_pointer (&user_data);
+       g_autoptr(GError) local_error = NULL;
+
+       results = gs_plugin_loader_job_process_finish (plugin_loader, result, &local_error);
+
+       if (results == NULL)
+               g_task_return_error (task, g_steal_pointer (&local_error));
+       else
+               g_task_return_pointer (task, g_steal_pointer (&results), (GDestroyNotify) g_object_unref);
+}
+
 /******************************************************************************/
 
 /**
@@ -3954,6 +4068,9 @@ static void app_create_cb (GObject      *source_object,
  * Create a #GsApp identified by @unique_id asynchronously.
  * Finish the call with gs_plugin_loader_app_create_finish().
  *
+ * If the #GsPluginLoader is still being set up, this function will wait until
+ * setup is complete before running.
+ *
  * Since: 41
  **/
 void