182678 - IMAP flags are lost if Mozilla crashes or connection is lost

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Ere Maijala (slow)]

At least when using UW-IMAP the changes to flags in INBOX (at least) are lost if
Mozilla crashes or the connection breaks. 

The problem often shows when filters are used to move incoming messages to other
folders. Messages are received to INBOX, copied to another folder and flagged
deleted. After a while the connection breaks or Mozilla crashes. Upon restarting
the messages are back, and the filters move them again causing duplicates.

David, would it be possible to enable the code in nsImapProtocol.cpp around line
2250 to do Check()? That would commit the changes so they wouldn't be lost in
any case. As far as I could see the code has been there almost since the
beginning of time but never enabled. I tested enabling Check() and it helped.

There are some other bugs about messages reappearing etc. Could be related to this.

Comment 1

[Ere Maijala (slow)]

Attachment: Simple patch to enable Check() after flag change

Comment 2

[David :Bienvenu]

It's not very polite to issue a check to the server so frequently - it adds to
server load because it makes the server flush state to disk, which you can
imagine is not free.

Comment 3

[Ere Maijala (slow)]

Ok. I just found the pref mail.imap.noop_check_count. This might solve the
problem for me, but would it be a good idea to set the default to a non-zero
value or do Check() instead of Noop() at certain intervals?

Comment 4

[David :Bienvenu]

yeah, we could change the default - what do you propose? maybe 5? We could also
do something a little tricky and examine the check for new mail interval and if
it's reasonably large (say, 30 minutes), always issue a check instead of a noop,
if the pref is NOT set.

Comment 5

[David :Bienvenu]

an other approach is to keep track of how many flag changes we've made in the
current session and issue a check after a certain number of changes, or a
certain time interval has gone by since the last check. The noop->check fix only
helps people who have check for new mail turned on, but it was dead simple to
implement...

Comment 6

[Ere Maijala (slow)]

Both of them have problems. 
With the counted solution, I start up Mozilla and get three new messages. After
one hour the connection is lost and the irritation is huge if check is done only
after, say, 10 flags. 
With the timed solution, I start up Mozilla and receive 70 new messages. It
crashes when I read the first message, causing the 70 messages to be duplicated. 

How about combination of both? We could Check() 
A. when a certain number of flags is reached 
OR 
B. instead of Noop() when checking for mail and flags have been set

If new mail check is off, we can't do the timed Check() anyway, as that might
cause notification of new messages (RECENT), right? Then we could just rely on
the counted approach, which is still much better than the current situation. How
does this sound? 

Comment 7

[David :Bienvenu]

yes, that sounds reasonable. The other thing to remember is that we should count
reading messages as a flag change, since that implicitly marks a message read on
the server. Or we could count flag changes when we're notified about them by the
server, but I think it's easier to count them when we make the changes.

Comment 8

[Ere Maijala (slow)]

I'll take this to myself and try crafting something if nobody minds. 

Any suggestions for a reasonable number of changes between checks?

Comment 9

[David :Bienvenu]

I'm not sure - maybe 5 or 10 flag changes? I think we should also check the
elapsed time since the last check, so if I just change one flag, and then 10
minutes later, I change another flag, we'd still do a check. So the logic would
be something like (If > 5 flags changed || time elapsed since last check > 10
minutes) do a check.

I think it's OK to do a timed check - we can get unsolicited responses about new
messages for any command we execute in the selected state.

Comment 10

[Ere Maijala (slow)]

But if we do timed checks outside of biff, it's effectively the same as doing
biff (if there are flag changes). Other commands are caused by user actions and
then the responses of new messages are OK, but if a timed check happens 10
minutes after user activity, the user might not be happy. I get tens of
non-urgent messages a day on one account and don't want to check them too often
as it would distract me from my work. Therefore I still think it would be better
to do check during biff or after altering flags. What do you say? Anyway, I can
do it either way.

Comment 11

[Ere Maijala (slow)]

Attachment: diff -uw of the way I was thinking

This is how I was thinking it would work best.

And if there is only a single paragraph here, Mozilla or Bugzilla doesn't think
I need many.

Comment 12

[David :Bienvenu]

I'm not saying do a timed check without there being a user action. I'm saying,
if the user does an action (like reading a message, or changing another flag),
then we can do a check based on the time elapsed as well as how many flags have
changed. Does that make sense?

Comment 13

[Ere Maijala (slow)]

Comment on attachment 107869 [details] [diff] [review]
diff -uw of the way I was thinking

Yes, that sounds good.

Comment 14

[Ere Maijala (slow)]

Attachment: v1 Patch (diff -uw)

This patch makes it so that a check is done after a flag is changed when 10
flags have been changed or it's at least 10 minutes since the last check.

Comment 15

[David :Bienvenu]

I think that looks reasonable. One nit:

+  return (deltaInSeconds >= kMaxSecondsBeforeCheck) ? PR_TRUE : PR_FALSE;

return (deltaInSeconds >= kMaxSecondsBeforeCheck);

If I'm reading this correctly, you'll Check potentially either when a flag is
changed, or a message is fetched? The latter is important. There are other imap
operations, but they're not really worth worrying about in terms of causing a check.

Comment 16

[David :Bienvenu]

Comment on attachment 108104 [details] [diff] [review]
v1 Patch (diff -uw)

r=bienvenu

Comment 17

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

Comment on attachment 108104 [details] [diff] [review]
v1 Patch (diff -uw)

sr=sspitzer

Comment 18

[Ere Maijala (slow)]

Yes, message fetches count also. I'll check in with corrected return line.

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 108104 [details] [diff] [review]
v1 Patch (diff -uw)

a=asa (on behalf of drivers) for checkin to 1.3a.

Comment 20

[Ere Maijala (slow)]

Fix checked in.