Closed Bug 840418 Opened 11 years ago Closed 9 years ago

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)

Categories

(MailNews Core :: Backend, defect, P1)

Tracking

(thunderbird_esr38 fixed)

RESOLVED FIXED
Thunderbird 38.0
Tracking Status
thunderbird_esr38 --- fixed

People

(Reporter: sergei.ivn+bugzilla, Assigned: aceman)

References

(Blocks 1 open bug)

Details

(Keywords: dataloss, regression, testcase, Whiteboard: [regression:tb12][workaround: do comment 11 on all messages])

Attachments

(2 files, 3 obsolete files)

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)
(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Untriaged → Backend
Product: Thunderbird → MailNews Core
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?
Keywords: regression
(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:.
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
(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.
Summary: replied/forwarded icons disappear after folder repair → replied/forwarded icons disappear after folder repair("Internally set message flag" is not written to X-Mozilla-Status: until "manually changeable message flag" is changed)
> 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.
(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.
Summary: replied/forwarded icons disappear after folder repair("Internally set message flag" is not written to X-Mozilla-Status: until "manually changeable message flag" is changed) → replied/forwarded icons disappear after folder repair("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)
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.
Severity: normal → major
Keywords: dataloss
OS: Windows 7 → All
Hardware: x86 → All
Summary: replied/forwarded icons disappear after folder repair("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) → 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)
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!
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.
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.
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?
Flags: needinfo?(mozilla)
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.
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
Blocks: 402392
Flags: needinfo?(mozilla)
Keywords: testcase
other possible related/dup: bug 832759

pre-version 12 bugs which sound similar: bug 709790, bug 645650
Whiteboard: [regression:tb12]
It bothers me that all my replied/forwarded messages have wrong status headers due to this bug. Let's fix it at last.
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Priority: -- → P1
Attached patch 840418.patch (obsolete) — Splinter Review
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.
Flags: needinfo?(archaeopteryx)
Attachment #8561044 - Flags: review?(rkent)
Attachment #8561044 - Flags: review?(mkmelin+mozilla)
Will it work for feeds too? X-Mozilla-Status should be 0041 for them, i think.
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.
Flags: in-testsuite+
Whiteboard: [regression:tb12] → [regression:tb12][workaround: do comment 11 on all messages]
Attached patch 840418.patch v1.1 (obsolete) — Splinter Review
OK, this fixes the mozmill failures (like OS X menu bar at its breaking job again).
Attachment #8561044 - Attachment is obsolete: true
Attachment #8561044 - Flags: review?(rkent)
Attachment #8561044 - Flags: review?(mkmelin+mozilla)
Attachment #8561100 - Flags: review?(rkent)
Attachment #8561100 - Flags: review?(mkmelin+mozilla)
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.
Attachment #8561100 - Flags: review?(rkent) → feedback+
Attached patch 840418.patch v1.2 (obsolete) — Splinter Review
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.
Attachment #8561100 - Attachment is obsolete: true
Attachment #8561100 - Flags: review?(mkmelin+mozilla)
Attachment #8564539 - Flags: review?(rkent)
Attachment #8564539 - Flags: review?(mkmelin+mozilla)
Attachment #8564539 - Flags: review?(rkent) → review+
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
Attachment #8564539 - Flags: review?(mkmelin+mozilla) → review+
Thanks, fixed.
Attachment #8564539 - Attachment is obsolete: true
Attachment #8564733 - Flags: review+
Keywords: checkin-needed
Pushed as https://hg.mozilla.org/comm-central/rev/bc4533660d85
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
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
Flags: needinfo?(archaeopteryx)
Blocks: 1134024
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.
Attachment #8565664 - Flags: review?(mkmelin+mozilla)
Attachment #8565664 - Flags: review?(mkmelin+mozilla) → review+
But please move the test fix patch to bug 1133957
Depends on: 1133957
Attachment #8564733 - Attachment description: 840418.patch v1.3 → 840418.patch v1.3 [checked in]
Attachment #8565664 - Attachment description: fix test on OS X → fix test on OS X [will be checked in in bug 1133957]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: