Closed Bug 1366591 Opened 7 years ago Closed 7 years ago

After copy to IMAP "Sent" folder failed, "Retry" just dismisses the panel without retrying and loses the sent message (making bug 28211 even worse)

Categories

(MailNews Core :: Networking: IMAP, defect)

Product:
MailNews Core
Component:
Networking: IMAP
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 56.0

People

(Reporter: bugzilla, Assigned: gds)

References

(Blocks 1 open bug)

Details

(Keywords: dataloss)

Attachments

(1 file, 9 obsolete files)

1366591-review-changes6.patch
29.25 KB, patch
jorgk-bmo
: review+
aceman
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.116 Safari/537.36

Steps to reproduce:

Sending an E-Mail via SMTP/IMAP.

The problem occurs randomly sometimes, it's not reproducable for me. 
(This is a bug since Thunderbird exists.)


Actual results:

It's a randomly event since Thunderbird exists: Sometimes, after successfully sending an E-Mail by SMTP TB is not able to copy the sent mail into the "Sent" Folder. 

In former versions one ended in an endless loop:
TB told about the problem, and asked to retry or abort. "Retry" could be clicked endlessly without any success (100%), and "Abort" stopped everything. The sent mail was always lost for the sender. No possibility to return to the edit window or to save the mail locally.
After this, most times the TB process had to be killed an TB new started, to work with TB again.

In the new version (52.0 32 Bit) the behavior has changed: There is only a short message window, that tells, that the mail could not be copied to the sent folder. Then you click on "Retry" (or "OK" (sorry, don't remember exactly)) and: everything seems to be fine.

But: The mail still is missing in the sent folder, and lost for the sender.


Expected results:

There are different levels for solution:

a) the best: TB should not run into this situation, where a mail cannot be copied to the "Sent" Folder on the remote IMAP server. 

b) if this problem cannot be solved, TB should provide a way save the already sent message for the sender: This could be: 1.) A possibility to return to the edit window 2.) A possibility to save the mail in a local folder or anything else.

c) This new behaviour, where one thinks, after pressing "retry" or "OK" everything is OK, is kind of misleading...
I want to add some important information:

a) This problem of TB not to be able to copy a mail into the "Sent" Folder on the IMAP Server has nothing to do with a broken connection to the internet or that the IMAP server is not reachable. 
With another mail client on the same computer, even a TB in a virtual machine on the same PC, sending mails over the same IMAP server and the same account works flawlessly.

b) Windows Version of TB. Here it's Win 8.1 and Win 7, where I have the problems with. But in former times it was also Win XP and Win ME..
One last addition:

This problem has nothing to do with offline-mode. It occurs, while TB is online all the time and while the IMAP servers and accounts are reachable all the time.

And a correction to my last comment: 
No, Windows ME was NOT involved - this was the time of the mozilla mail+browser package.
Sorry.
Basically a duplicate of bug 28211, one of our oldest bugs.

