[evolution-data-server] CamelFolder: Cancel save of the changes early in the dispose()

[Milan Crha <mcrha src gnome org>]

commit 9991f16b106ced9e10036aecec0d22bba82f2119
Author: Milan Crha <mcrha redhat com>
Date:   Mon Feb 8 11:56:28 2021 +0100

    CamelFolder: Cancel save of the changes early in the dispose()
    
    The save of the changes is scheduled for a GMainContext, with a chance
    to be cancelled when the folder is freed, but it could happen, with
    proper timing and thread interleaving, that the dipose() was called
    just before the schedule to save the changes was dispatched, but
    the dispose() already freed some internal structures, which could
    cause a crash elsewhere in the code. This change cancels the scheduled
    save as the first thing in the dispose() and it also changes the code
    to use a GWeakRef, which ensures the save is scheduled only if
    the dispose() was not called yet.
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1914917

 src/camel/camel-folder.c                       | 88 +++++++++++++++-----------
 src/camel/camel-utils.c                        | 44 +++++++++++++
 src/camel/camel-utils.h                        |  3 +
 src/camel/providers/imapx/camel-imapx-server.c | 32 +---------
 4 files changed, 100 insertions(+), 67 deletions(-)
---
diff --git a/src/camel/camel-folder.c b/src/camel/camel-folder.c
index e418b0a5b..594504c6a 100644
--- a/src/camel/camel-folder.c
+++ b/src/camel/camel-folder.c
@@ -152,50 +152,61 @@ folder_store_changes_job_cb (CamelSession *session,
        camel_folder_synchronize_sync (folder, FALSE, cancellable, error);
 }
 
-static gboolean
-folder_schedule_store_changes_job (gpointer user_data)
+static void
+folder_schedule_store_changes_job (CamelFolder *folder)
 {
-       CamelFolder *folder = user_data;
-       GSource *source;
+       CamelSession *session;
+       CamelStore *parent_store;
 
-       source = g_main_current_source ();
+       g_return_if_fail (CAMEL_IS_FOLDER (folder));
 
-       if (g_source_is_destroyed (source))
-               return FALSE;
+       parent_store = camel_folder_get_parent_store (folder);
+       session = parent_store ? camel_service_ref_session (CAMEL_SERVICE (parent_store)) : NULL;
+       if (session) {
+               gchar *description;
 
-       g_return_val_if_fail (CAMEL_IS_FOLDER (folder), FALSE);
+               /* Translators: The first “%s” is replaced with an account name and the second “%s”
+                  is replaced with a full path name. The spaces around “:” are intentional, as
+                  the whole “%s : %s” is meant as an absolute identification of the folder. */
+               description = g_strdup_printf (_("Storing changes in folder “%s : %s”"),
+                       camel_service_get_display_name (CAMEL_SERVICE (parent_store)),
+                       camel_folder_get_full_name (folder));
 
-       g_mutex_lock (&folder->priv->store_changes_lock);
+               camel_session_submit_job (session, description,
+                       folder_store_changes_job_cb,
+                       g_object_ref (folder), g_object_unref);
 
-       if (folder->priv->store_changes_id == g_source_get_id (source)) {
-               CamelSession *session;
-               CamelStore *parent_store;
+               g_free (description);
+       }
 
-               folder->priv->store_changes_id = 0;
+       g_clear_object (&session);
+}
 
-               parent_store = camel_folder_get_parent_store (folder);
-               session = parent_store ? camel_service_ref_session (CAMEL_SERVICE (parent_store)) : NULL;
-               if (session) {
-                       gchar *description;
+static gboolean
+folder_schedule_store_changes_job_cb (gpointer user_data)
+{
+       GWeakRef *weak_ref = user_data;
+       GSource *source;
+       CamelFolder *folder;
 
-                       /* Translators: The first “%s” is replaced with an account name and the second “%s”
-                          is replaced with a full path name. The spaces around “:” are intentional, as
-                          the whole “%s : %s” is meant as an absolute identification of the folder. */
-                       description = g_strdup_printf (_("Storing changes in folder “%s : %s”"),
-                               camel_service_get_display_name (CAMEL_SERVICE (parent_store)),
-                               camel_folder_get_full_name (folder));
+       source = g_main_current_source ();
+
+       if (g_source_is_destroyed (source))
+               return FALSE;
 
-                       camel_session_submit_job (session, description,
-                               folder_store_changes_job_cb,
-                               g_object_ref (folder), g_object_unref);
+       folder = g_weak_ref_get (weak_ref);
+       if (folder) {
+               g_mutex_lock (&folder->priv->store_changes_lock);
 
-                       g_free (description);
+               if (folder->priv->store_changes_id == g_source_get_id (source)) {
+                       folder->priv->store_changes_id = 0;
+                       folder_schedule_store_changes_job (folder);
                }
 
-               g_clear_object (&session);
-       }
+               g_mutex_unlock (&folder->priv->store_changes_lock);
 
-       g_mutex_unlock (&folder->priv->store_changes_lock);
+               g_object_unref (folder);
+       }
 
        return FALSE;
 }
@@ -235,8 +246,9 @@ folder_maybe_schedule_folder_change_store (CamelFolder *folder)
                if (interval == 0)
                        folder_schedule_store_changes_job (folder);
                else if (interval > 0)
-                       folder->priv->store_changes_id = g_timeout_add_seconds (interval,
-                               folder_schedule_store_changes_job, folder);
+                       folder->priv->store_changes_id = g_timeout_add_seconds_full (G_PRIORITY_DEFAULT, 
interval,
+                               folder_schedule_store_changes_job_cb,
+                               camel_utils_weak_ref_new (folder), (GDestroyNotify) 
camel_utils_weak_ref_free);
        }
 
        g_mutex_unlock (&folder->priv->store_changes_lock);
@@ -787,6 +799,12 @@ folder_dispose (GObject *object)
 
        folder = CAMEL_FOLDER (object);
 
+       g_mutex_lock (&folder->priv->store_changes_lock);
+       if (folder->priv->store_changes_id)
+               g_source_remove (folder->priv->store_changes_id);
+       folder->priv->store_changes_id = 0;
+       g_mutex_unlock (&folder->priv->store_changes_lock);
+
        if (folder->priv->summary) {
                camel_folder_summary_save (folder->priv->summary, NULL);
                g_clear_object (&folder->priv->summary);
@@ -799,12 +817,6 @@ folder_dispose (GObject *object)
                folder->priv->parent_store = NULL;
        }
 
-       g_mutex_lock (&folder->priv->store_changes_lock);
-       if (folder->priv->store_changes_id)
-               g_source_remove (folder->priv->store_changes_id);
-       folder->priv->store_changes_id = 0;
-       g_mutex_unlock (&folder->priv->store_changes_lock);
-
        /* Chain up to parent's dispose () method. */
        G_OBJECT_CLASS (camel_folder_parent_class)->dispose (object);
 }
diff --git a/src/camel/camel-utils.c b/src/camel/camel-utils.c
index aac22910a..98693197f 100644
--- a/src/camel/camel-utils.c
+++ b/src/camel/camel-utils.c
@@ -240,3 +240,47 @@ camel_time_value_apply (time_t src_time,
 
        return mktime (&tm);
 }
