Commit 9991f16b authored by Milan Crha's avatar Milan Crha

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
parent f7a5788c
......@@ -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);
}
......
......@@ -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);
}
......@@ -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 */
......@@ -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);
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment