180001 - imap inbox READ message move works but leaves message in inbox discovered on next get messages

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Richard Trout]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-GB; rv:1.0.1) Gecko/20020823 Netscape/7.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-GB; rv:1.0.1) Gecko/20020823 Netscape/7.0

Any READ messages in Inbox, after moving them to Local Folders, return on next
'Get Messages'.

Other details:
Reproduces on Netscape 6.2 / 7.0; Mozilla 1.0; 1.1
Server: Imail 7 by Ipswitch
Client: Windows 2000
Delete Message = Move to Trash
Tested with existing and new (empty) IMAP accounts

Reproducible: Always

Steps to Reproduce:
1. Read IMAP Inbox message
2. Move 1 or more read messages to Local Folder - message/s disappears from Inbox
3. 'Get Messages'

Actual Results:  
Read message/s returns to Inbox. Netscape 7 identifies as 'new messages' and
sounds 'new message' alert.

Expected Results:  
Expected moved message to no longer be in inbox.

If anyone has suggestions for other IMAP accounts we could connect with to test,
please advise details so we can confirm whether this is server dependant.

Comment 1

[Dan Star]

I don't think this is server dependent.  It does not seem to happen with 
CommunigatePro aka CPro (www.stalker.com).  I tested with a www.zipmail.com 
account which runs CPro IMAP server.

Comment 2

[Dan Star]

I forgot to state that I did verify the problem against an iMail v7.13 IMAP 
server.

Comment 3

[David :Bienvenu]

First of all, could you try this on a newer version of Mozilla? Either 1.2 or
1.3? Second, what imap delete model are you using? delete to trash, or mark deleted?

Dan, I don't quite understand your statement - if the problem doesn't happen
with another server, that would imply that the problem *is* server dependent.

Could someone who can reproduce this generate a client-side imap protocol log by
following these instructions, and post it here, or send it to me? thx. - david

http://www.mozilla.org/quality/mailnews/mail-troubleshoot.html#imap

Comment 4

[Richard Trout]

Attachment: Netscape 7 log of sample problem - move of 3 messages

Netscape 7.0; Win 2000 SP2; Imail Server 7

This log records:
login
read of 3 messages:
1. Read message, move to local folder (test)
2. Get messages, message from 1. reappears
3. Move message from 1. again.
4. Get messages, message from 1. reappears (again.)
5. Delete message from 1.
6. Repeated 1-5 for another message, same result.
7. Repeated 1, 2, 5 for another message, same result.

Comment 5

[Richard Trout]

bienvenu - am downloading 1.2 beta and will test with this. If problem continues
will go looking for 1.3 (I presume by 1.3 you mean the latest nightly build?).

Comment 6

[David :Bienvenu]

Richard, yeah, 1.3 is what the nightly builds are.  Please let me know if this
happens with 1.2b or 1.3 - thx.

Comment 7

[Dan Star]

Ah...Was I in a smeared quantum state?  I DID mean to say SERVER DEPENDENT.  
iMail has never worked correctly as far as IMAP goes with NS or Moz.  I guess 
everyone only focuses on the Outlook market.  The pessimistic side: MS 
intentionally makes its IMAP client a little non-standard; IMAP server vendors 
program to MS client > corrupt IMAP standard!

Comment 8

[Richard Trout]

Ok, I have tested the same bug with Moz 1.2b and Moz 1.3 (20021114) and the same
problem is still reproduced. I do tend to agree with Dan about Imail being a
factor, although I will note I approached Imail support about this and was quite
insistent about the problem so they put me through to their 'netscape' guru, but
he said he couldn't reproduce the problem. There is no problem move/reappear
problem with Outlook Express.

Bienvenu - you asked about delete model - I am using Delete to Trash (see
original description).

Comment 9

[Dan Star]

"Can't reproduce the problem".  That is a line of Bull S@3$!  We already have 
what 3-4 people who have verified the problem with iMail 7.13.

Comment 10

[David :Bienvenu]

That log has a lot more going on than just the steps outlined so I'm a having a
bit of trouble matching the steps to the log. A lot of folders are selected, for
example, and no messages selected in the folder. However, we don't seem to be
re-downloading the hdrs for the deleted messages again, so I suspect that they
are deleted on the server and something else odd is going on in the client. One
thing that stands out is that the UIDs returned by this imap server are
negative, which is allowed, of course, but I wonder if it's causing us some
strange problems in some parts of the code. Is it possible for anyone to give me
access to an account on an Imail server to try this out?

Comment 11

[David :Bienvenu]

I looked a bit more at the log, and I do see us downloading the header again for
a deleted message, but the odd thing is that we download it again before we
issue the delete to the server for the message in question. It's as if we've
deleted it from the local msg hdr db long before we get around to deleting it on
the server, and in the meantime, check for new mail fires, or the user does a
get new mail, and we redownload the header. 

I've downloaded the demo version of the server, and I didn't see this problem,
but   I'll keep trying. I don't think the negative UID's are an issue at this point.

Comment 12

[Richard Trout]

bienvenu - sorry about the log, you are right it does have a lot more in it - my
Inbox had other mail at the time. I can redo it if you prefer.

Yes, we have a test account: [e-mail] on mail.edex.com.au - email me
at [e-mail] for a password.

Comment 13

[David :Bienvenu]

I am seeing a problem like this, and it happens because the ipswitch server
seems to be returning a trailing null byte in the message fetch, which causes us
to throw up our hands and drop the connection. I don't know if that is because
of the way I appended the messages to the inbox. That doesn't look like what
happened in the log Richard attached, but I'll investigate.

Comment 14

[David :Bienvenu]

It looks to me like we have a bug in our imap parser - if the server doesn't
send a permanentflags response, we don't think any flags are permanent.
According to the Imap4 rev1 RFC, we're supposed to assume all flags are
permanent, unless we get a permanentflags response that lists a different set of
flags than the permanent flags. So, what's happening is that we don't think the
deleted flag is permanent, so we're not bothering to set it. Ouch. So, I need to
play a little with our parser. I guess the ipswitch server is the only one out
there that doesn't send a permanentflags response (which is perfectly
acceptable, according to my reading of the imap rfc.)

Comment 15

[David :Bienvenu]

Attachment: proposed fix

fix is to pay attention to the flags response, and just override the settable
flags when we get permanent flags. Also, use PRUint32 instead of PRInt32
because uids are unsigned ints.

Comment 16

[David :Bienvenu]

Cavin, can I get a review? thx.

Comment 17

[Richard Trout]

Ummm, I know this is a little late, but would this bug be any way related to the
fact that when I change message read flags on the IMAP inbox (on Ipswitch Imail
server) they don't stick? This is something I had been living with for a long
while and hadn't thought much about it.

Comment 18

[David :Bienvenu]

Richard, that's the same basic problem and this fix will fix that too. If I'd
have known it was all flags, I might have figured it out sooner (but maybe not :-) )

Comment 19

[Cavin Song]

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

r=cavin. Good fix.

Comment 20

[David :Bienvenu]

Attachment: better fix, with some extra cleanup

This fix makes it so we will set the user-defineable flags as well, if FLAGS
happens to return them (iMail does not), and makes it so we ignore the FLAGS
response if it comes after PERMANENTFLAGS. I've also changed some PR_FREEIF's
to PR_Free, and used PRPackedBools for space savings.

Comment 21

[David :Bienvenu]

Cavin, can you review the second patch? Seth will review this afternoon, so this
fix should be in Monday's build at the latest.

Comment 22

[Cavin Song]

Comment on attachment 107158 [details] [diff] [review]
better fix, with some extra cleanup

r=cavin.

Comment 23

[(not reading, please use [e-mail] instead)]

Comment on attachment 107158 [details] [diff] [review]
better fix, with some extra cleanup

1)

+  PRUint32 startSequence = -1;

curSequenceEnd and startSequence have moved from PRInt32 to PRUint32.

instead of doing -1, should we do PR_UINT32_MAX? (see
mozilla\nsprpub\pr\include\prtypes.h)

2)  nit

+    if (*fNextToken == '(') fNextToken++;

do:

+    if (*fNextToken == '(') 
+      fNextToken++;

3)  nit

+    if (!PL_strncasecmp(fNextToken, "$MDNSent", 8))
+    {
+      fSupportsUserDefinedFlags |= kImapMsgSupportMDNSentFlag;
+    }

why have the extra { and } ?

Comment 24

[David :Bienvenu]

fix checked in. Richard, please let me know if the next build (Monday's) works
for you. Thx!

Comment 25

[Aleksey Nogin]

What went in is *not* what was reviewed!

Reviewed:

+  PRUint32 startSequence = -1;
+  ...

Went in:

+  PRUint32 startSequence; // no need to init; we won't use it unless numKeys > 0
+  if (numKeys > 0)                                                              
+    startSequence = keys[0];                                                    
+  PRUint32 curSequenceEnd = startSequence;                                      

The "no need to init" is wrong - this checkin have added a warning brad TBox:

+mailnews/imap/src/nsImapMailFolder.cpp:1859
+ `PRUint32 startSequence' might be used uninitialized in this function

(and according to bug 179819 such warning ought to be considered compilation
errors) and indeed, the startSequence is used right away to initialize
curSequenceEnd, no matter what the value of numKeys is.

Comment 26

[David :Bienvenu]

we don't re-open bugs for warnings. We've been through that before. And if you
read the code, you'll see that no value is used if numKeys is not > 0. So try
reading the code again, stepping through it carefully in your mind. We'll never
enter the while loop if numKeys is not > 0, and we'll just return NS_OK. 

Comment 27

[Aleksey Nogin]

> we don't re-open bugs for warnings

But you do for compilation errors! Per bug 179819 "might be used uninitialized"
warnings ought to be compilation errors.

> And if you
> read the code, you'll see that no value is used if numKeys is not > 0

The problem is - if in some future revision of the code the value ever becomes
truly used uninitialized, it would be now be much harder to fugure out since the
warning is already there and further breaking the code would not generate a new one.

IMHO these warnings point to evil and hard to debug bugs sufficiently often and
deserve not to be ignored even in these "trivial" cases.

Comment 28

[David :Bienvenu]

It's not a compilation error.  Obviously, if the fix didn't compile, then it
wouldn't work and the bug wouldn't be fixed. Also, note the phrase "might be
used". It's not used. And a smarter compiler would realize that. I'm happy to
fix it, but please don't re-open bugs. A comment or e-mail is sufficient. It
makes people who care if the bug is fixed think the bug isn't fixed, and could
delay testing.

Comment 29

[Richard Trout]

FWIW, I confirm that the nightly build for Windows 2002112607 works just great
when moving READ messages, and also is working great with manual message READ
flag changes, and the Ipswitch Imail server. I'll keep using this build for a
while to verify the functionality.

Thanks heaps everyone!

Comment 30

[David :Bienvenu]

great, and thx for your help, Richard.

Comment 31

[Dan Star]

I confirmed the fix as well.  Thanks a bunch!  Long live Open Source!