530528 - Replies are not marked as "Replied" if a modified draft in IMAP Drafts folder is sent

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Miguel Ruiz]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.6pre) Gecko/20091116 Ubuntu/9.10 (karmic) Shiretoko/3.5.6pre
Build Identifier: 3.0.1pre

Replies are not marked if a modified draft is sent. The blue arrow only appears if the email is replied without the use of a draft.

Reproducible: Always

Steps to Reproduce:
1. Select an email from the inbox
2. Use the "Reply" button
3. Write something and save the draft
4. Edit the draft and send the email

Actual Results:  
The replied email is not marked as reply (with the blue arrow)

Expected Results:  
The replied email should be marked with the blue arrow.

My email account is IMAP.

Comment 2

[WADA:World Anti-bad-Duping Agency]

(In reply to Miguel Ruiz from comment #0)
> My email account is IMAP.

Replied mail is held in IMAP mail folder, and draft folder is IMAP folder?

If so, it's probably regresson seen in bug 522336 comment #29, because your STR is "Reply, Save as draft and exit composition, and Edit draft then Send.
Because "IMAP draft folder case or local drfat case" is still unclear in bug 522336 for both original bug report and test case which confirmed that bug, I believe duping of this crispy bug to that unclear bug will merely produce confusions.

Re-opening this bug, and duping bug 627989(for Gecko/20110117 Thunderbird/3.3a3pre) and bug 710586(for Tb 8) to this bug, for ease of problem analysis and for correct reporting of Tb's bug to developers.

Comment 5

[WADA:World Anti-bad-Duping Agency]

I post check result on regression window to this bug, instead of inappropriate place, bug 522336 comment #29. 

It looks IMAP only regression in Tb 3.0aX/3.0bY over fix of bug 128996 during Tb 3.0aX/3.0bY, as Magnus Melin says in comment #3 and in bug summary.

Multiple Tb builds were checked during my holidays with "replied mail in local mail folder" only.
Because replied mail is held in local mail folder, both bug 268589(Compact of the local mail folder) and bug 639702(negative offset if local mail folder size>2GB) were carefully excluded.

>  +----- local-draft case
>  |  +-- imap-draft case
>  |  |
>  V  V 
>       before first FIXED by bug 128996, replied status was not set in both local-draft case and IMAP-draft case.
>  X  X  2005-10-20-11-trunk : before-first-FIXED-of-bug-128996[2005-10-27]
>             bug 268589 comment #1 and bug 268589 comment #3 fot Tb 1.0.x seems this problem.
>
>       local-draft case was fixed by first FIXED of bug 128996.
>  O  X  2005-12-31-10-trunk : after-first-FIXED-of-bug-128996[2005-10-27]
>
>  O  X  2006-03-01-11-trunk : Tb1.6a1-final
              ==> Tb 2.0.0 perhaps had this problem.
>
>  O  X  2006-03-02-11-trunk : Tb3.0a1-first
>
>       regression happened on local-draft case.
>  X  X  2007-07-29-10-comm-central : Just-before-final-FIXED-of-bug-128996[2007-07-30, re-opened on 2007-01-13]
>
>       second(final) FIXED of bug 128996 resolved both local-draft case & IMAP case.
>  O  O  2007-08-10-04-comm-central : one-week-after-final-FIXED-of-bug-128996[2007-07-30]
>          note: imap-mail case is OK too. local-draft=ok, imap-draft=ok
>
>       was bug 128996 back ported to Tb 2.x?
>
>       after fix by bug 128996, regression happened on imap-draft case only.
>  O  X  2008-08-10-03-comm-central       : one-year-after-final-FIXED-of-bug-128996[2007-07-30]
>
>       and, the regression in imap-draft case is not resolved yet.
>  O  X  2009-01-01-03-comm-central       : first-build-in-2009
>  O  X  2009-01-08-03-comm-central       : trunk-Tb3.0b2pre-final
>  O  X  2009-01-09-03-comm-central-trunk : Tb3.1a1pre-first
>  O  X  2009-03-18-06-comm-central-trunk : Just-before-FIXED-of-bug-366968[2009-03-19]
>  O  X  2009-03-26-03-comm-central-trunk : one-week-after-FIXED-of-bug-366968[2009-03-19]
>  O  X  2009-12-30-03-comm-central-trunk : Tb3.1a1pre-final
>  O  X  2010-08-25-03-comm-central       : one-year-after-FIXED-of-bug-366968[2009-03-19]
>  O  X  2010-09-14-03-comm-central       : Tb3.2a1pre-final
>  O  X  2010-09-15-03-comm-central       : Tb3.3a1pre-first
>  O  X  2011-04-15-03-comm-2.0           : Tb3.3a1pre-final
>  O  X  2011-05-10-14-comm-central       : Tb3.4a1pre-first
>  O  X  2011-06-01-03-comm-central       : Tb7.0a1-first
>  O  X  2011-07-05-03-00-comm-central    : Tb7.0a1-final

Comment 6

[WADA:World Anti-bad-Duping Agency]

cc-ing to David :Bienvenu who is Assigned To: person of bug 128996.

Comment 7

[WADA:World Anti-bad-Duping Agency]

Regression window in previous comment was inaccurate. Sorry for my mistake.

>  +----- local-draft case
>  |  +-- imap-draft case
>  |  |
>  V  V 
> Second(final) FIXED of bug 128996 resolved both local-draft case & IMAP case.
>  O  O  2007-08-10-04-comm-central : one-week-after-final-FIXED-of-bug-128996[2007-07-30]
>          note: imap-mail case is OK too. local-draft=ok, imap-draft=ok
>
> Regression didn't happen in next build. Sorry for my mistake.
>  O  O  2008-08-10-03-comm-central       : one-year-after-final-FIXED-of-bug-128996[2007-07-30]
>
>  O  O  2008-11-24-03-comm-central/thunderbird-3.0b1pre.en-US.win32.installer.exe	
>            Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre)
>            Gecko/20081124 Shredder/3.0b1pre
>
> After fix by bug 128996, regression happened at here.
>
>  O  X  2008-11-25-03-comm-central/thunderbird-3.0b1pre.en-US.win32.installer.exe	
>            Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre)
>            Gecko/20081125 Shredder/3.0b1pre

Comment 8

[WADA:World Anti-bad-Duping Agency]

Attachment: IMAP Drafts.msf by 2008-11-24-03-comm-central (replied will be set)

URL of replied mail is seen in .msf.

> <(9D=91)(9C=21)(92=IMAP-Draft SMTP-DTI <[e-mail]>)(93
>     =Boyacky at RocketMail <[e-mail]>)(95
>     =mail-in-local-folder@imap-draft-folder)(8F
>     =[e-mail])(96
>     =<[e-mail]\
> ed-test>)(89=ISO-8859-1)(98=368)(9A=ffffffff)(90
>     =mailbox-message://nobody@Local%20Folders/Inbox#764)(91=replied)
>   (9E=3d2)(9F=14)>

Comment 9

[WADA:World Anti-bad-Duping Agency]

Attachment: IMAP Drafts.msf by 2008-11-25-03-comm-central (replied will NOT be set)

URL of replied mail is not seen in .msf.

> <(96=11)(8E=IMAP-Draft SMTP-DTI <[e-mail]>)(8F
>     =Boyacky at RocketMail <[e-mail]>)(91
>     =mail-in-local-folder@imap-draft-folder)(92
>     =[e-mail])(93
>     =<[e-mail]\
> ed-test>)(89=ISO-8859-1)(95=368)(97=ffffffff)>

Comment 10

[WADA:World Anti-bad-Duping Agency]

Refference: attachment 583378 [details]
  Local Drafts.msf after Reply/Save which is attached to bug 711909 comment #3.
URL of replied message & "replied" is seen in .msf.
> <(9D=mailbox-message://nobody@Local%20Folders/Inbox#764)(9E=replied)
>   (9F=id1)>[-0:^80(^88=10)(^8A=0)(^8B=21)(^82^93)(^85^94)(^81^96)(^83^97)

Comment 11

[WADA:World Anti-bad-Duping Agency]

Following is IMAP Drafts.msf content after;
  Replied mail is in IMAP Inbox, Reply, Save as draft to local Drafts folder,
  Move draft mail in local Drafts folder to IMAP Drafts folder.
> ied-test>)(92=4ef18f34)(93
>     =imap-message://[e-mail]/INBOX#465)
>   (94=replied)(95=id1)(96=19|Boyacky at RocketMail)>
URL & "replied" was copied to .msf of IMAP Drafts folder, and "Replied" status was set as expected when "Edit draft/Send" was executed for the moved draft mail in IMAP Drafts folder.

Setting of related properties looks to be done by next code.
> /mailnews/compose/src/nsMsgCompose.cpp
>     line 3311 -- msgDB->SetAttributeOnPendingHdr(msgHdr, ORIG_URI_PROPERTY, mOriginalMsgURI.get());
>     line 3313 -- msgDB->SetAttributeOnPendingHdr(msgHdr, QUEUED_DISPOSITION_PROPERTY, dispositionSetting);
>     line 3315 -- msgDB->SetAttributeOnPendingHdr(msgHdr, HEADER_X_MOZILLA_IDENTITY_KEY, identityKey.get());

Why ORIG_URI_PROPERTY and QUEUED_DISPOSITION_PROPERTY is not written if IMAP?

Following is source code which has "AttributeOnPendingHdr" too.
> /mailnews/db/msgdb/src/nsMsgDatabase.cpp
>     line 4743 -- NS_IMETHODIMP nsMsgDatabase::SetAttributeOnPendingHdr(nsIMsgDBHdr *pendingHdr, const char *property,
>     line 4749 -- NS_IMETHODIMP nsMsgDatabase::SetUint32AttributeOnPendingHdr(nsIMsgDBHdr *pendingHdr, const char *property,
>     line 4756 -- nsMsgDatabase::SetUint64AttributeOnPendingHdr(nsIMsgDBHdr *aPendingHdr,
> 4743 NS_IMETHODIMP nsMsgDatabase::SetAttributeOnPendingHdr(nsIMsgDBHdr *pendingHdr, const char *property,
> 4744                                   const char *propertyVal)
> 4745 {
> 4746   return NS_ERROR_NOT_IMPLEMENTED;
> 4747 }
> 4748 
> 4749 NS_IMETHODIMP nsMsgDatabase::SetUint32AttributeOnPendingHdr(nsIMsgDBHdr *pendingHdr, const char *property,
> 4750                                   PRUint32 propertyVal)
> 4751 {
> 4752   return NS_ERROR_NOT_IMPLEMENTED;
> 4753 }
> 4754 
> 4755 NS_IMETHODIMP
> 4756 nsMsgDatabase::SetUint64AttributeOnPendingHdr(nsIMsgDBHdr *aPendingHdr,
> 4757                                               const char *aProperty,
> 4758                                               PRUint64 aPropertyVal)
> 4759 {
> 4760   return NS_ERROR_NOT_IMPLEMENTED;
> 4761 }

If IMAP, Set{}AttributeOnPendingHdr is also seen in next codes.

> /mailnews/imap/src/nsImapMailFolder.cpp
>     line 7433 -- mDatabase->SetAttributeOnPendingHdr(msgDBHdr, "label", labelStr.get());
>     line 7438 -- mDatabase->SetAttributeOnPendingHdr(msgDBHdr, "keywords", keywords.get());
>     line 7460 -- mDatabase->SetAttributeOnPendingHdr(msgDBHdr, property.get(), sourceString.get());
>     line 7469 -- mDatabase->SetUint32AttributeOnPendingHdr(msgDBHdr, "offlineMsgSize", messageSize);
>     line 7470 -- mDatabase->SetUint64AttributeOnPendingHdr(msgDBHdr, "msgOffset", messageOffset);
>     line 7471 -- mDatabase->SetUint32AttributeOnPendingHdr(msgDBHdr, "flags", nsMsgMessageFlags::Offline);
>     line 7479 -- mDatabase->SetAttributeOnPendingHdr(msgDBHdr, "priority", priorityStr.get());

> /mailnews/db/msgdb/src/nsImapMailDatabase.cpp
>     line 267 -- NS_IMETHODIMP nsImapMailDatabase::SetAttributeOnPendingHdr(nsIMsgDBHdr *pendingHdr, const char *property,
>     line 278 -- nsImapMailDatabase::SetUint32AttributeOnPendingHdr(nsIMsgDBHdr *pendingHdr,
>     line 290 -- nsImapMailDatabase::SetUint64AttributeOnPendingHdr(nsIMsgDBHdr *aPendingHdr,
> 267 NS_IMETHODIMP nsImapMailDatabase::SetAttributeOnPendingHdr(nsIMsgDBHdr *pendingHdr, const char *property,
> 268                                   const char *propertyVal)
> 269 {
> 270   NS_ENSURE_ARG_POINTER(pendingHdr);
> 271   nsCOMPtr<nsIMdbRow> pendingRow;
> 272   nsresult rv = GetRowForPendingHdr(pendingHdr, getter_AddRefs(pendingRow));
> 273   NS_ENSURE_SUCCESS(rv, rv);
> 274   return SetProperty(pendingRow, property, propertyVal);
> 275 }
> 276 
> 277 NS_IMETHODIMP
> 278 nsImapMailDatabase::SetUint32AttributeOnPendingHdr(nsIMsgDBHdr *pendingHdr,
> 279                                                    const char *property,
> 280                                                    PRUint32 propertyVal)
> 281 {
> 282   NS_ENSURE_ARG_POINTER(pendingHdr);
> 283   nsCOMPtr<nsIMdbRow> pendingRow;
> 284   nsresult rv = GetRowForPendingHdr(pendingHdr, getter_AddRefs(pendingRow));
> 285   NS_ENSURE_SUCCESS(rv, rv);
> 286   return SetUint32Property(pendingRow, property, propertyVal);
> 287 }
> 288 
> 289 NS_IMETHODIMP
> 290 nsImapMailDatabase::SetUint64AttributeOnPendingHdr(nsIMsgDBHdr *aPendingHdr,
> 291                                                    const char *aProperty,
> 292                                                    PRUint64 aPropertyVal)
> 293 {
> 294   NS_ENSURE_ARG_POINTER(aPendingHdr);
> 295   nsCOMPtr<nsIMdbRow> pendingRow;
> 296   nsresult rv = GetRowForPendingHdr(aPendingHdr, getter_AddRefs(pendingRow));
> 297   NS_ENSURE_SUCCESS(rv, rv);
> 298   return SetUint64Property(pendingRow, aProperty, aPropertyVal);
> 299 }

This kind of difference in code is relevant to problem?

Pushlog between 2008-11-24(00:00:00) and 2008-11-26(00:00:00).
> http://hg.mozilla.org/comm-central/pushloghtml?startdate=2008-11-24&enddate=2008-11-26
IMAP relevant fix was next.
> Mon Nov 24 09:46:38 2008 -0800, patch for Bug 451877
Does this patch cause this bug?

Comment 12

[WADA:World Anti-bad-Duping Agency]

Following is draft related change.
> Tue Nov 25 09:00:29 2008 -0800, backout fix for bug 307023
> because it caused regressions like bug 465586, problems forwarding messages inline, editing drafts

Comment 15

[Magnus Melin [:mkmelin]]

Attachment: bug530528_drafts_set_reply_status_fix.patch

With this patch it should be working in all cases.

As I understand it - this was originally fixed in bug 128996 for a short while - but after bug 404397 we would never get to that code. Then things changed a lot in the area for tb3 and beyond...

The removed code related to the "If we did't find the msg hdr" comment we don't run at any time, and i'm unable to come up with a scenario it would be used.

This also fixes bug 597441 - that's the (mType == nsIMsgCompType::Draft) part in nsMsgCompose::RememberQueuedDisposition.

Comment 16

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 790923 [details] [diff] [review]
bug530528_drafts_set_reply_status_fix.patch

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

Looks good aside from the nits about using String.assignLiteral for initializing strings from a constant. r=me with those fixed.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +3270,5 @@
>        mType == nsIMsgCompType::ReplyToGroup ||
>        mType == nsIMsgCompType::ReplyToSender ||
>        mType == nsIMsgCompType::ReplyToSenderAndGroup)
> +  {
> +    dispositionSetting = NS_LITERAL_CSTRING("replied");

dispositionSetting.assignLiteral("replied");

@@ +3276,4 @@
>    else if (mType == nsIMsgCompType::ForwardAsAttachment ||
>             mType == nsIMsgCompType::ForwardInline)
> +  {
> +    dispositionSetting = NS_LITERAL_CSTRING("forwarded");

assignLiteral(...)

Comment 17

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/bd1af318844c -> FIXED