Open Bug 1077998 Opened 10 years ago Updated 2 years ago

read status of an imap message first read in a search folder stays unread, does not reliably propagate to the server. no imap store command sent after "mark as read"?

Categories

(MailNews Core :: Networking: IMAP, defect)

Product:
MailNews Core
Component:
Networking: IMAP
Platform:
x86
Windows XP
Type:
defect

Tracking

(Not tracked)

Status:
UNCONFIRMED

People

(Reporter: al_9x, Assigned: fvroman)

Details

(Whiteboard: [lame-network])

Attachments

(2 files, 4 obsolete files)

filtered_log.txt
1.58 KB, text/plain
Details
tb-imap-connectivity-patchset4.patch
6.02 KB, patch
Details | Diff | Splinter Review 
search folder: match all on inbox and sent, search online: unchecked

if a new message is first selected and read from the search folder, often/always it will become unread again after some time.
I have a long-standing problem of some read messages reappearing as "unread", and I also heavily use search folders. 

This bug might be related to CONDSTORE support. If yes, mail.server.default.use_condstore might be a workaround (see bug 594106 and bug 517461).
(In reply to martin.monperrus from comment #1)
> I have a long-standing problem of some read messages reappearing as
> "unread", and I also heavily use search folders. 
> 
> This bug might be related to CONDSTORE support. If yes,
> mail.server.default.use_condstore might be a workaround (see bug 594106 and
> bug 517461).

do you find this to help?
Flags: needinfo?(al_9x)
I was wrong. Disabling CONDSTORE does not fix the bug.
> Bug summary : read status of an imap message first read in a search folder does not reliably propagate to the server
(In reply to al_9x from comment #0)
> if a new message is first selected and read from the search folder, often/always it will become unread again after some time.

If such phenomenon of this summary, following should occur.
(a)  When you changed  status of mail of UID=AA in actual IMAP MboxA from Unread to Read, following should occur.
      At a connection where Mbox is selected, "uid store AA +Flags(\Seen)".
      Once \Seen flag is stored to the mail, all is up to server and IM clients who accesses the MboxA.
(b) When some one changed to Unread from Read, someone does do  "uid store AA -Flags(\Seen)".
      and, it' reflected to Tb's status when Tb detected "removal of \Seen flag by someone".
      It's done only by "uid fetch 1:* Flags", "uid fetch AA:* Flags",  "uid fetch AA Flags", flag change notification via IDLE etc.
      In this "detection of flag state change by other client", there is known issue. See Bug 693204.

Show Order Received column(value is UID of mail), and get IMAP log when you marked as Read, and check IMAP log file content using Text Editor by yourself. See bug 402793 comment #28 for getting IMP log.


(In reply to martin.monperrus from comment #1)
> I have a long-standing problem of some read messages reappearing as "unread", (snip)
(In reply to martin.monperrus from comment #1)
> I have a long-standing problem of some read messages reappearing as "unread", (snip)

POP3(or Local Folders)? If so, Bug 840418 occurs(Fixed in Tb 38).
IMAP? If other IMAP client changed state of the mail to Unread, it's pretty normal phenomenon.
Yes I use IMAP and use a single client (Thunderbird). I'll enable the log.
(In reply to martin.monperrus from comment #6)
> Yes I use IMAP and use a single client (Thunderbird).

Gmail IMAP? If so, phenomenon of bug 518581 occurs even when single client only. Please rule out such phenomenon.
No, it's my employer's IMAP server.
Seems related to bug 1123617.
I've activated my IMAP logs.

Usually, when I click on "mark as read", an IMAP command such as the following happens

1955800:imap.univ-lille1.fr:S-INBOX:SendData: 20 uid store 60395 +Flags (\Seen)

However, when the bug happens, it seems **there is no store command sent** after "mark as read".
It seems that those messages for which this happens all already have one single flag (NonJunk).

To be verified.
Disabled Junk detection. The bug has nothing to do with (NonJunk).  

So the reason is that there is no store command sent after "mark as read". Why?
(In reply to martin.monperrus from comment #10)
> I've activated my IMAP logs.
> However, when the bug happens, it seems **there is no store command sent**　after "mark as read".

If IMAP log is obtined and is checked, result is one of next only :
  (i)  Store +Flags(\Seen) is sent by Tb for the UID when you did "Mark as read".
  (ii) Store +Flags(\Seen) is NOT sent by Tb when you did "Mark as read".
So, "it seems" will never happen  for the UID when your problem occurs.

You did "mark as read" while "Work Offline" mode?
If so, "Read state set while Work Offline or while no server connection" may be overridden by "no \Seen flag staate of the UID" which was obtained upon next server connection.

If "Search Online" is requested for Seaarch Folder, "SEARCH UNDELETED" is issued upon Search Folder click(open).
Do you see your problem with "Search Online"?
If Unified Folder(View/Folder/Unified), "Search Online" is internally set by Tb always.
Do you see your problem with Unified Folder?
> You did "mark as read" while "Work Offline" mode?
No
> Do you see your problem with "Search Online"?
I try
(In reply to martin.monperrus from comment #15)
> > Do you see your problem with "Search Online"?
> I try
Flags: needinfo?(martin.monperrus)
> Do you see your problem with "Search Online"?
Yes.

(it took me some time to figure this out because this bug is not deterministic)
Flags: needinfo?(martin.monperrus)
(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #2)
> (In reply to martin.monperrus from comment #1)
> > I have a long-standing problem of some read messages reappearing as
> > "unread", and I also heavily use search folders. 
> > 
> > This bug might be related to CONDSTORE support. If yes,
> > mail.server.default.use_condstore might be a workaround (see bug 594106 and
> > bug 517461).
> 
> do you find this to help?

no, condstore is off
Flags: needinfo?(al_9x)
I tested with condstore=on and condstore=off and the bug happens in both cases.
By the way, the bug may happen for all flags (\Seen, \Answered, etc.)
Component: General → Networking: IMAP
Product: Thunderbird → MailNews Core
Summary: read status of an imap message first read in a search folder does not reliably propagate to the server → read status of an imap message first read in a search folder stays unread, does not reliably propagate to the server. no imap store command sent after "mark as read"?
FYI: Bug still present in TB40 beta channel.
> Your statement in Bug summary : no imap store command sent after "mark as read"?

Same question to you.
Is \Seen flag stored to the mail by Thunderbird when you marked as Read via Search Online?
1. Show Order Received column at real folder, virtual folder(saved search), Search panel etc.
   Received column value == UID of mail if IMAP.
2. Get IMAP log, and check log content using Text Editor.
   "uid Store nnn Flags(\Seen)" is issued for the mail in Mbox?
   How about tag? (if ...tag.XXX.description="tag_name", flag=XXX is stored/removed for tag_name)
I've simplified the problem (no search folder, no search online/offline, no condstore). 

This bug happens when you have a bad internet connection with intermittent disconnections.

To reproduce it, simply "mark as read" when or just after the connection disappears. 

The store request is lost (or times-out), and there is no new store request re-issued afterwards.
Whiteboard: [lame-network]
I've stopped using search folders, and the bug still happens. 

So it's probably not caused by the search folder code.
Attached file filtered_log.txt — 
hi,

I have the same kind of issue and I managed to get a log file (I am attaching a filtered version of it which gives a focus on the event).

By looking at it we can see that if you get a "close socket connection" while processing an "addmsgflags" url then the flags modification is lost forever (no retry and next status fetch does not have new flags).

My assumption is: 
* URL are queued for processing and retry should happens on error
* Error (socket connection error) during URL processing is not correctly caught and the URL leaves the queue
(In reply to fvroman from comment #25)
> Created attachment 8856845 [details]
> filtered_log.txt
> 
> hi,
> 
> I have the same kind of issue and I managed to get a log file (I am
> attaching a filtered version of it which gives a focus on the event).
> 
> By looking at it we can see that if you get a "close socket connection"
> while processing an "addmsgflags" url then the flags modification is lost
> forever (no retry and next status fetch does not have new flags).
> 
> My assumption is: 
> * URL are queued for processing and retry should happens on error
> * Error (socket connection error) during URL processing is not correctly
> caught and the URL leaves the queue

I forgot to give the Thunderbird version: 38.8
Reproductible with TB 52.0
The bug occurs when you get disconnected from the server while trying to send a Set/Add/Substract imap flags command. If the second retry fails then the commands is dropped forever and any icon associated to those flags (seen, replied, forwarded) will disappear on next folder synchronization (IMAP server returning an unmodified list of flags)

This patch allow infinite retry for Set/Add/Substract flags imap command.

There was a protection which authorizes only one retry on network failure. However, this should be relaxed for imap flags alteration commands (they are trivial commands).

This patch may solve bug 653373, bug 199433, bug 195787
Attachment #8860101 - Flags: review?(rkent)
Comment on attachment 8860101 [details] [diff] [review]
Fix Set/Add/Substract Imap Flags when dealing with unreliable connection

Review of attachment 8860101 [details] [diff] [review]:
-----------------------------------------------------------------

Drive-by style comments.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +4838,2 @@
>              {
> +                // keep trying for the most simple operations

We use indentation of two spaces and I think the cases sit right under the {

switch (foobar)
{
case 1:
  break;

Please check other occurrences in the file.
Comment on attachment 8860101 [details] [diff] [review]
Fix Set/Add/Substract Imap Flags when dealing with unreliable connection

Review of attachment 8860101 [details] [diff] [review]:
-----------------------------------------------------------------

It seems to me that allowing an infinite number of retries on a failure is very rarely the right answer, and I don't see that this case is so critical to suggest that.

What I would accept is maintaining a failure count, and allowing more retries on the simpler calls, say 3.
Attachment #8860101 - Flags: review?(rkent) → review-
(In reply to Kent James (:rkent) from comment #30)
> Comment on attachment 8860101 [details] [diff] [review]
> Fix Set/Add/Substract Imap Flags when dealing with unreliable connection
> 
> Review of attachment 8860101 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It seems to me that allowing an infinite number of retries on a failure is
> very rarely the right answer, and I don't see that this case is so critical
> to suggest that.
> 
> What I would accept is maintaining a failure count, and allowing more
> retries on the simpler calls, say 3.

I totally agree with you.

I can change my contribution with the following modifications:
* Add one preference "mail.imap.command_max_retry" set by default to 3
* When set to 0 do infinite retry 

Are you in line with this ?
Previous patchset modifications:
* Add one preference "mail.imap.command_max_retry" set by default to 3
* When set to 0 do infinite retry
Attachment #8860101 - Attachment is obsolete: true
Attachment #8864030 - Flags: review?(rkent)
Comment on attachment 8864030 [details] [diff] [review]
Patchset2 : add an option to set the maximum number of retry when dealing with unreliable connection

Review of attachment 8864030 [details] [diff] [review]:
-----------------------------------------------------------------

mailnews.js did not apply cleanly, you need to rebase the patch to the current comm-central.

I don't have any way to test this to prove that it works, but it seems harmless enough so we can accept it.

Because of the nits and the rebase issues, the patch should be looked at again, but the changes are routine and I'll accept it after they are done. Jorg can also review this prior to landing instead of me, as I am only available a day a week but he is available most days.

::: mailnews/imap/public/nsIImapUrl.idl
@@ +96,4 @@
>  
>    /// Server disconnected first time so we're retrying.
>    attribute boolean rerunningUrl;
> +  /// Count the number of time the URL has been retryed due to server disconnection.

Nits: Count the number of times the URL has been retried ...

("times" and "retried" are changed)

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +89,4 @@
>  #define IMAP_ENV_AND_DB_HEADERS IMAP_ENV_HEADERS IMAP_DB_HEADERS
>  static const PRIntervalTime kImapSleepTime = PR_MillisecondsToInterval(60000);
>  static int32_t gPromoteNoopToCheckCount = 0;
> +static int32_t gSendCmdMaxRetryCount = 3; // maximum number of retry on seding imap command error

nit: of "retries"

@@ +2007,4 @@
>        SetConnectionStatus(rv);
>        if (m_runningUrl && !m_retryUrlOnError)
>        {
> +        int32_t currentRetryCount = 0;

Nit: everywhere you call it simply retryCount except here. Is there any reason it needs the current suffix? If not, simply name the variable retryCount.

@@ +4746,3 @@
>              // don't rerun if we already were rerunning. And don't rerun
>              // online move/copies that timeout.
> +            if ((gSendCmdMaxRetryCount == 0 || retryCount < gSendCmdMaxRetryCount) && 

Nit: remove trailing space after &&

::: mailnews/mailnews.js
@@ +106,4 @@
>  // Should we filter imap messages based on new messages since the previous
>  // highest UUID seen instead of unread?
>  pref("mail.imap.filter_on_new", false);
> +// Maximum number of retry on imap command send error

Nit: "retries"
Attachment #8864030 - Flags: review?(rkent)
Attachment #8864030 - Flags: review-
Attachment #8864030 - Flags: feedback+
I will post a new patch in few days which will:
* include the changes according to your comments 
* work on current comm-central.

Thanks for the review.
Comment on attachment 8864030 [details] [diff] [review]
Patchset2 : add an option to set the maximum number of retry when dealing with unreliable connection

Review of attachment 8864030 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +2013,2 @@
>          m_runningUrl->GetRerunningUrl(&alreadyRerunningUrl);
> +        if (!alreadyRerunningUrl && currentRetryCount < gSendCmdMaxRetryCount)

I thought the value of the pref can also be zero for infinite retry. Then the condition will never be fulfilled.

@@ +4741,5 @@
>              m_runningUrl->GetImapAction(&imapAction);
> +
> +            m_runningUrl->GetRetryCount(&retryCount);
> +            ++retryCount;
> +            m_runningUrl->SetRetryCount(retryCount);

Do we ever reset the retry count for a specific URL?

IOW, if the same URL is requested again later, do we start with a fresh count at zero since it's a new "IMAP URL object".
One more time ... thanks for the reviews

I have prepared a patch file which should take into account all your comments (comment 33 & comment 35) and which can be applied on master.

However I still need more information regarding your last remark:
* I did not know that an URL object could be reused during runtime. I did not check that but I assumed that URL objects were stacked and then destroyed once successfully processed
* I would be tempted to insert a counter reset in nsImapProtocol::ImapThreadMainLoop() (mailnews/imap/src/nsImapProtocol.cpp) 
** line 1416 before or after m_runningUrl->SetRerunningUrl(false);
        if (NS_FAILED(rv) || !isAlive)
        {
          // This says we never started running the url, which is the case.
          m_runningUrl->SetRerunningUrl(false);
          RetryUrl();
          return;
        }
** OR line 1407 after
    if (readyToRun && m_runningUrl)
    {
This way the counter would be reseted whether the transport is alive or not (case were it is to be killed)

Any advice on this ?
(In reply to fvroman from comment #36)
> I have prepared a patch file which should take into account all your
> comments (comment 33 & comment 35) and which can be applied on master.
Let's see it ;-)

> However I still need more information regarding your last remark:
> * I did not know that an URL object could be reused during runtime. I did
> not check that but I assumed that URL objects were stacked and then
> destroyed once successfully processed
I'm not really an IMAP expert although I've made a lot of changes to nsImapProtocol.cpp. Reviewers sometimes ask questions without knowing the answer. In that case the assignee needs to do the research.

On this new block
  m_runningUrl->GetRetryCount(&retryCount);
  ++retryCount;
  m_runningUrl->SetRetryCount(retryCount);
you can simply print retryCount and see whether whether you always start at zero, if that code runs at all since this only runs in the "retry case" which is not the normal case. Can you force a retry?

> * I would be tempted to insert a counter reset in ...
It's better to do this in a patch or at least mark the location with a comment.
(In reply to Jorg K (GMT+2) from comment #37)
> (In reply to fvroman from comment #36)
> > I have prepared a patch file which should take into account all your
> > comments (comment 33 & comment 35) and which can be applied on master.
> Let's see it ;-)

Let's give it a try (see new attachement patchset3)

> 
> > However I still need more information regarding your last remark:
> > * I did not know that an URL object could be reused during runtime. I did
> > not check that but I assumed that URL objects were stacked and then
> > destroyed once successfully processed
> I'm not really an IMAP expert although I've made a lot of changes to
> nsImapProtocol.cpp. Reviewers sometimes ask questions without knowing the
> answer. In that case the assignee needs to do the research.

I will dig. But asking is sometimes the best (and fastest) way.

> 
> On this new block
>   m_runningUrl->GetRetryCount(&retryCount);
>   ++retryCount;
>   m_runningUrl->SetRetryCount(retryCount);
> you can simply print retryCount and see whether whether you always start at
> zero, if that code runs at all since this only runs in the "retry case"
> which is not the normal case. Can you force a retry?

Yes I can force a retry (just stop the code with debugger when trying to send data, force TCP connection close, resume the execution). This is how I did for debugging.

> 
> > * I would be tempted to insert a counter reset in ...
> It's better to do this in a patch or at least mark the location with a
> comment.

Done (insertion line 1407)
Take into account comment 33 and comment 35
Attachment #8866380 - Flags: review?
Take into account comment 33 & comment 35
+ line to reset retry counter on URL object
Attachment #8866380 - Attachment is obsolete: true
Attachment #8866380 - Flags: review?
Attachment #8866381 - Flags: review?
Comment on attachment 8866381 [details] [diff] [review]
tb-imap-connectivity-patchset3.patch

Review of attachment 8866381 [details] [diff] [review]:
-----------------------------------------------------------------

A few nits and one question.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +91,4 @@
>  #define IMAP_ENV_AND_DB_HEADERS IMAP_ENV_HEADERS IMAP_DB_HEADERS
>  static const PRIntervalTime kImapSleepTime = PR_MillisecondsToInterval(60000);
>  static int32_t gPromoteNoopToCheckCount = 0;
> +static int32_t gSendCmdMaxRetryCount = 3; // maximum number of retries on seding imap command error

Nit: "sending"

@@ +1404,4 @@
>  
>      if (readyToRun && m_runningUrl)
>      {
> +      // Reset retry counter just in case the URL object would be reused

Nit: "... is reused." (full stop at the end)

@@ +4860,2 @@
>              nsImapAction imapAction;
>              m_runningUrl->GetRerunningUrl(&rerunningUrl);

Is rerunningUrl still used?

@@ +4860,5 @@
>              nsImapAction imapAction;
>              m_runningUrl->GetRerunningUrl(&rerunningUrl);
>              m_runningUrl->GetImapAction(&imapAction);
> +
> +            m_runningUrl->GetRetryCount(&retryCount);

Nit: Move variable declaration before this line.

@@ +4867,3 @@
>              // don't rerun if we already were rerunning. And don't rerun
>              // online move/copies that timeout.
> +            if ((gSendCmdMaxRetryCount == 0 || retryCount < gSendCmdMaxRetryCount) &&

Why are you removing |!rerunningUrl| from the condition? It's still retrieved a few lines above.
(In reply to Jorg K (GMT+2) from comment #41)
> @@ +4860,2 @@
> >              nsImapAction imapAction;
> >              m_runningUrl->GetRerunningUrl(&rerunningUrl);
> 
> Is rerunningUrl still used?

No it is not and I will remove it

> 
> @@ +4867,3 @@
> >              // don't rerun if we already were rerunning. And don't rerun
> >              // online move/copies that timeout.
> > +            if ((gSendCmdMaxRetryCount == 0 || retryCount < gSendCmdMaxRetryCount) &&
> 
> Why are you removing |!rerunningUrl| from the condition? It's still
> retrieved a few lines above.

I removed it because otherwise it would invalidate the retryCount check as it is set to true on first retry.
Here is the patchset 4

By the way, i confirm the ImapUrl objects are destroyed once processed with success. Anyway resetting the counter won't hurt.
Attachment #8864030 - Attachment is obsolete: true
Attachment #8866381 - Attachment is obsolete: true
Attachment #8866381 - Flags: review?
Attachment #8866690 - Flags: review?
Attachment #8866690 - Flags: review? → review?(jorgk)
(In reply to fvroman from comment #42)
> > Why are you removing |!rerunningUrl| from the condition? It's still
> > retrieved a few lines above.
> I removed it because otherwise it would invalidate the retryCount check as
> it is set to true on first retry.

I think you need to describe what we're doing here. In the first patch, "simple operations" were retried forever, where as copy/move were retried once, right? They had the original |if (!rerunningUrl ...|.

In the second patch and its later versions, everything is retried three times.

That's what you want, so basically, instead of retrying once, retry three times, right?
Comment on attachment 8866690 [details] [diff] [review]
tb-imap-connectivity-patchset4.patch

Review of attachment 8866690 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +2052,2 @@
>          m_runningUrl->GetRerunningUrl(&alreadyRerunningUrl);
> +        if (!alreadyRerunningUrl && (gSendCmdMaxRetryCount == 0 || retryCount < gSendCmdMaxRetryCount))

Why isn't |if (!alreadyRerunningUrl && ...| also conflicting with your new scheme?
Assignee: nobody → fvroman
Jorg, 

one of my users still have the issue so the patch does not seem to fully fix the issue.

I still need to investigate :(
Comment on attachment 8866690 [details] [diff] [review]
tb-imap-connectivity-patchset4.patch

Clearing review as per comment #46. There was also one question in comment #45.
Attachment #8866690 - Flags: review?(jorgk)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: