From bbbded81df5d91bc80a0cc2b40e0281a8e884188 Mon Sep 17 00:00:00 2001 From: Michael James Gratton Date: Mon, 4 Dec 2017 11:00:25 +1100 Subject: [PATCH] Work around ongoing crashes in SoupCache when loading avatars. This ensures that only a single request for a resource is made at once, to work around crashes in SoupCache when multiple requests for the same resource. Hopefully fixes Bug 778720 once and for all. * src/client/conversation-viewer/conversation-list-box.vala (ConversationListBox): Add new AvatarStore class and internal AvatarLoader class to manage avatar loads for the conversation. Construct an instance of the store in the constructor and pass it to ConversationEmail::start_loading so its messages can use it for loading their sender avatars. Update call sites. * src/client/conversation-viewer/conversation-message.vala (ConversationMessage::load_avatar): Add AvatarLoader param for loading avatars, use that rather than making the Soup calls directly. Update call sites. --- src/client/application/geary-controller.vala | 2 + .../conversation-email.vala | 8 +- .../conversation-list-box.vala | 119 +++++++++++++++++- .../conversation-message.vala | 59 +++------ .../conversation-viewer.vala | 7 +- 5 files changed, 138 insertions(+), 57 deletions(-) diff --git a/src/client/application/geary-controller.vala b/src/client/application/geary-controller.vala index 08fd3269..8f2fa506 100644 --- a/src/client/application/geary-controller.vala +++ b/src/client/application/geary-controller.vala @@ -1465,6 +1465,8 @@ public class GearyController : Geary.BaseObject { viewer.load_conversation.begin( Geary.Collection.get_first(selected), this.current_folder, + this.application.config, + this.avatar_session, (obj, ret) => { try { viewer.load_conversation.end(ret); diff --git a/src/client/conversation-viewer/conversation-email.vala b/src/client/conversation-viewer/conversation-email.vala index 7a69a3cf..45f1a6c2 100644 --- a/src/client/conversation-viewer/conversation-email.vala +++ b/src/client/conversation-viewer/conversation-email.vala @@ -503,16 +503,14 @@ public class ConversationEmail : Gtk.Box { * primary message and any attached messages, as well as * attachment names, types and icons. */ - public async void start_loading(Cancellable load_cancelled) { + public async void start_loading(ConversationListBox.AvatarStore avatars, + Cancellable load_cancelled) { foreach (ConversationMessage view in this) { if (load_cancelled.is_cancelled()) { break; } yield view.load_message_body(load_cancelled); - view.load_avatar.begin( - GearyApplication.instance.controller.avatar_session, - load_cancelled - ); + view.load_avatar.begin(avatars, load_cancelled); } // Only load attachments once the web views have finished diff --git a/src/client/conversation-viewer/conversation-list-box.vala b/src/client/conversation-viewer/conversation-list-box.vala index 9dc7caf5..48f1d741 100644 --- a/src/client/conversation-viewer/conversation-list-box.vala +++ b/src/client/conversation-viewer/conversation-list-box.vala @@ -220,6 +220,106 @@ public class ConversationListBox : Gtk.ListBox { } + + /** + * Email address avatar loader and cache. + */ + public class AvatarStore { + + + private Soup.Session session; + private Gee.Map loaders = + new Gee.HashMap(); + + + internal AvatarStore(Soup.Session session) { + this.session = session; + } + + internal async Gdk.Pixbuf? load(Geary.RFC822.MailboxAddress address, + int pixel_size, + Cancellable load_cancelled) + throws Error { + string key = address.to_string(); + AvatarLoader loader = this.loaders.get(key); + if (loader == null) { + // Haven't started loading the avatar, so do it now + loader = new AvatarLoader(address, pixel_size); + this.loaders.set(key, loader); + yield loader.load(this.session, load_cancelled); + } else { + // Load has already started, so wait for it to finish + yield loader.lock.wait_async(); + } + return loader.avatar; + } + + } + + + // Initiates and manages an avatar load + private class AvatarLoader : Geary.BaseObject { + + + internal Gdk.Pixbuf? avatar = null; + internal Geary.Nonblocking.Semaphore lock = + new Geary.Nonblocking.Semaphore(); + + private Geary.RFC822.MailboxAddress address; + private int pixel_size; + + + internal AvatarLoader(Geary.RFC822.MailboxAddress address, + int pixel_size) { + this.address = address; + this.pixel_size = pixel_size; + } + + internal async void load(Soup.Session session, + Cancellable load_cancelled) + throws Error { + Soup.Message message = new Soup.Message( + "GET", + Gravatar.get_image_uri( + this.address, + Gravatar.Default.NOT_FOUND, + this.pixel_size + ) + ); + + Error? workaround_err = null; + try { + // We want to just pass load_cancelled to send_async + // here, but per Bug 778720 this is causing some + // crashy race in libsoup's cache implementation, so + // for now just let the load go through and manually + // check to see if the load has been cancelled before + // setting the avatar + InputStream data = yield session.send_async( + message, + null // should be 'load_cancelled' + ); + if (message.status_code == 200 && + data != null && + !load_cancelled.is_cancelled()) { + this.avatar = yield new Gdk.Pixbuf.from_stream_at_scale_async( + data, pixel_size, pixel_size, true, load_cancelled + ); + } + } catch (Error err) { + workaround_err = err; + } + + this.lock.blind_notify(); + + if (workaround_err != null) { + throw workaround_err; + } + } + + } + + static construct { // Set up custom keybindings unowned Gtk.BindingSet bindings = Gtk.BindingSet.by_class( @@ -290,7 +390,10 @@ public class ConversationListBox : Gtk.ListBox { // Contacts for the account this conversation exists in private Geary.ContactStore contact_store; - // Contacts for the account this conversation exists in + // Avatars for this conversation + private AvatarStore avatar_store; + + // Account this conversation belongs to private Geary.AccountInformation account_info; // Was this conversation loaded from the drafts folder? @@ -306,7 +409,7 @@ public class ConversationListBox : Gtk.ListBox { private ConversationEmail? body_selected_view = null; // Maps displayed emails to their corresponding rows. - private Gee.HashMap email_rows = + private Gee.Map email_rows = new Gee.HashMap(); // The id of the draft referred to by the current composer. @@ -389,11 +492,13 @@ public class ConversationListBox : Gtk.ListBox { Geary.AccountInformation account_info, bool is_draft_folder, Configuration config, + Soup.Session avatar_session, Gtk.Adjustment adjustment) { this.conversation = conversation; this.location = location; this.email_store = email_store; this.contact_store = contact_store; + this.avatar_store = new AvatarStore(avatar_session); this.account_info = account_info; this.is_draft_folder = is_draft_folder; this.config = config; @@ -476,7 +581,9 @@ public class ConversationListBox : Gtk.ListBox { // Start the first expanded row loading before any others, // scroll the view to it when its done - yield first_expanded_row.view.start_loading(this.cancellable); + yield first_expanded_row.view.start_loading( + this.avatar_store, this.cancellable + ); first_expanded_row.should_scroll.connect(scroll_to); first_expanded_row.enable_should_scroll(); @@ -485,7 +592,9 @@ public class ConversationListBox : Gtk.ListBox { if (!this.cancellable.is_cancelled()) { EmailRow? row = child as EmailRow; if (row != null && row != first_expanded_row) { - row.view.start_loading.begin(this.cancellable); + row.view.start_loading.begin( + this.avatar_store, this.cancellable + ); } } }); @@ -742,7 +851,7 @@ public class ConversationListBox : Gtk.ListBox { if (!this.cancellable.is_cancelled()) { EmailRow row = add_email(full_email); update_first_last_row(); - yield row.view.start_loading(this.cancellable); + yield row.view.start_loading(this.avatar_store, this.cancellable); } } diff --git a/src/client/conversation-viewer/conversation-message.vala b/src/client/conversation-viewer/conversation-message.vala index fd16bc75..4d4b196a 100644 --- a/src/client/conversation-viewer/conversation-message.vala +++ b/src/client/conversation-viewer/conversation-message.vala @@ -263,7 +263,7 @@ public class ConversationMessage : Gtk.Grid { /** - * Constructs a new view to display an RFC 823 message headers and body. + * Constructs a new view to display an RFC 822 message headers and body. * * This method sets up most of the user interface for displaying * the message, but does not attempt any possibly long-running @@ -427,35 +427,26 @@ public class ConversationMessage : Gtk.Grid { /** * Starts loading the avatar for the message's sender. */ - public async void load_avatar(Soup.Session session, Cancellable load_cancelled) { + public async void load_avatar(ConversationListBox.AvatarStore loader, + Cancellable load_cancelled) { Geary.RFC822.MailboxAddress? primary = message.get_primary_originator(); if (primary != null) { int window_scale = get_scale_factor(); - int pixel_size = this.avatar.get_pixel_size(); - Soup.Message message = new Soup.Message( - "GET", - Gravatar.get_image_uri( - primary, Gravatar.Default.NOT_FOUND, pixel_size * window_scale - ) - ); - + int pixel_size = this.avatar.get_pixel_size() * window_scale; try { - // We want to just pass load_cancelled to send_async - // here, but per Bug 778720 this is causing some - // crashy race in libsoup's cache implementation, so - // for now just let the load go through and manually - // check to see if the load has been cancelled before - // setting the avatar - InputStream data = yield session.send_async( - message, - null // should be 'load_cancelled' + Gdk.Pixbuf? avatar_buf = yield loader.load( + primary, pixel_size, load_cancelled ); - if (!load_cancelled.is_cancelled() && - data != null && message.status_code == 200) { - yield set_avatar(data, load_cancelled); + if (avatar_buf != null) { + this.avatar.set_from_surface( + Gdk.cairo_surface_create_from_pixbuf( + avatar_buf, window_scale, get_window() + ) + ); } } catch (Error err) { - debug("Error loading Gravatar response: %s", err.message); + debug("Avatar load failed for %s: %s", + primary.to_string(), err.message); } } } @@ -648,28 +639,6 @@ public class ConversationMessage : Gtk.Grid { } } - private async void set_avatar(InputStream data, - Cancellable load_cancelled) - throws Error { - Gdk.Pixbuf avatar_buf = - yield new Gdk.Pixbuf.from_stream_async(data, load_cancelled); - - if (avatar_buf != null && !load_cancelled.is_cancelled()) { - int window_scale = get_scale_factor(); - int avatar_size = this.avatar.pixel_size * window_scale; - if (avatar_buf.width != avatar_size) { - avatar_buf = avatar_buf.scale_simple( - avatar_size, avatar_size, Gdk.InterpType.BILINEAR - ); - } - this.avatar.set_from_surface( - Gdk.cairo_surface_create_from_pixbuf( - avatar_buf, window_scale, get_window() - ) - ); - } - } - // This delegate is called from within Geary.RFC822.Message.get_body while assembling the plain // or HTML document when a non-text MIME part is encountered within a multipart/mixed container. // If this returns null, the MIME part is dropped from the final returned document; otherwise, diff --git a/src/client/conversation-viewer/conversation-viewer.vala b/src/client/conversation-viewer/conversation-viewer.vala index b8361a93..387197bd 100644 --- a/src/client/conversation-viewer/conversation-viewer.vala +++ b/src/client/conversation-viewer/conversation-viewer.vala @@ -183,7 +183,9 @@ public class ConversationViewer : Gtk.Stack { * Shows a conversation in the viewer. */ public async void load_conversation(Geary.App.Conversation conversation, - Geary.Folder location) + Geary.Folder location, + Configuration config, + Soup.Session avatar_session) throws Error { remove_current_list(); @@ -195,7 +197,8 @@ public class ConversationViewer : Gtk.Stack { account.get_contact_store(), account.information, location.special_folder_type == Geary.SpecialFolderType.DRAFTS, - ((MainWindow) get_ancestor(typeof(MainWindow))).application.config, + config, + avatar_session, this.conversation_scroller.get_vadjustment() );