238680 - On a heavily filtered inbox, "mark as read" generates needlessly long IMAP commands

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Rob Vassar]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7b) Gecko/20040316
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7b) Gecko/20040316


Using recent releases of SunONE messaging server (6.0 patched), with a large
number of client (Moz only) side mail filters (30% - 50% on incoming messages
sorted to folders), I've experienced some problems marking messages as read in
my inbox. I typically have 20 to 100 unread messages in an inbox of roughly 3000
messages.

It appears the filtering/foldering is chewing large holes in the message UID
space.  When I go to mark the remain messages as read, it typically generates a
single IMAP command nearly 20K bytes in length.  The command includes large
ranges of UID's that very clearly have the "Seen" flag stored already.   

The Sun mail server currently limits IMAP commands to 8K.  I'm not sure if
there's a RFC specified limit.  Regardless, Moz appears to be getting confused
about which messages need to have the Seen flag stored, and which alreday have
it stored. Having 10 unread messages should generate a nice short flag store
with 10 UID's.

I may be able to provide server side telemetry of the IMAP transactions. I first
noticed this on Moz 1.6 MacOS X, which returned an error dialog. 1.7b MacOS X
fails silently.  The flag store not being apparent until the inbox is reselected.


Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[David :Bienvenu]

How are you marking the messages read? Select all - mark read? Or Mark all read?
I fixed this bug for fetching headers, but not for storing flags, by using the
flag state (that's the object that knows what the existing message keys in the
folder are)

Comment 2

[Rob Vassar]

I reproduce this by right-clicking my inbox in the folder panel, and selecting "mark as read".

 

Comment 3

[David :Bienvenu]

Attachment: only store /SEEN flag on messages we think are read

this makes it so we only try to store the /SEEN flag on messages we think are
unread.

Comment 4

[David :Bienvenu]

Comment on attachment 144852 [details] [diff] [review]
only store /SEEN flag on messages we think are read

also removed unused numChanged var.

Comment 5

[David :Bienvenu]

Attachment: imap part of proposed fix

This fixes the store command to both coalesce ranges using the flag state, and
to limit store command lines to 950 bytes. Along the way, I found a bug in the
code that limits the command line length in that it didn't correctly say how
many entries had been handled - this wasn't a problem for fetch headers,
usually, because headers tend to get fetched in ranges anyway, but it could be
an issue for downloading for offline use...

Comment 6

[David :Bienvenu]

Comment on attachment 144861 [details] [diff] [review]
imap part of proposed fix

The 970 byte command line length limit is somewhat arbitrary - I don't think
the RFC says anything about long lines, though I could have sworn it did.

Comment 7

[Scott MacGregor]

Comment on attachment 144861 [details] [diff] [review]
imap part of proposed fix

will this trailing semi colon cause compiler problems for unix?

 for (const char *curCharPtr = uidString; curChar && *curCharPtr;)

Should messageIdList and idString be nsCAutoStrings? or are they typically
going to be larger than 64 character strings?

Comment 8

[David :Bienvenu]

that code just moved from nsImapMailFolder.cpp to nsImapUtils.cpp so it should
compile fine. It's not actually a trailing ';' - there's just no third clause to
the for statement 

re autostrings - I thought about that, and didn't do it because the bug case has
long strings, but I should have made them auto strings because the typical case
will have short strings. I'll fix it locally.

Comment 9

[David :Bienvenu]

fixed .

Comment 10

[Scott MacGregor]

fixed on the tbird 0.6 branch