From e45c63f52b1d01bb5b721905d1c68451dbb94303 Mon Sep 17 00:00:00 2001 From: Milan Crha Date: Wed, 8 Aug 2012 11:27:30 +0200 Subject: Bug #677695 - Crash on quit under emu_free_mail_cache() This is reverting previous patch for this bug and fixes it with a different approach. The previous patch had regression, instead of freezing evolution on quit it crashed it when there was pending addressbook lookups. --- libemail-engine/e-mail-utils.c | 211 ++++++++++++++++++++++------------------- libemail-engine/e-mail-utils.h | 3 +- mail/e-mail-backend.c | 2 +- 3 files changed, 116 insertions(+), 100 deletions(-) diff --git a/libemail-engine/e-mail-utils.c b/libemail-engine/e-mail-utils.c index fffe388fbe..b1308cdc3b 100644 --- a/libemail-engine/e-mail-utils.c +++ b/libemail-engine/e-mail-utils.c @@ -358,8 +358,15 @@ try_open_book_client (EBookClient *book_client, return data.result && (!error || !*error); } +extern gint camel_application_is_exiting; + #define NOT_FOUND_BOOK (GINT_TO_POINTER (1)) +/* to be able to cancel pending requests on exit; this lock + should not be held while contact_cache lock is held */ +G_LOCK_DEFINE_STATIC (search_addressbook_cancellables); +static GSList *search_addressbook_cancellables = NULL; + G_LOCK_DEFINE_STATIC (contact_cache); /* key is lowercased contact email; value is EBook pointer @@ -391,11 +398,30 @@ search_address_in_addressbooks (ESourceRegistry *registry, gchar *query; const gchar *extension_name; - if (!address || !*address) + if (!address || !*address || g_cancellable_is_cancelled (cancellable) || camel_application_is_exiting) return FALSE; + G_LOCK (search_addressbook_cancellables); + if (cancellable) + g_object_ref (cancellable); + else + cancellable = g_cancellable_new (); + search_addressbook_cancellables = g_slist_prepend (search_addressbook_cancellables, cancellable); + G_UNLOCK (search_addressbook_cancellables); + G_LOCK (contact_cache); + if (camel_application_is_exiting || g_cancellable_is_cancelled (cancellable)) { + G_UNLOCK (contact_cache); + + G_LOCK (search_addressbook_cancellables); + search_addressbook_cancellables = g_slist_remove (search_addressbook_cancellables, cancellable); + g_object_unref (cancellable); + G_UNLOCK (search_addressbook_cancellables); + + return FALSE; + } + if (emu_books_hash == NULL) { emu_books_hash = g_hash_table_new_full ( g_str_hash, g_str_equal, g_free, g_object_unref); @@ -410,11 +436,15 @@ search_address_in_addressbooks (ESourceRegistry *registry, if (ptr != NULL && (check_contact == NULL || ptr == NOT_FOUND_BOOK)) { g_free (lowercase_addr); G_UNLOCK (contact_cache); + + G_LOCK (search_addressbook_cancellables); + search_addressbook_cancellables = g_slist_remove (search_addressbook_cancellables, cancellable); + g_object_unref (cancellable); + G_UNLOCK (search_addressbook_cancellables); + return ptr != NOT_FOUND_BOOK; } - G_UNLOCK (contact_cache); - book_query = e_book_query_field_test (E_CONTACT_EMAIL, E_BOOK_QUERY_IS, address); query = e_book_query_to_string (book_query); e_book_query_unref (book_query); @@ -422,7 +452,7 @@ search_address_in_addressbooks (ESourceRegistry *registry, extension_name = E_SOURCE_EXTENSION_ADDRESS_BOOK; list = e_source_registry_list_sources (registry, extension_name); - for (link = list; link != NULL; link = g_list_next (link)) { + for (link = list; link != NULL && !g_cancellable_is_cancelled (cancellable); link = g_list_next (link)) { ESource *source = E_SOURCE (link->data); ESourceExtension *extension; const gchar *backend_name; @@ -455,7 +485,11 @@ search_address_in_addressbooks (ESourceRegistry *registry, g_list_free_full (list, (GDestroyNotify) g_object_unref); - for (link = addr_sources; !stop && !found && link != NULL; link = g_list_next (link)) { + stop = g_cancellable_is_cancelled (cancellable); + + for (link = addr_sources; !stop && !found && link != NULL + && !g_cancellable_is_cancelled (cancellable); + link = g_list_next (link)) { ESource *source = E_SOURCE (link->data); GSList *contacts; EBookClient *book_client = NULL; @@ -467,19 +501,10 @@ search_address_in_addressbooks (ESourceRegistry *registry, uid = e_source_get_uid (source); display_name = e_source_get_display_name (source); - G_LOCK (contact_cache); - - /* can be NULL on quit */ - if (!emu_books_hash || !emu_broken_books_hash) { - G_UNLOCK (contact_cache); - break; - } - /* failed to load this book last time, skip it now */ if (g_hash_table_lookup (emu_broken_books_hash, uid) != NULL) { d(printf ("%s: skipping broken book '%s'\n", G_STRFUNC, display_name)); - G_UNLOCK (contact_cache); continue; } @@ -487,8 +512,6 @@ search_address_in_addressbooks (ESourceRegistry *registry, book_client = g_hash_table_lookup (emu_books_hash, uid); if (!book_client) { - G_UNLOCK (contact_cache); - book_client = e_book_client_new (source, &err); if (book_client == NULL) { @@ -500,20 +523,9 @@ search_address_in_addressbooks (ESourceRegistry *registry, source_uid = g_strdup (uid); - G_LOCK (contact_cache); - - /* can be NULL on quit */ - if (emu_broken_books_hash) { - g_hash_table_insert ( - emu_broken_books_hash, - source_uid, source_uid); - } else { - G_UNLOCK (contact_cache); - g_clear_error (&err); - break; - } - - G_UNLOCK (contact_cache); + g_hash_table_insert ( + emu_broken_books_hash, + source_uid, source_uid); g_warning ( "%s: Unable to create addressbook '%s': %s", @@ -534,20 +546,9 @@ search_address_in_addressbooks (ESourceRegistry *registry, source_uid = g_strdup (uid); - G_LOCK (contact_cache); - - /* can be NULL on quit */ - if (emu_broken_books_hash) { - g_hash_table_insert ( - emu_broken_books_hash, - source_uid, source_uid); - } else { - G_UNLOCK (contact_cache); - g_clear_error (&err); - break; - } - - G_UNLOCK (contact_cache); + g_hash_table_insert ( + emu_broken_books_hash, + source_uid, source_uid); g_warning ( "%s: Unable to open addressbook '%s': %s", @@ -558,8 +559,6 @@ search_address_in_addressbooks (ESourceRegistry *registry, g_clear_error (&err); } } else { - G_UNLOCK (contact_cache); - cached_book = TRUE; } @@ -568,20 +567,10 @@ search_address_in_addressbooks (ESourceRegistry *registry, book_client, query, &contacts, cancellable, &err)) { if (contacts != NULL) { if (!found_any) { - G_LOCK (contact_cache); - - /* can be NULL on quit */ - if (contact_cache) { - g_hash_table_insert ( - contact_cache, - g_strdup (lowercase_addr), - book_client); - } else { - G_UNLOCK (contact_cache); - break; - } - - G_UNLOCK (contact_cache); + g_hash_table_insert ( + contact_cache, + g_strdup (lowercase_addr), + book_client); } found_any = TRUE; @@ -607,19 +596,9 @@ search_address_in_addressbooks (ESourceRegistry *registry, if (err && !stop) { gchar *source_uid = g_strdup (uid); - G_LOCK (contact_cache); - - /* can be NULL on quit */ - if (emu_broken_books_hash) { - g_hash_table_insert ( - emu_broken_books_hash, - source_uid, source_uid); - } else { - G_UNLOCK (contact_cache); - break; - } - - G_UNLOCK (contact_cache); + g_hash_table_insert ( + emu_broken_books_hash, + source_uid, source_uid); g_warning ( "%s: Can't get contacts from '%s': %s", @@ -633,18 +612,8 @@ search_address_in_addressbooks (ESourceRegistry *registry, if (stop && !cached_book && book_client) { g_object_unref (book_client); } else if (!stop && book_client && !cached_book) { - G_LOCK (contact_cache); - - /* can be NULL on quit */ - if (emu_books_hash) { - g_hash_table_insert ( - emu_books_hash, g_strdup (uid), book_client); - } else { - G_UNLOCK (contact_cache); - break; - } - - G_UNLOCK (contact_cache); + g_hash_table_insert ( + emu_books_hash, g_strdup (uid), book_client); } } @@ -652,9 +621,7 @@ search_address_in_addressbooks (ESourceRegistry *registry, g_free (query); - G_LOCK (contact_cache); - - if (!found_any && contact_cache) { + if (!found_any) { g_hash_table_insert (contact_cache, lowercase_addr, NOT_FOUND_BOOK); lowercase_addr = NULL; } @@ -663,6 +630,11 @@ search_address_in_addressbooks (ESourceRegistry *registry, g_free (lowercase_addr); + G_LOCK (search_addressbook_cancellables); + search_addressbook_cancellables = g_slist_remove (search_addressbook_cancellables, cancellable); + g_object_unref (cancellable); + G_UNLOCK (search_addressbook_cancellables); + return found_any; } @@ -759,16 +731,12 @@ em_utils_contact_photo (ESourceRegistry *registry, last = p; } - G_UNLOCK (photos_cache); - /* !p means the address had not been found in the cache */ if (!p && search_address_in_addressbooks ( registry, addr, local_only, extract_photo_data, &photo, cancellable)) { PhotoInfo *pi; - G_LOCK (photos_cache); - /* keep only up to 10 photos in memory */ if (last && (cache_len >= 10)) { pi = last->data; @@ -783,8 +751,6 @@ em_utils_contact_photo (ESourceRegistry *registry, pi->photo = photo; photos_cache = g_slist_prepend (photos_cache, pi); - - G_UNLOCK (photos_cache); } /* some photo found, use it */ @@ -803,6 +769,8 @@ em_utils_contact_photo (ESourceRegistry *registry, } } + G_UNLOCK (photos_cache); + return part; } @@ -865,11 +833,31 @@ emu_remove_from_mail_cache_1 (const gchar *address) g_slist_free (l); } -/* frees all data created by call of em_utils_in_addressbook() or - * em_utils_contact_photo() */ -void -emu_free_mail_cache (void) +struct FreeMailCacheData { + GDestroyNotify done_cb; + gpointer user_data; +}; + +static gboolean +free_mail_cache_idle (gpointer user_data) +{ + struct FreeMailCacheData *fmc = user_data; + + g_return_val_if_fail (fmc != NULL, FALSE); + + if (fmc->done_cb) + fmc->done_cb (fmc->user_data); + g_free (fmc); + + return FALSE; +} + +static gpointer +free_mail_cache_thread (gpointer user_data) +{ + g_return_val_if_fail (user_data != NULL, NULL); + G_LOCK (contact_cache); if (emu_books_hash) { @@ -896,6 +884,33 @@ emu_free_mail_cache (void) photos_cache = NULL; G_UNLOCK (photos_cache); + + g_idle_add (free_mail_cache_idle, user_data); + + return NULL; +} + +/* frees all data created by call of em_utils_in_addressbook() or + * em_utils_contact_photo(); done_cb is called when freeing is done, + * in an idle callback + */ +void +emu_free_mail_cache (GDestroyNotify done_cb, + gpointer user_data) +{ + struct FreeMailCacheData *fmc; + GThread *thread; + + G_LOCK (search_addressbook_cancellables); + g_slist_foreach (search_addressbook_cancellables, (GFunc) g_cancellable_cancel, NULL); + G_UNLOCK (search_addressbook_cancellables); + + fmc = g_new0 (struct FreeMailCacheData, 1); + fmc->done_cb = done_cb; + fmc->user_data = user_data; + + thread = g_thread_new (NULL, free_mail_cache_thread, fmc); + g_thread_unref (thread); } static ESource * diff --git a/libemail-engine/e-mail-utils.h b/libemail-engine/e-mail-utils.h index 7c993f662f..1844b49e36 100644 --- a/libemail-engine/e-mail-utils.h +++ b/libemail-engine/e-mail-utils.h @@ -64,7 +64,8 @@ ESource * em_utils_ref_mail_identity_for_store CamelStore *store); void emu_remove_from_mail_cache (const GSList *addresses); void emu_remove_from_mail_cache_1 (const gchar *address); -void emu_free_mail_cache (void); +void emu_free_mail_cache (GDestroyNotify done_cb, + gpointer user_data); void em_utils_uids_free (GPtrArray *uids); gboolean em_utils_is_local_delivery_mbox_file (CamelURL *url); diff --git a/mail/e-mail-backend.c b/mail/e-mail-backend.c index 164e5a090d..e706d5571e 100644 --- a/mail/e-mail-backend.c +++ b/mail/e-mail-backend.c @@ -270,7 +270,7 @@ mail_backend_poll_to_quit (EActivity *activity) static void mail_backend_ready_to_quit (EActivity *activity) { - emu_free_mail_cache (); + emu_free_mail_cache (g_object_unref, g_object_ref (activity)); /* Do this last. It may terminate the process. */ g_object_unref (activity); -- cgit