220256 - downloading imap mails when going offline fails with large numbers of non-contiguous messages

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Lee Essen]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.1; Linux)
Build Identifier: Mozilla Thunderbird 0.2 (20030911)

When downloading mails prior to going offline, with a large number of non-contiguous 
ID's the server responds with "invalid command argument". 
 
(NOTE:  suggested fix included) 

Reproducible: Always

Steps to Reproduce:
1. Fill your mailbox with several hundred messages 
2. Delete every other one 
3. Go offline and download the mail 
Actual Results:  
Message box pops up saying the command failed with "invalid command argument" 

Expected Results:  
The mail should have downloaded. 

Upon investigation it seems that the problem is caused by the routine that forms the list of 
message ids for the PEEK[] command. 
 
AllocateImapUidString (in mailnews/imap/src/nsImapUtils.cpp) 
 
If the list is very long (over 950 characters) the code exits a loop AFTER putting a trailing 
comma on the list, this violates the IMAP protocol and the server complains. 
 
My suggested patch is to move the comma-adding code after the length checking code... 
i.e. 
 
diff -uNr mozilla.orig/mailnews/imap/src/nsImapUtils.cpp 
mozilla/mailnews/imap/src/nsImapUtils.cpp 
--- mozilla.orig/mailnews/imap/src/nsImapUtils.cpp      2003-07-22 14:57:44.000000000 
+0100 
+++ mozilla/mailnews/imap/src/nsImapUtils.cpp   2003-09-25 06:58:45.000000000 +0100 
@@ -421,8 +421,8 @@ 
       returnString.AppendInt(startSequence); 
       returnString += ':'; 
       returnString.AppendInt(curSequenceEnd); 
-      if (!lastKey) 
-        returnString += ','; 
+//      if (!lastKey) 
+//        returnString += ','; 
       startSequence = nextKey; 
       curSequenceEnd = startSequence; 
       curFlagStateIndex = -1; 
@@ -432,8 +432,8 @@ 
       startSequence = nextKey; 
       curSequenceEnd = startSequence; 
       returnString.AppendInt(msgUids[keyIndex]); 
-      if (!lastKey) 
-        returnString += ','; 
+//      if (!lastKey) 
+//        returnString += ','; 
       curFlagStateIndex = -1; 
     } 
     // check if we've generated too long a string - if there's no flag state, 
@@ -444,6 +444,10 @@ 
       msgCount = total; 
       break; 
     } 
+    // LEE: If we are not the last item then we need to add the comma 
+    //      but it's important we do it here, after the length check 
+    if (!lastKey) 
+      returnString += ','; 
   } 
 }

Comment 1

[David :Bienvenu]

I'll look at this on Thursday...this got lost, sorry.

Comment 2

[David :Bienvenu]

Attachment: proposed fix

cleaned up patch

Comment 3

[David :Bienvenu]

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

sr=me. thx, Lee, I'll check this in.

Comment 4

[David :Bienvenu]

fix checked in, r/sr=me. thx, Lee.

Comment 5

[Greg Ubben]

To help others find this bug report, what you would see with a
Microsoft Exchange server when you simply try to access your mailbox,
is the following:

9 UID fetch 100,130:138,.....,3584,3588:3595, (UID RFC822.SIZE FLAGS BODY.PEEK[...])
9 BAD Protocol Error: "Invalid message set in FETCH command"

The UW imapd seems to be ok with the extra comma in the sequence set.