840418 - replied/forwarded icons disappear after folder repair, detach/delete ("Internally set message flag" icluding "READ flag by Mark Folder Read" is not written to X-Mozilla-Status: until "manually changeable message flag" is changed manually)

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect, P1)

Description

[Serg Iv]

it's pop3 account
1. repair folder
2. result: no replied/forwarded icons in the mail list anymore

it seems like TB does not update x-mozilla-status (doesn't set MSG_FLAG_REPLIED) for replied emails since v12 (not sure about version) and replied/forwarded status stored only in msfs.

http://forums.mozillazine.org/viewtopic.php?f=39&t=2658059

related(?) info:
Mark Folder Read does not update x-mozilla-status as well (doesn't set MSG_FLAG_READ)

Comment 1

[WADA:World Anti-bad-Duping Agency]

(In reply to Serg Iv from comment #0)
> it's pop3 account
> 1. repair folder
> 2. result: no replied/forwarded icons in the mail list anymore
> it seems like TB does not update x-mozilla-status (doesn't set
> MSG_FLAG_REPLIED) for replied emails since v12 (not sure about version) and
> replied/forwarded status stored only in msfs.

I also could observe "Replied flag is not written to X-Mozilla-Staus: header of local msFolder file after reply" in Tb 17.0.2.
After "Compact", Replied flag was physically written to X-Mozilla-Staus: header.
[ X-Mozilla-Status: header]
(1) Initial : 0001 (read)
(2) Reply   : 0001 (replied icon is normally shown at thread pane)
(3) Compact : 0007 (replied flag is written)

Confirming.

See following web page for other flags in X-Mozilla-Status: and X-Mozilla-Status-2:.
> http://www.eyrich-net.org/mozilla/X-Mozilla-Status.html?en

If since Tb 12, it may be a regression by big change for "pluggable message store" enhancement.

Comment 2

[Serg Iv]

Just tried compacting but X-Mozilla-Status still the same 0001. I deleted several emails beforehand to get "real" compacting.

I tested Tb11 vs Tb12. After reply in TB11 X-Mozilla-Status=0003, in Tb12 X-Mozilla-Status=0001

Can you please comment this: Mark Folder Read does not update x-mozilla-status as well (doesn't set MSG_FLAG_READ). Is it bug or by design?

Comment 3

[WADA:World Anti-bad-Duping Agency]

(In reply to Serg Iv from comment #2)
> Just tried compacting but X-Mozilla-Status still the same 0001.
> I deleted several emails beforehand to get "real" compacting.

Which do you mean?
- You deleted some mails before Compact, but still 0001 even after Compact.
- Before delete some mails, still 0001 after Compact request,
  but after delete some mails, "Replied" flag was written to X-Mozilla-Status:.

Comment 4

[Serg Iv]

To be more specific

1. I sent two emails to my acc from another address (let's call them message A and B)
2. I replied to the message A.
3. I deleted the message B.
4. I compacted the Inbox folder.

X-Mozilla-Status for the message A is still 0001

Comment 5

[WADA:World Anti-bad-Duping Agency]

(In reply to Serg Iv from comment #4)
> To be more specific
> 1. I sent two emails to my acc from another address (let's call them message
> A and B)
> 2. I replied to the message A.
> 3. I deleted the message B.
> 4. I compacted the Inbox folder.
> X-Mozilla-Status for the message A is still 0001

I also saw it, without marking "Starred" what I accidentally set before Compact in my previous test.
When I marked "Starred" or "Unread", all flags were written to X-Mozilla-Status: header.
In my new test, I replied to mail which has Re: in subject.
(A)  0<expungedbytes of Inbox  
     Initial : 0001 => Reply : 0001 => Compact : 0001
     => mark as Starred : 0017 (HAS_RE, MARKED, REPLIED, READ)
(B)  Initial : 0001 => Reply : 0001
     => mark as Unread  : 0012 (HAS_RE, REPLIED, not-READ==Unread)
So, REPLIED flag was set by "mark as Starred" in my previous test instead of by Compact, and HAS_RE is also an example which is not automatically written to X-Mozilla-Status:.
Sorry for my confusion.

Phenomenon looks:
  "Internally set flag" is not written to X-Mozilla-Status:
  until "Manually changeable flag" is changed,
  where "Internally set flag"      : REPLIED, HAS_RE, FORWARDED, ATTACHMENT etc.
        "Manually changeable flag" : READ, MARKED etc.

(In reply to Serg Iv from comment #0)
> related(?) info:
> Mark Folder Read does not update x-mozilla-status as well (doesn't set MSG_FLAG_READ)

I think it's following.
> If mail is already marked as READ, READ flag is not updated by "Mark Folder Read".
> And, Replied mail usually has READ flag already upon Reply.

Comment 6

[Serg Iv]

> If mail is already marked as READ, READ flag is not updated by "Mark Folder Read".
> And, Replied mail usually has READ flag already upon Reply.

I meant if you instead of actual reading make "mark folder read" then X-Mozilla-Status will be 0000, not 0001. This issue is not related to replying, just another case of the same bug I think. X-Mozilla-Status is not updated when it should be.
To reproduce get some emails, instead of reading make "mark folder read", look in the Inbox file and you'll see that X-Mozilla-Status=0000. Side effect - after a folder repair procedure those marked email will be UNreaded. It's very confusing.

Comment 7

[WADA:World Anti-bad-Duping Agency]

(In reply to Serg Iv from comment #6)
> To reproduce get some emails, instead of reading make "mark folder read",
> look in the Inbox file and you'll see that X-Mozilla-Status=0000.

I could see it too, by "chanhe all mails to Unread(xxx0) manually, then Mark Folder Read".
 X-Mozilla-Status: X-Mozilla-Status-2:   After Mark Folder Read
   0000              00000000              0000  00000000
   0000              00800000              0000  00800000
   0010              00800000              0010  00800000
"Mark Folder Read" seems "internal flag change like REPLIED" for current Tb.

Comment 8

[Serg Iv]

Today my TB did some automatic maintenance work and automatically rebuilt the msf file for my Inbox. No more replied & forwarded information for me.
WADA, can we change the bug importance to "major" because it seems like TB loses important information even without specific user actions?

To be honest I managed to create simple and stupid Python script that scans the Sent files, collects all 'In-Reply-To:' info and then changes 'X-Mozilla-Status' for the corresponding emails but it's just a workaround.

Comment 10

[Göran Schmidt]

Hej everybody!

We have TB 25 now and still this bug persists! Does nobody exept for SergeIV see the major impact of dataloss in this Bug? - I didn't realize before today, that all my FORWARDED and REPLIED data since early 2013 has been lost! I do alot office-work with TB and it's crucial to know, which mails have been answered/forwarded, sometimes I need that knowledge half a year later, like this time.

So I really hope someone will start to get rid of this bug! IN THE MEANTIME I really need to get a workaround for this problem to stop losing these email connections, I think I've already lost quite enough of them... - so could anyone explain the workaround to me? Or at least the thing "about the manual changeable flag" - how could I make use of it as a workaround?

Many thanks to anyone
who'd like to help me
or who'd like to fix this bug!

Comment 11

[Serg Iv]

Here's workaround. To save the replied/forwarded status to the Inbox file you should "star"/"unstar" the incoming message after you replied to/forwarded it.

Comment 12

[Göran Schmidt]

Thank you Serg Iv for your quick help!

Though it's uncomfortable and demands a lot of discipline not to forget about it, that's easier than I thought and quite effective. I doublechecked the effect by removing the msf-file and restarting TB. Works fine!

But to anyone else reading by: in my opinion this is still a major bug and the workaround is not a sufficient solution to it.

Comment 13

[:aceman]

I can also confirm this bug on TB17 (and also that adding+removing star makes the replied/forward flag to get propagated to mbox file). It is true that bug 402392 makes changes to writing out X-Mozilla-Status (it removes some functions).

David Bienvenu, are you able to help us here?

Comment 14

[Eckard Berberich]

I just had the same problem in TB 24.1.1 after "repairing" my Inbox (Mac OS X 10.6.8).
I did some testing with older versions of TB and can confirm Serg Iv's observation that the regression must have happened between v11 and v12.

Comment 16

[Wayne Mery (:wsmwk)]

modified testcase of realraven 
1. set display setting to "as read" immediately on display; 
2. in a folder set all messages as unread
3. used [N] to step through a bunch of mails to set some as read, note the number of unread messages M 
4. Mark Folder Read => 0 unread
5. Repair Folder => M unread messages

Failure using these steps happens with TB12.0a1 20111225, but not with 20111124.  This squarely puts the bead on maildir bug 402392
2011-12-24 16:18:06 PST
checked in - http://hg.mozilla.org/comm-central/rev/097bc36f180d

Comment 17

[Wayne Mery (:wsmwk)]

other possible related/dup: bug 832759

pre-version 12 bugs which sound similar: bug 709790, bug 645650

Comment 18

[:aceman]

It bothers me that all my replied/forwarded messages have wrong status headers due to this bug. Let's fix it at last.

Comment 19

[:aceman]

Attachment: 840418.patch

So this works for me. The bug was that the nsMsgLocalMailFolder was missing the necessary overriding methods and UI functions called e.g. folder->MarkAllMessagesRead() which thus called the nsIMsgFolder version (non-overriden) that just operated on the msg database, not on the msgStore.
IMAP had the functions implemented so it shouldn't be affected. So I modeled the nsMsgLocalMailFolder methods after IMAP. And made a nice test :) The code is probably useful for maildir too, but the test will probably not work. It relies on the fact that compaction adds the X-Mozilla-Status headers into the mbox file. And I don't know how that works in maildir.

Aryx, please push to try server.

Comment 20

[Serg Iv]

Will it work for feeds too? X-Mozilla-Status should be 0041 for them, i think.

Comment 21

[:aceman]

Thanks for watching the bug. Yes, I have now tested it and Feeds do work too (reply/forward/mark folder read). They should be also handled by the nsMsgLocalMailFolder code (like POP3/Local folders).
After reading them, they get status of 0041. I am not sure why they have 0000, instead of 0040 while unread (new), but that is probably a different bug.

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed to Thunderbird-Try as https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=478daa2445a4

Comment 23

[:aceman]

Attachment: 840418.patch v1.1

OK, this fixes the mozmill failures (like OS X menu bar at its breaking job again).

Comment 24

[Kent James (:rkent)]

Comment on attachment 8561100 [details] [diff] [review]
840418.patch v1.1

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

Overall the backend stuff looks good. There are just a few minor issues that need cleaning up before this is done, hence the feedback+.

I don't do mozmill, so I did not review the test.

::: mailnews/base/util/nsMsgUtils.cpp
@@ +2055,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  for (uint32_t kindex = 0; kindex < aNumKeys; kindex++) {
> +    nsCOMPtr<nsIMsgDBHdr> msgHdr;
> +    aDB->GetMsgHdrForKey(aMsgKeys[kindex], getter_AddRefs(msgHdr));

How do you handle missing keys? The existing method using nsTArray silently skips those. You are adding a nullptr to the array. It seems to me like silently skipping is safer.

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +1311,5 @@
> +
> +    // Setup a undo-state
> +    if (aMsgWindow)
> +      rv = AddMarkAllReadUndoAction(aMsgWindow, thoseMarked, numMarked);
> +    nsMemory::Free(thoseMarked);

You can't have any possible returns between the allocation of thoseMarked and the free. I would just use a do {} while(false) block myself with breaks to handle errors. Also, initialize thoseMarked == nullptr and check for non-null before calling Free.

@@ +1337,5 @@
> +    rv = msgStore->ChangeFlags(messages, nsMsgMessageFlags::Read, true);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    mDatabase->Commit(nsMsgDBCommitType::kLargeCommit);
> +    nsMemory::Free(thoseMarked);

Same issues about no intermediate returns here of course.

Comment 25

[:aceman]

Attachment: 840418.patch v1.2

OK, thanks.
I have intentionally done it via mozmill as I need to test the path from UI to the backend. That it calls the proper override method.
I hope Magnus can check the test.

Comment 26

[Magnus Melin [:mkmelin]]

Comment on attachment 8564539 [details] [diff] [review]
840418.patch v1.2

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

LGTM! r=mkmelin

::: mail/test/mozmill/folder-display/test-message-commands-on-msgstore.js
@@ +14,5 @@
> +const MODULE_REQUIRES = ["folder-display-helpers",
> +                         "compose-helpers",
> +                         "window-helpers"];
> +
> +//Cu.import("resource:///modules/MailUtils.js");

remove

@@ +27,5 @@
> +var gOutbox;
> +var gAutoRead;
> +
> +function setupModule(module) {
> +  for (let lib of MODULE_REQUIRES)

please use braces for all loops

@@ +84,5 @@
> +
> +  let mboxstring = IOUtils.loadFileToString(folder.filePath);
> +
> +  let expectedStatusString = aStatus.toString(16);
> +  while (expectedStatusString.length < 4)

braces

Comment 27

[:aceman]

Attachment: 840418.patch v1.3 [checked in]

Thanks, fixed.

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed as https://hg.mozilla.org/comm-central/rev/bc4533660d85

Comment 29

[:aceman]

Attachment: fix test on OS X [will be checked in in bug 1133957]

OS X must be different again. Meta-Shift-Enter only works there if the focus is in the body. So this should fix it.

Aryx, please push to try server, OS X mozmill only. Thanks

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Test fix pushed to try as https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=99367b3af40d

Comment 31

[:aceman]

Comment on attachment 8565664 [details] [diff] [review]
fix test on OS X [will be checked in in bug  	1133957]

Yes! The try run is green on OS X.

Comment 32

[Magnus Melin [:mkmelin]]

But please move the test fix patch to bug 1133957