436151 - support IMAP RFC 4551 - CondStore extension

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, enhancement, P1)

Description

[David :Bienvenu]

This extension allows the imap client to only fetch changes to a folder since the last time the client accessed a folder. This would be a huge win for users with large folders and/or low bandwidth connections. This is part of the Lemonade set of features, which is geared towards mobile devices, but which benefit desktop clients as well. Cyrus and Sun's Messaging Server already support CondStore, apparently, and other servers will roll out support, I'm sure.

I'm going to see how easy this would be to add to TB, since I've got a test account on a server that supports it.

Comment 1

[David :Bienvenu]

See http://wiki.mozilla.org/Thunderbird:IMAP_RFC_4551_Implementation for more info.

Comment 2

[David :Bienvenu]

Attachment: wip

just checkpointing some work - this doesn't work yet (afaik) because my test server isn't giving me highestmodseq or modseqnums yet. But we're part of the way there...
This patch doesn't do anything about using CHANGEDSINCE fetches because of the EXPUNGE issue.

Comment 3

[David :Bienvenu]

Attachment: more wip

this gets the basic stuff working, and handles the fact that expunges don't show up in CHANGEDSINCE queries. It also adds a CHANGEDSINCE query for non imap delete model users, and remembers the highest uid an imap folder has seen, so we can do check for new messages from there...

Now I just need to test this quite a bit more.

Comment 4

[David :Bienvenu]

Attachment: more wip

this fixes some bugs I found testing with a changing inbox. It adds a per server pref to turn off condstore. It fixes our use of CHANGEDSINCE to actually work.

Due to a problem with Zimbra (which will be fixed in 5.08 - the Zimbra folks fixed it within a couple hours of the time I reported it), it's pretty clear to me that we're going to need to either turn this off by default, or turn it off for a given server if we encounter a syntax error when condstore is turned on. I'll do that tomorrow, along with fixing a problem where we're not propagating the highestmod seq we see all the way back to the db.

Comment 5

[David :Bienvenu]

Attachment: fix a few more problems.

this works around the Zimbra bug by checking for that particular error. It also propagates the mHighestModSeq back to the imap folder.

The remaining issue is how to deal with the expunge problem - if an other client deletes messages and then expunges a mailbox, those deleted messages won't show up in the CHANGEDSINCE query. To fix this, I think I need to do a sanity check on the number of messages in a mailbox.

Comment 6

[David :Bienvenu]

To elaborate, when we issue a select to the server, we get told how many messages exist in that folder  - that count includes /DELETED messages. If the user is not using the IMAP delete model, we no longer have the /DELETED messages in our db, and don't include them in our msg count for the folder.

I believe if this value doesn't change, we know that an expunge of old previously non deleted messages hasn't happened:

(server exists count - delete notifications from changedsince) - hdrs in db after sync.

So if we store that value in the db, and check it after a select, we can tell if an expunge might have happened, and if so, fall back to a full sync of flags. I do need to convince myself that if that value hasn't changed, existing messages can't have been expunged.

Alternatively, we could do a search on /DELETED messages and use that information somehow to track possible expunges.

Comment 7

[David :Bienvenu]

Attachment: track the number of deleted messages

I'm saving this for posterity - it tracks the number of deleted messages, and persists it in the db. However, if the user uses the trash model, we do implicit expunges on shutdown/re-using a cached connection, so we really don't need to track deleted messages for the trash case.

Since that complicates the code/patch somewhat, and we really mainly care about the trash model, I'm going to rip out the tracking of the deleted messages, and put in a final patch.

Comment 8

[David :Bienvenu]

Attachment: proposed fix

Sorry this patch is so large - some of it is just cleanup (e.g., nsIImapMessageSink.idl), part of it is getting rid of the IdsAreUIDs parameter which was always set to true. The rest of it has to do with implementing CONDSTORE. I've tried to add comments to make it a bit easier to understand what's going on in the code.

Comment 9

[[e-mail]]

Comment on attachment 327971 [details] [diff] [review]
proposed fix 

I don't have a server that supports CONDSTORE so I can't really test this...


>+    m_flagState->fPartialUIDFetch = PR_TRUE; // optimistic assumption
I'd prefer the pessimistic assumption ;-)