I'm not sure abort the "new bad behaviour". Retry should retry and offer to retry again if it fails.
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
(In reply to Jorg K (GMT+2) from comment #3)

> I'm not sure abort the "new bad behaviour". Retry should retry and offer to
> retry again if it fails.

This is the "new behaviour" I mentioned: 

With the new version >= 50 of TB this behaviour has changed:

If you click "retry", then the dialog box disappears, as if the mail would have been copied successfully - and TB has not to be killed - but the mail is not beeing copied to the "Sent" folder anyhow.
OK, let's reopen this then with an adjusted summary. Any idea how to reproduce this bug to confirm it? Gene, can you assist here?
Status: RESOLVED → UNCONFIRMED
Flags: needinfo?(gds)
Resolution: DUPLICATE → ---
Summary: Copy to "Sent" folder fails - new (bad) behaviour → Alfter copy to IMAP "Sent" folder failed, "Retry" just dismisses the panel without retrying and loses the sent message (making bug 28211 even worse)
Not saying it's the same but this seems to have some commonality with bug 1364748 that I am also looking at now. With that bug the send fails and (I think) never goes to the recipient and nothing is saved (and to other apps the imap and smtp connections are still good). Also, a tb restart is required to do anything else. In this bug, not sure if he is saying the email is never actually sent or if it is just not "saved".
FWIW, I have been using tb forever and have never seen this. Have been using IMAP on tb since early 2000's and before that pop on whatever email program netscape had. However, in recent years my email sending has gone down quite a bit and I have mostly used tb on linux, if that matters.
My only idea for duplicating this is to "force" the imap server to be bad (set wrong ip address maybe) and see what happens when an email is sent. I tried that with bug 1364748 and indeed the email is "lost" unless you tell it to save sent emails to local folder. Other than that, I have no idea how to dup this.
Flags: needinfo?(gds)
Hi Gene, 
thanks that you are looking for this bug.

Referring to the "basic bug" existing since the beginning of Thunderbird, but also to the "new bad behaviour", possibly this test-scenario could lead to a reproduction:

(Unfurtunately I do not have a firwall here at my home, where I could do this):

- In a firewall like pfsense make a rule, that locks out outgoing traffic on lan on the imap-port used for the account for testint, for example 143. Leave this rule inactive.
- Now open TB and write a test message
- klick on the activation of rule for locking port 143 and wait a few seconds
- send the mail
- immediately deactivate the rule for locking port 143 and see what happens in TB ....


Just an idea - sorry that I can't check it here.

Another thing: I'm not sure, if in reality a broken IMAP connection has to be the trigger for this bug, or if there are other reasons.

Here's a summary of the circumstances, when this bug happens to me:

- Windows
- IMAP Server at service provider, for example "1und1.de"
- very stable internet connection (10/50 MBit)
- no offline mode, always online with TB
- No synchronizing of mails, headers only
- The problem has nothing to do with the size of a mail or if it has attachments
- For me the problem occurs about 1-2 times a month (while sending about 200-400 E-Mails during this time)
- In the "old" behavour this happened after the bug appeared: 
- I could endlessly klick on "retry", but nothing happened (the reaction time was VERY short, so I guess, nothing happened in the backround). With "Cancel" I could remove the dialog box. But after this:
-- TB could not access any IMAP-Folders anymore. In this case the mouse pointer showes an hour glass within some regions of the TB window. 
-- Restarting TB does not solve this, because it's process ist still running in the background.
-- I have to kill the process thunderbird.exe in the Task manager, and then start Thunderbird - then it works again.
Knut, did what you suggested with disabling port 993 (using imaps here) then sending. I see it trying to copy to Sent and sending but with the port 993 disabled, unable to save to Sent. But as soon as I re-enable 993 the message gets saved to Sent mailbox.
Then if I compose another messaage and send again and this time leaving port 993 disabled until I get the error message "There is an error saving the message to Sent. Retry?" then click OK and then enable port 993 it shortly goes ahead and works (message was already sent and goes ahead and saves to Sent folder). I have tried this several times with various timing but I can't seem to duplicate the problem with this method. It seems to recover fine.
I also clicked "cancel" at that that point and don't see a tb lockup like you describe.
I also tried to leave the smtp sending port disabled too (Port 587). As soon as I re-enable it tb goes ahead and sends the email (even after waiting a long time) and if and when imaps port is enabled it saves it to Sent folder.
Any other ideas on how to cause what you see to happen?
(In reply to Knut Singer from comment #4)
> (In reply to Jorg K (GMT+2) from comment #3)
>
> With the new version >= 50 of TB this behaviour has changed:
> 
> If you click "retry", then the dialog box disappears, as if the mail would
> have been copied successfully - and TB has not to be killed - but the mail
> is not beeing copied to the "Sent" folder anyhow.

Knut: I don't see this, at least with my imap port disabled. It takes about 2m 15s for the "There is an error saving the message to Sent. Retry?" dialog to appear. If I hit OK, a new dialog appears with title "Sending message [email subject string]" and says inside "Status: Copy failed" and a progress indicator widget plus a cancel button. This stays up for another 2m 15s and then the original dialog appears. Saying OK again repeats the "Status: Copy failed" dialog for another 2m 15s and this can repeat forever it seems as long a cancel is not clicked in either dialog. If any time during these dialogs the imap port is re-enabled, then the save to Sent finishes correctly and is visible in the folder.

Then again, maybe it is different when there is a spontaneous loss of imap connectivity like you see and not a contrived loss like I am forcing with the firewall as you suggested.

One question. Just to be clear. When this happens do you know for sure that the email is actually sent or not? Is it actually sent to the recipient and just not saved to Sent?
I checked another email client with imap port disabled. It behaves about the same as tb but if you cancel the dialog that occurs while trying to save, it returns to the edit window (unlike tb that closes the compose/edit window). The only option is to save to Drafts when you choose to close the edit window but, since imap is down, that doesn't work either. I think there was a save-as option under File that saves to the filesystem but not to a user selectable imap folder (same as on tb compose window).

So it seems the question is does tb *really* somehow internally lose the imap connection to the server while other clients on the same subnet remain connected? If so why, is this. Is it intermittent or does it require a tb restart to fix? If so, this sounds very similar to bug 1364748.

Other variants/duplicates of knut's bug (this bug) indicate that there was often a known reason for the loss of imap connectivity (bad address, wrong security setting etc). In this case, still don't want to lose the unsaved sent email. So don't want the compose dialog to spontaneously disappear and want to have options to save the email somewhere in the tb imap "filesystem". Possibly a widget could appear (like appears with right-click move/copy) if the save fails that offers a selection of all imap folders plus local folders for saving the just sent but yet un-saved email.
Others have reported a related problem when saving draft when closing compose window with imap down. After the time out (also about 2m 15s) a retry and cancel option appear. Retry tries to save again as it should. However, cancel dismisses the compose window somewhat unexpectedly. Maybe the options should be:
"close and lose work"  (same as current "cancel")
"keep composing"       (compose window just stays open)
"retry save to drafts" (same as current "retry")
"save to selected tb folder" (this invokes a folder picker widget).

If save to drafts or selected folder times-out or fails again, the same dialog re-appears.
Hi Gene,



(In reply to gene smith from comment #8)
> Knut, did what you suggested with disabling port 993 (using imaps here) then
> sending. I see it trying to copy to Sent and sending but with the port 993
> disabled, unable to save to Sent. But as soon as I re-enable 993 the message
> gets saved to Sent mailbox.
[...]

thanks for trying out my suggested test-scenario. Sorry, that this did not help to reproduce this error.
But - so we know for sure, that this bug has nothing to do with a temporarily broken IMAP-Connection.

>One question. Just to be clear. 
> When this happens do you know for sure that the email is actually sent or not? 
> Is it actually sent to the recipient and just not saved to Sent?

Yes, in 100% when I asked the receivers, if they have received this mail, they confirmed. 
So, yes: The mail is sent via SMTP, but when trying to save the message to the sent folder on the remote IMAP Server, TB locks up.
Gene, if I run into this bug the next time:
Is there something, I can check to make things perhaps more clearly? 
Open the error console or do something else?
Tried the firewall exercise again but this time on win10. Before I was using linux and iptables firewall to block outgoing port 993. The first time I tried to send with port 993 blocked by win10 firewall, it sent the message and almost immediately went to the dialog asking to retry. I hit retry (actually OK) several times and the dialog immediately repeated with no timeout. (On linux it waits for 2m 15s before asking to retry again.) But again, when I enable 993 and hit retry/OK it goes ahead and saves the email to Sent and compose window goes away.

The 2nd and subsequent times I tried to send with window f/w port 993 blocked, it sent the message and then said "copying message to sent mail folder..." with the only option being cancel. This seems to keep running indefinitely (much more than the linux timeout of 2m 15s) and if port 993 is unblocked the dialog just continues and it doesn't seem to save (looking with wireshark, there is no attempt to save during this time). But if I cancel, I then see the "retry" dialog. Since 993 is now unblocked, the retry saves the message and compose goes away, but I have to hit retry twice before it saves. 

So, even with somewhat different reactions and behavior, I am seeing the same basically correct behavior on windows when imap connection goes and comes back when sending email.

Knut, I think you said you are running win8.1. You said you don't have a firewall but I expected it has a built-in firewall that you could use to try this too. I see it on both win10 and win7-home here under control panel/firewall/advanced/outgoing rules/new rule.
Anyhow, when you see the problem again I guess the info in the "error console" might be helpful. 
Also, you might run with imap logging enabled. You do this with a couple env vars set while running tb and it dumps continuously to a file, e.g., c://imap.log. Google for "thunderbird logging" should bring up links like this that explain how: https://www.lifewire.com/pop-imap-smtp-traffic-thunderbird-1173156 The imap log file can get pretty large so you might need to deleted it every now and then.
Another thing you might do when this happens (but ideally a bit before and during) is have wireshark running and capture traffic to your imap port (993 or 143 or whatever). It might be encrypted but at least you can tell if something is really happening between your computer and the imap port on the server.
Knut: I should also ask if you are leaving windows and/or thunderbird running continuously without rebooting or restarting (or sleeping/hiberating too)? Reporter for bug 1364748 says after a few days of continuous running (no sleeps/hibernates/reboots/restarts) that he is unable to send/receive emails. Would be interested to know if that's how you operate too.
Also, can you tell me as much as possible about your exact server setting, including the "advanced" settings (just anything that is not default would be OK). Thanks!
Some interesting things in related Bug 817392. The 4th attachment, https://bugzilla.mozilla.org/attachment.cgi?id=773448, shows a dialog I never see. It implies you can return to compose window after a failure to save to Sent folder. I can find the strings shown in the attachment in tb source in files composeMsgs.properties but can't find where they are actually used any more. Is it possible the dialog in the attachment was for some reason removed from tb? Not being able to return to compose folder after cancelling a failed save to Sent is a major complaint.

Also, later on at https://bugzilla.mozilla.org/show_bug.cgi?id=817392#c20 WADA mentions that the problem of loss of imap connection to a mbox (Send, Drafts, etc) might be due to an unspecified SELECT problem. He suggested going offline and then back online, I think to "force" the select on each mailbox. At least one commenter indicated that this brought his imap connections back (https://bugzilla.mozilla.org/show_bug.cgi?id=817392#c33). There was a recent fix that I worked on to force an imap SELECT for certain servers to properly detect new emails. I don't think it has been released yet, even in beta; and not sure it would really affect this problem (weird loss of imap when saving to sent or drafts, not inability to return to compose after a failed save).

As workaround to avoid weird loss of imap, set max cached connections to 1 to force a new SELECT when another mbox is accessed. Or try the online->offline->online transition to see if that unlocks imap. 

Additional note: I have never seen any of these problems in the real world. I asked my wife too and she has never seen it either and she uses tb (currently on win8.1) much more than I do.
Bug 543746 comment 65 has another explanation by Mr. WADA about why reducing the number of cached connection down to 1 might help. He says it has something to do with the IDLE function for "instantly" being notified of new mail. Maybe doesn't have anything to do with SELECT. You might try going to 1 cached connection or disable IDLE usage by unchecking the "Allow immediate server notification when new messages arrive" for server setting. Of course, this is irrelevant if your imap server doesn't support IDLE. The only way I know to tell for sure is look at the "capability" IMAP response in the imap.log or log in with command line (telnet or openssl) to your imap server and issue the capability imap command.
He also provides some information about setting up logging here: Bug 543746 comment 46
Gene: Let me answer the simple questions, that I know "out of the box", first:

- No power saving modes are involved. Just a clean AC powered energy role. At the end of each "session" the PC is shut down completely. Always :-)

- Wireshark ssems to be too much effort to me, as this bug happens so rarely. But I will hava a look at the "thunderbird logging", that you mentioned.

- I'll try the test with my windows firewall, as you explained. But in the meantime I guess, that I will receive the same results as you did. But I'll try.

- The number of concurrent connections to the server does not seem to be relevant: sometimes I have set them to two, sometimes they use the defautl 5 connections. With 1 connection I noticed some restrictions (maybe with push? I don't remember so clearly), so I have not used this setting anymore. 

- The problem also has nothing to do with an encrypted or not encrypted connection - it happened in former times with unencrypted connections too. And it happens with StartTLS the same as with SSL encryption.

- The settings for the mentioned 1und1.de account are:
port: 143 / STARTTLS / password, normal
Check on start for new mails
Check every 10 minuts for new mails
Inform immideately whet new messages arrive
Deleting a message moves to (my trash folder on IMAP)

Storage:
Expunge when leaving
Delete Trash when leaving

(These ara my standard settings for most of my accounts)

The expanded settins are (also my standard settings at home):
- removed check from "show subscribed folders only"
- server supports subfoders -> checked
- max. number of simulteanously connections: 2


OK, that's it so far..
Gene: Let me answer the simple questions, that I know "out of the box", first:

- No power saving modes are involved. Just a clean AC powered energy role. At the end of each "session" the PC is shut down completely. Always :-)

- Wireshark ssems to be too much effort to me, as this bug happens so rarely. But I will hava a look at the "thunderbird logging", that you mentioned.

- I'll try the test with my windows firewall, as you explained. But in the meantime I guess, that I will receive the same results as you did. But I'll try.

- The number of concurrent connections to the server does not seem to be relevant: sometimes I have set them to two, sometimes they use the defautl 5 connections. With 1 connection I noticed some restrictions (maybe with push? I don't remember so clearly), so I have not used this setting anymore. 

- The problem also has nothing to do with an encrypted or not encrypted connection - it happened in former times with unencrypted connections too. And it happens with StartTLS the same as with SSL encryption.

- The settings for the mentioned 1und1.de account are:
port: 143 / STARTTLS / password, normal
Check on start for new mails
Check every 10 minuts for new mails
Inform immideately whet new messages arrive
Deleting a message moves to (my trash folder on IMAP)

Storage:
Expunge when leaving
Delete Trash when leaving

(These ara my standard settings for most of my accounts)

The expanded settins are (also my standard settings at home):
- removed check from "show subscribed folders only"
- server supports subfoders -> checked
- max. number of simulteanously connections: 2


OK, that's it so far..
Additionally:
Yesterday I noticed something interesting:

About 3 minutes after I had sent my last mail via GMX.de (without problems), I noticed, that thunderbird showed the hour glass, when I moved the cursor over the folder pane or the header pane. 

When clicking a folder, the headers of it appeared in the header pane. 

But when I clicked on a header, the preview window kept blank. 

I could change folders, and always the hader pane actualized, but no preview of a mail was visible.

Instead, as mentioned: an hour-glass (but not on the preview pane), and in the status bar the green bar showed the whole time activity (until I closed TB), and in the left corner also an activity message was shown (sorry, don't remember anymore, perhaps it was "connecting..". 

After restarting TB everything was fine again.

I guess, if I had written a mail in this situation, the mail also wouldn't be copied to the sent folder. 

And so, maybe (?):
The trigger of this bug is NOT a (temporarily) broken IMAP connection, and perhaps not some bug in the sending code, but something completely different as a third reason. And only, if this third reason occured, then the problems with sending arise (?). OK, that's my personal miracle only, I know..

I looked into the error log - but I had real problems to understand, which of the shown errors were new and which perhaps had an age of many days (?).
> The only way I
> know to tell for sure is look at the "capability" IMAP response in the
> imap.log or log in with command line (telnet or openssl) to your imap server
> and issue the capability imap command.

I checked the capabilities of the servers with openssl, and they all support idle, like in this example of imap.1und1.de:

subject=/C=DE/O=1&1 Internet SE/ST=Rheinland-Pfalz/L=Montabaur/CN=imap.1und1.de
issuer=/C=DE/O=T-Systems International GmbH/OU=T-Systems Trust Center/ST=Nordrhe
in Westfalen/postalCode=57250/L=Netphen/street=Untere Industriestr. 20/CN=TeleSe
c ServerPass DE-2
---
No client certificate CA names sent
---
SSL handshake has read 5054 bytes and written 491 bytes
---
New, TLSv1/SSLv3, Cipher is ECDHE-RSA-AES256-GCM-SHA384
Server public key is 2048 bit
Secure Renegotiation IS supported
Compression: NONE
Expansion: NONE
SSL-Session:
    Protocol  : TLSv1.2
    Cipher    : ECDHE-RSA-AES256-GCM-SHA384
    Session-ID: E91899DCD4B43C688B9B1952E66704586970235CD94EE530A232255A814919FF

    Session-ID-ctx:
    Master-Key: E431C1F8D375B2F306403E153F8ACC2B180C1EAC587B95C128B29AC117B7A5A1
D89193F5F68D0B346E764B601A42A0DF
    Key-Arg   : None
    PSK identity: None
    PSK identity hint: None
    SRP username: None
    Start Time: 1495731491
    Timeout   : 300 (sec)
    Verify return code: 19 (self signed certificate in certificate chain)
---
* OK [CAPABILITY IMAP4rev1 CHILDREN ENABLE ID IDLE LIST-EXTENDED LIST-STATUS LIT
ERAL+ MOVE NAMESPACE QUOTA SASL-IR SORT SPECIAL-USE THREAD=ORDEREDSUBJECT UIDPLU
S UNSELECT WITHIN AUTH=LOGIN AUTH=PLAIN] IMAP server ready H mieue054 3700075029
 IMAP-1N3rmK-1dwAeJ3TQu-0105aG



About the 1 connection at a time: I remember, that I had tried this some years ago to solve this problem - with no effect. 

BTW: Is there a possibility to remove by double post? I can't find any..
I double posted once too. It warned me about a "collision" but I ignored it. Don't see a way to delete or edit after posting here.

Unless "immediate" notification of new mail is essential, you might try disabling IDLE for a while by un-selecting the "inform immediately..." option. Frankly, I don't 100% understand Mr. WADA's theory on how IDLE affects the problem. I need to study it more.

I think you said you are only storing headers locally (e.g., have only INBOX.msf file and no INBOX in your .../ImapMail/). On tb on win10 I am typing on right now I store both headers and email bodies. When I disable port 993, I don't see the "hourglass" you mention and I can see emails even though I am essentially offline. However, on linux I am only storing headers. There when I disable 993 I see exactly what you describe. Spinning circle (hourglass widget) over folders and when folder selected all the email subjects etc appear in the pane. But when I click on an email from the list I get nothing in the reading pane. So I think what you see is what would happen if the connection to the imap server is unavailable for some reason. But, then again, it might be something internal to tb. This is where it might be informative to start up wireshark and see if tb is even trying to talk to the server when you select various things. I also don't know much about the content of "error console". I found recently to get full error outputs you have to rebuild tb with "debug" enabled and then look at the stuff dumped to stdout/stderr, which is mostly fairly cryptic to me.
For at least one imap server it may not really matter that sometimes a failure to save to Sent occurs when sending. When I send an email via gmail smtp, gmail smtp/imap server automatically puts the sent email in the Sent folder. I see this with port 993 blocked at my router. I get the usual tries and re-tries in tb which I cancel and the email never shows up in sent (as expected) until I unblock 993, after the tb copy to sent processing is aborted. However, charter smtp/imap server doesn't do this. Anyhow, just an observation.
(In reply to gene smith from comment #16)
> Some interesting things in related Bug 817392. The 4th attachment,
> https://bugzilla.mozilla.org/attachment.cgi?id=773448, shows a dialog I
> never see. It implies you can return to compose window after a failure to
> save to Sent folder. I can find the strings shown in the attachment in tb
> source in files composeMsgs.properties but can't find where they are
> actually used any more. Is it possible the dialog in the attachment was for
> some reason removed from tb? Not being able to return to compose folder
> after cancelling a failed save to Sent is a major complaint.

Looking closer at the above attachment above from 8 years ago, it appears the strings shown for "returning to compose" are still used. However, they never seem to be displayed -- at least I haven't found what causes it. I would assume doing "cancel" after failing to copy to Sent should invoke the strings, but it doesn't. I also found this bug from 8 years ago that fixes a display problem for these same strings: Bug 528600 that is mentioned in the hg log:

changeset:   4736:6f60894753e5
user:        Michael Lissner <mlissner>
date:        Fri Dec 11 21:03:00 2009 -0500
files:       mailnews/compose/src/nsMsgSendReport.cpp
description:
Bug 528600 - "Typo in Error Message: The message was sent successfully, but could not be copied to your Sent folder.Would you like to return to the compose window?" [r+sr=Standard8] (Also pushed to force builders to update and hopefully clear red)

All this bugfix did was add a space where the string were concatenated. So apparently 8 years ago someone saw the subject strings appear.

Although not a complete solution to this bug, not being able to return to compose window after a failed save to Sent is a common observation and sometimes a complaint. Looking through hg log and after doing various searches in bugzilla I am unable to find what has changed (apparently in last 4 years) that now precludes the dialog ["The message was sent successfully, but could not be copied to Sent folder. Would you like to return to the compose window" Cancel OK] from occurring.

Inputs from tb historians and/or expert bug searchers appreciated!
(In reply to gene smith from comment #24)
>
> Although not a complete solution to this bug, not being able to return to
> compose window after a failed save to Sent is a common observation and
> sometimes a complaint. Looking through hg log and after doing various
> searches in bugzilla I am unable to find what has changed (apparently in
> last 4 years) that now precludes the dialog ["The message was sent
> successfully, but could not be copied to Sent folder. Would you like to
> return to the compose window" Cancel OK] from occurring.

Ok, I found the change from about 4 years ago that prevents the "return to the compose window" prompt when aborting a save to Sent. 

changeset:   12562:bd5e3381ef6a
user:        Mark Banner <bugzilla@standard8.plus.com>
date:        Mon Jun 10 21:40:44 2013 +0100
files:       mailnews/compose/public/nsIMsgSend.idl mailnews/compose/public/nsMsgAttachmentData.h mailnews/compose/src/nsMsgSend.cpp
description:
Bug 724522 Don't prompt twice when we can't save a draft message. r=irving,ui-r=bwinton

Apparently this was only tested when saving to Drafts and they didn't realize it also affect saving to Sent. By making one change as seen in the following experimental patch, I was able to now see the "return to compose" prompt.

However, even with this change, aborted save to Drafts sometimes doesn't return to the compose screen. So more looking/experimenting is needed.
Just an experimental change. Much more needed.
Why would I wanna go back to the "compose" window? The email has been composed and sent. It should just be saved locally and pushed to the sent-folder silently in the background when the IMAP server is available again. There's no sense in editing the already sent email. Actually, it would give the people the impression that the email hasn't been sent (which it has) and they'll likely edit it and send it again and again.
(In reply to t_jeske from comment #27)

I never claimed this is an optimum or even a good solution. I was just curious as to why this feature, by all evidence, seemed to stop being available about 4 years ago. I found no bug report requesting the removal of this prompt.
Note also that the prompt says "your email was sent OK but save to Sent failed, would you like to return to compose?". So this lets the user know the send did occur so no reason to send again. Once back to the compose window they can save the text via copy/paste or save the email to filesystem. Again, this is not the optimum solution but the reporter of this bug did say in Comment 0 above it was better than just losing data: "TB should provide a way save the already sent message for the sender: This could be: 1.) A possibility to return to the edit window 2.) A possibility to save the mail in a local folder or anything else."

Anyhow, thanks for you input on how your would prefer this bug be fixed. 

Note also there are several aspects to this bug. Saving the email after a successful send but with imap server connection down is fairly straight forward. Another problem, which I have never seen, is that network and imap server are reported as fine but sent email *still* does not save to Sent (reported by this bug). Another problem involves the save to Drafts which is very similar and often occurs while saving to Sent is going on and probably fails for the same reasons. I have never seen a save to Drafts problem. Another related problem is unable to send or receive on any imap account after tb has been running continuously for several days or maybe weeks, Bug 1364748, and have yet to see this.
Gene, thanks for looking into it. When you refer to ancient bugs, please paste a link to the respective changeset, so we can see quickly what was changed:
Bug 528600 - https://hg.mozilla.org/comm-central/rev/6f60894753e5
Bug 724522 - https://hg.mozilla.org/comm-central/rev/bd5e3381ef6a
I think you were considering undoing this change here:
https://hg.mozilla.org/comm-central/rev/bd5e3381ef6a#l3.124

I think the plan "B" called for in bug 28211 is to simply save the failed message in a local folder. Later we can add bells and whistles to move it back to an IMAP folder once available again. As a first cut users would be very happy not to *lose* their messages. Note the patch in bug 28211.

Personally I'd just add a folder with the account name of the IMAP account to local folders and place the message into a subfolder Drafts or Sent in there after prompting the user. So the prompt becomes:
Retry, Cancel, Store in local folder YourAccount/Sent.
(In reply to gene smith from comment #14)

> Also, you might run with imap logging enabled. You do this with a couple env
> vars set while running tb and it dumps continuously to a file, e.g.,
> c://imap.log. Google for "thunderbird logging" should bring up links like
> this that explain how:
> https://www.lifewire.com/pop-imap-smtp-traffic-thunderbird-1173156 The imap
> log file can get pretty large so you might need to deleted it every now and
> then.

I've tried to enable logging via the instructions of the above link.
To make it simple for me to ALWAYS activate this logging while starting TB I wrote the commands into a cmd batch file. 
It looks (exactly) like this:

set NSPR_LOG_MODULES=IMAP:4
set NSPR_LOG_FILE=C:\TB-IMAP.log
start thunderbird
pause

Unfortunately no log file is generated. 
Has anybody any idea what I should correct/change?

Thanks..
(In reply to Knut Singer from comment #30)
> (In reply to gene smith from comment #14)

It doesn't work for me either with tb 52.1.1 (32-bit) on windows. No log file created. There is recent change of names (*):
s/NSPR_LOG_MODULES/MOZ_LOG/
s/NSPR_LOG_FILE/MOZ_FILE/
which was to only change at tb 55. Tried them and they didn't work either. 

Maybe Jorg can help!

P/S: I continuously run logging on linux using very close the the HEAD version. No problems with the NSPR_* style vars.
(*) https://wiki.mozilla.org/MailNews:Logging#Logging_prior_to_Thunderbird_55.0a1_.2F_SeaMonkey_2.52a1
Flags: needinfo?(jorgk)
All I know is that we're looking at these in bug 1353919. There you have the current set and remember that they are now case sensitive since bug 1222244.
Flags: needinfo?(jorgk)
(In reply to Jorg K (GMT+2) from comment #29)
Thanks for putting some parameter and requirements on this which sound reasonable. 
Yes, I saw the patch in the original bug but haven't looked at the code. Does something with 'outbox' which I'm not sure where it even is (probably Local Folders). 
Reading the bug again, I see some comments suggest rebuilding the Sent mbox when a failure to save occurs. I am seeing with gmail that if copy to Sent fails (socket disabled with firewall) you have to start tb twice (on first restart, all gmail folders are gone) and it "rebuilds" all the gmail mboxes (re-downloads them again). Not sure if this happens for other accounts.
(In reply to gene smith from comment #33)
> (In reply to Jorg K (GMT+2) from comment #29)

> Does something with 'outbox' which I'm not sure where it even is (probably
> Local Folders). 

Concerning to "outbox" I can give you a hint:

In the settings of TB you can set a key, that changes the behaviour of TB for sending Mails:

If activated (what is not the default), Thunderbird first copys the mail, that has to be sent to a local folder "outbox".
And then, after this, TB sens the mail first via SMT, then copies it to the IMAP Sent folder, and finally it deletes the mail from the outbox folder.

The advantage of this ist, that large mails with big attachments on very slow internet connections do not prevent the user from writing the next E-Mail. Instead TB acts, as if the mail would have been sent already. This behaviour is like that of Outlook, that transmits E-Mails in the background, too.

The disadvantages of this method are:
- The user does not recognize immideately, when mails could not be sent or are not sent already (when shutting down the PC)
- He does not see an error message, why a message could not be sent
- A not sent E-Mail prevents all future E-Mails from beeing sent.

So this method of sending is not the best of all, but helps in envirementments with huge attachments and slow internet connections.

The local folder "outbox" is created automatically by TB, if it does not exisit.
Actually, to activate the use of the local "outbox" folder you need to add this "key":

user_pref("mailnews.sendInBackground", true);
Knut, Thanks for info on Outbox.  Were you ever able to get the logging to work? The last time I tried several days ago with the tb release version on window, no luck. Tried again today, still no luck.
> Knut, Thanks for info on Outbox.  Were you ever able to get the logging to
> work? The last time I tried several days ago with the tb release version on
> window, no luck. Tried again today, still no luck.

Hi Gene, 

no, unfortunatelly I did not manage, too, to get a log file from Thunderbird. 
But, fortunatelly, until today the error of losing a sent mail did not occur again until now. (But I`m sure, it will..)

Perhaps this outbox-behaviour could be a workaround for this problem?

So, that thunderbird *always* first copies the mail to the local "outbox" folder. And then sends the mail. But without spooling (=sending in the backround) if the  switch "mailnews.sendInBackground" is not set to true. Otherwise of cours with spooling. 

So after a restart the mails would still be there and sent a second time. The only disadvantage would be, that the receiver would get the mail a second time - but I think this is better than if the sender loses the mail..
(let's have more eyes)
Component: Untriaged → Networking: IMAP
Product: Thunderbird → MailNews Core
Version: 52 Branch → 52
(In reply to Knut Singer from comment #37)
Knut, I put in this bug 1371498 for unable to produce imap log. However, I it seem my problem was that I put quotes around the log filename. Your problem may be that you are trying to create the log at the root of C:\ per comment 30 above. I got permission errors when I tried to create the file at that location, so you might try another location like c:\Users\knut\ . Might also reboot first (but I doubt that matters).
Attached patch 1366591-review-changes0.patch (obsolete) — — Splinter Review 
This is first cut at Jorg's suggestion in Comment 29. There are several things that may need tweaking, fixing and improving:
1. The prompt to save to Local Folders is a 2 step prompt because I could only find a way to do a "binary" prompt from C++, e.g., OK and Cancel. I didn't see an existing function to allow "tertiary" choices, e.g., OK, Cancel and Save. (This does exist in *.js code however, like when saving drafts: Save, Don't Save and Cancel.)
2. When IMAP connection is down, attempts to save Drafts and Sent can occur together. This doesn't prevent saving to Local Folder/<my acccount> but it seem a bit confusing (and maybe redundant). Also, keeps trying to save drafts based on the autosave timer even after you have saved to Local Folders.
3. I made the Drafts and Sent saving strings a bit more verbose. They require more reading but I think they explain better what clicking on each button does. 
4. After the first or second retry of saving to Sent, the progress box vanishes until it times out with no opportunity to Cancel. Have to wait for the time out before you can manually save to Local Folders. This occurred even without this patch.
5. I have left in several printf's for debug/tracing. They will be removed in future patches.
6. Haven't tried this with the "Send later" or "send in background" modes.
Attachment #8872216 - Attachment is obsolete: true
Severity: normal → critical
Keywords: dataloss
See Also: → 28211
Summary: Alfter copy to IMAP "Sent" folder failed, "Retry" just dismisses the panel without retrying and loses the sent message (making bug 28211 even worse) → After copy to IMAP "Sent" folder failed, "Retry" just dismisses the panel without retrying and loses the sent message (making bug 28211 even worse)
Attached patch 1366591-review-changes1.patch (obsolete) — — Splinter Review 
Found a function that allows 3 buttons from C++, ConfirmEx(). Therefore the 1st item in my list in Comment 40 is resolved since there is now only one prompt.

Also, instead of just saving to Local Folders/<account> both drafts and sent messages, I now save to Local Folders/Sent-<account> or Local Folders/Drafts-<account>. I wanted to save to Local Folders/<account>/Sent and Local Folders/<account>Drafts but it would not automatically create a three-level folder tree. This would only work if I manually created the folders first with the UI when I used a 3-level uri strings.

Adding another item to my list from Comment 40:
7. There are some hardcoded strings in the code such as "Drafts-" and "Sent-" and others that may need to be obtained from a string file so they can be adjusted for locale.
Attachment #8880255 - Attachment is obsolete: true
So you'll ask for feedback or review once you're ready, right? How do we test this? Start a draft that would be saved in an IMAP folder, then pull out the network cable?
Sure, review at any time is fine. I did put r=jorgk in the patch comment, is there more I should do?

To test fail to "save to Sent" I just disable connections to remote imap port 993 using the local firewall. I keep the smtp port for sending enabled. Then I try to send an email and improved dialog appears, after a timeout, about failure to save to Sent but the send did work and prompting to save to Local Folders. You can also click Cancel during this timeout to cause the improved dialog to appear sooner.

I basically do the same thing for testing "save to drafts", that doesn't involve sending, which also fails when port 993 is disabled. To get some quicker response I set the pref for autosave down from default of 5m to 1m. I usually do "edit as new" on an existing email to test, but you have to make some text change before drafts will attempt to be saved. You can also make a change and close the Write window to trigger the save attempt using an existing dialog. Only after the network times out does it put up the improved dialog to save to Local Folders; there is no way to cancel or shorten this timeout as is possible when sending.

So, just removing your ethernet cable (or disable wifi) will mainly be useful only when testing "save to drafts" since a send never occurs. However, to be honest, I've never actually tried that to see what happens.
In general you request feedback or review by clicking "Details" or when attaching a patch.

I might try the patch when a find a spare 15 minutes. So far I've been waiting since you haven't cleared your list of open issues (comment #40, comment #41).
Not sure that all are problems and probably others I have missed so try it when you have a chance. -gene
(In reply to gene smith from comment #43)
> So, just removing your ethernet cable (or disable wifi) will mainly be
> useful only when testing "save to drafts" since a send never occurs.
> However, to be honest, I've never actually tried that to see what happens.

I have now tried it with wi-fi disconnected and seems OK to me. You still have the opportunity to save your message as an email file to the filesystem or to your account's drafts folder which will fail and then offer the option to save to Local Folders. In any case, the Write window remains open and, as a last resort, you can always copy and paste.
So, is this meant to fix bug 28211 as well by offering a "Plan B"?
(In reply to Jorg K (GMT+2) from comment #47)
> So, is this meant to fix bug 28211 as well by offering a "Plan B"?

I think it definitely fixes the reported problem bug 28211. Don't really understand the meaning of "Plan B" but per your defintion from above,

>I think the plan "B" called for in bug 28211 is to simply save the failed message in a local folder. Later we can add bells and whistles to move it back to an IMAP folder once available again. As a first cut users would be very happy not to *lose* their messages. Note the patch in bug 28211.

>Personally I'd just add a folder with the account name of the IMAP account to local folders and place the message into a subfolder Drafts or Sent in there after prompting the user. So the prompt becomes:
>Retry, Cancel, Store in local folder YourAccount/Sent.

it does that using your preference in the 2nd paragraph. I don't think it needs the "bells and whistles" to automatically move the messages saved in Local Folders back to Sent. The user can just do that manually with Copy or Move right-click menus when IMAP connection comes back (or after a TB restart if necessary).

It also fixes the problem of saving to Drafts which bug 28211 doesn't directly address, at least by the original reporter.
 
Also, I haven't considered implementing or studied in detail the patch mentioned in bug 28211 since it tries to fix the problem with automatic retries and/or deferred sends from OUTBOX which seems overly complex, i.e., bells/whistles.
I'll look at it, I'll put NI so I won't forget. Looking at the patch in bug 28211, attachment 381502 [details] [diff] [review], there was a new test. Are you planning to add this test here?
Flags: needinfo?(jorgk)
Comment on attachment 8880721 [details] [diff] [review]
1366591-review-changes1.patch

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

A few nits I spotted. I'll try it now.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +113,5 @@
>  ## Strings used for the save message dialog shown when the user closes a message compose window
>  saveDlogTitle=Save Message
>  
>  ## LOCALIZATION NOTE (SaveDlogMessages): %1$S is the folder name
> +saveDlogMessages=\Message has not been sent or saved in your drafts folder (%1$S)\n"Save" copies the message to your %1$S folder and closes the Write window.\n"Don't Save" closes the Write window without saving.\n"Cancel" allows you continue writing without saving.

You need to make this saveDlogMessages2.

@@ +332,5 @@
>  removeAttachmentMsgs=Remove Attachment;Remove Attachments
>  
>  ## LOCALIZATION NOTE(errorSavingMsg): Do not translate the word %S. It
>  ## will be replaced with the name of the folder the message is being saved to.
> +errorSavingMsg=There was an error saving the message to folder %2$S/%1$S.\nClick OK to retry.\nClick Cancel for opportunity to save locally.

Again, append 2.

::: mailnews/compose/src/nsMsgSend.cpp
@@ +294,5 @@
>    mCompFieldRemoteAttachments = 0;
>    mMessageWarningSize = 0;
>  
>    mSendReport = new nsMsgSendReport();
> +  m_server = nullptr;

No need to initialise nsCOMPtr in the constructor.

@@ +3833,5 @@
>      nsCOMPtr<nsIStringBundle> bundle;
>      rv = bundleService->CreateBundle("chrome://messenger/locale/messengercompose/composeMsgs.properties", getter_AddRefs(bundle));
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    if (m_server == nullptr)

if (!m_server)

@@ +4284,5 @@
>    bool          folderIsLocal = true;
>    nsCString     turi;
>    char16_t     *printfString = nullptr;
>    nsString msg;
> +  nsCOMPtr<nsIMsgFolder> folder = nullptr;

No need to initialise nsCOMPtr.

@@ +4374,5 @@
> +    // typically because imap connection is down. Folder is created if
> +    // it doesn't yet exist.
> +
> +    // Make sure m_server has been set. Need this to get account name.
> +    if (m_server == nullptr)

!m_server
Another nit:
"Cancel" allow you to continue writing without saving your draft.

I did two things:
I wrote a message and disconnected the network before trying to save the message.
I get a nice prompt, and all three options work.

Next I sent myself a message with a 5 MB attachment. I waited until the message was sent and disconnected the network while "Copying message to Sent folder" was on the screen. That was basically hung. I pressed cancel and up came the nice panel.

It doesn't make sense to offer "Cancel" to continue writing. The message was sent, so no need to keep the write window open, IMHO. So I clicked "Save" and got a copy in "Sent-...". I also got a copy in "Drafts-...", hmm, it got a time stamp of a few minutes later, so maybe some auto-save kicked in since the compose window was still open.

I also got this error:
  Your message has been sent and saved, but there was an error while running message filters on it.

So I think we're on the right track here, but not quite there yet.
Flags: needinfo?(jorgk)
The last err due to @ in account name. Will check others when inside. -g
(In reply to Jorg K (GMT+2) from comment #51)
> Another nit:
> "Cancel" allow you to continue writing without saving your draft.

OK, I see the typo: s/allow/allows/

> 
> I did two things:
> I wrote a message and disconnected the network before trying to save the
> message.
> I get a nice prompt, and all three options work.

Good!

> 
> Next I sent myself a message with a 5 MB attachment. I waited until the
> message was sent and disconnected the network while "Copying message to Sent
> folder" was on the screen. That was basically hung. I pressed cancel and up
> came the nice panel.
> 
> It doesn't make sense to offer "Cancel" to continue writing. The message was
> sent, so no need to keep the write window open, IMHO. So I clicked "Save"
> and got a copy in "Sent-...". I also got a copy in "Drafts-...", hmm, it got
> a time stamp of a few minutes later, so maybe some auto-save kicked in since
> the compose window was still open.

You should only see the "cancel to continue writing" in the the save to drafts prompt. When the autosave timer triggers (default 5m) you will get the save to draft prompt after a while too, so that must be where you see this. Also, there is no autosave to Local Folders/* so you must have done two saves to local folders with the new dialogs (first with "Your message was sent but not saved..." and then with the "Your draft message was not saved..." prompts). Could this explain it? This dual prompting was my issue 2 in comment 40.

> 
> I also got this error:
>   Your message has been sent and saved, but there was an error while running
> message filters on it.

Yes, I see that too. I always see that prompt twice in succession after saving to local. However, I don't see it when I get rid of the @ in my account name, e.g., s/eugene_smith@charter.net/eugene_smithATcharter.net. (Note: If I change my configured "Sent" folder to have an @ in the name, it saves OK to it without giving that prompt with the network completely up, regardless if it is a local folder or an imap folder.)

> 
> So I think we're on the right track here, but not quite there yet.

Good to know.
The weird filter error is detected in the call to FilterSentMessage() in nsMsgSend.cpp after the save to local folders. The bad "rv" is passed to OnStopOperation(rv) which just pops up the message. The error stack seems to be this:

NotifyListenerOnStopCopy() <-- nsMsgSend.cpp
  FilterSentMessage()      <-- nsMsgSend.cpp
    GetExistingFolder()    <-- nsMsgUtils.cpp
      GetFolderForURL()  <-- virtual with no c++ definition I can find
(maybe) getFolderForURL() <-- a JS function from folderLookupService.js

Question: does c++ GetFolderForURL() "call" JS getFolderForURL()? If not, where is the c++ code? Regardless, why is URL with @ in it rejected by GetFolderForURL() / getFolderForURL()? 

(Poking around, I found this Bug 453908 that touches getFolderForURL() but probably not relevant.)
(In reply to gene smith from comment #54)
> Question: does c++ GetFolderForURL() "call" JS getFolderForURL()?
Yes, defined in nsIFolderLookupService.idl and implemented in folderLookupService.js.

> Regardless, why is URL with @ in it rejected by
> GetFolderForURL() / getFolderForURL()?
I don't know, you'd have to debug it.
 
> (Poking around, I found this Bug 453908 that touches getFolderForURL() but
> probably not relevant.)
Yes, we're going to discontinue using RDF in the long term.

More answers later, gotta run now.
This may be moot. See last paragraph...
I changed my account name to, literally, eugene_smith%40charter.net and it saves to Local Folders/Sent-eugene_smith@charter.net and no filter problems occur. It also creates folder L.F./Sent-eugene_smith@charter.net if it doesn't exist (with literal @).
The bad url is this which I dumped from js getFolderForURL():
mailbox://nobody@Local%20Folders/Sent-eugene_smith@charter.net
But this one, also dumped, does not produce the weird filter prompts:
mailbox://nobody@Local%20Folders/Sent-eugene_smith%40charter.net

So should m_folderName (actually the full url) with "invalid" characters be replaced with %XX before passing to GetExistingFolder() or should the user just not use use @ (or other "invalid" chars) in their account name? 

I found this in nsIfolderLookupService.idl that provides an "interface" for getFolderForURL():

 * - invalid characters in paths must be percent-encoded
 * - the URL MUST NOT have a trailing slash (excepting root folders)
 * - the case must match the expected value exactly
 * An example of a valid URL is thus:
 * imap://someuser%40google.com@imap.google.com/INBOX

Don't know why the first @ is encoded as %40 but the 2nd one is still just @. Other then @ and space, what are other invalid chars?
:
After writing all of the above and looking closer, I think a better solution might just be to skip the call to FilterSentMessage() if "saveLocally" is done. Maybe filtering in these new local (sort of temporary) folders is not really necessary? Is it?
I'd hope that in those emergency/makeshift folders no filtering took place. If it were a sent filter, where would the filter likely move the message? To another IMAP folder - not very likely successful.

I'd have to dig though the other issues, like the percent encoding, etc. Since the basic format of a URI is:
  scheme://username:password@hostname:portnumber/pathname?query#ref
I can imagine that two @ create an invalid URI.

As for your problem with folder creation: Just pass the string through:
  MsgEscapeURL(org, nsINetUtil::ESCAPE_URL_QUERY, escaped);
since you're saying that then the correct folder is created and no filter prompts appear.
(In reply to Jorg K (GMT+2) from comment #57)

> As for your problem with folder creation: Just pass the string through:
>   MsgEscapeURL(org, nsINetUtil::ESCAPE_URL_QUERY, escaped);
> since you're saying that then the correct folder is created and no filter
> prompts appear.

Actually, no problem with "folder creation" just the @ in in the folder name triggers two warnings about "can't filter".

Anyhow, tried the MsgEscapeURL() function you pointed out but, trying many combinations of ESCAPE_URL_* flags, I could never get it to change the @ to %40 in the folder name. The best I could get it to do was escape a blank in the folder name with %20. Apparently that function doesn't consider @ as a character needing escaped.

So rather than running the whole URL through MsgEscapeURL(), I just ran the account name that potentially contains the @ through a different function called MsgEscapeString() that works. E.g., it converts the account string "eugene_smith@charter net" to "eugene_smith%40charter%20net". Then since the account string is appended to either "Sent-" or "Drafts-" to make the folder name, it works with no filtering complaints. This doesn't affect the strings display in the UI (still see literal @ and the blank, not %40 or %20).

(In reply to gene smith from comment #56)

> This may be moot. See last paragraph...
<snip to last paragraph>
> After writing all of the above and looking closer, I think a better solution
> might just be to skip the call to FilterSentMessage() if "saveLocally" is
> done. Maybe filtering in these new local (sort of temporary) folders is not
> really necessary? Is it?

Skipping this call was harder than I expected due to what looks like recursion going on. So just fixing this by escaping the @ in the folder name is much better.

I will shortly submit another patch incorporating your review comments, this escaping fix and adding support for "Send Later".
Observation: When saving to Drafts or when "Sent Later" is triggered via right-click on Outbox, unlike when saving to Sent, there is no progress dialog that pops up with the option to Cancel the save. Most of the time this is OK in that the save times-out when the connection to the IMAP server is broken and this triggers the dialog to save locally (sometimes instantly and other times after 2-5 minutes). However, sometimes this timeout never occurs to trigger the dialog so a local save is not possible since the dialog never appears. I only seem to see this "hung" situation after several successful local saves with the connection down.

Also, while hung, when I re-establish the IMAP connection, the save to IMAP Sent or Drafts does not occur until a new folder selection is made.

You can always save to Local when you are doing a normal send since you have the Cancel option that triggers the (new) save to local dialog. However, for Drafts and Send Later the save to local dialog may never occur unless the timeout occurs to trigger the dialog, and sometimes this timeout that triggers the dialog never occurs.

I don't think this is caused directly by anything I have done since the dialogs I added are triggered by the timeout or Cancel. 

Also, I don't know how much of a problem this really is.
(In reply to Jorg K (GMT+2) from comment #50)
> Comment on attachment 8880721 [details] [diff] [review]
> 1366591-review-changes1.patch
> 
> Review of attachment 8880721 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A few nits I spotted. I'll try it now.
> 
> :::
> mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
> @@ +113,5 @@
> >  ## Strings used for the save message dialog shown when the user closes a message compose window
> >  saveDlogTitle=Save Message
> >  
> >  ## LOCALIZATION NOTE (SaveDlogMessages): %1$S is the folder name
> > +saveDlogMessages=\Message has not been sent or saved in your drafts folder (%1$S)\n"Save" copies the message to your %1$S folder and closes the Write window.\n"Don't Save" closes the Write window without saving.\n"Cancel" allows you continue writing without saving.
> 
> You need to make this saveDlogMessages2.

OK. And modified localization note. Also, removed typo (leading \) and improved the string.

> 
> @@ +332,5 @@
> >  removeAttachmentMsgs=Remove Attachment;Remove Attachments
> >  
> >  ## LOCALIZATION NOTE(errorSavingMsg): Do not translate the word %S. It
> >  ## will be replaced with the name of the folder the message is being saved to.
> > +errorSavingMsg=There was an error saving the message to folder %2$S/%1$S.\nClick OK to retry.\nClick Cancel for opportunity to save locally.
> 
> Again, append 2.

Removed errorSavingMsg. Replaced by promptToSaveSentLocally and promptToSaveDraftLocally and added localization notes and improved the string.

> 
> ::: mailnews/compose/src/nsMsgSend.cpp
> @@ +294,5 @@
> >    mCompFieldRemoteAttachments = 0;
> >    mMessageWarningSize = 0;
> >  
> >    mSendReport = new nsMsgSendReport();
> > +  m_server = nullptr;
> 
> No need to initialise nsCOMPtr in the constructor.

Ok.

> 
> @@ +3833,5 @@
> >      nsCOMPtr<nsIStringBundle> bundle;
> >      rv = bundleService->CreateBundle("chrome://messenger/locale/messengercompose/composeMsgs.properties", getter_AddRefs(bundle));
> >      NS_ENSURE_SUCCESS(rv, rv);
> >  
> > +    if (m_server == nullptr)
> 
> if (!m_server)
> 
> @@ +4284,5 @@
> >    bool          folderIsLocal = true;
> >    nsCString     turi;
> >    char16_t     *printfString = nullptr;
> >    nsString msg;
> > +  nsCOMPtr<nsIMsgFolder> folder = nullptr;
> 
> No need to initialise nsCOMPtr.

Ok. (I haven't researched it, but I suppose this means the default constructor for nsCOMPtr<> creates it null.)

> 
> @@ +4374,5 @@
> > +    // typically because imap connection is down. Folder is created if
> > +    // it doesn't yet exist.
> > +
> > +    // Make sure m_server has been set. Need this to get account name.
> > +    if (m_server == nullptr)
> 
> !m_server

OK. (changed to !m_server in several places)

New patch on the way.
Attached patch 1366591-review-changes2.patch (obsolete) — — Splinter Review 
Please review. This adds support for deferred send from Outbox of messages stored with "Send Later". It also adds string text improvements and fixes Jorg's review comments he submitted for patch 1. In addition, this fixes the extraneous "cannot filter" status messages that pop up because the local folder names where sent and drafts are stored may contained the @ character, which is now escaped to %40.
Still contains several temporary printf("gds:...")'s for tracing/debugging. Also files in the "suite" area have not yet been updated.
Attachment #8880721 - Attachment is obsolete: true
Comment on attachment 8882510 [details] [diff] [review]
1366591-review-changes2.patch

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

Some comments. I'll try it now.

::: mailnews/compose/src/nsMsgSend.cpp
@@ +3844,3 @@
>        {
> +        // This call only used to init m_server, don't care about saving to folder.
> +        CanSaveMessagesToFolder(fcc);

This is a hack, right? Why not write a function: GetIncomingServer() or similar? And split CanSaveMessagesToFolder() so it calls the new function first.

@@ +3905,5 @@
> +                     mCompFields->GetBcc(),
> +                     nullptr,
> +                     mCompFields->GetNewspostUrl());
> +
> +      if (!NS_SUCCEEDED(rv))

!NS_SUCCEEDED() is NS_FAILED(), just mentioning, since the debug code will go.

@@ +3914,5 @@
> +    // locally. So we're just going to call Fail and close the window.
> +    // However, give Fail a success code so that it doesn't prompt
> +    // the user again since they already know about the failure and have
> +    // reacted. But keep compose/write window open if save was a draft.
> +    if (m_deliver_mode != nsMsgSaveAsDraft)

What about save as template?

@@ +4387,5 @@
> +    NS_NAMED_LITERAL_STRING(localFolderUri, "mailbox://nobody@Local%20Folders/");
> +
> +    nsAutoString folder = (nsAutoString)localFolderUri;
> +    if (m_deliver_mode == nsMsgSaveAsDraft)
> +      folder += u"Drafts-";

This should be localised.

@@ +4389,5 @@
> +    nsAutoString folder = (nsAutoString)localFolderUri;
> +    if (m_deliver_mode == nsMsgSaveAsDraft)
> +      folder += u"Drafts-";
> +    else
> +      folder += u"Sent-";

And this.

::: mailnews/compose/src/nsMsgSend.h
@@ +370,5 @@
>    bool                    mGUINotificationEnabled;      // Should we throw up the GUI alerts on errors?
>    bool                    mAbortInProcess;              // Used by Abort to avoid reentrance.
>  
>    nsCOMPtr<nsIMsgComposeSecure> m_crypto_closure;
> +  nsCOMPtr<nsIMsgIncomingServer> m_server;              // Use to obtain account name for local storage

I'm not sure I like this. The send object is now holding a reference to an incoming server.
Comment on attachment 8882510 [details] [diff] [review]
1366591-review-changes2.patch

I quite like this. I've tested interrupting saving a draft and moving a sent message to the Sent folder by pulling out the network cable.

In both cases I got a suitable prompt and the saving worked nicely.

One unrelated problem: When pulling out the network cable during the "move to Sent", the prompt hangs and I need to hit cancel to get the choices presented to me. It would be good if the system could give up after - say - 30 seconds and present the choices panel.
Attachment #8882510 - Flags: feedback+
Comment on attachment 8882510 [details] [diff] [review]
1366591-review-changes2.patch

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

More nits.

::: mailnews/compose/src/nsMsgSend.cpp
@@ +4283,4 @@
>    char          *ibuffer = nullptr;
>    uint32_t      n;
>    bool          folderIsLocal = true;
>    nsCString     turi;

While we're there, please change this to tmpURI, "turi" sounds like a tourist.

@@ +4298,2 @@
>      if (canceled)
>        return NS_ERROR_ABORT;

This is now dead code that needs to be removed.
(In reply to Jorg K (GMT+2) from comment #63)
> One unrelated problem: When pulling out the network cable during the "move
> to Sent", the prompt hangs and I need to hit cancel to get the choices
> presented to me. It would be good if the system could give up after - say -
> 30 seconds and present the choices panel.

This is what I was referring to in comment 59. It can be a bigger problems when saving to Draft or sending from Outbox because there is no prompt to cancel. So if it hangs it just hangs forever (or until you restart tb). I have not been able to figure you why it sometimes hangs and, at this time, have no idea how and where to put a timeout timer (30s may be a bit short since I see timeouts from between 0s and 5m; if > 5m it never times out it seems). Timeout occurs when NotifyListenerOnStopCopy() is called in function OnStopCopy() in nsMsgCopy.cpp.
(In reply to Jorg K (GMT+2) from comment #62)
> Comment on attachment 8882510 [details] [diff] [review]
> 1366591-review-changes2.patch
> 
> @@ +4387,5 @@
> > +    NS_NAMED_LITERAL_STRING(localFolderUri, "mailbox://nobody@Local%20Folders/");
> > +
> > +    nsAutoString folder = (nsAutoString)localFolderUri;
> > +    if (m_deliver_mode == nsMsgSaveAsDraft)
> > +      folder += u"Drafts-";
> 
> This should be localised.

Are you referring only to u"Drafts-" here? Or do you mean also "mailbox://nobody@Local%20Folders/" should be localized? I see the string "mailbox://..." in mailnews/mailnews.js such as ...default.fcc_folder and ...default.sendlater_uri that serve a similar purpose that don't seem to be also localized.
Usually the review comment applies to the line immediately above, so "Drafts" should be localised, no? So you need to get the localised version of the string.
(In reply to Jorg K (GMT+2) from comment #62)
> Comment on attachment 8882510 [details] [diff] [review]
> 1366591-review-changes2.patch
> 
> Review of attachment 8882510 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some comments. I'll try it now.
> 
> ::: mailnews/compose/src/nsMsgSend.cpp
> @@ +3844,3 @@
> >        {
> > +        // This call only used to init m_server, don't care about saving to folder.
> > +        CanSaveMessagesToFolder(fcc);
> 
> This is a hack, right? Why not write a function: GetIncomingServer() or
> similar? And split CanSaveMessagesToFolder() so it calls the new function
> first.

Done. See elimination of m_server variable below.

> 
> @@ +3905,5 @@
> > +                     mCompFields->GetBcc(),
> > +                     nullptr,
> > +                     mCompFields->GetNewspostUrl());
> > +
> > +      if (!NS_SUCCEEDED(rv))
> 
> !NS_SUCCEEDED() is NS_FAILED(), just mentioning, since the debug code will
> go.

Changed NS_FAILED() here. However, still haven't seen a failure here.

> 
> @@ +3914,5 @@
> > +    // locally. So we're just going to call Fail and close the window.
> > +    // However, give Fail a success code so that it doesn't prompt
> > +    // the user again since they already know about the failure and have
> > +    // reacted. But keep compose/write window open if save was a draft.
> > +    if (m_deliver_mode != nsMsgSaveAsDraft)

Actually, this check for m_deliver_mode is now removed and Fail() is always called here. This really had no effect on compose/write window saying open.

> 
> What about save as template?

Yes, added a new prompt for m_deliver_mode nsMsgSaveAsTemplate. It works exactly the same as drafts but saves to (typically) Local Folders/Templates-<account>/ 

> 
> @@ +4387,5 @@
> > +    NS_NAMED_LITERAL_STRING(localFolderUri, "mailbox://nobody@Local%20Folders/");
> > +
> > +    nsAutoString folder = (nsAutoString)localFolderUri;
> > +    if (m_deliver_mode == nsMsgSaveAsDraft)
> > +      folder += u"Drafts-";
> 
> This should be localised.

OK, per your earlier response I will keep the "mailbox://..." string above as is.
See next comment for localization details.

> 
> @@ +4389,5 @@
> > +    nsAutoString folder = (nsAutoString)localFolderUri;
> > +    if (m_deliver_mode == nsMsgSaveAsDraft)
> > +      folder += u"Drafts-";
> > +    else
> > +      folder += u"Sent-";
> 
> And this. 

I have made a significant change here. Now instead of literal string "Drafts-" and "Sent-" I now get the actual folder names using function mSavedToFolderName.get(). This will typically be mailbox://..../Drafts or mailbox://..../Sent etc. However, you can configure where you save drafts, sent or templates to any folder with any name, even on another server. So doing this eliminates the need to localize these strings since they no longer exist. 

> 
> ::: mailnews/compose/src/nsMsgSend.h
> @@ +370,5 @@
> >    bool                    mGUINotificationEnabled;      // Should we throw up the GUI alerts on errors?
> >    bool                    mAbortInProcess;              // Used by Abort to avoid reentrance.
> >  
> >    nsCOMPtr<nsIMsgComposeSecure> m_crypto_closure;
> > +  nsCOMPtr<nsIMsgIncomingServer> m_server;              // Use to obtain account name for local storage
> 
> I'm not sure I like this. The send object is now holding a reference to an
> incoming server.

I have removed the m_server variable. It is now replaced with nsString m_accountName that is set by the new function GetIncomingServer() that returns pointer to server which, as before, is again just a local (automatic) variable.

Updated patch on the way.
Attached patch 1366591-review-changes3.patch (obsolete) — — Splinter Review 
Please review. Addresses most of the comments and critiques from Jorg.
Attachment #8882510 - Attachment is obsolete: true
Let's get the incoming server this way. I've never seen an nsCOMPtr returned as a function value.
Attachment #8882972 - Attachment is obsolete: true
Attachment #8882981 - Flags: review?(jorgk)
Tweaked error handling slightly.
Attachment #8882981 - Attachment is obsolete: true
Attachment #8882981 - Flags: review?(jorgk)
Comment on attachment 8882982 [details] [diff] [review]
1366591-review-changes3.patch - tweaked by JK (take 2)

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

This looks OK now, only two comment nits. I'll try it now. You'd need to remove the debug prints and then present it for a final review, OK?

::: mailnews/compose/src/nsMsgSend.cpp
@@ +3644,5 @@
>    if (NS_FAILED(rv) || !thisFolder)
> +    return rv;
> +
> +  // Obtain server which is also used to create and name a local folder
> +  // when saving to default folder fails. This avoids re-doing all the above steps.

This comment needs to go.

::: mailnews/compose/src/nsMsgSend.h
@@ +206,2 @@
>    // Check to see if it's ok to save msgs to the configured folder.
> +  // This calls GetIncomingServer()

Lose this comment.
Comment on attachment 8882982 [details] [diff] [review]
1366591-review-changes3.patch - tweaked by JK (take 2)

Works just as well as the previous versions. If you could kindly remove the debug statements, the next version can get my r+, but I will ask Aceman to take a look at it, too. Altogether, it's great that we finally tackled this annoying bug that has upset so many people.

Oh, I forgot, can we port the test from bug 28211 or should I do this myself or ask Aceman to do it.
Attachment #8882982 - Flags: feedback+
(In reply to Jorg K (GMT+2) from comment #72)
> Comment on attachment 8882982 [details] [diff] [review]
>
> > +
> > +  // Obtain server which is also used to create and name a local folder
> > +  // when saving to default folder fails. This avoids re-doing all the above steps.
> 
> This comment needs to go.
> 
> ::: mailnews/compose/src/nsMsgSend.h
> @@ +206,2 @@
> >    // Check to see if it's ok to save msgs to the configured folder.
> > +  // This calls GetIncomingServer()
> 
> Lose this comment.

In both cases just the last line shown or the whole comment?
Well, the first comment is a left-over from a previous approach. If you remove the second sentence, it makes some sense, but in a function called "get server" you'd expect that at some stage we get the server. So I'd lose the entire comment since it adds no value.

For the second comment, I'd lose the added line. Why mention some implementation detail in the .h file?
Attached patch 1366591-review-changes4.patch (obsolete) — — Splinter Review 
I will look at that test code and see if I can figure out what it is about, however, the last time I looked at the test stuff I was pretty lost. But, if someone else wants to go ahead and do it, its OK by me.

When I started removing the printf I found a few issues that I had overlooked, so this patch now includes these additional differences compared to the last patch you submitted:
1. Fixes the c++ comments discussed in Comment 72, Comment 74 and Comment 75.
2. Removes the printf("gds: ...)'s used for my debug and tracing.
3. Changed spelling of "cancelled" to "canceled" to be consistent.
4: Remove leading "gds: " from some comments.
5. Bad return from MimeDoFcc() when call after "if (saveLocally)" was not handled. Now a dialog appears saying "Unable to save to Local Folders" / OK.
6. Add new saveToLocalFoldersFail=... string to support item 5.
7. Long comment before call to Fail() in NotifyListenerOnStopCopy() was not quite right (comment begins in nsMsgSend.cpp at line 3942).
8. At beginning of MimeDoFcc() function, remove commented out code I did that checked if "cancel" has occurred. If imap connection down and cancel while saving to Sent, save to Local is skipped with the original code.
9. Changed if (fcc_header == 0) to if (!fcc_header) nsMsgSend.cpp:4387
10. "mailbox://noboby@Local%20Folder/" still a hardcoded string. Removed "todo/XXX" comment suggesting it be made a pref.
P/S:
Don't know why, but bugzilla says I can't obsolete your last patch...???
Comment on attachment 8883448 [details] [diff] [review]
1366591-review-changes4.patch

Thanks, I'll take a look later.
Attachment #8883448 - Flags: review?(jorgk)
Attachment #8882982 - Attachment is obsolete: true
Comment on attachment 8883448 [details] [diff] [review]
1366591-review-changes4.patch

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

Looks good, a few nits. Also, let's not break SM with this, so please add the strings for them.

r+ with the below fixed.

I'd like Aceman to look this over, too.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3888,5 @@
>      let draftFolderName = MailUtils.getFolderForURI(draftFolderURI).prettyName;
>      let result = Services.prompt
>                           .confirmEx(window,
>                                      getComposeBundle().getString("saveDlogTitle"),
> +                                    getComposeBundle().getFormattedString("saveDlogMessages2",[draftFolderName]),

Please apply the same change to suite/mailnews/compose/MsgComposeCommands.js, we don't like to break our SM friends.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +112,5 @@
>  
>  ## Strings used for the save message dialog shown when the user closes a message compose window
>  saveDlogTitle=Save Message
>  
> +## LOCALIZATION NOTE (SaveDlogMessages): Do not translate the words %1$S, %2$S and \n.

This needs to be saveDlogMessages2. Also, please add all the strings to suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties.

::: mailnews/compose/src/nsMsgSend.cpp
@@ +4260,5 @@
>      //
>      // The caller of MimeDoFCC needs to deal with failure.
>      //
>      if (NS_FAILED(rv))
> +    {

why add {} here? Maybe you had debugging here?

@@ +4391,5 @@
> +    // it doesn't yet exist.
> +
> +    NS_NAMED_LITERAL_STRING(localFolderUri, "mailbox://nobody@Local%20Folders/");
> +
> +    nsAutoString folder = (nsAutoString)localFolderUri;

This looks funny. I've never seen NS_NAMED_LITERAL_STRING. Please lose localFolderUri and make this:
  nsAutoString folder;
  folder.AssignLiteral("mailbox://nobody@Local%20Folders/");
Attachment #8883448 - Flags: review?(jorgk)
Attachment #8883448 - Flags: review?(acelists)
Attachment #8883448 - Flags: review+
Attached patch 1366591-review-changes5.patch (obsolete) — — Splinter Review 
This implements your last review comments.
I noticed that the "suite" strings title the "Write" window "Compose" window. So I adapted some strings I added there accordingly. Hope that's OK.
Attachment #8883448 - Attachment is obsolete: true
Attachment #8883448 - Flags: review?(acelists)
Comment on attachment 8884100 [details] [diff] [review]
1366591-review-changes5.patch

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

Thanks, Gene. We can fix the nits later. Let's have Aceman look at this.

How are you going with the tests?

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +112,5 @@
>  
>  ## Strings used for the save message dialog shown when the user closes a message compose window
>  saveDlogTitle=Save Message
>  
> +## LOCALIZATION NOTE (SaveDlogMessages2): Do not translate the words %1$S, %2$S and \n.

Shouldn't that be lower case to match the string ID?

::: suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties
@@ +111,5 @@
>  errorIllegalLocalPart=There are non-ASCII characters in the local part of the recipient address %s. This is not yet supported. Please change this address and try again.
>  
>  ## Strings used for the save message dialog shown when the user closes a message compose window
>  saveDlogTitle=Save Message
> +## LOCALIZATION NOTE (SaveDlogMessages2): Do not translate the words %1$S, %2$S.

And here.
Attachment #8884100 - Flags: review+
Attachment #8884100 - Flags: review?(acelists)
(In reply to Jorg K (GMT+2) from comment #80)
> 
> How are you going with the tests?

Not so great. The test patches for the other bug seem to check what happens when a "fake imap server" is shut down while messages are sent. That bug solution in the patch takes the approach, I think, of doing a retry on the save and/or copying the message to Outbox for later saving. It doesn't involve explicit copy to Local Folders like I do. My method, as you know, involves a few dialogs that the user must respond to. Not sure how the unit tests deal with user response through dialogs. Can it do that by simulating or automating the user response to the dialogs? If so, can you point to a good example that I might look at?

> > +## LOCALIZATION NOTE (SaveDlogMessages2): Do not translate the words %1$S, %2$S and \n.
> Shouldn't that be lower case to match the string ID?
:
> And here.

Dang! Will fix these later after more reviews.
Aceman, when we talk about tests we're referring to Mark Banner's attachment 381502 [details] [diff] [review] from bug 28211. That provides a new Xpcshell test test_sendFCC.js.

Gene, basically we have to types of tests: Xpcshell tests that just exercise some internal functions and Mozmill tests, that run TB and operate it via the hand of a ghost and see that is behaves correctly. Aceman and myself are knowledgable in Mozmill tests, but I have fought with and written the occasional (simple) Xpcshell test.

Xpcshell tests don't do user interaction, you typically have to redefine/replace any functions that do the user interaction in the test. Like in this case we would need to redefine NotifyListenerOnStop() and have it always return "yes, save to local folder". That's the theory. I haven't looked at it any further.

We could land this patch here and then shift the test to a new bug, like:
  "Salvage test from bug 28211 and make it apply to bug 1366591".
I do this all the time, I land the code fix and then poor Aceman writes a test for me ;-)
Assignee: nobody → gds
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8884100 [details] [diff] [review]
1366591-review-changes5.patch

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

Nice improvement. Hopefully users will no longer loose messages after composing them. The saving to Local folders is a nice try to save the message.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +112,5 @@
>  
>  ## Strings used for the save message dialog shown when the user closes a message compose window
>  saveDlogTitle=Save Message
>  
> +## LOCALIZATION NOTE (SaveDlogMessages2): Do not translate the words %1$S, %2$S and \n.

It seems there is no %2$S in the string itself.

@@ +336,5 @@
> +## LOCALIZATION NOTE(promptToSaveSentLocally): Do not translate the stings %1$S, %2$S and \n.
> +## %2$S will be replaced with the account name. $1$S will be replaced by the folder name
> +## configured to contain saved sent messages (typically the "Sent" folder).
> +## Translate "Write" to match the translation of item "windowTitlePrefix" above.
> +promptToSaveSentLocally=Your message was sent but not saved to your Sent folder (%1$S) probably because of network errors.\n"Retry" attempts the save again.\n"Save" copies the message to Local Folders/%1$S-%2$S and closes the Write window if it is present.\n"Cancel" does not save the sent message and closes the Write window if it is present.

Please not "Local Folders" name can be changed by user. Do not hardcode it in the string but fetch it from the account (see bug 831516).
This also occurs in the following strings. And the Seamonkey version.

::: mailnews/compose/src/nsMsgSend.cpp
@@ +3665,5 @@
> +  bool canSave = false;
> +  // Get pointer to server (also sets m_accountName)
> +  nsCOMPtr<nsIMsgIncomingServer> server;
> +  nsresult rv = GetIncomingServer(folderURL, getter_AddRefs(server));
> +  if (NS_FAILED(rv))

What about "NS_FAILED(rv) || !server" to save the block below?

@@ +4387,5 @@
> +    // Set to a special local folder since can't save to Sent mbox,
> +    // typically because imap connection is down. Folder is created if
> +    // it doesn't yet exist.
> +    nsAutoString folder;
> +    folder.AssignLiteral("mailbox://nobody@Local%20Folders/");

Get you fetch this URI from the local folders account instead of hardcoding it?

::: mailnews/compose/src/nsMsgSend.h
@@ +374,5 @@
>    bool                    mGUINotificationEnabled;      // Should we throw up the GUI alerts on errors?
>    bool                    mAbortInProcess;              // Used by Abort to avoid reentrance.
>  
>    nsCOMPtr<nsIMsgComposeSecure> m_crypto_closure;
> +  nsString                m_accountName;                // When, for example, save to Sent fails, use this to

Why do you need this as a global variable? The server is always fetched via GetIncomingServer so why not account name too? Isn't it just server->GetPrettyName() ?
Attachment #8884100 - Flags: feedback+
(In reply to :aceman from comment #83)
> Comment on attachment 8884100 [details] [diff] [review]
> 1366591-review-changes5.patch
> 
> Review of attachment 8884100 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice improvement. Hopefully users will no longer loose messages after
> composing them. The saving to Local folders is a nice try to save the
> message.
> 
> :::
> mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
> @@ +112,5 @@
> >  
> >  ## Strings used for the save message dialog shown when the user closes a message compose window
> >  saveDlogTitle=Save Message
> >  
> > +## LOCALIZATION NOTE (SaveDlogMessages2): Do not translate the words %1$S, %2$S and \n.
> 
> It seems there is no %2$S in the string itself.

Yes, will get rid of that.

> 
> @@ +336,5 @@
> > +## LOCALIZATION NOTE(promptToSaveSentLocally): Do not translate the stings %1$S, %2$S and \n.
> > +## %2$S will be replaced with the account name. $1$S will be replaced by the folder name
> > +## configured to contain saved sent messages (typically the "Sent" folder).
> > +## Translate "Write" to match the translation of item "windowTitlePrefix" above.
> > +promptToSaveSentLocally=Your message was sent but not saved to your Sent folder (%1$S) probably because of network errors.\n"Retry" attempts the save again.\n"Save" copies the message to Local Folders/%1$S-%2$S and closes the Write window if it is present.\n"Cancel" does not save the sent message and closes the Write window if it is present.
> 
> Please not "Local Folders" name can be changed by user. Do not hardcode it
> in the string but fetch it from the account (see bug 831516).
> This also occurs in the following strings. And the Seamonkey version.

Yes, I see how to get the local folders account name (typically "Local Folders" but can be renamed) from the local folders "server". Can also get the URI string. Will do that and encode the local folders account name in the strings in place of "Local Folders".

> 
> ::: mailnews/compose/src/nsMsgSend.cpp
> @@ +3665,5 @@
> > +  bool canSave = false;
> > +  // Get pointer to server (also sets m_accountName)
> > +  nsCOMPtr<nsIMsgIncomingServer> server;
> > +  nsresult rv = GetIncomingServer(folderURL, getter_AddRefs(server));
> > +  if (NS_FAILED(rv))
> 
> What about "NS_FAILED(rv) || !server" to save the block below?

Internally GetIncomingServer() returns a rv failure code (NS_ERROR_NULL_POINTER) when server pointer is null so adding the check for !server is redundant.

> 
> @@ +4387,5 @@
> > +    // Set to a special local folder since can't save to Sent mbox,
> > +    // typically because imap connection is down. Folder is created if
> > +    // it doesn't yet exist.
> > +    nsAutoString folder;
> > +    folder.AssignLiteral("mailbox://nobody@Local%20Folders/");
> 
> Get you fetch this URI from the local folders account instead of hardcoding
> it?

Yes, I see how to do that now.

> 
> ::: mailnews/compose/src/nsMsgSend.h
> @@ +374,5 @@
> >    bool                    mGUINotificationEnabled;      // Should we throw up the GUI alerts on errors?
> >    bool                    mAbortInProcess;              // Used by Abort to avoid reentrance.
> >  
> >    nsCOMPtr<nsIMsgComposeSecure> m_crypto_closure;
> > +  nsString                m_accountName;                // When, for example, save to Sent fails, use this to
> 
> Why do you need this as a global variable? The server is always fetched via
> GetIncomingServer so why not account name too? Isn't it just
> server->GetPrettyName() ?

The idea was to avoid calling GetIncomingServer() more than once since it seems kind of a lot of code just to get the server pointer. So it was intended as an optimization. However, it would only occur more than once when save to Sent (or Drafts or Templates) fails so may not optimize that much. Will look at removing m_accountName.
drafts has a similar issue, eg bug 250706
(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #85)
> drafts has a similar issue, eg bug 250706

This also fixes drafts & template saves as well as save later, not just sends.
Jorg and aceman review comments addressed including getting rid of the new member variable.
Attachment #8884100 - Attachment is obsolete: true
Attachment #8884100 - Flags: review?(acelists)
Attachment #8884729 - Flags: review?(jorgk)
Attachment #8884729 - Flags: review?(acelists)
Comment on attachment 8884729 [details] [diff] [review]
1366591-review-changes6.patch

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

I haven't tested this, but my issues were addressed, thanks.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3888,5 @@
>      let draftFolderName = MailUtils.getFolderForURI(draftFolderURI).prettyName;
>      let result = Services.prompt
>                           .confirmEx(window,
>                                      getComposeBundle().getString("saveDlogTitle"),
> +                                    getComposeBundle().getFormattedString("saveDlogMessages2",[draftFolderName]),

I think there should be a space after comma: ", [draftFolderName]".

::: suite/mailnews/compose/MsgComposeCommands.js
@@ +2145,5 @@
>      let draftFolderURI = gCurrentIdentity.draftFolder;
>      let draftFolderName = MailUtils.getFolderForURI(draftFolderURI).prettyName;
>      switch (Services.prompt.confirmEx(window,
>                sComposeMsgsBundle.getString("saveDlogTitle"),
> +              sComposeMsgsBundle.getFormattedString("saveDlogMessages2",[draftFolderName]),

I think there should be a space after comma: ", [draftFolderName]".
Attachment #8884729 - Flags: review?(acelists) → review+
Comment on attachment 8884729 [details] [diff] [review]
1366591-review-changes6.patch

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

I've tested it and if works as before.

Personally I find this message funny:
Your draft message was not saved to your Drafts folder (Drafts) probably because of network errors.
I'd make the first word "Drafts" lower case. I can do this when I check it in. Likewise for Sent and Templates.
Do you agree? I can also fix Aceman's nits.

Some questions:
1) What about the tests from bug 28211. Move to a different bug? Do here as part 2?
2) What about bug 28211? I'll close it since it would be fixed here, right?

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +115,5 @@
>  
> +## LOCALIZATION NOTE (saveDlogMessages2): Do not translate the words %1$S and \n.
> +## %1$S is replaced by the folder name configured for saving drafts (typically the "Drafts" folder).
> +## Translate "Write" to match the translation of item "windowTitlePrefix" below.
> +saveDlogMessages2=Message has not been sent or saved in your drafts folder (%1$S).\n"Save" copies the message to your drafts folder (%1$S) and closes the Write window.\n"Don't Save" closes the Write window without saving a draft.\n"Cancel" allows you continue writing without saving a draft.

Note: Lower case "drafts folder".

@@ +337,5 @@
> +## %2$S will be replaced with the account name. $1$S will be replaced by the folder name
> +## configured to contain saved sent messages (typically the "Sent" folder).
> +## %3$S will be replaced with the local folders account name (typically "Local Folders").
> +## Translate "Write" to match the translation of item "windowTitlePrefix" above.
> +promptToSaveSentLocally=Your message was sent but not saved to your Sent folder (%1$S) probably because of network errors.\n"Retry" attempts the save again.\n"Save" copies the message to %3$S/%1$S-%2$S and closes the Write window if it is present.\n"Cancel" does not save the sent message and closes the Write window if it is present.

Note: Upper case Sent folder.

@@ +345,5 @@
> +## LOCALIZATION NOTE(promptToSaveDraftLocally): Do not translate the stings %1$S, %2$S, %3$S and \n.
> +## %2$S will be replaced with the account name. $1$S will be replaced by the folder name
> +## configured to contain saved draft messages (typically the "Drafts" folder).
> +## %3$S will be replaced with the local folders account name (typically "Local Folders").
> +promptToSaveDraftLocally=Your draft message was not saved to your Drafts folder (%1$S) probably because of network errors.\n"Retry" attempts to save again.\n"Save" copies the message to %3$S/%1$S-%2$S and you can continue writing.\n"Cancel" allows you to continue writing without saving your draft.

Upper case.

@@ +352,5 @@
> +## LOCALIZATION NOTE(promptToSaveTemplateLocally): Do not translate the stings %1$S, %2$S, %3$S and \n.
> +## %2$S will be replaced with the account name. $1$S will be replaced by the folder name
> +## configured to contain saved templates (typically the "Templates" folder).
> +## %3$S will be replaced with the local folders account name (typically "Local Folders").
> +promptToSaveTemplateLocally=Your template was not saved to your Templates folder (%1$S) probably because of network errors.\n"Retry" attempts to save again.\n"Save" copies the message to %3$S/%1$S-%2$S and you can continue writing.\n"Cancel" allows you to continue writing without saving your template.

Upper case.

@@ +358,5 @@
> +## LOCALIZATION NOTE(saveToLocalFoldersFailed): Message appears after normal
> +## save fails (e.g., to Sent) and save to Local Folders also fails. This could
> +## occur if network is down and filesystem problems are present such as disk
> +## full, permission issues or hardware failure.
> +saveToLocalFoldersFailed=Unable to save your message to local folders. Possibly out of file storage space.

lower case.
Attachment #8884729 - Flags: review?(jorgk) → review+
(In reply to Jorg K (GMT+2) from comment #89)
> Comment on attachment 8884729 [details] [diff] [review]
> 1366591-review-changes6.patch
> 
> Review of attachment 8884729 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've tested it and if works as before.
> 
> Personally I find this message funny:
> Your draft message was not saved to your Drafts folder (Drafts) probably
> because of network errors.
> I'd make the first word "Drafts" lower case. I can do this when I check it
> in. Likewise for Sent and Templates.
> Do you agree? I can also fix Aceman's nits.

Yes, I agree probably Drafts, Sent and Templates are generic and should be lower case. You can add Aceman's spaces too. 

> 
> Some questions:
> 1) What about the tests from bug 28211. Move to a different bug? Do here as
> part 2?

This bug is getting pretty long so I think a new one would be appropriate to handle the tests.

> 2) What about bug 28211? I'll close it since it would be fixed here, right?

I think so.

> :::
> mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
> @@ +115,5 @@
> >  
> > +## LOCALIZATION NOTE (saveDlogMessages2): Do not translate the words %1$S and \n.
> > +## %1$S is replaced by the folder name configured for saving drafts (typically the "Drafts" folder).
> > +## Translate "Write" to match the translation of item "windowTitlePrefix" below.
> > +saveDlogMessages2=Message has not been sent or saved in your drafts folder (%1$S).\n"Save" copies the message to your drafts folder (%1$S) and closes the Write window.\n"Don't Save" closes the Write window without saving a draft.\n"Cancel" allows you continue writing without saving a draft.
> 
> Note: Lower case "drafts folder".

Yes, this one's OK. The next 3 below (snipped) need fixed and last one OK, I think you are saying. This is in "suite" area too.
https://hg.mozilla.org/comm-central/rev/a6a1279292d055c75b9554b01dab78b1cd9f27de

Landed with small issues fixed (lower case, spaces). I've added bug 28211 to the commit message since this is really the "Plan B" that we needed. It's also good to see a really old bug from 2000 fixed. Gene came close to fixing a bug older than the current "record", bug 26734 ;-)

Gene, thank you so much for your work, TB has just become so much better!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 56.0
Blocks: 1379918
(In reply to gene smith from comment #90)
> This bug is getting pretty long so I think a new one would be appropriate to
> handle the tests.
Done, bug 1379918.
https://hg.mozilla.org/l10n-central/nl/rev/fc3963afb54ea792f0f8ab08669139754135ef3c
Bug 1366591 - When save to Sent (or Drafts or Templates) fails, save to subfolder of local folders
Sorry for the bugmail, hg added this comment for me. I hope I have disabled this now for the future.
Saving to local folders is not a real solution. The whole idea of IMAP is to store email on the server so it can be accessed from any mail client.
Please let us know where you want to store the message when the network connection to the IMAP server is lost.

Once the connection is re-established, the user can move the message from the local folder to the server.
Of course if the IMAP server cannot be reached, saving locally IS the only solution, but then you wouldn't be able to send the email until the connection is re-established. I find it odd that the server doesn't internally place all sent mail in the Sent folder, at the moment it seems the email is first sent once to the Recipient and a second time to the email server's Sent folder, seems like a waste of bandwidth. I guess that's just how it works.
You need to understand the technology. E-Mail is *always* sent via SMTP to an SMTP server which can be different from the IMAP server. Once the e-mail has been sent successfully, the e-mail client Thunderbird wants to store an IMAP copy of the sent mail. And of this fails, there is no other option than to save it to a local folder.

The only option to solve this would be to turn processing around and store the message as sent first and then send it. That runs into further trouble if sending after storing fails.

Please understand, that the whole bug is about error recovery, so to recover from some malfunction elegantly.
Oops! Wrong bug, I came here from a link on another related bug. The IMAP and SMTP servers can indeed be different. Gmail automatically puts all sent email in the Sent folder, but that seems to be the way Google have chosen to implement it. Anyway, wrong thread :p
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: