From b51bce6ebf6fcae9a8a5a261f252faf9c443d4f0 Mon Sep 17 00:00:00 2001 From: Daniel Applebaum <danapple@danapple.com> Date: Sat, 15 May 2010 19:35:07 +0000 Subject: [PATCH] Fixes Issue 1551 Fixes Issue 1577 Issue 1551: Some IMAP servers send untagged EXPUNGEs to IDLEing clients without ever haven't sent an untagged FETCH. The untagged EXPUNGEs are harder to deal with because they don't have a UID. So, if the user has elected to have the IDLE connection start with a poll, we can maintain a map of message sequence numbers to UIDs that we can use to figure out which message to delete. To mitigate the risk of the map falling out of date, we do a UID SEARCH UID before removing the local copy of the message, just to make sure the message is really gone from the server. If we detect an error, do another poll to resync the map. Issue 1577: Restore the removal of notifications for an account when the account's unread message count goes to 0. --- src/com/fsck/k9/MessagingController.java | 162 +++++++++++------- .../k9/MessagingControllerPushReceiver.java | 7 +- src/com/fsck/k9/activity/FolderList.java | 2 +- src/com/fsck/k9/activity/MessageList.java | 2 +- .../setup/AccountSetupCheckSettings.java | 2 +- src/com/fsck/k9/mail/PushReceiver.java | 1 + src/com/fsck/k9/mail/store/ImapStore.java | 131 ++++++++++++-- 7 files changed, 228 insertions(+), 79 deletions(-) diff --git a/src/com/fsck/k9/MessagingController.java b/src/com/fsck/k9/MessagingController.java index ea385bca4..b2fe4a873 100644 --- a/src/com/fsck/k9/MessagingController.java +++ b/src/com/fsck/k9/MessagingController.java @@ -929,7 +929,7 @@ public class MessagingController implements Runnable LocalStore localStore = account.getLocalStore(); LocalFolder localFolder = localStore.getFolder(folder); localFolder.setVisibleLimit(localFolder.getVisibleLimit() + account.getDisplayCount()); - synchronizeMailbox(account, folder, listener); + synchronizeMailbox(account, folder, listener, null); } catch (MessagingException me) { @@ -962,14 +962,15 @@ public class MessagingController implements Runnable * @param account * @param folder * @param listener + * @param providedRemoteFolder TODO */ - public void synchronizeMailbox(final Account account, final String folder, final MessagingListener listener) + public void synchronizeMailbox(final Account account, final String folder, final MessagingListener listener, final Folder providedRemoteFolder) { putBackground("synchronizeMailbox", listener, new Runnable() { public void run() { - synchronizeMailboxSynchronous(account, folder, listener); + synchronizeMailboxSynchronous(account, folder, listener, providedRemoteFolder); } }); } @@ -981,8 +982,9 @@ public class MessagingController implements Runnable * @param folder * * TODO Break this method up into smaller chunks. + * @param providedRemoteFolder TODO */ - public void synchronizeMailboxSynchronous(final Account account, final String folder, final MessagingListener listener) + private void synchronizeMailboxSynchronous(final Account account, final String folder, final MessagingListener listener, Folder providedRemoteFolder) { Folder remoteFolder = null; LocalFolder tLocalFolder = null; @@ -1052,47 +1054,54 @@ public class MessagingController implements Runnable localUidMap.put(message.getUid(), message); } - if (K9.DEBUG) - Log.v(K9.LOG_TAG, "SYNC: About to get remote store for " + folder); - - Store remoteStore = account.getRemoteStore(); - - if (K9.DEBUG) - Log.v(K9.LOG_TAG, "SYNC: About to get remote folder " + folder); - - remoteFolder = remoteStore.getFolder(folder); - - /* - * If the folder is a "special" folder we need to see if it exists - * on the remote server. It if does not exist we'll try to create it. If we - * can't create we'll abort. This will happen on every single Pop3 folder as - * designed and on Imap folders during error conditions. This allows us - * to treat Pop3 and Imap the same in this code. - */ - if (folder.equals(account.getTrashFolderName()) || - folder.equals(account.getSentFolderName()) || - folder.equals(account.getDraftsFolderName())) + if (providedRemoteFolder != null) { - if (!remoteFolder.exists()) - { - if (!remoteFolder.create(FolderType.HOLDS_MESSAGES)) - { - for (MessagingListener l : getListeners()) - { - l.synchronizeMailboxFinished(account, folder, 0, 0); - } - if (listener != null && getListeners().contains(listener) == false) - { - listener.synchronizeMailboxFinished(account, folder, 0, 0); - } - if (K9.DEBUG) - Log.i(K9.LOG_TAG, "Done synchronizing folder " + folder); + if (K9.DEBUG) + Log.v(K9.LOG_TAG, "SYNC: using providedRemoteFolder " + folder); - return; + remoteFolder = providedRemoteFolder; + } + else + { + + if (K9.DEBUG) + Log.v(K9.LOG_TAG, "SYNC: About to get remote store for " + folder); + + Store remoteStore = account.getRemoteStore(); + if (K9.DEBUG) + Log.v(K9.LOG_TAG, "SYNC: About to get remote folder " + folder); + remoteFolder = remoteStore.getFolder(folder); + + /* + * If the folder is a "special" folder we need to see if it exists + * on the remote server. It if does not exist we'll try to create it. If we + * can't create we'll abort. This will happen on every single Pop3 folder as + * designed and on Imap folders during error conditions. This allows us + * to treat Pop3 and Imap the same in this code. + */ + if (folder.equals(account.getTrashFolderName()) || + folder.equals(account.getSentFolderName()) || + folder.equals(account.getDraftsFolderName())) + { + if (!remoteFolder.exists()) + { + if (!remoteFolder.create(FolderType.HOLDS_MESSAGES)) + { + for (MessagingListener l : getListeners()) + { + l.synchronizeMailboxFinished(account, folder, 0, 0); + } + if (listener != null && getListeners().contains(listener) == false) + { + listener.synchronizeMailboxFinished(account, folder, 0, 0); + } + if (K9.DEBUG) + Log.i(K9.LOG_TAG, "Done synchronizing folder " + folder); + + return; + } } } - } - /* * Synchronization process: Open the folder @@ -1115,11 +1124,11 @@ public class MessagingController implements Runnable /* * Open the remote folder. This pre-loads certain metadata like message count. */ - if (K9.DEBUG) - Log.v(K9.LOG_TAG, "SYNC: About to open remote folder " + folder); - - remoteFolder.open(OpenMode.READ_WRITE); - + if (K9.DEBUG) + Log.v(K9.LOG_TAG, "SYNC: About to open remote folder " + folder); + + remoteFolder.open(OpenMode.READ_WRITE); + } if (Account.EXPUNGE_ON_POLL.equals(account.getExpungePolicy())) { if (K9.DEBUG) @@ -1290,7 +1299,7 @@ public class MessagingController implements Runnable } finally { - if (remoteFolder != null) + if (providedRemoteFolder == null && remoteFolder != null) { remoteFolder.close(); } @@ -1360,7 +1369,11 @@ public class MessagingController implements Runnable for (Message message : messages) { - if (isMessageSuppressed(account, folder, message) == false) + if (message.isSet(Flag.DELETED)) + { + syncFlagMessages.add(message); + } + else if (isMessageSuppressed(account, folder, message) == false) { Message localMessage = localFolder.getMessage(message.getUid()); @@ -1756,16 +1769,23 @@ public class MessagingController implements Runnable */ if (remoteFolder.supportsFetchingFlags()) { - - if (K9.DEBUG) Log.d(K9.LOG_TAG, "SYNC: About to sync flags for " + syncFlagMessages.size() + " remote messages for folder " + folder); - fp.clear(); fp.add(FetchProfile.Item.FLAGS); - remoteFolder.fetch(syncFlagMessages.toArray(new Message[0]), fp, null); + + List<Message> undeletedMessages = new LinkedList<Message>(); + for (Message message : syncFlagMessages) + { + if (message.isSet(Flag.DELETED) == false) + { + undeletedMessages.add(message); + } + } + + remoteFolder.fetch(undeletedMessages.toArray(new Message[0]), fp, null); for (Message remoteMessage : syncFlagMessages) { Message localMessage = localFolder.getMessage(remoteMessage.getUid()); @@ -4243,7 +4263,7 @@ public class MessagingController implements Runnable } try { - synchronizeMailboxSynchronous(account, folder.getName(), listener); + synchronizeMailboxSynchronous(account, folder.getName(), listener, null); } finally { @@ -4286,6 +4306,19 @@ public class MessagingController implements Runnable if (K9.DEBUG) Log.v(K9.LOG_TAG, "Clearing notification flag for " + account.getDescription()); account.setRingNotified(false); + try + { + AccountStats stats = account.getStats(context); + int unreadMessageCount = stats.unreadMessageCount; + if (unreadMessageCount == 0) + { + notifyAccountCancel(context, account); + } + } + catch (MessagingException e) + { + Log.e(K9.LOG_TAG, "Unable to getUnreadMessageCount for account: " + account, e); + } } } ); @@ -4429,6 +4462,17 @@ public class MessagingController implements Runnable */ private boolean notifyAccount(Context context, Account account, Message message) { + int unreadMessageCount = 0; + try + { + AccountStats stats = account.getStats(context); + unreadMessageCount = stats.unreadMessageCount; + } + catch (MessagingException e) + { + Log.e(K9.LOG_TAG, "Unable to getUnreadMessageCount for account: " + account, e); + } + // Do not notify if the user does not have notifications // enabled or if the message has been read if (!account.isNotifyNewMail() || message.isSet(Flag.SEEN)) @@ -4504,17 +4548,7 @@ public class MessagingController implements Runnable messageNotice.append(context.getString(R.string.notification_new_title)); } - int unreadMessageCount = 0; - try - { - AccountStats stats = account.getStats(context); - unreadMessageCount = stats.unreadMessageCount; - } - catch (MessagingException e) - { - Log.e(K9.LOG_TAG, "Unable to getUnreadMessageCount for account: " + account, e); - } - + NotificationManager notifMgr = (NotificationManager)context.getSystemService(Context.NOTIFICATION_SERVICE); Notification notif = new Notification(R.drawable.stat_notify_email_generic, messageNotice, System.currentTimeMillis()); diff --git a/src/com/fsck/k9/MessagingControllerPushReceiver.java b/src/com/fsck/k9/MessagingControllerPushReceiver.java index bd3e53681..d41f51528 100644 --- a/src/com/fsck/k9/MessagingControllerPushReceiver.java +++ b/src/com/fsck/k9/MessagingControllerPushReceiver.java @@ -69,12 +69,15 @@ public class MessagingControllerPushReceiver implements PushReceiver List<Message> messages) { controller.messagesArrived(account, folder, messages, true); - } public void messagesArrived(Folder folder, List<Message> messages) { controller.messagesArrived(account, folder, messages, false); } + public void messagesRemoved(Folder folder, List<Message> messages) + { + controller.messagesArrived(account, folder, messages, true); + } public void syncFolder(Folder folder) { @@ -96,7 +99,7 @@ public class MessagingControllerPushReceiver implements PushReceiver { latch.countDown(); } - }); + }, folder); if (K9.DEBUG) Log.v(K9.LOG_TAG, "syncFolder(" + folder.getName() + ") about to await latch release"); diff --git a/src/com/fsck/k9/activity/FolderList.java b/src/com/fsck/k9/activity/FolderList.java index c97194a10..547f223f9 100644 --- a/src/com/fsck/k9/activity/FolderList.java +++ b/src/com/fsck/k9/activity/FolderList.java @@ -202,7 +202,7 @@ public class FolderList extends K9ListActivity wakeLock.release(); } }; - MessagingController.getInstance(getApplication()).synchronizeMailbox(mAccount, folder.name, listener); + MessagingController.getInstance(getApplication()).synchronizeMailbox(mAccount, folder.name, listener, null); sendMail(mAccount); } diff --git a/src/com/fsck/k9/activity/MessageList.java b/src/com/fsck/k9/activity/MessageList.java index 025131729..8acba2c4e 100644 --- a/src/com/fsck/k9/activity/MessageList.java +++ b/src/com/fsck/k9/activity/MessageList.java @@ -1097,7 +1097,7 @@ public class MessageList private void checkMail(Account account, String folderName) { - mController.synchronizeMailbox(account, folderName, mAdapter.mListener); + mController.synchronizeMailbox(account, folderName, mAdapter.mListener, null); sendMail(account); } diff --git a/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java b/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java index 5dfd3114b..a891c1b2b 100644 --- a/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java +++ b/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java @@ -109,7 +109,7 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList store.checkSettings(); MessagingController.getInstance(getApplication()).listFolders(mAccount, true, null); - MessagingController.getInstance(getApplication()).synchronizeMailbox(mAccount, K9.INBOX , null); + MessagingController.getInstance(getApplication()).synchronizeMailbox(mAccount, K9.INBOX , null, null); } if (mDestroyed) diff --git a/src/com/fsck/k9/mail/PushReceiver.java b/src/com/fsck/k9/mail/PushReceiver.java index 113b39798..5c5d1e7e7 100644 --- a/src/com/fsck/k9/mail/PushReceiver.java +++ b/src/com/fsck/k9/mail/PushReceiver.java @@ -9,6 +9,7 @@ public interface PushReceiver public void syncFolder(Folder folder); public void messagesArrived(Folder folder, List<Message> mess); public void messagesFlagsChanged(Folder folder, List<Message> mess); + public void messagesRemoved(Folder folder, List<Message> mess); public String getPushState(String folderName); public void pushError(String errorMessage, Exception e); public void setPushActive(String folderName, boolean enabled); diff --git a/src/com/fsck/k9/mail/store/ImapStore.java b/src/com/fsck/k9/mail/store/ImapStore.java index 76c07919f..a810f7739 100644 --- a/src/com/fsck/k9/mail/store/ImapStore.java +++ b/src/com/fsck/k9/mail/store/ImapStore.java @@ -39,6 +39,7 @@ import java.security.NoSuchAlgorithmException; import java.security.SecureRandom; import java.security.Security; import java.util.*; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; @@ -448,6 +449,8 @@ public class ImapStore extends Store private OpenMode mMode; private boolean mExists; private ImapStore store = null; + Map<Integer, String> msgSeqUidMap = new ConcurrentHashMap<Integer, String>(); + public ImapFolder(ImapStore nStore, String name) { @@ -522,6 +525,7 @@ public class ImapStore extends Store // 2 OK [READ-WRITE] Select completed. try { + msgSeqUidMap.clear(); String command = String.format((mode == OpenMode.READ_WRITE ? "SELECT" : "EXAMINE") + " \"%s\"", encodeFolderName(getPrefixedName())); @@ -966,6 +970,19 @@ public class ImapStore extends Store }; return search(searcher, listener); } + + protected Message[] getMessagesFromUids(final List<String> mesgUids, final boolean includeDeleted, final MessageRetrievalListener listener) + throws MessagingException + { + ImapSearcher searcher = new ImapSearcher() + { + public List<ImapResponse> search() throws IOException, MessagingException + { + return executeSimpleCommand(String.format("UID SEARCH UID %s" + (includeDeleted ? "" : " NOT DELETED"), Utility.combine(mesgUids.toArray(), ','))); + } + }; + return search(searcher, listener); + } private Message[] search(ImapSearcher searcher, MessageRetrievalListener listener) throws MessagingException { @@ -1152,6 +1169,22 @@ public class ImapStore extends Store { ImapList fetchList = (ImapList)response.getKeyedValue("FETCH"); String uid = fetchList.getKeyedString("UID"); + int msgSeq = response.getNumber(0); + if (uid != null) + { + try + { + msgSeqUidMap.put(msgSeq, uid); + if (K9.DEBUG) + { + Log.v(K9.LOG_TAG, "Stored uid '" + uid + "' for msgSeq " + msgSeq + " into map " + msgSeqUidMap.toString()); + } + } + catch (Exception e) + { + Log.e(K9.LOG_TAG, "Unable to store uid '" + uid + "' for msgSeq " + msgSeq); + } + } Message message = messageMap.get(uid); if (message == null) @@ -1371,7 +1404,7 @@ public class ImapStore extends Store { mMessageCount--; if (K9.DEBUG) - Log.d(K9.LOG_TAG, "Got untagged EXPUNGE with value " + mMessageCount + " for " + getLogId()); + Log.d(K9.LOG_TAG, "Got untagged EXPUNGE with mMessageCount " + mMessageCount + " for " + getLogId()); } // if (response.size() > 1) { // Object bracketedObj = response.get(1); @@ -2588,8 +2621,9 @@ public class ImapStore extends Store final AtomicBoolean doneSent = new AtomicBoolean(false); final AtomicInteger delayTime = new AtomicInteger(NORMAL_DELAY_TIME); final AtomicInteger idleFailureCount = new AtomicInteger(0); + final AtomicBoolean needsPoll = new AtomicBoolean(false); List<ImapResponse> storedUntaggedResponses = new ArrayList<ImapResponse>(); - + public ImapFolderPusher(ImapStore store, String name, PushReceiver nReceiver) { super(store, name); @@ -2650,7 +2684,7 @@ public class ImapStore extends Store Log.e(K9.LOG_TAG, "Unable to get oldUidNext for " + getLogId(), e); } ImapConnection oldConnection = mConnection; - List<ImapResponse> responses = internalOpen(OpenMode.READ_ONLY); + internalOpen(OpenMode.READ_ONLY); if (mConnection == null) { receiver.pushError("Could not establish connection for IDLE", null); @@ -2664,12 +2698,11 @@ public class ImapStore extends Store throw new MessagingException("IMAP server is not IDLE capable:" + mConnection.toString()); } - if (responses != null) - { - handleUntaggedResponses(responses); - } - if (mAccount.isPushPollOnConnect() && mConnection != oldConnection) + if (mAccount.isPushPollOnConnect() && (mConnection != oldConnection || needsPoll.getAndSet(false) == true)) { + List<ImapResponse> untaggedResponses = new ArrayList<ImapResponse>(storedUntaggedResponses); + storedUntaggedResponses.clear(); + processUntaggedResponses(untaggedResponses); receiver.syncFolder(ImapFolderPusher.this); } int startUid = oldUidNext; @@ -2842,10 +2875,11 @@ public class ImapStore extends Store skipSync = true; } List<Integer> flagSyncMsgSeqs = new ArrayList<Integer>(); + List<String> removeMsgUids = new LinkedList<String>(); for (ImapResponse response : responses) { - oldMessageCount += processUntaggedResponse(oldMessageCount, response, flagSyncMsgSeqs); + oldMessageCount += processUntaggedResponse(oldMessageCount, response, flagSyncMsgSeqs, removeMsgUids); } if (skipSync == false) { @@ -2865,6 +2899,10 @@ public class ImapStore extends Store { syncMessages(flagSyncMsgSeqs); } + if (removeMsgUids.size() > 0) + { + removeMessages(removeMsgUids); + } } private void syncMessages(int end, boolean newArrivals) throws MessagingException @@ -2938,8 +2976,46 @@ public class ImapStore extends Store receiver.pushError("Exception while processing Push untagged responses", e); } } + + private void removeMessages(List<String> removeUids) + { + List<Message> messages = new ArrayList<Message>(removeUids.size()); + + try + { + Message[] existingMessages = getMessagesFromUids(removeUids, true, null); + for (Message existingMessage : existingMessages) + { + needsPoll.set(true); + msgSeqUidMap.clear(); + String existingUid = existingMessage.getUid(); + Log.w(K9.LOG_TAG, "Message with UID " + existingUid + " still exists on server, not expunging"); + removeUids.remove(existingUid); + } + for (String uid : removeUids) + { + ImapMessage message = new ImapMessage(uid, this); + try + { + message.setFlagInternal(Flag.DELETED, true); + } + catch (MessagingException me) + { + Log.e(K9.LOG_TAG, "Unable to set DELETED flag on message " + message.getUid()); + } + messages.add(message); + } + receiver.messagesRemoved(this, messages); + } + catch (Exception e) + { + Log.e(K9.LOG_TAG, "Cannot remove EXPUNGEd messages", e); + return; + } + + } - protected int processUntaggedResponse(int oldMessageCount, ImapResponse response, List<Integer> flagSyncMsgSeqs) + protected int processUntaggedResponse(int oldMessageCount, ImapResponse response, List<Integer> flagSyncMsgSeqs, List<String> removeMsgUids) { super.handleUntaggedResponse(response); int messageCountDelta = 0; @@ -2950,7 +3026,9 @@ public class ImapStore extends Store Object responseType = response.get(1); if (ImapResponseParser.equalsIgnoreCase(responseType, "FETCH")) { + Log.i(K9.LOG_TAG, "Got FETCH " + response); int msgSeq = response.getNumber(0); + if (K9.DEBUG) Log.d(K9.LOG_TAG, "Got untagged FETCH for msgseq " + msgSeq + " for " + getLogId()); @@ -2984,6 +3062,39 @@ public class ImapStore extends Store } } flagSyncMsgSeqs.addAll(newSeqs); + + + List<Integer> msgSeqs = new ArrayList<Integer>(msgSeqUidMap.keySet()); + Collections.sort(msgSeqs); // Have to do comparisons in order because of msgSeq reductions + + for (Integer msgSeqNumI : msgSeqs) + { + if (K9.DEBUG) + { + Log.v(K9.LOG_TAG, "Comparing EXPUNGEd msgSeq " + msgSeq + " to " + msgSeqNumI); + } + int msgSeqNum = msgSeqNumI; + if (msgSeqNum == msgSeq) + { + String uid = msgSeqUidMap.get(msgSeqNum); + if (K9.DEBUG) + { + Log.d(K9.LOG_TAG, "Scheduling removal of UID " + uid + " because msgSeq " + msgSeqNum + " was expunged"); + } + removeMsgUids.add(uid); + msgSeqUidMap.remove(msgSeqNum); + } + else if (msgSeqNum > msgSeq) + { + String uid = msgSeqUidMap.get(msgSeqNum); + if (K9.DEBUG) + { + Log.d(K9.LOG_TAG, "Reducing msgSeq for UID " + uid + " from " + msgSeqNum + " to " + (msgSeqNum - 1)); + } + msgSeqUidMap.remove(msgSeqNum); + msgSeqUidMap.put(msgSeqNum-1, uid); + } + } } } catch (Exception e)