1108609 - nsMsgComposeSendListener::RemoveCurrentDraftMessage deletes newly saved draft upon Save As Draft, if messageKey==messageOffset of draftID(==editing draft) is changed by Compact of Drafts folder

Status: NEW

(MailNews Core :: Composition, defect)

Description

[WADA:World Anti-bad-Duping Agency]

+++ This bug was initially created as a clone of Bug #1108441 +++

[Step to reproduce]

1. In local Drafts folder, two draft mails.
    Delete mail, so it's not shown.
       messageKey=messageOffset=0, messageSize=SSSS, Subject=any
    Mail which is edited.
       messageKey=messageOffset=SSSS, messageSize=nnnn, Subject=Subject-000
2. Edit the draft of Subject=Subject-000 draftID=mailbox-message://.../Drafts#SSSS
3. Change Subject to Subject=Subject-001
4. Compact Drafts folder => Offset of Subject-000 is changed by Compact in Drafts.
       messageKey=messageOffset=0, messageSize=nnnn, Subject=Subject-000
5. Save as draft at Compser Window
    =>
    In Drafts file, at Offset=nnnn, Subject-001 is writen with X-Mozilla-Keys: 0008 (<=deleted mail)
    New draft mail of Subject-001  is not shown at Thread Pane of Drfts folder.
    After Save, gmsgCompose.compFields.draftID = mailbox-message://.../Drafts#nnnn
    This is :
        (i). New draft of Subject-001 is appended to Offset=nnnn
        (ii). Try to delete editing draft set in draftID(messgeKey=SSSS)
             => Because mail of messgeKey=messageOffset=SSSS doesn't exist after Compact,
                   error is returned to  GetMsgDBHdrFromURI(draftID of key=SSSS).
        (iii). RemoveCurrentDraftMessage deletes newly saved draft mail at Offset=nnnn
6. Because newly appended mail to file named Drafts is deleted by Tb himself after "Save as draft",
    newly saved mail won't be kept if following occurs after it.
     6-1. Compact
     6-2. Change subject, and Save as draft
     6-3. Repeat 6-1 and 6-2.
             Because above phenomenon happens upon each set of Compact/Save as draft,
             no new draft is saved. Only original of Subject-000 is available.

This is caused by wrong assumption about error return code  to "GetMsgDBHdrFromURI(curDraftIdURL.get(), getter_AddRefs(msgDBHdr));
" in RemoveCurrentDraftMessage(). 
> nsMsgComposeSendListener::RemoveCurrentDraftMessage
> http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3728

Note:
Similar wrong assumption of "getting msgDBHdr for draftID failed == imap folder case" is seen in RemoveDraft() which is called upon Compser Window Close by user.
> ComposeCanClose()
> http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#3315
> RemoveDraft() 
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#3402

Comment 1

[WADA:World Anti-bad-Duping Agency]

In this case, if messenger.msgHdrFromURI(gMsgCompose.compFields.draftId) is called from JavaScript, following exception occurs.
> [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIMessenger.msgHdrFromURI]"
>  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"
>  location: "JS frame :: resource://wbmt/WBMT_Button_7.jsm :: Button_7[31] :: line 229"  data: no]
However, phenomenon of this bug is not always observed when this exception occurs due to Compact.
Required condition seems.
   Because of Compact, msgDBHdr for draftID is not found => Error is returned to GetMsgDBHdrFronURI(exception if JavaScript).
   Number of message in Drafts folder=1, messageKey=messageOffset=0.
It's perhaps following phenomenon.
   Save of new draft ends. gEditingDraft is updated. draftID is not updated.
   If error is returned to GetMsgDBHdrFronURI(draftID),  RemoveCurrentDraftMessage assumes it as "Error in imap".
   In imap, messageKey=UID, and UID starts from 1. messageKey=0 perhaps produces additional error or misunderstanding.
   Then, Tb perhaps deletes message of draftID or gEditingDraft.
   Unfortunatelly, due to Compact, messgeOffset of newly saved mail == messageKey in draftID for previous version of draft.
   So, newly saved draft mail is deleted.

This is pretty special case. However, this is surely an evidence of that Tb's current logic is incorrect.

Comment 2

[WADA:World Anti-bad-Duping Agency]

Correction. Sorry, I misunderstood phenomenon.
Required conditions was:
    messgeOffset of editing draft before Compact  == messageOffset of  newly saved draft
    gMsgCompose.compFields.draftId                    == gMsgCompose.messaeSend 
Phenomenon was :
   1.  After save of new draft, RemoveCurrentDraftMessage deletes draftID.
        Because messageOffset of  newly saved draft == messgeOffset of editing draft before Compact,
        newly saved draft is deleted.
   2. Because newly saved draft is deleted, GetMsgDBHdrFronURI(messaeSend) returns error RC.
       Then RemoveCurrentDraftMessage returns without updating draftID by messageSend.
   3. Because draftId  == messaeSend, it looked for me that update of draftID is normally executed.
   4. After calling RemoveCurrentDraftMessage, messaeSend is cleared.
       http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3698
       So messaeSend==null is shown if JavaScript code accesses it after Save As Draft completion. 
   5. Because mail of draftID(==messaeSend) is deleted, GetMsgDBHdrFronURI(messaeSend) returns error RC,
       so messenger.msgHdrFromURI(draftId) from JavaScript code throws exception.

This is thrd case of following.
   messageOffset of draftID(key=BBB) is changed to AAA(<BBB) by Compact of locl Drafts folder.
   (i) "old draft mail which was moved to AAA by Compact" is not deleted. (already known issue)
   (ii-a) if messaeOffset of other mail after Compact == BBB, the other mail is deleted. (already known issue)
   (ii-b) if messaeOffset of newly saved draft after Compact == BBB, the newly saved draft is deleted. (this bug)

Simplest and most effective solution is : Don't change messageKey by Compact of local mail folder(Bug 854798)

Comment 3

[WADA:World Anti-bad-Duping Agency]

Above (i) /(ii-a) is phenomenon reported to bug 482836 comment #89.
I don't know whether specific bug for such issue already exists or not.

Comment 4

[Wayne Mery (:wsmwk)]

(In reply to WADA from comment #2)
> ...
> Simplest and most effective solution is : Don't change messageKey by Compact
> of local mail folder(Bug 854798)

With Bug 854798 fixed in version 38, do we know if this bug is well and truly fixed?

Comment 5

[WADA:World Anti-bad-Duping Agency]

(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #4)
> With Bug 854798 fixed in version 38, do we know if this bug is well and truly fixed?

No.
Even after fix of Bug 854798, this bug can occur by "Compact + Repait Folder" instead of "Compact only".
Bug 854798 merely minimizes probability of that this bug happens in actual environment.