Merge branch 'mjog/misc-criticals' into 'mainline'

Misc criticals

Closes #802, #865, #875, and #886

See merge request GNOME/geary!525

src/client/conversation-list/conversation-list-view.vala

@@ -541,12 +541,15 @@ public class ConversationListView : Gtk.TreeView, Geary.BaseInterface {
     public void select_conversations(Gee.Collection<Geary.App.Conversation> new_selection) {
         if (this.selected.size != new_selection.size ||
             !this.selected.contains_all(new_selection)) {
-            Gtk.TreeSelection selection = get_selection();
+            var selection = get_selection();
             selection.unselect_all();
-            foreach (var conversation in new_selection) {
-                Gtk.TreePath path = get_model().get_path_for_conversation(conversation);
-                if (path != null) {
-                    selection.select_path(path);
+            var model = get_model();
+            if (model != null) {
+                foreach (var conversation in new_selection) {
+                    var path = model.get_path_for_conversation(conversation);
+                    if (path != null) {
+                        selection.select_path(path);
+                    }
                 }
             }
         }

src/engine/imap-db/imap-db-message-row.vala

@@ -104,13 +104,7 @@ private class Geary.ImapDB.MessageRow {
         Geary.Email email = new Geary.Email(id);
 
         if (fields.is_all_set(Geary.Email.Field.DATE)) {
-            try {
-                email.set_send_date(
-                    !String.is_empty(date) ? new RFC822.Date.from_rfc822_string(date) : null
-                );
-            } catch (GLib.Error err) {
-                debug("Error loading message date from db: %s", err.message);
-            }
+            email.set_send_date(unflatten_date(date));
         }
 
         if (fields.is_all_set(Geary.Email.Field.ORIGINATORS)) {
@@ -127,9 +121,10 @@ private class Geary.ImapDB.MessageRow {
 
         if (fields.is_all_set(Geary.Email.Field.REFERENCES)) {
             email.set_full_references(
-                (message_id != null) ? new RFC822.MessageID(message_id) : null,
-                (in_reply_to != null) ? new RFC822.MessageIDList.from_rfc822_string(in_reply_to) : null,
-                (references != null) ? new RFC822.MessageIDList.from_rfc822_string(references) : null);
+                unflatten_message_id(message_id),
+                unflatten_message_id_list(in_reply_to),
+                unflatten_message_id_list(references)
+            );
         }
 
         if (fields.is_all_set(Geary.Email.Field.SUBJECT))
@@ -274,22 +269,79 @@ private class Geary.ImapDB.MessageRow {
         return (addrs == null || addrs.size == 0) ? null : addrs.to_rfc822_string();
     }
 
-    private RFC822.MailboxAddress? unflatten_address(string? str)
-        throws RFC822.Error {
-        return (
-            String.is_empty_or_whitespace(str)
-            ? null
-            : new RFC822.MailboxAddress.from_rfc822_string(str)
-        );
+    private RFC822.Date? unflatten_date(string? str) {
+        RFC822.Date? date = null;
+        if (!String.is_empty_or_whitespace(str)) {
+            try {
+                date = new RFC822.Date.from_rfc822_string(str);
+            } catch (RFC822.Error err) {
+                // There's not much we can do here aside from logging
+                // the error, since a lot of email just contain
+                // invalid addresses
+                debug("Invalid RFC822 date \"%s\": %s", str, err.message);
+            }
+        }
+        return date;
     }
 
-    private RFC822.MailboxAddresses? unflatten_addresses(string? str)
-        throws RFC822.Error {
-        return (
-            String.is_empty_or_whitespace(str)
-            ? null
-            : new RFC822.MailboxAddresses.from_rfc822_string(str)
-        );
+    private RFC822.MailboxAddress? unflatten_address(string? str) {
+        RFC822.MailboxAddress? address = null;
+        if (!String.is_empty_or_whitespace(str)) {
+            try {
+                address = new RFC822.MailboxAddress.from_rfc822_string(str);
+            } catch (RFC822.Error err) {
+                // There's not much we can do here aside from logging
+                // the error, since a lot of email just contain
+                // invalid addresses
+                debug("Invalid RFC822 mailbox address \"%s\": %s", str, err.message);
+            }
+        }
+        return address;
+    }
+
+    private RFC822.MailboxAddresses? unflatten_addresses(string? str) {
+        RFC822.MailboxAddresses? addresses = null;
+        if (!String.is_empty_or_whitespace(str)) {
+            try {
+                addresses = new RFC822.MailboxAddresses.from_rfc822_string(str);
+            } catch (RFC822.Error err) {
+                // There's not much we can do here aside from logging
+                // the error, since a lot of email just contain
+                // invalid addresses
+                debug("Invalid RFC822 mailbox addresses \"%s\": %s", str, err.message);
+            }
+        }
+        return addresses;
+    }
+
+    private RFC822.MessageID? unflatten_message_id(string? str) {
+        RFC822.MessageID? id = null;
+        if (!String.is_empty_or_whitespace(str)) {
+            try {
+                id = new RFC822.MessageID.from_rfc822_string(str);
+            } catch (RFC822.Error err) {
+                // There's not much we can do here aside from logging
+                // the error, since a lot of email just contain
+                // invalid addresses
+                debug("Invalid RFC822 message id \"%s\": %s", str, err.message);
+            }
+        }
+        return id;
+    }
+
+    private RFC822.MessageIDList? unflatten_message_id_list(string? str) {
+        RFC822.MessageIDList? ids = null;
+        if (!String.is_empty_or_whitespace(str)) {
+            try {
+                ids = new RFC822.MessageIDList.from_rfc822_string(str);
+            } catch (RFC822.Error err) {
+                // There's not much we can do here aside from logging
+                // the error, since a lot of email just contain
+                // invalid addresses
+                debug("Invalid RFC822 message id \"%s\": %s", str, err.message);
+            }
+        }
+        return ids;
     }
 
 }

src/engine/imap-engine/imap-engine-account-synchronizer.vala

@@ -121,7 +121,7 @@ private class Geary.ImapEngine.RefreshFolderSync : FolderOperation {
     }
 
     ~RefreshFolderSync() {
-        Geary.Folder? folder = this.folder;
+        weak Geary.Folder? folder = this.folder;
         if (folder != null) {
             this.folder.closed.disconnect(on_folder_close);
         }

src/engine/imap/api/imap-folder-session.vala

@@ -1,9 +1,9 @@
 /*
- * Copyright 2016 Software Freedom Conservancy Inc.
- * Copyright 2018 Michael Gratton <mike@vee.net>.
+ * Copyright © 2016 Software Freedom Conservancy Inc.
+ * Copyright © 2018, 2020 Michael Gratton <mike@vee.net>.
  *
  * This software is licensed under the GNU Lesser General Public License
- * (version 2.1 or later).  See the COPYING file in this distribution.
+ * (version 2.1 or later). See the COPYING file in this distribution.
  */
 
 
@@ -873,6 +873,10 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject {
         if (header_specifiers != null) {
             // Header fields are case insensitive, so use a
             // case-insensitive map.
+            //
+            // XXX this is bogus because it doesn't take into the
+            // presence of multiple headers. It's not common, but it's
+            // possible for there to be two To headers, for example
             Gee.Map<string,string> headers = new Gee.HashMap<string,string>(
                 String.stri_hash, String.stri_equal
             );
@@ -882,7 +886,7 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject {
                 if (fetched_headers != null) {
                     RFC822.Header parsed_headers = new RFC822.Header(fetched_headers);
                     foreach (string name in parsed_headers.get_header_names()) {
-                        headers.set(name, parsed_headers.get_header(name));
+                        headers.set(name, parsed_headers.get_raw_header(name));
                     }
                 } else {
                     warning(
@@ -896,60 +900,34 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject {
             }
 
             // DATE
-            if (required_but_not_set(Geary.Email.Field.DATE, required_fields, email)) {
-                string? value = headers.get("Date");
-                RFC822.Date? date = null;
-                if (!String.is_empty(value)) {
-                    try {
-                        date = new RFC822.Date.from_rfc822_string(value);
-                    } catch (GLib.Error err) {
-                        warning(
-                            "Error parsing date from FETCH response: %s",
-                            err.message
-                        );
-                    }
+            if (required_but_not_set(DATE, required_fields, email)) {
+                RFC822.Date? date = unflatten_date(headers.get("Date"));
+                if (date != null) {
+                    email.set_send_date(date);
                 }
-                email.set_send_date(date);
             }
 
             // ORIGINATORS
-            if (required_but_not_set(Geary.Email.Field.ORIGINATORS, required_fields, email)) {
-                RFC822.MailboxAddresses? from = null;
-                string? value = headers.get("From");
-                if (!String.is_empty(value))
-                    from = new RFC822.MailboxAddresses.from_rfc822_string(value);
-
-                RFC822.MailboxAddress? sender = null;
-                value = headers.get("Sender");
-                if (!String.is_empty(value))
-                    sender = new RFC822.MailboxAddress.from_rfc822_string(value);
-
-                RFC822.MailboxAddresses? reply_to = null;
-                value = headers.get("Reply-To");
-                if (!String.is_empty(value))
-                    reply_to = new RFC822.MailboxAddresses.from_rfc822_string(value);
-
-                email.set_originators(from, sender, reply_to);
+            if (required_but_not_set(ORIGINATORS, required_fields, email)) {
+                // Allow sender to be a list (contra to the RFC), but
+                // only take the first from it
+                RFC822.MailboxAddresses? sender = unflatten_addresses(
+                    headers.get("Sender")
+                );
+                email.set_originators(
+                    unflatten_addresses(headers.get("From")),
+                    (sender != null && !sender.is_empty) ? sender.get(0) : null,
+                    unflatten_addresses(headers.get("Reply-To"))
+                );
             }
 
             // RECEIVERS
-            if (required_but_not_set(Geary.Email.Field.RECEIVERS, required_fields, email)) {
-                RFC822.MailboxAddresses? to = null;
-                string? value = headers.get("To");
-                if (!String.is_empty(value))
-                    to = new RFC822.MailboxAddresses.from_rfc822_string(value);
-
-                RFC822.MailboxAddresses? cc = null;
-                value = headers.get("Cc");
-                if (!String.is_empty(value))
-                    cc = new RFC822.MailboxAddresses.from_rfc822_string(value);
-
-                RFC822.MailboxAddresses? bcc = null;
-                value = headers.get("Bcc");
-                if (!String.is_empty(value))
-                    bcc = new RFC822.MailboxAddresses.from_rfc822_string(value);
-
-                email.set_receivers(to, cc, bcc);
+            if (required_but_not_set(RECEIVERS, required_fields, email)) {
+                email.set_receivers(
+                    unflatten_addresses(headers.get("To")),
+                    unflatten_addresses(headers.get("Cc")),
+                    unflatten_addresses(headers.get("Bcc"))
+                );
             }
 
             // REFERENCES
@@ -957,21 +935,17 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject {
             // References header will be present if REFERENCES were required, which is why
             // REFERENCES is set at the bottom of the method, when all information has been gathered
             if (message_id == null) {
-                string? value = headers.get("Message-ID");
-                if (!String.is_empty(value))
-                    message_id = new RFC822.MessageID(value);
+                message_id = unflatten_message_id(headers.get("Message-ID"));
             }
-
             if (in_reply_to == null) {
-                string? value = headers.get("In-Reply-To");
-                if (!String.is_empty(value))
-                    in_reply_to = new RFC822.MessageIDList.from_rfc822_string(value);
+                in_reply_to = unflatten_message_id_list(
+                    headers.get("In-Reply-To")
+                );
             }
-
             if (references == null) {
-                string? value = headers.get("References");
-                if (!String.is_empty(value))
-                    references = new RFC822.MessageIDList.from_rfc822_string(value);
+                references = unflatten_message_id_list(
+                    headers.get("References")
+                );
             }
 
             // SUBJECT
@@ -993,13 +967,20 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject {
         // if preview was requested, get it now ... both identifiers
         // must be supplied if one is
         if (preview_specifier != null || preview_charset_specifier != null) {
-            assert(preview_specifier != null && preview_charset_specifier != null);
+            Memory.Buffer? preview_headers = fetched_data.body_data_map.get(
+                preview_charset_specifier
+            );
+            Memory.Buffer? preview_body = fetched_data.body_data_map.get(
+                preview_specifier
+            );
 
-            if (fetched_data.body_data_map.has_key(preview_specifier)
-                && fetched_data.body_data_map.has_key(preview_charset_specifier)) {
-                email.set_message_preview(new RFC822.PreviewText.with_header(
-                    fetched_data.body_data_map.get(preview_charset_specifier),
-                    fetched_data.body_data_map.get(preview_specifier)));
+            if (preview_headers != null && preview_headers.size > 0 &&
+                preview_body != null && preview_body.size > 0) {
+                email.set_message_preview(
+                    new RFC822.PreviewText.with_header(
+                        preview_headers, preview_body
+                    )
+                );
             } else {
                 warning("No preview specifiers \"%s\" and \"%s\" found",
                     preview_specifier.to_string(), preview_charset_specifier.to_string());
@@ -1154,4 +1135,64 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject {
         return users_fields.require(check) ? !email.fields.is_all_set(check) : false;
     }
 
+    private RFC822.Date? unflatten_date(string? str) {
+        RFC822.Date? date = null;
+        if (!String.is_empty_or_whitespace(str)) {
+            try {
+                date = new RFC822.Date.from_rfc822_string(str);
+            } catch (RFC822.Error err) {
+                // There's not much we can do here aside from logging
+                // the error, since a lot of email just contain
+                // invalid addresses
+                debug("Invalid RFC822 date \"%s\": %s", str, err.message);
+            }
+        }
+        return date;
+    }
+
+    private RFC822.MailboxAddresses? unflatten_addresses(string? str) {
+        RFC822.MailboxAddresses? addresses = null;
+        if (!String.is_empty_or_whitespace(str)) {
+            try {
+                addresses = new RFC822.MailboxAddresses.from_rfc822_string(str);
+            } catch (RFC822.Error err) {
+                // There's not much we can do here aside from logging
+                // the error, since a lot of email just contain
+                // invalid addresses
+                debug("Invalid RFC822 mailbox addresses \"%s\": %s", str, err.message);
+            }
+        }
+        return addresses;
+    }
+
+    private RFC822.MessageID? unflatten_message_id(string? str) {
+        RFC822.MessageID? id = null;
+        if (!String.is_empty_or_whitespace(str)) {
+            try {
+                id = new RFC822.MessageID.from_rfc822_string(str);
+            } catch (RFC822.Error err) {
+                // There's not much we can do here aside from logging
+                // the error, since a lot of email just contain
+                // invalid addresses
+                debug("Invalid RFC822 message id \"%s\": %s", str, err.message);
+            }
+        }
+        return id;
+    }
+
+    private RFC822.MessageIDList? unflatten_message_id_list(string? str) {
+        RFC822.MessageIDList? ids = null;
+        if (!String.is_empty_or_whitespace(str)) {
+            try {
+                ids = new RFC822.MessageIDList.from_rfc822_string(str);
+            } catch (RFC822.Error err) {
+                // There's not much we can do here aside from logging
+                // the error, since a lot of email just contain
+                // invalid addresses
+                debug("Invalid RFC822 message id \"%s\": %s", str, err.message);
+            }
+        }
+        return ids;
+    }
+
 }

src/engine/rfc822/rfc822-mailbox-addresses.vala

@@ -83,7 +83,10 @@ public class Geary.RFC822.MailboxAddresses :
 
     public MailboxAddresses.from_rfc822_string(string rfc822)
         throws Error {
-        var list = GMime.InternetAddressList.parse(null, rfc822);
+        var list = GMime.InternetAddressList.parse(
+            Geary.RFC822.get_parser_options(),
+            rfc822
+        );
         if (list == null) {
             throw new Error.INVALID("Not a RFC822 mailbox address list");
         }

src/engine/rfc822/rfc822-message-data.vala

@@ -285,7 +285,7 @@ public class Geary.RFC822.Subject :
     }
 
     public Subject.from_rfc822_string(string rfc822) {
-        base(GMime.utils_header_decode_text(get_parser_options(), rfc822));
+        base(GMime.utils_header_decode_text(get_parser_options(), rfc822).strip());
         this.rfc822 = rfc822;
     }
 
@@ -391,6 +391,15 @@ public class Geary.RFC822.Header :
         return value;
     }
 
+    public string? get_raw_header(string name) {
+        string? value = null;
+        var header = this.message.get_header_list().get_header(name);
+        if (header != null) {
+            value = header.get_raw_value();
+        }
+        return value;
+    }
+
     public string[] get_header_names() {
         if (this.names == null) {
             GMime.HeaderList headers = this.message.get_header_list();

src/engine/rfc822/rfc822.vala

@@ -24,7 +24,9 @@ namespace Geary.RFC822 {
      */
     public const string ASCII_CHARSET = "US-ASCII";
 
-    internal Regex? invalid_filename_character_re = null;
+    internal GMime.ParserOptions gmime_parser_options;
+
+    internal Regex? invalid_filename_character_re;
 
     private int init_count = 0;
 
@@ -34,7 +36,12 @@ namespace Geary.RFC822 {
             return;
 
         GMime.init();
-        GMime.ParserOptions.get_default().set_allow_addresses_without_domain(true);
+
+        gmime_parser_options = GMime.ParserOptions.get_default();
+        gmime_parser_options.set_allow_addresses_without_domain(true);
+        gmime_parser_options.set_address_compliance_mode(LOOSE);
+        gmime_parser_options.set_parameter_compliance_mode(LOOSE);
+        gmime_parser_options.set_rfc2047_compliance_mode(LOOSE);
 
         try {
             invalid_filename_character_re = new Regex("[/\\0]");
@@ -44,11 +51,11 @@ namespace Geary.RFC822 {
     }
 
     public GMime.FormatOptions get_format_options() {
-        return GMime.FormatOptions.get_default().clone();
+        return GMime.FormatOptions.get_default();
     }
 
     public GMime.ParserOptions get_parser_options() {
-        return GMime.ParserOptions.get_default().clone();
+        return Geary.RFC822.gmime_parser_options;
     }
 
     public string? get_charset() {

src/engine/util/util-logging.vala

@@ -54,6 +54,7 @@ namespace Geary.Logging {
     private GLib.Mutex writer_lock;
     private unowned FileStream? stream = null;
 
+    private GLib.LogLevelFlags set_breakpoint_on = 0;
     private Gee.Set<string> suppressed_domains;
 
 
@@ -71,6 +72,18 @@ namespace Geary.Logging {
             Logging.record_lock = GLib.Mutex();
             Logging.writer_lock = GLib.Mutex();
             Logging.max_log_length = DEFAULT_MAX_LOG_BUFFER_LENGTH;
+
+            var debug_var = GLib.Environment.get_variable("G_DEBUG");
+            if (debug_var != null) {
+                var parts = debug_var.split(",");
+                if ("fatal-warnings" in parts) {
+                    Logging.set_breakpoint_on |= GLib.LogLevelFlags.LEVEL_WARNING;
+                }
+                if ("fatal-criticals" in parts) {
+                    Logging.set_breakpoint_on |= GLib.LogLevelFlags.LEVEL_WARNING;
+                    Logging.set_breakpoint_on |= GLib.LogLevelFlags.LEVEL_CRITICAL;
+                }
+            }
         }
     }
 
@@ -299,6 +312,10 @@ namespace Geary.Logging {
             out.puts(record.format());
             out.putc('\n');
             Logging.writer_lock.unlock();
+
+            if (levels in Logging.set_breakpoint_on) {
+                GLib.breakpoint();
+            }
         }
     }
 
@@ -538,12 +555,14 @@ public interface Geary.Logging.Source : GLib.Object {
                                        va_list args) {
         Context context = Context(this.logging_domain, levels, fmt, args);
 
-        // Don't attempt to this object if it is in the middle of
-        // being destructed, which can happen when logging from
-        // the destructor.
-        Source? decorated = (this.ref_count > 0) ? this : this.logging_parent;
+        weak Source? decorated = this;
         while (decorated != null) {
-            context.append_source(decorated);
+            // Check ref counts of logged objects and don't log them
+            // if they are or have been being destroyed, which can
+            // happen when e.g. logging from the destructor.
+            if (decorated.ref_count > 0) {
+                context.append_source(decorated);
+            }
             decorated = decorated.logging_parent;
         }