793270 - When saving draft, dialog box says "sending"

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[WBT]

Attachment: Screenshot of dialog

Steps to reproduce:
1. Compose a message.
2. Save the message as a draft [in my setup, I'm saving to the Drafts folder on an IMAP server, over a slow connection].
3. While it is saving, press X to exit the compose window.

Actual results:
4. Thunderbird reports that it is currently in the process of SENDING a message; would you like to wait until the message has been SENT?  (emphasis added; see screenshot for actual dialog).
5. User freaks out because s/he didn't want to send the message, and sending the draft may have negative consequences.

Expected results:
4. Message is saved as draft; draft-saving continues in background.
5. If a dialog is needed, it reports that Thunderbird is in the process of SAVING the message, not SENDING.  
6. User calmly proceeds.

I am observing this bug today and really thinking that this is a dup of Bug 255050 (which originally had this title), but that's marked as fixed, while I observe the specific issue just described, in FF 15.

This is related to Bug 774172.
The mix-ups between "sending" and "saving" actions also scare me every time, not only when they appear but even when they don't, because I know the code is crossed.  These bugs are why I rarely save messages that I don't want sent.

Comment 1

[:aceman]

WBT, notice that bug 255050 is only fixed starting with TB17, but you say you have 15. And that one only fixes the Save progress dialog while the msg is saved.

You attempt to interrupt the progress by closing the window. So you get a different text that was not fixed.
I can look into that. I can reproduce this in TB18.

Maybe splitting the gSendOrSaveOperationInProgress variable into two to determine the type of operation a way to fix this.

http://mxr.mozilla.org/comm-central/ident?i=gSendOrSaveOperationInProgress

Or is it possible at http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#3312 to determine if we are saving or sending?

Mkmelin, what do you think?

Comment 2

[WBT]

Ideally, I'd like to see the code for saving separated "completely" from the code for sending.  There have been a few times where it's actually sent the message instead of saving to drafts, and there have been negative consequences.  I haven't yet nailed it down to a reproducible bug, and I suspect it's a moving target between versions, but having these two so closely intertwined is just asking for that kind of trouble (also, saving to drafts instead of sending, which may have consequences just as bad).  This is the #1 reason why I don't consider Thunderbird "enterprise-grade."

An approach that increases the separation between these two, rather than having the same main code and some potentially-corruptible out-of-channel control variable distinguishing between the two, gets my vote.

Comment 3

[:aceman]

The problem is that the code needs to be almost the same, as Sending the message first needs to serialize it (or save to Outbox) and only then Send it out. So the paths are identical up to the last step.

Comment 4

[WBT]

Maybe the sequence of steps is the same, but there are clearly multiple points along the way where that control signal is either being not read, misread, or miswritten.  This causes observable, reproducible issues like the present bug and justifies fears that there are (and were, and will be in the future) bugs that actually lead to the wrong action happening.  

Anyway, I get the sense that the architecture of that code is not going to change because the dev perspective is to keep that functionality together in the same unit even if it increases the probability of heavy-consequential bugs.  It's sort of like mixing business and personal funds because it's easier than keeping two separate accounts.  Especially if there's some change in security policy or interest rate that should apply to both, it's easier to just make that change in one place, and keep separate ledgers (out-of-band control signal separating the two).  Most experienced people (and some laws) advise against that, because the consequences of mixing them up, or having issues reading/writing/ignoring the control signal, are potentially quite significant. 


I'd still like to hear Mkmelin's opinion as requested in Comment 1 :-).

Comment 5

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #1)
I think keeping track of msgType along with gSendOrSaveOperationInProgress would be the way to go.

Comment 6

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch

So, this patch sort of fixes the bug.
And, after the error message, there is another retry alert, which just keeps prompting on clicking ok, without doing anything. It exists with or without my patch so maybe that should be a followup bug.

This patch needs to be applied over the patch for bug 521158 as both are from the compose files and that is probably about to land so I created this one above that.

Thanks.

Comment 7

[:aceman]

Comment on attachment 822723 [details] [diff] [review]
Patch

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

Yes, so this is one way to go - split the variable into 2. But then we must always remember to check both.
The other way would be what mkmelin says in comment 5. Keep the variable as is and add a new one that tracks if we are saving only.
Third approach would be to change the possible values of gSaveOrSendOperation variable from 2 state boolean to 3 state (0=none, 1=save, 2=send).

Technically this is all that is needed for this bug so f+ from me.
It now depends on mkmelin which approach he prefers :)

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +294,5 @@
>  CheckMsg=Do not show me this dialog box again.
>  
>  ## Strings used by prompt when Quitting while in progress
>  quitComposeWindowTitle=Sending Message
> +## LOCALIZATION NOTE (quitComposeWindowMessage2): don't translate \n

Nice catch :)

@@ +300,5 @@
>  quitComposeWindowQuitButtonLabel2=&Quit
>  quitComposeWindowWaitButtonLabel2=&Wait
> +quitComposeWindowSaveTitle=Saving Message
> +## LOCALIZATION NOTE (quitComposeWindowSaveMessage): don't translate \n
> +quitComposeWindowSaveMessage=%1$S is currently in the process of saving a message.\nWould you like to wait untill the message has been saved before quitting or quit now?

"until".

Comment 8

[:aceman]

Suyash, the other dialog you mention, may it be bug 781467 ?

Comment 9

[Suyash Agarwal (:sshagarwal)]

No, not this one.

Comment 10

[Suyash Agarwal (:sshagarwal)]

This is to be applied over the patch for bug 521158.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 822723 [details] [diff] [review]
Patch

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

Didn't try it, but the code looks ok. r=mkmelin with my and acemans nit fixed

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4383,5 @@
>  
>  function AutoSave()
>  {
>    if (gMsgCompose.editor && (gContentChanged || gMsgCompose.bodyModified)
> +      && !gSendOperationInProgress && !gSaveOperationInProgress)

&& at the end of the previous line

Comment 12

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch to be checked-in

Carrying over review from mkmelin and feedback from aceman.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/b9bbcb0851bd

Comment 14

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

(In reply to :aceman from comment #8)
> Suyash, the other dialog you mention, may it be bug 781467 ?
File bug 961743.