1108441 - nsMsgComposeSendListener::RemoveCurrentDraftMessage should look gMsgCompose.deleteDraft in addition to gMsgCompose.compFields.draftID, because design of "delete editing draft mail" in Composer is "delete draftID upon Save/Send when deleteDraft==true"

Status: NEW

(MailNews Core :: Composition, defect)

Description

[WADA:World Anti-bad-Duping Agency]

+++ This bug was initially created as a clone of Bug #321355 +++
This is spin-off of Bug 321355 Comment #36.

When "Edit As New of non-Drafts folder", gMsgCompose.compFields.draftID=original mail, gMsgCompose.deleteDraft=false was set, and original mail(in draftID) is not deleted upon "Save as draft".
However, when "Edit As New of a draft mail in Drafts folder", gMsgCompose.compFields.draftID=edited draft, gMsgCompose.deleteDraft=false was set, but edited draft=original=draftID was deleted upon save.
(Note: This is phenomenon processed by Bug 321355.)
If draftID is set, and if Drafts folder, Tb always deletes draftID regardless of deleteDraft upon draft save/send?

Why "edited mail by Edit As New is deleted if Drafts" but "edited mail by Edit As New is not deleted if non-Drafts" is perhaps following :
> http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3674
>    After end of SaveAsDrft/SaveAsTemplate/QueueForLater/DeliverBackground,  RemoveCurrentDraftMessage() is called.
> http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3728
>   if draftID  is not set, RemoveCurrentDraftMessage does do nothing.
>   if draftID is set && folderFlags has nsMsgFolderFlags::Drafts, RemoveCurrentDraftMessage deletes draftID.
>   even if draftID  is set, if folderFlags doesn't have nsMsgFolderFlags::Drafts, RemoveCurrentDraftMessage does do nothing.
Small mismatch on "delete original or not" is seen in composer component modules.
    Someone deletes original if draftID is set && deleteDraft=true regardless of folder type,
   (I think this is concept/design in composer since initial, because deleteDraft is set only when composition-type=Draft)
    but other someone deletes original if draftID is set && folder is Drafts folder regardless of deleteDraft.
This mismatch maybe is difference between "process during mail composition" and "post save/send process".
   "post save/send process", RemoveCurrentDraftMessage(), looks mainly coded for IMAP.
   Because UID of newly saved draft mail can not known upon draft save execution(append) unless UIDPLUS is supported,
   UID of newly saved draft mail should be obtained by SEARCH command.
   And delete of old draft mail == "uid store UIDofOldDraft +Flags(\Deleted)" should be executed independently if imap.
But it maybe fault in OnStopCopy() and RemoveCurrentDraftMessage() : 
-  OnStopCopy() sets deleteDraft=true before first call of RemoveCurrentDraftMessage().
-  RemoveCurrentDraftMessage() desn't check gMsgCompose.deleteDraft.
-  RemoveCurrentDraftMessage() deletes message of draftID only when draftID is set && the draftID is in Drafts folder.
-  In RemoveCurrentDraftMessage(), if msgDBHdr for draftID is not found after save, 
    Tb assumes that it's IMAP and Drafts folder is not opened.
    However, "msgDBHdr for draftID is not found" can occur if following conditions are satisfied.
             Edit As New of mail in local mail folder
       && edited mail is deleted/Compacted or Offset of edited mail is changed by Compact
       &&  it's after first save.
    Is this a reason why phenomenon of "old draft is not deleted" on IMAP Drafts sometimes?
        If "append to IMAP Drafts for first draft save" is executed and mail data is accepted by server,
        and if above condition happens,
        and if IMAP Drafts folder access fails upon getting UID of newly saved draft mail,
        draftID may not be updated by UID of the newly saved draft mail.
        => After next draft save, "msgDBHdr for draftID is not found" occurs => "Delete previous draft" is skipped

Following code change is needed before removing "set draftiD when Composition-type=Template in mimedrft.cpp#l1455" by Bug 321355.
And, sorting out of "delete currently edited mail" is needed for some reasons.
(1) Problem like Bug 638390 which is relevan to "delete editing draft" exists.
Affter AutoSave, some quirks around "delete editing draft" is already made.
- Because AutoSave can be invoked while Send/Save from UI is already requested,
  and because "Send/Save from UI" can be invoked while AutoSave is already requested,
  set document-is-chaneded to false upon send request to reduce "AutoSave after send request",
  then change back to document-is-chaneded to ftrue, in case of  "send failure while sending".