+
+/**
+ * camel_utils_weak_ref_new: (skip)
+ * @object: (nullable): a #GObject or %NULL
+ *
+ * Allocates a new #GWeakRef and calls g_weak_ref_set() with @object.
+ *
+ * Free the returned #GWeakRef with camel_utils_weak_ref_free().
+ *
+ * Returns: (transfer full): a new #GWeakRef
+ *
+ * Since: 3.40
+ **/
+GWeakRef *
+camel_utils_weak_ref_new (gpointer object)
+{
+       GWeakRef *weak_ref;
+
+       /* Based on e_weak_ref_new(). */
+
+       weak_ref = g_slice_new0 (GWeakRef);
+       g_weak_ref_init (weak_ref, object);
+
+       return weak_ref;
+}
+
+/**
+ * camel_utils_weak_ref_free: (skip)
+ * @weak_ref: a #GWeakRef
+ *
+ * Frees a #GWeakRef created by camel_utils_weak_ref_new().
+ *
+ * Since: 3.40
+ **/
+void
+camel_utils_weak_ref_free (GWeakRef *weak_ref)
+{
+       g_return_if_fail (weak_ref != NULL);
+
+       /* Based on e_weak_ref_free(). */
+
+       g_weak_ref_clear (weak_ref);
+       g_slice_free (GWeakRef, weak_ref);
+}
diff --git a/src/camel/camel-utils.h b/src/camel/camel-utils.h
index c256057e9..8fde25095 100644
--- a/src/camel/camel-utils.h
+++ b/src/camel/camel-utils.h
@@ -41,6 +41,9 @@ time_t                camel_time_value_apply          (time_t src_time,
                                                 CamelTimeUnit unit,
                                                 gint value);
 
+GWeakRef *     camel_utils_weak_ref_new        (gpointer object);
+void           camel_utils_weak_ref_free       (GWeakRef *weak_ref);
+
 G_END_DECLS
 
 #endif /* CAMEL_UTILS_H */
diff --git a/src/camel/providers/imapx/camel-imapx-server.c b/src/camel/providers/imapx/camel-imapx-server.c
index cb646fbcf..63a26f70d 100644
--- a/src/camel/providers/imapx/camel-imapx-server.c
+++ b/src/camel/providers/imapx/camel-imapx-server.c
@@ -380,32 +380,6 @@ fetch_changes_info_free (gpointer ptr)
        }
 }
 
-static GWeakRef *
-imapx_weak_ref_new (gpointer object)
-{
-       GWeakRef *weak_ref;
-
-       /* XXX Might want to expose this in Camel's public API if it
-        *     proves useful elsewhere.  Based on e_weak_ref_new(). */
-
-       weak_ref = g_slice_new0 (GWeakRef);
-       g_weak_ref_init (weak_ref, object);
-
-       return weak_ref;
-}
-
-static void
-imapx_weak_ref_free (GWeakRef *weak_ref)
-{
-       g_return_if_fail (weak_ref != NULL);
-
-       /* XXX Might want to expose this in Camel's public API if it
-        *     proves useful elsewhere.  Based on e_weak_ref_free(). */
-
-       g_weak_ref_clear (weak_ref);
-       g_slice_free (GWeakRef, weak_ref);
-}
-
 static const CamelIMAPXUntaggedRespHandlerDesc *
 replace_untagged_descriptor (GHashTable *untagged_handlers,
                              const gchar *key,
@@ -700,8 +674,8 @@ imapx_server_reset_inactivity_timer (CamelIMAPXServer *is)
        g_source_set_callback (
                is->priv->inactivity_timeout,
                imapx_server_inactivity_timeout_cb,
-               imapx_weak_ref_new (is),
-               (GDestroyNotify) imapx_weak_ref_free);
+               camel_utils_weak_ref_new (is),
+               (GDestroyNotify) camel_utils_weak_ref_free);
        g_source_attach (is->priv->inactivity_timeout, NULL);
 
        g_mutex_unlock (&is->priv->inactivity_timeout_lock);
@@ -7199,7 +7173,7 @@ camel_imapx_server_schedule_idle_sync (CamelIMAPXServer *is,
        is->priv->idle_pending = g_timeout_source_new_seconds (IMAPX_IDLE_WAIT_SECONDS);
        g_source_set_callback (
                is->priv->idle_pending, imapx_server_run_idle_thread_cb,
-               imapx_weak_ref_new (is), (GDestroyNotify) imapx_weak_ref_free);
+               camel_utils_weak_ref_new (is), (GDestroyNotify) camel_utils_weak_ref_free);
        g_source_attach (is->priv->idle_pending, NULL);
 
        g_mutex_unlock (&is->priv->idle_lock);