>+    if (needFullFolderSync || needFolderSync)
>     {
>       nsCString idsToFetch("1:*");
>-      FetchMessage(idsToFetch, kFlags, PR_TRUE);  // id string shows uids
>+      nsCString fetchModifier;
>+      if (!needFullFolderSync && !GetShowDeletedMessages())
>+      {
>+        fetchModifier.Assign(" (CHANGEDSINCE ");
>+        char intStrBuf[40];
>+        PR_snprintf(intStrBuf, sizeof(intStrBuf), "%llu",  mFolderLastModSeq);
>+        fetchModifier.Append(intStrBuf);
>+        fetchModifier.Append(")");
char fetchModifier[40] = "";
if (!needFullFolderSync && !GetShowDeletedMessages())
  PR_snprintf(fetchModifier, sizeof(fetchModifier), " (CHANGEDSINCE %llu)", mFolderLastModSeq);

>+        else if (lastTokenChar < '0' || lastTokenChar > '9')
>+        {
>+          // GIANT HACK
>+          // this is a corrupt uid - see if it's pre 5.08 Zimbra omitting
>+          // a space between the UID and MODSEQ
>+          if (strlen(fNextToken) > 6 && 
>+              !strcmp("MODSEQ", fNextToken + strlen(fNextToken) - 6))
>+            fNextToken += strlen(fNextToken) - 6;
>+        }
>         else
>           AdvanceToNextToken();
Does it matter what the last token char is? Why not just check for a "Q"?

>+        if (PL_strcasestr(fNextToken, ")"))
>+        {
>+          // eat token chars until we get the ')'
>+          while (*fNextToken != ')')
>+            fNextToken++;
Can't you use strchr for this?

>+     if (!PL_strcmp("CONDSTORE", fNextToken))
Aren't we supposed to use normal strcmp these days?

Comment 10

[David :Bienvenu]

Comment on attachment 327971 [details] [diff] [review]
proposed fix 

I doubt Standard8's going to have time to look at this before his vacation, but I'll ask anyway :-) I'd like to land this soon after a2 to give it a lot of time for baking before beta.

Comment 11

[David :Bienvenu]

Attachment: incorporate Neil's review comments

This incorporates Neil's comments, to some extent. I tried to avoid the "optimistic assumption" about the flag state, that it was just a partial fetch, but then I realized that we have to assume that, and maintain the setting, unless and until we do a full fetch, so I moved the setting of fPartialUIDFetch to TRUE to the constructor and only set it to false when we do a full fetch.

I left the Zimbra hack check for MODSEQ as is, since I really only want to handle that exact incorrect server response.

I also changed nsImapProtocol::UseCondStore() to also check GetServerStateParser().fUseModSeq in case the server has disabled MODSEQ for the current folder.

Comment 12

[David :Bienvenu]

Standard8, I'd love to get this in before we switch to hg :-)

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 329466 [details] [diff] [review]
incorporate Neil's review comments

>Index: imap/public/nsIImapFlagAndUidState.idl
>+
>+  // if a full update, the total number of deleted messages
>+  // in the folder; if a partial update, the number of deleted
>+  // messages in the partial update
>+  readonly attribute long numberOfDeletedMessages;
>+  /**
>+   * If this is true, instead of fetching 1:* (FLAGS), and putting all
>+   * UIDs and flags in the array, we only fetched the uids and flags
>+   * that changed since the last time we were selected on this folder.
>+   * This means we have a sparse array, and should not assume missing
>+   * UIDs have been deleted.
>+   **/
>+  readonly attribute boolean partialUIDFetch;
>   void getUidOfMessage(in long zeroBasedIndex, out unsigned long result);

nit: Please make the first set of comments the /** * */ style that I believe doxygen supports.

nit: I think you need a blank line after both of these attributes as well.

>Index: imap/public/nsIImapIncomingServer.idl

>+  attribute boolean useCondStore;

nit: Please add the RFC ref # for what this is, so that folks looking at the interface know where to look even if they don't know what it is.

>Index: imap/public/nsIMailboxSpec.idl

>+  attribute unsigned long long highestModSeq;

nit: I think this could do with a comment with what it relates to.

>Index: imap/src/nsImapFlagAndUidState.h
> class nsImapFlagAndUidState : public nsIImapFlagAndUidState
> {
> public:
>     NS_DECL_ISUPPORTS
>     nsImapFlagAndUidState(int numberOfMessages, PRUint16 flags = 0);
>     nsImapFlagAndUidState(const nsImapFlagAndUidState& state, PRUint16 flags = 0);
>     virtual ~nsImapFlagAndUidState();
>     
>-
>     NS_DECL_NSIIMAPFLAGANDUIDSTATE
> 
>-    PRInt32               GetNumberOfDeletedMessages();
>+    PRInt32               NumberOfDeletedMessages();
>     
>     imapMessageFlagsType  GetMessageFlagsFromUID(PRUint32 uid, PRBool *foundIt, PRInt32 *ndx);
>     PRBool                IsLastMessageUnseen(void);
>     
>     PRUint32              GetHighestNonDeletedUID();
>     PRUint16              GetSupportedUserFlags() { return fSupportedUserFlags; }
>+    PRBool                fPartialUIDFetch;
>+    PRInt32               fNumberDeleted;

I'm not sure I really like public member variables. I especially don't see why you've got NumberOfDeletedMessages() which returns fNumberDeleted and fNumberDeleted itself both as public.

Make both these members private and add another function for fPartialUIDFetch?

>Index: imap/src/nsImapMailFolder.cpp

> NS_IMETHODIMP
>-nsImapMailFolder::GetMessageSizeFromDB(const char * id, PRBool idIsUid, PRUint32 *size)
>+nsImapMailFolder::GetMessageSizeFromDB(const char * id, PRUint32 *size)
...
>-    NS_ASSERTION(idIsUid, "ids must be uids to get message size");
>-    if (idIsUid)
>       rv = mDatabase->GetMsgHdrForKey(key, getter_AddRefs(mailHdr));

nit: Please fix the indentation of this last line


got to nsImapProtocol::SelectMailbox, more coming later...

Comment 14

[David :Bienvenu]

I've addressed locally all the comments except for:
>nit: Please fix the indentation of this last line

which is just a -w diff artifact.

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 329466 [details] [diff] [review]
incorporate Neil's review comments

Some more whitespace comments, I think these are valid when taking into account the -w.

>@@ -3891,17 +3959,17 @@ void nsImapProtocol::FolderMsgDumpLoop(P

>-    FetchMessage(idString,  fields, PR_TRUE);                  // msg ids are uids                 
>+    FetchMessage(idString,  fields);                  // msg ids are uids                 

nit: I think this could do with some whitespace clean up (I checked the earlier non -w version as well) there's an unnecessary space between the two parameters, and there's a lot of whitespace on the end of the line.

>+    else if (!PL_strcasecmp(fNextToken, "NOMODSEQ"))
>+    {
>+      fHighestModSeq = 0;
>+      fUseModSeq = PR_FALSE;
>+      skip_to_CRLF();
>+    }      

nit: unnecessary whitespace on the end of this last line.

>+void nsImapServerResponseParser::enable_data()
>+{
>+  do
>+  {
>+    // eat each enable response;
>+     AdvanceToNextToken();
>+     if (!strcmp("CONDSTORE", fNextToken))
>+       fCondStoreEnabled = PR_TRUE;
>+  } while (fNextToken && !fAtEndOfLine && ContinueParse());
>+  
>+}
> void nsImapServerResponseParser::language_data()

nit: a blank line in between the two functions please.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 329466 [details] [diff] [review]
incorporate Neil's review comments

I haven't tested this (as my server doesn't support it).

Given it is quite new, I suggest you blog/post about it when you check it in and let folks know how to check if their server is using it and how to disable it if it is causing them problems (and report bugs ;-) ).

r=me with the other comments fixed.

Comment 17

[David :Bienvenu]

Attachment: patch for checkin

this is a diff including Standard8's comments addressed, and white space changes included.

Comment 18

[Nikolay Shopik]

David should't this marked as RESOLVED FIXED?

Comment 19

[David :Bienvenu]

Yes, thx, Nikolay - we should open new bugs for any issues arising from this feature.

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

For posterity, the commit (which accidentally had a bug number typo):
https://hg.mozilla.org/comm-central/rev/f1cac7bc0e1d