"delete currently edited mail" should be sorted out before resolving proble of Bug 638390.
(2) In RemoveCurrentDraftMessage, "action when Drafts folder only" is seen.
It looks that "action when Drafts folder only" is mainly for "imap Drafts is not accessible when an action is requested".
However, it seems that "draftID is not found in local mail folder upon delete draftID due to Offset change by Compact" case is not cared.

"Check gMsgCompose.deleteDraft in delete editing draft mail" is first step of sorting out of "delete currently edited draft mail".
For test of it, "draftID is set, deleteDraft=false" condition is needed.
Yhis is reason why "patch for bug 321355 shouln't be landed until this bug is closed".

(A) Set deleteDraft=true after first RemoveCurrentDraftMessage call, in case of "deleteDraft=false for edited mail"
> http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3646
>     nsMsgComposeSendListener::OnStopCopy(nsresult aStatus)
> 3676       // We should only close the window if we are done. Things like templates
> 3677       // and drafts aren't done so their windows should stay open
> 3678       if (mDeliverMode == nsIMsgSend::nsMsgSaveAsDraft ||
> 3679           mDeliverMode == nsIMsgSend::nsMsgSaveAsTemplate)
> 3680       {
> 3681         msgCompose->NotifyStateListeners(nsIMsgComposeNotificationType::SaveInFolderDone, aStatus);
> 3682         // Remove the current draft msg when saving as draft/template is done.
> 3683  -      msgCompose->SetDeleteDraft(true); <== Shouldn't be set before first beforeRemoveCurrentDraftMessage call
> 3684         RemoveCurrentDraftMessage(msgCompose, true);
>          +      msgCompose->SetDeleteDraft(true);  <== Should be set after first beforeRemoveCurrentDraftMessage call
> 3685       }
> 3686       else
> 3687       {
> 3688         // Remove (possible) draft if we're in send later mode
> 3689         if (mDeliverMode == nsIMsgSend::nsMsgQueueForLater ||
> 3690             mDeliverMode == nsIMsgSend::nsMsgDeliverBackground)
> 3691         {
> 3692  -        msgCompose->SetDeleteDraft(true); <== Shouldn't be set before first beforeRemoveCurrentDraftMessage call
> 3693           RemoveCurrentDraftMessage(msgCompose, true);
>           +      msgCompose->SetDeleteDraft(true);  <== Should be set after first beforeRemoveCurrentDraftMessage call
> 3694         }
> 3695         msgCompose->CloseWindow(true);
> 3696       }
(B) Check deleteDraft in RemoveCurrentDraftMessage, according to design of deleteDraft
> http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3728
>     nsMsgComposeSendListener::RemoveCurrentDraftMessage(nsIMsgCompose *compObj, bool calledByCopy)
> http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3746
> +        bool deleteDraft=false;
>            // See http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#1383
> +        rv = compObj->GetDeleteDraft(getter_AddRefs(deleteDraft));
> +        NS_ASSERTION(NS_SUCCEEDED(rv), "RemoveCurrentDraftMessage can't get deleteDraft");
> +        if (NS_FAILED(rv))
> +        return rv;
> 3746   // Skip if no draft id (probably a new draft msg).
> 3747   if (NS_SUCCEEDED(rv) && !curDraftIdURL.IsEmpty())
>      => if (NS_SUCCEEDED(rv) && !curDraftIdURL.IsEmpty()  && deleteDraft )
> );
> 3761         // only do this if it's a drafts or templates folder. <= Comment should be changed
> 3762         if (folderFlags & nsMsgFolderFlags::Drafts)
> 3763         { 
>                      => Delete of  message set in draftID is dione in this block. This is reason why problem hppens on Drafts only.
>                            After removing mimedrft.cpp#l1455, this check is not needed, but the is no need to remove this check.


Note:
composition-type=Template is "Never delete original(edited) mail" mode(i.e. original is never draft mail even in Drafts folder),
and composition-type=Draft is "Surely delete original(edited) mail" mode,
even if gMsgCompose.deleteDraft=false is always set when composition-type=Template
and even if gMsgCompose.deleteDraft=true is always set when composition-type=Draft.
If "original mail" is needed, it's always saved in gMsgCompose.originalMsgURI.
So, even if above change works and is correct action, I believe "removing mimedrft.cpp#l1455 by Bug 321355" is correct and mandatory action.

Comment 1

[WADA:World Anti-bad-Duping Agency]

FYI.
Bug 854798 is for following issue:
   "messageKey==currently messageOffset if local/berkleystore folder" is changed by Compact.
Even if 854798 will be fixed, change of messgeKey will be needed at least upon Repair Folder(Rebuild-Index), because messageKey=+Infinity, +Infinity+1, ,,,. won't be supported.
So, "gMsgCompose.compFields.draftID is not fould" can occur on any local mail folder in normal situation.

Comment 2

[WADA:World Anti-bad-Duping Agency]

Change like following is sufficient?
> http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3746
>    3746   // Skip if no draft id (probably a new draft msg).
>      =>   // Skip if no draft id (no need to try to delete) OR deleteDraft is not requested(==Edit As New)
>    3747   if (NS_SUCCEEDED(rv) && !curDraftIdURL.IsEmpty())
>      =>   if (NS_SUCCEEDED(rv) && !curDraftIdURL.IsEmpty()  && deleteDraft )
> http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3752
>    3752     if (NS_SUCCEEDED(rv) && msgDBHdr)
>    3753     {
>                    // delete of draftID is executed, if Drafts folder or Teplates folder
>                    // If deleteDraft is correctly checked, there is no need of checking folder type. 
>    3781     }
>                 // I believe "delete of draftID can be skipped even if draftID is not found or accessible,
>                 // as far as "save of newly saved draft is available, and as fr as deleteDraft flag is correctly respected
>                 // because deleteDraft=false if edit-mode=Template and deleteDraft=true ifedit-mode=Draft always.
>                 else if (msgDBHdr of newly saved draft exists && msgFolder of newly saved draft mail is local mail folder)
>                      Skip "delete draftID", Store newly saved drft in draftsID,
>                      deleteDrafts = true, because this is process in "Save as draft normally ended"
>                 else if (msgDBHdr of newly saved draft exists && msgFolder of newly saved draft mail is imap mail folder)
>                      Skip "delete draftID", Store newly saved drft in draftsID,
>                      deleteDrafts = true, because this is process in "Save as draft normally ended"
>                 else if (msgDBHdr of newly saved draft doesn't exists || newly saved draft is not accessible)
>                      Throw Error, because this is process in "Save as draft normally ended"
>    3782     else
>    3783     {
>    3784       // If we get here we have the case where the draft folder
>    3785       // is on the server and
>    3786       // it's not currently open (in thread pane), so draft
>    3787       // msgs are saved to the server

Possible normal case of "!curDraftIdURL.IsEmpty() && deleteDraft" is true but msgDBHdr for draftID is not available.
(a) Edit As New or Edit As Draft at a folder,
      Drafts folder to save draft != the folder
      (if Edit As New, Drafts folder!=the folder usually)
      (if Edit As Draft, Drafts folder of initiallly edited draft != Drafts folder to save as draft)
(a-1) the folder=local/berkleystore, and the Folder is Compacted while editing
          => messageKey=messageOffset is changed while editing.
(a-2) the folder=imap folder,
         after start of edit, other folder is selected at folder pane, then msgDB of the folder is closed
         => msgDBHdr for draftID(==internal URI) is not accessible becuse folder is already closed.
         Note: If Edit As Draft, Drafts folder of initiallly edited draft == Drafts folder to save as draft)
                   So, this case is rare if Edit As Draft.

Comment 3

[WADA:World Anti-bad-Duping Agency]

FYI.
Similar assumption of "delete draftID failure == imap folder case" is seen in RemoveDraft() which is called upon Compser Window close by user.
> http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#3315
  function ComposeCanClose()
  upon Compser Window close, if Send/Save is in progress, ask to user whether Cancel or wait for completion.
  if Cancel, Send/Save operation is done, if (gContentChanged || gMsgCompose.bodyModified || gAutoSaveKickedIn),
  ask for user about currently editig mail data,
  if request is "Don't save" &&  if (!gEditingDraft && gAutoSaveKickedIn), calls  RemoveDraft();.
In this RemoveDraft() which tries to delete mail pointed by draftID, similar assumption of "delete draftID failure == imap folder case" is seen.

Note:
gEditingDraft=true is "last save was SaveAsDraft, last save was not SaveTemplate".
However, gEditingDraft!=true is not always "save was executed and it was not SaveAsDraft". It contains "no save request except AutoSave" case.
And, gAutoSaveKickedIn=true is "save was requested" && "last save was SaveAsAutoSave. last save was not SaveAsDraft nor SaveAsTemplate".
However, "last AutoSave request completed then previous version of draft was deleted and new draft was set in draftID" or "last AutoSave request is in progress" can not know by the flag, and completed  gEditingDraft=true is also set if draftID is set upon composition start by gEditingDraft = gMsgCompose.compFields.draftId;
(when Edit As New at non Drafts folder, before fix of Bug 321355, draftId is set, and after fix of fix of Bug 321355, draftId is not set.)
So, I'm not sure that "last save request was AutoSave, it completed or it was in progress is not sure, and it was caceled" can always be correct condition to delete currently set draftID when Composer Window is closed.
This kind of issue is perhaps better processed in Bug 638390. "When, by whom, what, how, should draftID be deleted, and should be kept" is better analyzed in that bug for "old draft is not deleted" issue.

Comment 4

[WADA:World Anti-bad-Duping Agency]

New discovery.

When IMAP Drafts folder(checked with auto-sync=On,Offline-Use=On), and Save As Draft is executed while Work Offline Mode, following occurs.
  1. One draft mail only. Subject-1, UID=19. Go Work Offline mode.
  2. Edit draft  UID=19, draftID(key=19), deleteDraft=true.
  3. Change Subject to Subject-2, Save as draft => new draft generated with faked-key, UID=4294967168.
      Old draft of UID=19 is deleted at threaad pane.
      draftID at Composer window is not changed. Still draftID(key=19)
  4. Repeat step 3 with increasing number in Subject.
      Upon each save, previous draft is not deleted.
                                                              Subject-2          Subject-3           Subject-4          Subject-5           Subject-6
      UID of saaved draft is changed like 4294967168 -> 4294967167 -> 4294967166 -> 4294967165 -> 4294967164
      (decremented by one).
      draftID at Composer window is not changed. Still draftID(key=19)
  5. Go Work Online, click other folder, and click Drafts(re-open Drafts).
      Re-sync with server is executed, and following mail appears.
                Subject-2          Subject-3           Subject-4          Subject-5           Subject-6
      UID    27                      26                      25                      24                      23
      Uploaded is done in messaageKey order, so UID is reversed.
       Note: draftID at Composer window at this stage is not changed. Still draftID(key=19)
  6. Change to Subject-7, Save As Draft
      => Suject-1/UID=28 appears at Thread Pane, 
            Prompt of "There was an error saving the messaage in Drafts, Retry?"
            OK => Suject-1/UID=28 disappears => Suject-7/UID=28  appears(Upload is done)
       => draftID at Composer window is not changed. Still draftID(key=19)
  7. Change to Subject-8, Save As Draft
      => Suject-7/UID=29 appears at Thread Pane, 
            Suject-8/UID=30 appears at Thread Pane,
       => draftID at Composer window is now changed. draftID(key=30).
             Returned to normal state at last!
   8. Repair Folder => all of UID=23 to 30 appears.
My miss-operation may be involved in above(forgot to change Subject, etc.), but generated UID is accurate.

Main cause of above is that "messageSend" is not correctly set or not set as expected by imap code upon "Save As Draft while Work Offline". And, IIRC, bug is already opened for "issue in Edit Draft while Working Offline mode".
However, change in RemoveCurrentDraft code, code for updating draftID from messageSend, is needed for tolerance with such special situation.

Comment 5

[WADA:World Anti-bad-Duping Agency]

FYI.
Bug 638358 was for phenomenon of "Draft mail saved in IMAP Drafts folder during Work Offline mode is not deleted by next Save As Draft, resulting in multiple drafts".