Improve send failure handling

We now no longer parse the exception message in MessagingController to find
out if it was a permanent SMTP failure.
Overall the code is still a mess, but the error handling should be a little
bit better and more readable now.

k9mail-library/src/main/java/com/fsck/k9/mail/transport/SmtpTransport.java

@@ -491,7 +491,6 @@ public class SmtpTransport extends Transport {
 
     private void sendMessageTo(List<String> addresses, Message message)
     throws MessagingException {
-        boolean possibleSend = false;
 
         close();
         open();
@@ -503,13 +502,11 @@ public class SmtpTransport extends Transport {
         // the size of messages, count the message's size before sending it
         if (mLargestAcceptableMessage > 0 && message.hasAttachments()) {
             if (message.calculateSize() > mLargestAcceptableMessage) {
-                MessagingException me = new MessagingException("Message too large for server");
+                throw new MessagingException("Message too large for server", true);
-                //TODO this looks rather suspicious... shouldn't it be true?
-                me.setPermanentFailure(possibleSend);
-                throw me;
             }
         }
 
+        boolean entireMessageSent = false;
         Address[] from = message.getFrom();
         try {
             executeSimpleCommand("MAIL FROM:" + "<" + from[0].getAddress() + ">"
@@ -527,20 +524,14 @@ public class SmtpTransport extends Transport {
             // We use BufferedOutputStream. So make sure to call flush() !
             msgOut.flush();
 
-            possibleSend = true; // After the "\r\n." is attempted, we may have sent the message
+            entireMessageSent = true; // After the "\r\n." is attempted, we may have sent the message
             executeSimpleCommand("\r\n.");
+        } catch (NegativeSmtpReplyException e) {
+            throw e;
         } catch (Exception e) {
             MessagingException me = new MessagingException("Unable to send message", e);
+            me.setPermanentFailure(entireMessageSent);
 
-            // "5xx text" -responses are permanent failures
-            String msg = e.getMessage();
-            if (msg != null && msg.startsWith("5")) {
-                Log.w(LOG_TAG, "handling 5xx SMTP error code as a permanent failure");
-                possibleSend = false;
-            }
-
-            //TODO this looks rather suspicious... why is possibleSend used, and why are 5xx NOT permanent (in contrast to the log text)
-            me.setPermanentFailure(possibleSend);
             throw me;
         } finally {
             close();
@@ -775,11 +766,15 @@ public class SmtpTransport extends Transport {
         private final String mReplyText;
 
         public NegativeSmtpReplyException(int replyCode, String replyText) {
-            super("Negative SMTP reply: " + replyCode + " " + replyText);
+            super("Negative SMTP reply: " + replyCode + " " + replyText, isPermanentSmtpError(replyCode));
             mReplyCode = replyCode;
             mReplyText = replyText;
         }
 
+        private static boolean isPermanentSmtpError(int replyCode) {
+            return replyCode >= 500 && replyCode <= 599;
+        }
+
         public int getReplyCode() {
             return mReplyCode;
         }

k9mail/src/main/java/com/fsck/k9/controller/MessagingController.java

@@ -3367,6 +3367,7 @@ public class MessagingController implements Runnable {
     public void sendPendingMessagesSynchronous(final Account account) {
         Folder localFolder = null;
         Exception lastFailure = null;
+        boolean wasPermanentFailure = false;
         try {
             Store localStore = account.getLocalStore();
             localFolder = localStore.getFolder(
@@ -3463,38 +3464,40 @@ public class MessagingController implements Runnable {
                             processPendingCommands(account);
                         }
 
-                    } catch (Exception e) {
+                    } catch (CertificateValidationException e) {
-                        // 5.x.x errors from the SMTP server are "PERMFAIL"
+                        lastFailure = e;
-                        // move the message over to drafts rather than leaving it in the outbox
+                        wasPermanentFailure = false;
-                        // This is a complete hack, but is worlds better than the previous
-                        // "don't even bother" functionality
-                        if (getRootCauseMessage(e).startsWith("5")) {
-                            localFolder.moveMessages(Collections.singletonList(message), (LocalFolder) localStore.getFolder(account.getDraftsFolderName()));
-                        }
 
                         notifyUserIfCertificateProblem(context, e, account, false);
-                        addErrorMessage(account, "Failed to send message", e);
+                        handleSendFailure(account, localStore, localFolder, message, e, wasPermanentFailure);
-                        message.setFlag(Flag.X_SEND_FAILED, true);
+                    } catch (MessagingException e) {
-                        Log.e(K9.LOG_TAG, "Failed to send message", e);
-                        for (MessagingListener l : getListeners()) {
-                            l.synchronizeMailboxFailed(account, localFolder.getName(), getRootCauseMessage(e));
-                        }
                         lastFailure = e;
+                        wasPermanentFailure = e.isPermanentFailure();
+
+                        handleSendFailure(account, localStore, localFolder, message, e, wasPermanentFailure);
+                    } catch (Exception e) {
+                        lastFailure = e;
+                        wasPermanentFailure = true;
+
+                        handleSendFailure(account, localStore, localFolder, message, e, wasPermanentFailure);
                     }
                 } catch (Exception e) {
-                    Log.e(K9.LOG_TAG, "Failed to fetch message for sending", e);
-                    for (MessagingListener l : getListeners()) {
-                        l.synchronizeMailboxFailed(account, localFolder.getName(), getRootCauseMessage(e));
-                    }
-                    addErrorMessage(account, "Failed to fetch message for sending", e);
                     lastFailure = e;
+                    wasPermanentFailure = false;
+
+                    Log.e(K9.LOG_TAG, "Failed to fetch message for sending", e);
+
+                    addErrorMessage(account, "Failed to fetch message for sending", e);
+                    notifySynchronizeMailboxFailed(account, localFolder, e);
                 }
             }
+
             for (MessagingListener l : getListeners()) {
                 l.sendPendingMessagesCompleted(account);
             }
+
             if (lastFailure != null) {
-                if (getRootCauseMessage(lastFailure).startsWith("5")) {
+                if (wasPermanentFailure) {
                     notifySendPermFailed(account, lastFailure);
                 } else {
                     notifySendTempFailed(account, lastFailure);
@@ -3517,6 +3520,35 @@ public class MessagingController implements Runnable {
         }
     }
 
+    private void handleSendFailure(Account account, Store localStore, Folder localFolder, Message message,
+            Exception exception, boolean permanentFailure) throws MessagingException {
+
+        Log.e(K9.LOG_TAG, "Failed to send message", exception);
+
+        if (permanentFailure) {
+            moveMessageToDraftsFolder(account, localFolder, localStore, message);
+        }
+
+        addErrorMessage(account, "Failed to send message", exception);
+        message.setFlag(Flag.X_SEND_FAILED, true);
+
+        notifySynchronizeMailboxFailed(account, localFolder, exception);
+    }
+
+    private void moveMessageToDraftsFolder(Account account, Folder localFolder, Store localStore, Message message)
+            throws MessagingException {
+        LocalFolder draftsFolder = (LocalFolder) localStore.getFolder(account.getDraftsFolderName());
+        localFolder.moveMessages(Collections.singletonList(message), draftsFolder);
+    }
+
+    private void notifySynchronizeMailboxFailed(Account account, Folder localFolder, Exception exception) {
+        String folderName = localFolder.getName();
+        String errorMessage = getRootCauseMessage(exception);
+        for (MessagingListener listener : getListeners()) {
+            listener.synchronizeMailboxFailed(account, folderName, errorMessage);
+        }
+    }
+
     public void getAccountStats(final Context context, final Account account,
             final MessagingListener listener) {