[geary/wip/778720-soup-avatar-crash] Work around ongoing crashes in SoupCache when loading avatars.
- From: Michael Gratton <mjog src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [geary/wip/778720-soup-avatar-crash] Work around ongoing crashes in SoupCache when loading avatars.
- Date: Mon, 4 Dec 2017 00:00:55 +0000 (UTC)
commit bbbded81df5d91bc80a0cc2b40e0281a8e884188
Author: Michael James Gratton <mike vee net>
Date: Mon Dec 4 11:00:25 2017 +1100
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-viewer/conversation-email.vala | 8 +-
.../conversation-viewer/conversation-list-box.vala | 119 +++++++++++++++++++-
.../conversation-viewer/conversation-message.vala | 59 +++--------
.../conversation-viewer/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 08fd326..8f2fa50 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 7a69a3c..45f1a6c 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 9dc7caf..48f1d74 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<string,AvatarLoader> loaders =
+ new Gee.HashMap<string,AvatarLoader>();
+
+
+ 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<Geary.EmailIdentifier,EmailRow> email_rows =
+ private Gee.Map<Geary.EmailIdentifier,EmailRow> email_rows =
new Gee.HashMap<Geary.EmailIdentifier,EmailRow>();
// 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 fd16bc7..4d4b196 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 b8361a9..387197b 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()
);
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
