1231592 - With Charter ISP's IMAP server, deleted or marked as spam emails won't restore to inbox and are lost

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[gene smith]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151102093849

Steps to reproduce:

I have recently attempt to using my ISP's IMAP server instead of POP. It basically works in that new emails are received and can be sent. However, when an email in the inbox is deleted and moved to trash, an attempt to move it back to inbox does not occur and the email is no longer present in any folder. The same thing happens when an email in inbox is marked as spam. It moves from inbox to spam folder but within the spam folder an attempt to move the message back to inbox results in the email vanishing. Un-marking it as spam also causes it it disappear instead of going back into inbox.   


Actual results:

The ISP IMAP server is mobile.charter.net and according to the capabilities obtained by logging in with openssl it does NOT support IDLE:

b capability
* CAPABILITY IMAP4rev1 UIDPLUS NAMESPACE QUOTA SORT ID
b OK CAPABILITY completed

However, doing the actions suggested in bug 693204, disable use of IDLE and set cached connections to 1, seem to also fix my problem (but changing the read/unread status at another client, a phone, does not immediately show in Tb as would be expected and is not a big problem for me). 


Expected results:

I have also tried enabling the CONDSTORE flag but seems to have no effect on my problem so it is now back to false. 
I have contacted the ISP (charter) about this but they have no reports of problems like this with thunderbird but I suspect this is still a problem on their end.  Their email server is this:
* OK IMAP4 server (InterMail vM.9.00.021.00 201-2473-182) ready Wed, 9 Dec 2015 23:40:20 -0600 (CST).
Note: with default settings (use IDLE if supported and cache 5 connections) other IMAP accounts at gmail, yahoo and locally with cyrus-imap seem to work flawlessly with Tb and I don't see the problems in bug 693204.

Comment 1

[gene smith]

With default "advanced" server settings (use IDLE if supported and cache 5 conns) the following is observed using the charter web mail page:

1. In Tb, delete an email from inbox. Deleted mail moves from Inbox to Trash in Tb.
2. On charter webmail page, the deleted mail appears in both Inbox and Trash.
3. Shutdown Tb. On webmail, deleted email is then removed from Inbox in response to Tb shutdown.
4. Start Tb. Move deleted mail from Trash to Inbox in Tb. This deletes mail from tb Trash but it does not appear back in Tb Inbox. On webmail, the moved email is now in BOTH Trash and Inbox.
5. Shutdown Tb. On webmail, the mail is now only present in Inbox. It is deleted from webmail Trash when Tb shuts down.
6. Start Tb. The un-deleted email now appears correctly in Tb Inbox.

Without the scenario above, the deleted email cannot be restored from Trash when advanced server settings are default. This is not necessary and works as expected when IDLE usage is disabled and cached connections are set to 1.

This behavior was observed by the charter support tech when I originally contacted them assuming the problem was with them. Now I am not so sure that the problems is not with Tb.

Comment 2

[gene smith]

Note: At step 1. above, sometimes the deleted mail does not appear in Tb Trash until after Tb restarted at step 4.

If mail-imap.expunge_after_delete is changed to true the deleted mail no longer remains in webmail Inbox but only appears in webmail Trash. However Tb restarts are still required for moved mails to appear at their destinations folders at steps 3 and 6 above. So expunge_after_delete does not fix the problem.

I have provide an imap log for the server imap.charter.net account name "eds" as recorded while doing steps 1-6 above: file imap_eds.log

Comment 3

[gene smith]

Attachment: imap_eds.log

Comment 4

[gene smith]

I looked at this another way using Wireshark filtered on protocol "imap" but using the Evolution email client. When messages are "moved" from one folder to another in Evolution they are copied and then marked as \deleted and when the source folder is exited with the gui and the source folder is expunged. Evolution works correctly with the charter email site even when idle is enabled and 5 connections are cached since the source folder is explicitly sent the imap expunge command.

However, in Tb, with idle enabled and 5 connections cached (default advanced setting), when a message is "moved" it is copied and marked as \disabled but an expunge never occurs on any folder. However, with idle disabled and 1 connection cached, the message is copied and the message in the source folder is marked as \deleted as with default advance settings; however, when the source folder is exited in the Tb gui, a "close" operation occurs on the folder, that, per the imap RFC, expunges the folder which causes Tb to work correctly with the charter server (but only when idle disabled and 1 connection cached).

So the bug seems to be that Tb with default advanced settings is leaving the moved message in the source folder marked as \deleted. Also, when the message in the destination folder is "moved" back to the source folder (e.g., when it is undeleted) it becomes marked as \deleted in both folders since no "close" or explicit "expunge" imap operation is done by Tb. Since both remained marked as \deleted, the message is appears to be now gone/lost in Tb. However, shutting down Tb at the appropriate times, as in steps 3 and 5 above (comment 1), causes Tb to produce the "close" operation that effectively expunges the \deleted email, thus simulating correct behavior and the message moved to Trash can be moved back to Inbox and is not lost. When advanced setting are NOT default (disable idle and 1 cached connection) Tb sends the "close" command to the charter server at the proper times without Tb shutdowns being done and messages can be "moved" and unexpected message loss does not occur.

Other than the fact the charter webmail continues to show identical messages marked as \deleted (and not expunged) in both the Inbox and Trash, the charter imap server seems to be operating properly (even though it has minimal imap capabilities). Tb does not seem to be adapting properly to these capabilities.

Comment 5

[gene smith]

Observation regarding comment 2 -- when expunge after delete is set as in comment 2, an explicit expunge is correctly done on the source folder when a message is moved or when deleted. The moved/deleted message appears correctly in Tb in the destination folder, e.g., in Trash. When the message is then moved back to the original source folder (or any other folder) the folder is again correctly expunged but when the Tb user moves to the original source folder (or the new destination folder) no IMAP "select" occurs on that folder so the message appears to be lost. However, if Tb is shut down and restarted, the restored message appears correctly in the folder because, on restart, Tb properly "selects" that folder.
Note: the situation described here (and in comment 2) is:
mail-imap.expunge_after_delete = true (not default which is false)
server advanced settings: use idle and cache 5 connections (default).
Note also that in comment 4 mail-imap.expunge_after_delete is set always false (default).

Comment 6

[gene smith]

Attachment: default-fails.txt

This is the wireshark dump that shows the primary problem when all default advanced server parameters are set (i.e., use Idle and cache up to 5 connections). The internal parameter "expunge on delete" is also set to default (false).  The problems seems to be that no expunge occurs.

Comment 7

[gene smith]

Attachment: expunge-on-delete-fails.txt

This is the wireshark dump that shows the primary problem when all default advanced server parameters are set (i.e., use Idle and cache up to 5 connections) but the internal parameter "expunge on delete" is also set to non-default (true).  The expunge now occurs but the Inbox is not SELECTed until a Tb restart is done causing the restored email to appear lost until Tb is restarted.

Comment 8

[gene smith]

Attachment: works.txt

This is the wireshark dump that shows successful operation when advanced server parameters are set to NOT use Idle and cache only 1 connection. The internal parameter "expunge on delete" is also set to default (false). With this configuration, a deleted message is correctly restored from Trash to Inbox with no other action needed. The IMAP "close" on the correctly selected mailbox, when focus is moved, effectively expunges the mailbox even though no IMAP expunge occurs. Also, only with these setting can emails be moved from one mailbox to another without problems on the charter IMAP server.

Comment 9

[Wayne Mery (:wsmwk)]

Hey Ron, are you familiar with this issue?

Comment 10

[Wayne Mery (:wsmwk)]

Brian, 
Has this happened to you on charter?

(I guess Ron isn't using Thunderbird anymore.)

Comment 11

[Brian]

Let me run the scenario:
A. Delete message in TB and see if Charter replicates. Note observations in trash and inbox folders..


I have multiple monitors so I have both TB and Charter email Open simultaneously, here is what I found.
1. Delete msg in TB Inbox...
RESULT: There is NO Msg in TB inbox and there is NO msg in TB Trash...The Charter Inbox still shows msg..
2. Select "Get Message" on TB Tab: No change from 1...ie Has no effect on either.
3. Close TB and observe results: Closing TB has no effect on Charter INBOX as msg still exists. No msg in TB Trash.
4. Open TB and observe results: Opening TB has no effect on Charter INBOX as msg still exists. No msg in TB Trash.
5. Close Charter email and reopen and Observe result: Msg has been deleted/removed from INBOX. There is NO msg in Charter TRASH...So this msg cannot be recovered? under this setup.

REPEAT for Confirmation:
1. In TB deleted msg in INBOX...TB Trash now has msg in it....Charter INBOX still shows msg...
2. Closed TB: Does not change status of Charter INBOX.. BUT I selected "Get Mail" on Charter Tab and INBOX msg disappeared... There is NO msg in Charter TRASH...BUT after a few minutes I selected Charter "Get Mail" again and now msg shows in Charter TRASH

Note:
During this test two new msgs are in Charter INBOX....Opening TB now shows new these same two msg in INBOX... Deleting these msg in Charter immediately moves them to Charter TRASH...There is NO change in TB, so msg are still in TB INBOX.....Selecting "Get Messages" in TB has No effect...BUT closing TB and reopening has moved these msg to TRASH folder...

NOTE 2: These msg were deleted in Charter "unread" status...and appear in Charter TRASH as unread.. TB follows this exactly when it is closed then opened...

Comment 12

[gene smith]

(In reply to Brian from comment #11)
Wayne send me an email asked me to see if I agree with what you see. First, I assume you are using default values for several parameter:
mail.server.default.use_idle = true
mail.server.serverX.max_cached_connections = 5
mail.imap.expunge_after_delete = false
With these set as shown I see similar to what you see. When I delete an email from tb inbox I see it go away and it appears in tb trash. Looking at charter webmail I see the same movement. Then in tb if I move the email from trash back to inbox, it leaves trash but never appears in inbox of tb. It also leaves charter trash and *seems* to appear in charter inbox. But if I try to open the seemingly restored email in charter inbox it never opens and I just see a continuously spinning progress icon. If I click on charter inbox, the icon stops and the list of emails reappears but with the "restored" email missing from the list (it hasn't move to another charter folder either). In tb, doing "get messages" or restarting tb also never brings back the "restored" email. It seems to be gone.
If I set just expunge_after_delete to true, it seems to (usually) help in that the the deleted and then restored email is not completely deleted but can be seen in charter inbox and opened and can be seen in tb inbox after tb is restarted.
However, if I set all the parameters above to non-default values:
mail.server.default.use_idle = false
mail.server.serverX.max_cached_connections = 1
mail.imap.expunge_after_delete = true
It works fine and I see no problems. That's how I have had it set for over a years since I filed this bug (that I had almost forgotten about). However, I just did another experiment and found that only max_cached_connections for the charter server needs to be set non-default, i.e., to 1. The other two, use_idle and expunge_after_delete can remain default and it still seems to works fine.

Comment 13

[gene smith]

Another problem with charter: I can't always see newly received email in tb. I see them on the charter webmail but often they don't appear in tb inbox even after clicking "get messages" or by setting "check for new messages every 2 minutes and waiting. Setting "allow immediate server notification when msg arrive" doesn't help either. The only way to see new emails is to restart tb. However, sometimes they do come in but usually not without a restart. 
I am testing this by sending emails to my charter address from gmail. FWIW, all the test emails appear on my 2 android devices.

Comment 14

[gene smith]

One more observation: After tb startup, I can receive 1 email that was sent from gmail (in tb) after tb startup. If another is sent, it is never seen in tb unless tb is restarted. On restart, the email is received and subsequently sent emails are again not received until tb is restarted, etc.  (Again, this problem only occurs with the charter imap server. I can send send multiple sequential messages to yahoo imap (again, from gmail in tb) and received them in tb without restarting tb.)

Comment 15

[gene smith]

And another: My previous observations have been on very long-time thunderbird installations over many fedora linux version. So I installed tb on a fresh "KDE Neon linux distribution" on another machine. I see the same problems. Also, on tb on all linux installations I found that to receive new messages it is possible just to switch from inbox to another folder (e.g., drafts) and then back to inbox on the charter account. This receives the pending email without having to restart tb.
Also, on a windows 8 machine that has had tb installed, emails that are sent are always received without doing anything except to wait. Charter account is set with the same parameters: don't check for new emails on startup, check emails every 1 minute, don't allow immediate notification when new emails arrive (don't use idle). It is also set to use 1 cached connection, default.use_idle=true. However, even though set to check emails every minute, it usually takes at least 5 minutes before a new email appears in inbox, but it eventually does appear even though a tb restart (or folder switch) brings it in almost immediately after it is sent from gmail.

Comment 16

[gene smith]

I think I see what the problem is and it is with the charter IMAP server. New emails don't come in when "get messages" is pressed because tb does not do a SELECT on the inbox if it was already selected. Apparently charter's server requires this even though the IMAP RFC3501 definitely does not require it. If tb is configured for a maximum of 1 connection, clicking on another folder in the account causes that folder (mailbox) to be SELECTed. Then when you click back on to inbox, a SELECT of inbox occurs and any new messages appear because charter only flags new messages in UID FETCH response if a SELECT has occurred right before. To receive any new messages in inbox, charter seems to require a SELECT on inbox and then a fetch on inbox. Fetches on inbox without a preceding select on inbox never responds with a new message available.

This also explains why a maximum of 1 "cached" connection must be set with charter. If the default of 5 is set, 5 folders/mailboxes are connected and each is SELECTed in the connection at tb startup. Moving between folders by clicking on a different folder does not cause a SELECT since they are already selected at startup. So messages can't be copied/moved between the 5 folders and new messages received since FETCH never reveals a change since tb realizes that each folder/mailbox is already selected.

For comparison, I tried kmail and claws-mail on the same charter imap server. kmail works OK with charter because it always does a SELECT and then FETCH on inbox when the "get new mail" button is pressed or when moving between folders in my account. However, claws-mails works similar to tb in that a SELECT on inbox does not occur when clicking "get new mail" (since it is typically already SELECTed). So with claws-mail too you must go to another folder and than back to inbox to receive any new messages (since this causes inbox to be SELECTED before the FETCH occurs). With both of these clients, there is no configuration for multiple tcp/ip connection per account.

I found this information by monitoring with the wireshark IMAP filter. I did notice one other thing regarding tb. After sending a new email from gmail to my charter account, if I just let tb sit without receiving the new email in inbox with "focus" removed from tb for, maybe, 1 hour and then return focus to tb, I then observer in wireshare that tb sends an IMAP logout. Charter, for unknown reasons, responds with CAPABILITIES and then tb treats this like a restart and SELECTs and FETCHes each folder. (Charter never produces the expected response to logout.) This brings in any new emails that had been sent to my account without pressing "get new mail" or moving between folders. This may be what is causing some people to report greatly delayed email receipt in tb when using charter imap. Possibly this is faster in windows explaining why I see somewhat faster email receipt on my windows box (haven't run wireshark on windows to check this, just on linux).

Comment 17

[Wayne Mery (:wsmwk)]

gene, thanks for the analysis.

FWIW, the list of charter reports https://mzl.la/2lvMGWJ

Comment 18

[WADA:World Anti-bad-Duping Agency]

FYI.
Problem of mobile.charter.net was already analyzed by bug 1325192(and by bug 1324077) in 2016.
mobile.charter.net is never normal IMAP server. Charter net utilizes some IMAP commands but it's never RFC 3501 complient IMAP server.

Comment 19

[gene smith]

Attachment: gds.diff -- experimental fixes

(In reply to WADA:World Anti-bad-Duping Agency from comment #18)
Yes, I agree with your conclusion. However, the work-around described at https://bugzilla.mozilla.org/show_bug.cgi?id=1325192#c27 definitely helps, but, for me, it is not always reliable. Usually emails arrive without switching focus from INBOX to not INBOX back to INBOX. However, sometimes new mails still don't arrive without moving focus. The problems seems to be that the UID FETCH response from imap.charter.net just doesn't detect the the new email present on the charter server. (This is with charter account on tb set to 1 cached connection. With the default of 5, new emails are only received at startup.)

The original problem for *this* bug report was that emails could not reliably be copied or moved between folders for the charter account with default parameters. Setting the max cached connections to 1 fixes this problem with charter imap.

I wanted to see if I could get charter to work correctly with default high-level parameter: 5 cached connection. A solution is to force a select before fetching the INBOX and to force an expunge after delete. I cloned the mozilla thunderbird build system and built the "daily" version and then made some experimental changes seen in the attachment, gds.diff. I defined a new "about:config" parameter called mail.server.useCharterWorkarounds to enable a place in the code to force the parameter mail.imap.expunge_after_delete to true and to do an imap SELECT even if the current folder is already selected. These changes cause reliable operation with default high level parameter (e.g., max 5 cached connection) and fix both problems: mail can be moved/copied between folders and new email is always detected while using default high level imap account parameters on charter.

Of course, this does not excuse the charter server from its obvious non-compliance. It just shows that a work-around is possible in the thunderbird code. 

I retried a few other email clients and most have the same problem. kmail always does a SELECT even when the a folder is already selected when fetching new mail so it works OK with charter. However, claws-mail and evolution both fail to detect new email from charter and also have problems moving/copying emails between charter folders for the same reasons as thunderbird.

Comment 20

[WADA:World Anti-bad-Duping Agency]

(In reply to gene smith from comment #19)
> (In reply to WADA:World Anti-bad-Duping Agency from comment #18)
> Yes, I agree with your conclusion. However, the work-around described at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1325192#c27 definitely helps,
> but, for me, it is not always reliable. 
> Usually emails arrive without switching focus from INBOX to not INBOX back to INBOX.
> However, sometimes new mails still don't arrive without moving focus.
> The problems seems to be that the UID FETCH response from imap.charter.net
> just doesn't detect the the new email present on the charter server.

Your observation/understanding is almost correct and same as mine.
You perhaps ignore following:
> (a) If no connection is established after restart and new mail check upon restart is enabled,
>     or if no connection is established after upon new mail check interval
>     (go WOrk Offline, then go back Work Online => this states occurrs)
>      Thunderbird logins to IMAP server and request "select Inbox" and "uid fetch 1:* Flags".
> (b) If Inbox is not selected at any cached connection and one or more cached connection is established,
>     Thunderbird uses "STATUS Inbox" upon new mail check if Inbox is not selected at any connection.
>    (max cached connection=1, select Inbox, select other than Inbox => Inbox is de-selected at only one connection.
> (c) If Inbox is selected at a cached connection.
>     Thunderbird uses "uid fetch NextUID:* Flags" at a cached connection upon new mail check
>    (max cached connection=1, select Inbox, select other than Inbox => Inbox is de-selected at only one connection.

I don't think Charter net IMAP correctly return number of new mails to "statu Inbox" command, although I didn't check it in bug 1324077 and bug 1325192.
It looks that Charter net IMAP server requests access like POP3 even though he says "used protocol=IMAP" amd used command name is identical to command defined by protocol named IMAP.
- POP3 : login to POP3 server, get msgNo/Size/UIDL of all messages in Mbox by LIST/UIDL pop3 command.
         RETR(or TOP 0/TOP nn and RETR after it, according to option setting)
- IMAP : login to IMAP server if no connection is establised,
         "select Inbox" instead of "LIST", "uid fetch 1:* Flags" instead of "UIDL".
         "uid fetch XX body.peek[header.fields ...]" instead of "TOP 0" for header fetch.     
         "uid fetxh XX body.peek[]" instead of "RETR" for fetch of entire mail data.

In your workaround operation with "max cached connection=1", did you surely force "select MboxName" + "uid fetch 1:* Flags"?
(where MboxName=Inbox for new mails, MboxName=Trash after delete mail in Inbox==move mail from Inbox to Trash)?

> Of course, this does not excuse the charter server from its obvious non-compliance.
> It just shows that a work-around is possible in the thunderbird code. 

What should be done first by you is "request correction of broken server" instead of "request modification of Thunderbird code to volunteer developers at bugzilla.mozilla.org for your broken serever", isn't it?

Comment 21

[gene smith]

(In reply to WADA:World Anti-bad-Duping Agency from comment #20)

> What should be done first by you is "request correction of broken server"
> instead of "request modification of Thunderbird code to volunteer developers
> at bugzilla.mozilla.org for your broken serever", isn't it?

Thanks for the reply. I need to parse and understand all the good information you provide. However, just to be clear, I am not requesting a modification to any Thunderbird code. I did contact charter about these issues over a year ago and was told it was a client (tb) problem. At the time, I really had no way to refute that since I had never actually researched how IMAP works. So I just want to be 100% sure that their imap server is at fault before contacting them again. (Even then, I have my doubts that anything will be done by charter.)

Comment 22

[WADA:World Anti-bad-Duping Agency]

(In reply to gene smith from comment #21)
> I did contact charter about these issues over a year ago and was told it was a client (tb) problem.

Bug opnener of bug 1324077 and bug 1325192 were also told so repeatedly by Charter net.

As for *Essential Fault* of Charter net, fault is problem of bug 1324077 and bug 1325192 only.
As for "Deleted mail at Inbox via Thunderbird still shown in Inbox by Web Mail" part, it's merely difference between Web Mail and mail access via protocol named IMAP.
(1) Delete a mail(UID=XX) at Inbox with IMAP delete model=move to trash,
    Move a mail(UID=XX) in Inbox to Spam via "mark as Junk",
    are as follows if IMAP.
At cached connection where Inbox is selected,
(1-a) uid copy XX Trash(or Spam) => UID=YY at Trash(or Spam) is returned because it's supported by your server.
      (MOVE command is not usable in your case, because server doesn't support MOVE extension)
(1-b) uid store XX +Flag(\Deleted and some other flags)"

(2) By (1-b), IMAP flag of \Deleted is stored to mail of UID=XX in Inbox, 
    but because "UID=XX of the mail in Inbox" and "IMAP flag of \Deleted of the mail of UID=XX" are IMAP only entity,
    Web mail usually doesn't care about the IMAP \Delete flag.
This is because Web Mail of ISP != Mail data access via IMAP supported by the provider.

(3) After delete mail(move to Trash) or move to Spam at step (1),
    newly copied mail(UID=YY) in Trash(or Spam) won't be returned from your server,
    unless "Select Trash(or Spam)" + "uid fetch 1:* Flags()" is requested by Thunderbird.
This is identical to problem in detecting new mail when Charter net.
This is problem of Charter net cited by bug 1325192 and stated by your comment #16.
"Disable IDLE command use" + "Max cached connection=1" + "Switch folder at Thunderbird when required" is for ease of forcing it on Thunderbird.
"Go Work Offline, go back Work Online" can be used on Thunderbird for it.

Status of (2) is easily observed with IMAP delete model = Just mark it as deleted.
- Mail of UID=XX in Inbox(\Deleted flag is stored) is shown with trash can icon/strike-thru line.
If IMAP delete model = Remove it immediately, or if IMAP delete model = Move to trash,  
the "mail of UID=XX with \Deleted flag" is not shown at thread pane by Thunderbird.

It looks for users as mismatch between Web Mail and Thunderbird.
For reducing such confusion/mismatch, some user actions are possible.
1. Use IMAP delete model=Just mark it as deleted, and if you want to do EXPUNGE, do Compact Folder(Inbox) at Thunderbird.
2. Use IMAP delete model=Remove it immediately, and if you want to do EXPUNGE, do Compact Folder(Inbox) at Thunderbird.
3. Use IMAP delete model=Move to trash, and if you want to do EXPUNGE, do Compact Folder(Inbox) at Thunderbird.
4. Use IMAP delete model=Move to trash, and use mail.imap.expunge_after_delete=true.
   (Your choice)
5. Server supports MOVE extension. (Gmail IMAP supports MOVE extension)
   If MOVE extension is available, Thunderbird uses "UID MOVE XX Trash(or Spam)" command
   instead of "uid copy XX Trash(or Spam)" + "uid store XX Flags(\Deleted)".
   Because MOVE command implies "Remove the moved mail of UID=XX from move source folder(Inbox in this case)",
   "mail moved to Trash", "mail moved to Spam", won't be shown at Inbox by Web Mail.

Please report problem of Charter net to Charter net correctly.

Comment 23

[Brian]

I would like to thank all of you for the support on this BUG.I called Charter at one point and was told by the operator that email is NOT there core product, most likely 3rd/4th party. Charter only assist in customer interface and was advised they reluctantly included this product to compete with Comcast..So again, THANK YOU ALL for your expert time to root out this issue.

Comment 24

[gene smith]

(In reply to WADA:World Anti-bad-Duping Agency from comment #20)
> In your workaround operation with "max cached connection=1", did you surely
> force "select MboxName" + "uid fetch 1:* Flags"?
> (where MboxName=Inbox for new mails, MboxName=Trash after delete mail in
> Inbox==move mail from Inbox to Trash)?

In gds.diff, I didn't force or change the FETCH behavior. All I did was force a SELECT to occur before doing the fetch since, with 5 cached connections, typically a SELECT does not occur before the FETCH and for charter to report RECENT (new) messages it seems to require a redundant re-SELECT. I don't know why this is since redundant SELECT is not explicitly required by the imap RFC. With this experimental fix, charter works fine receiving new mail with default tb settings (max_cached_conns=5, use idle if available, 10m poll time, etc.), but it does cause more network traffic.

Comment 25

[WADA:World Anti-bad-Duping Agency]

"select Mbox at cached connection where Mbox is already selected" is never explicitly prohibited by RFC 3501, but it is also never recommended by RFC 3501 for IMAP because "select Mbox at cached connection where Mbox is already selected" is rather bad behavior as IMAP client. "select Mbox at a cached connection when the Mbox is already selected at other cached connection" is also never recomennded(it may be prohibited). 

There is no explicit way in protocol named IMAP to de-select Mbox at cached connection where the Mbox is slready selected.
- CLOSE is not usable for simple de-select.
- To de-select Mbox:
(a) "select other existent mbox than Mbox at cached connection where the Mbox is already selected
    => Mbox is de-selected, and other mbox is newly selectedt
(b) "select non-existent mbox at cached connection where Mbox is already selected
    => Mbox is de-selected, and state of the cached connection changed to "no mbox is selected" state.
    https://tools.ietf.org/html/rfc3501#section-6.3.1
      Consequently, if a mailbox is selected and a SELECT command that
      fails is attempted, no mailbox is selected.
(c) "EXAMINE Mbox or other existent mbox than Mbox at cached connection where the Mbox is already selected
    => EXAMINE command != SELECT command, so there is no problem.
    => Mbox or other existent mbox than Mbox is selected with READ-ONLY at the cached connection
    => IMAP client should issue "select" when he wants to access the mbox normally :-)

I prefer following if quirks for server like Charter net will be impokemented.
- mail.imap.quirks_for_charter_net = true, or mail.server.server#.keep_connection_with_no_mbox_is_selected = true
  - when IDLE cammand is issued in normal case, issue "select non-existent mboxname" instead of IDLE.
  - when IDLE cammand is not used after imap commands execution,
    issue "select non-existent mboxname" instead of keeping the Mbox selected at the cached connection.
Required change is perhaps minimum.
Upon new mail check, I guess at least "select Inbox(followed by uid fetch 1:* Flags)" is issued by Thunderbird.

Comment 26

[gene smith]

(In reply to WADA:World Anti-bad-Duping Agency from comment #22)
> (In reply to gene smith from comment #21)
> > I did contact charter about these issues over a year ago and was told it was a client (tb) problem.
> 
> Bug opnener of bug 1324077 and bug 1325192 were also told so repeatedly by
> Charter net.
> 
> As for *Essential Fault* of Charter net, fault is problem of bug 1324077 and
> bug 1325192 only.

I mostly agree with this. Those two bug reports were because charter requires the extra/redundant select for new (RECENT) emails to be detected by FETCH. Also, charter responds to STATUS with un-quoted mailbox names that cause tb to reject the response when whitespace is in the mbox name. This causes tb to logout and restart the connection. However, since tb never reports this reaction to the user (unless they are running and checking the imap logging) it is still might be considered a tb bug. (Personally, I think maybe tb should tolerate whitespace in un-quoted mbox names in the STATUS response.)

> As for "Deleted mail at Inbox via Thunderbird still shown in Inbox by Web
> Mail" part, it's merely difference between Web Mail and mail access via
> protocol named IMAP.

Just be clear, this bug was not really about differences in the charter webmail and what is seen in tb. Charter webmail was only used as a diagnostic tool to approximate the state of the emails that charter's imap server contains. The original bug report was only for behavior observed independently in thunderbird.

> (1) Delete a mail(UID=XX) at Inbox with IMAP delete model=move to trash,
>     Move a mail(UID=XX) in Inbox to Spam via "mark as Junk",
>     are as follows if IMAP.
> At cached connection where Inbox is selected,
> (1-a) uid copy XX Trash(or Spam) => UID=YY at Trash(or Spam) is returned
> because it's supported by your server.
>       (MOVE command is not usable in your case, because server doesn't
> support MOVE extension)
> (1-b) uid store XX +Flag(\Deleted and some other flags)"
> 
> (2) By (1-b), IMAP flag of \Deleted is stored to mail of UID=XX in Inbox, 
>     but because "UID=XX of the mail in Inbox" and "IMAP flag of \Deleted of
> the mail of UID=XX" are IMAP only entity,
>     Web mail usually doesn't care about the IMAP \Delete flag.
> This is because Web Mail of ISP != Mail data access via IMAP supported by
> the provider.
> 
> (3) After delete mail(move to Trash) or move to Spam at step (1),
>     newly copied mail(UID=YY) in Trash(or Spam) won't be returned from your
> server,

With all default tb parameters and running official tb 45.7.0 (gds.diff not applied), when I delete an email from inbox it does appear in trash. Not sure that it is "returned from server" or if this occurs all locally in tb. It also disappears from inbox. So at this point, all is well.

>     unless "Select Trash(or Spam)" + "uid fetch 1:* Flags()" is requested by
> Thunderbird.
> This is identical to problem in detecting new mail when Charter net.
> This is problem of Charter net cited by bug 1325192 and stated by your
> comment #16.
> "Disable IDLE command use" + "Max cached connection=1" + "Switch folder at
> Thunderbird when required" is for ease of forcing it on Thunderbird.
> "Go Work Offline, go back Work Online" can be used on Thunderbird for it.
> 
> Status of (2) is easily observed with IMAP delete model = Just mark it as
> deleted.
> - Mail of UID=XX in Inbox(\Deleted flag is stored) is shown with trash can
> icon/strike-thru line.

Yes, when I set delete model to "just mark deleted" I see a red X and a line through the email in inbox when I delete it. Nothing gets copied to trash.

> If IMAP delete model = Remove it immediately, or if IMAP delete model = Move
> to trash,  
> the "mail of UID=XX with \Deleted flag" is not shown at thread pane by
> Thunderbird.
> 
> It looks for users as mismatch between Web Mail and Thunderbird.

Yes, since it is marked as /deleted it disappears in tb but still visible on webmail.

> For reducing such confusion/mismatch, some user actions are possible.
> 1. Use IMAP delete model=Just mark it as deleted, and if you want to do
> EXPUNGE, do Compact Folder(Inbox) at Thunderbird.

I didn't know compact folder does an expunge, but you are right, it does!

> 2. Use IMAP delete model=Remove it immediately, and if you want to do
> EXPUNGE, do Compact Folder(Inbox) at Thunderbird.

I think closing tb does an effective expunge so this might be optional at this point.

> 3. Use IMAP delete model=Move to trash, and if you want to do EXPUNGE, do
> Compact Folder(Inbox) at Thunderbird.

Yes and if you ever decide to "undelete" or copy or move it back to inbox, I think you have to do an expunge at this point.

> 4. Use IMAP delete model=Move to trash, and use
> mail.imap.expunge_after_delete=true.
>    (Your choice)

When imap server is charter.net this is necessary for moving emails between folders.

> 5. Server supports MOVE extension. (Gmail IMAP supports MOVE extension)
>    If MOVE extension is available, Thunderbird uses "UID MOVE XX Trash(or
> Spam)" command
>    instead of "uid copy XX Trash(or Spam)" + "uid store XX Flags(\Deleted)".
>    Because MOVE command implies "Remove the moved mail of UID=XX from move
> source folder(Inbox in this case)",
>    "mail moved to Trash", "mail moved to Spam", won't be shown at Inbox by
> Web Mail.

Here are the step I found that are needed to "move" emails between folders with default tb parameters:

Click inbox from folder list
Delete an email from inbox (default delete model "moves" email to trash)
Click compact inbox (does expunge on inbox)
Click trash from folder list, deleted email now there
Decide to un-delete it: click move to inbox from menu
Click compact trash folder
Click inbox from folder list, un-deleted email may not be there :-(
If not, Go offline and then back online (use icon at bottom left corner of tb screen).
   This reconnects and forces a SELECT to occur before the FETCH.
Un-deleted email is now there :-)  (May need to click "get messages", but usually not.)

If about:config mail.imap.expunge_after_delete=true (default is false) then the "Compact" steps above can be omitted. Also, the same basic steps are required when moving an email between other folders: after moving an email, the source folder must be expunged and, when the destination folder is inbox, an offline/online transition must occur after the move for the email be appear in inbox. The offline/online is also needed when moving to destination folders that have a dedicated connection. This is because tb does not send a redundant SELECT, required by charter, for folders that have a dedicated connection. On my charter setup, folders with a dedicated connection are Inbox, Drafts, Send and Queue. The remaining several folders (Junk and several others) are multiplexed on the same tcp/ip connection so a SELECT always occurs when they are selected in the list.

> Please report problem of Charter net to Charter net correctly.

I think the main problem with charter is that they require the redundant SELECT before FETCH to report new messages. Also, their STATUS response does not quote the mailbox name. But as Brian pointed out, probably any changes on their part are unlikely if these details are pointed out.

As to possible changes to tb, maybe it should do a mbox expunge whenever a "move" is requested when default delete model is selected, even if expunge_after_delete is false. 

tb could be a little more lenient on mbox names received in STATUS response. Looking at the tb code, it just does a parse check on the mboxname in the STATUS response and then throws it way. For example, tb might accept a mbox name "qwer ty" that is returned without quotes like charter does:

Response: * STATUS qwer ty (MESSAGES 1 RECENT 0 UNSEEN 0 UIDNEXT 1019)

I.e., just accept whatever is between STATUS and the next left parenthesis.

Also, tb might also report to the user that mailbox names with internal whitespace or other syntax errors are causing problems. A workaround is to suggest in documentation athat replacing whitespace with _ or - or putting double quotes around the mailbox name which I found to work with charter (e.g., rename mbox from qwer ty to "qwer ty" causes tb to not reject the response.

Comment 27

[gene smith]

(In reply to WADA:World Anti-bad-Duping Agency from comment #25)
> "select Mbox at cached connection where Mbox is already selected" is never
> explicitly prohibited by RFC 3501, but it is also never recommended by RFC
> 3501 for IMAP because "select Mbox at cached connection where Mbox is
> already selected" is rather bad behavior as IMAP client. 

Maybe, but this seems to be the only way for charter imap server to allow access to RECENT or new emails when INBOX has a dedicated connection. You could SELECT a nonexistant mbox such as "a&sfjasd_" and then SELECT INBOX again but seems like more work than just re-SELECTing INBOX.

>"select Mbox at a
> cached connection when the Mbox is already selected at other cached
> connection" is also never recomennded(it may be prohibited).

Yes, not explicitly recommended but I found nowhere in the RFC does it says you can't re-SELECT the same already SELECTed mbox. I do this in my experimental patch and it seems to work fine.
 
> 
> There is no explicit way in protocol named IMAP to de-select Mbox at cached
> connection where the Mbox is slready selected.
> - CLOSE is not usable for simple de-select.
> - To de-select Mbox:
> (a) "select other existent mbox than Mbox at cached connection where the
> Mbox is already selected
>     => Mbox is de-selected, and other mbox is newly selectedt
> (b) "select non-existent mbox at cached connection where Mbox is already
> selected
>     => Mbox is de-selected, and state of the cached connection changed to
> "no mbox is selected" state.
>     https://tools.ietf.org/html/rfc3501#section-6.3.1
>       Consequently, if a mailbox is selected and a SELECT command that
>       fails is attempted, no mailbox is selected.
> (c) "EXAMINE Mbox or other existent mbox than Mbox at cached connection
> where the Mbox is already selected
>     => EXAMINE command != SELECT command, so there is no problem.
>     => Mbox or other existent mbox than Mbox is selected with READ-ONLY at
> the cached connection
>     => IMAP client should issue "select" when he wants to access the mbox
> normally :-)
> 
> I prefer following if quirks for server like Charter net will be
> impokemented.
> - mail.imap.quirks_for_charter_net = true, or
> mail.server.server#.keep_connection_with_no_mbox_is_selected = true
>   - when IDLE cammand is issued in normal case, issue "select non-existent
> mboxname" instead of IDLE.
>   - when IDLE cammand is not used after imap commands execution,
>     issue "select non-existent mboxname" instead of keeping the Mbox
> selected at the cached connection.
> Required change is perhaps minimum.
> Upon new mail check, I guess at least "select Inbox(followed by uid fetch
> 1:* Flags)" is issued by Thunderbird.

Not sure what you are discussing above regarding "Idle" command but according to capabilites response, charter does not support IDLE. Just pointing this out in case it is relevant.

I used a telnet connection to port 143 on at imap.charter.net and experimented with the IMAP commands freely by typing them in. However, I still see the same thing: "uid fetch 1:* (flags)" will not show any new received email until either a "select inbox" or "examine inbox" is sent to charter. The command "status inbox (messages)" shows the count of inbox emails correctly after a new email is sent and before doing select or examine.  But you still can't access the new email(s) with "uid fetch NEXTUID:* (FLAGS)" without first doing select or examine on already selected inbox.

Comment 28

[Jorg K (CEST = GMT+2)]

Comment on attachment 8839338 [details] [diff] [review]
gds.diff -- experimental fixes

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

Thanks for your contribution and persistence! Sorry I don't have time (on a Sunday morning) to study every detail of the bug.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +368,5 @@
>                              &gExpungeThreshold);
>      aPrefBranch->GetIntPref("mailnews.tcptimeout", &gResponseTimeout);
> +
> +    // gds temp test: force expunge after delete with charter.net imap server
> +    aPrefBranch->GetBoolPref("mail.server.useCharterWorkarounds", &gUseCharterWorkarounds);

We would use a better name, especially in view of the fact that you want to force a select, so maybe:
mail.imap.force_select

@@ +371,5 @@
> +    // gds temp test: force expunge after delete with charter.net imap server
> +    aPrefBranch->GetBoolPref("mail.server.useCharterWorkarounds", &gUseCharterWorkarounds);
> +    if (gUseCharterWorkarounds)
> +    {
> +      gExpungeAfterDelete = true;

You can get this effect by setting mail.imap.expunge_after_delete, see
https://dxr.mozilla.org/comm-central/rev/a263d7290af0172fbcb1f5ccf4f89e2732e9b714/mailnews/mailnews.js#104
https://dxr.mozilla.org/comm-central/rev/a263d7290af0172fbcb1f5ccf4f89e2732e9b714/mailnews/imap/src/nsImapProtocol.cpp#361
Have you tested that? Oh, I see you have, please forgive me, put I don't have time to read through a bug with 27 long twisted comments right now.

@@ +2521,5 @@
> +        if (gUseCharterWorkarounds)
> +        {
> +          SelectMailbox(mailboxName.get());
> +          selectIssued = true;
> +          printf("\nat gds2");

I think another preference that only forces this would be a good option.
To to fix Charter problems you'd set:
mail.imap.expunge_after_delete and
mail.imap.force_select both to true.

Comment 29

[gene smith]

(In reply to Jorg K (GMT+1) from comment #28)

> I think another preference that only forces this would be a good option.
> To to fix Charter problems you'd set:
> mail.imap.expunge_after_delete and
> mail.imap.force_select both to true.

Just now saw this. Yes, I think that is OK. Somehow I think it should be documented that:

mail.imap.force_select=true fixes problems of emails received very late, sporadically or only on tb startup.

and

mail.imap.expunge_after_delete=true fixes problems with moving emails or restoring emails from junk or trash. However, as I explained in Comment 7 and Comment 8 above, this *alone* does not fix the email moving issues (you must also either set max cached connections to 1 and/or set the new/proposed mail.imap.force_select=true).

Specifically these fix problems with Charter's imap server; however, it is possible that other email providers may have these problems if they use the InterMail or Openwave or similarly mis-behaved imap servers.

(Note: I think Charter has pretty much deprecated the "Charter" name and they are calling themselves "Spectrum" now that they have merged with some other cable companies. I suggest that the alternate name "Spectrum" also be included in any documentation.)

Comment 31

[Jorg K (CEST = GMT+2)]

Thanks for the comments in bug 1258429. It's always complicated to discuss a similar issue in two bugs at the same time so I closed the other one.

This is my proposed solution to which you already agreed ;-)

Create a new preference mail.imap.force_select, and then document existing and new preferences as follows:

mail.imap.force_select: Set to true to fix problems when emails received very late, sporadically or only on Thunderbird startup.
This problem can be observed with Charter/Spectrum IMAP servers which use InterMail or Openwave or similarly misbehaved IMAP server software which requires extra (redundant) select commands to function properly.

mail.imap.expunge_after_delete: Set to true to fix problems with moving emails or restoring emails from junk or trash. At times, setting mail.imap.force_select is also required.

So Gene, if you could kindly change your patch, I can review it and commit it to the code base and you can have it in TB 52.2. We have strict rules and can't include patch into the current release (which will be TB 52.0 in the next few days). I'm a Mailnews peer/reviewer, so I can approve your work. I'm also the TB 52 release manager, so I can get it included into the release. Sounds good?

I need you to send this patch since we work based-on the four-eye principle. If I changed your patch, I'd have to find another reviewer to approve it and the will delay proceedings. Plus, you want to have the honour to be author and become a TB contributor, right?

Comment 32

[Jorg K (CEST = GMT+2)]

Gene, I assigned the bug to you. I hope you don't mind. It will be easy for you just to send the update patch.

Comment 33

[Jorg K (CEST = GMT+2)]

Make that: It will be easy for you just to send the *updated* patch, grr, grammar errors.

Comment 34

[gene smith]

Attachment: 1231592-charter-imap.patch

Here's the updated patch. I also included the change to the "pref" file to add the new parameter mail.imap.force_select (default false). 
Before I updated the files I did a 
python client.py checkout
to make sure I was using the latest version of the files (afternoon, 3 Mar 2017). I also did a "mach build" and retested the effect of parameters expunge_after_delete and the new force_select. Works as expected. Let me know if this is OK. Thanks!

-gene

Comment 35

[gene smith]

Typo alert! Meant to write that I did the update Monday afternoon 6 Mar 2017.

Comment 36

[Jorg K (CEST = GMT+2)]

Comment on attachment 8844308 [details] [diff] [review]
1231592-charter-imap.patch

Gene, thanks for the updated patch. I can see no problem with it. I'm known to pick on the comments people write, so let me note that we try to avoid putting bug numbers into the code, since you wouldn't see the code for all the comments ;-) We have a system called DXR (https://dxr.mozilla.org/comm-central/source/) which has a "blame" function which tells us which line was added in which bug. No need to update your patch again, I will adjust the comments slightly when landing it. Unless, see below, we need another change from you.

Whilst I have the authority to approve your patch (which I'm doing), let me get a second opinion on it from our IMAP expert, R Kent James.

Hey Kent, Gene pinned down why some IMAP clients work with Charter servers (running InterMail or Openwave or similarly misbehaved IMAP server software) and some, like TB, don't. Charter servers need an additional SELECT being issued. We're now doing that behind a preference. Do you see any problem with that?

It was criticised elsewhere that this preference applies globally do all IMAP accounts of a profile, but so do other global IMAP preferences. Do you see a problem with that?

If approved by you, do you see a problem in rushing this into TB 52? I don't see how an additional preference whose default value doesn't change current behaviour could do any damage.

Comment 37

[Jorg K (CEST = GMT+2)]

Sorry, Kent, I should have mentioned that this patch is the fix to bug 1258429 as well.

Comment 38

[Magnus Melin [:mkmelin]]

Doing this kind of thing behind a pref seems quite bogus to me. Nobody (generally speaking) will find it, or know they need it, and there's no reasonable way to have UI to let someone know. 

So I think you should do this unconditionally unless there are good reasons not to. But there appears not to be.

Comment 39

[gene smith]

(In reply to Magnus Melin from comment #38)
> Doing this kind of thing behind a pref seems quite bogus to me. Nobody
> (generally speaking) will find it, or know they need it, and there's no
> reasonable way to have UI to let someone know. 
> 
> So I think you should do this unconditionally unless there are good reasons
> not to. But there appears not to be.

I have tried the fix (mail.imap.force_select=true) with two other imap email accounts I have in addition to charter and see no problem (yahoo and gmail). I don't know if that is enough to know that it will work OK with *all* imap servers out there. Maybe others could test it with their account(s)?
If it is innocuous with other imap servers then, I agree, it shouldn't be hidden behind a pref since most users never look at release notes or other docs to know it needs to be turned on.

Comment 40

[Brian]

I still have this issue with Charter, close to 8mths or more old....Would you like me to test any patch you have?

Comment 41

[gene smith]

(In reply to Brian from comment #40)
> I still have this issue with Charter, close to 8mths or more old....Would
> you like me to test any patch you have?

Yes, that would be great! However, all I have now is a linux tb build. If you are on windows or mac then I would have to figure out how to produce the appropriate executable, that I have never done. I assume it is possible without special proprietary tools...? Or maybe you know how to build with my patch for your platform...?

Comment 42

[Jorg K (CEST = GMT+2)]

If you can't build it yourself, I can build a version containing this patch on our try servers and you can download it. Which platform do you need?

Comment 43

[gene smith]

(In reply to Jorg K (GMT+1) from comment #42)
> If you can't build it yourself, I can build a version containing this patch
> on our try servers and you can download it. Which platform do you need?

Not sure if you are replying to me or Brian, but sounds good if you can build a windows version that I will try on windows 10, 8.1 and 7. I don't have a windows build env. setup here.
Thanks.

Comment 44

[Jorg K (CEST = GMT+2)]

OK, in a few hours you can download a try build for Windows from here:
https://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-9a7d218451939eccf54db4da45b7c5fd8cc83412/

You need to set the preferences as discussed above.

Comment 45

[Kent James (:rkent)]

Comment on attachment 8844308 [details] [diff] [review]
1231592-charter-imap.patch

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

I can't claim to be an expert on IMAP or Select, but reading the bug I think the correct compromise is to enable this new choice by default, but leave the preference intact so that we can diagnostically disable it if we see new problems and wonder if this change caused it. That is a legitimate use of a hidden preference.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +320,4 @@
>  static bool gUseEnvelopeCmd = false;
>  static bool gUseLiteralPlus = true;
>  static bool gExpungeAfterDelete = false;
> +static bool gForceSelect = false; // bug 1231592

default true.

@@ +2516,5 @@
>        }
>        else
>        {
> +        // For some imap servers, must send imap SELECT even when already
> +        // SELECTed on same mailbox (bug 1231592).

As I read the bug, the relevant IMAP servers are misbehaving, so I would be clearer here in the comment about that:

// For some misbehaving imap servers ...

::: mailnews/mailnews.js
@@ +110,5 @@
>  // highest UUID seen instead of unread?
>  pref("mail.imap.filter_on_new", true);
> +// Force send of extra/redundant imap SELECT before FETCH if needed by
> +// some imap servers (bug 1231592)
> +pref("mail.imap.force_select", false);

Make this default true. Also, s/some imap/some misbehaving imap/

Comment 46

[Jorg K (CEST = GMT+2)]

Thanks, Kent. I'll get the patch fixed up and landed. Let's see what experience people have with the try build.

Comment 47

[gene smith]

(In reply to Jorg K (GMT+1) from comment #44)
> OK, in a few hours you can download a try build for Windows from here:
> https://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-
> 9a7d218451939eccf54db4da45b7c5fd8cc83412/
> 
> You need to set the preferences as discussed above.

Ran the install file for "daily" on a win 7 and win 10 box. Tested with and without the new force_select and results were as expected. It fixes the problem with charter when mail.imap.force_select is set true with other parameters at default (except for get-mail timer set to 2 minutes instead of 10 just for faster testing).

However on another win 8.1 box that my wife uses and that I have heard few complaints about email not being received, I haven't installed "daily" yet. With check email timer at 1 minute and 1 cached connection it basically works OK as is! Why? 

I just ran it with logging enabled and see that it SELECTs and fetches junk mbox after several minutes. Therefore after new emails arrive it shortly SELECTs inbox again (since there is only one tcp/ip connection). So it detects new emails. But the select of junk occurs maybe every 5 minutes so it takes more than the configured 1 minute to receive new email. (Note: test emails I send from yahoo are almost instantly available at charter.)

I think the win 8.1 box is selecting junk mbox periodically because it has a non-default setting to delete emails in junk every 7 days. The other systems I have on linux and windows are not set to automatically delete junk so they need the new force_select to receive emails. I set one of them, the win 10 box now running "daily", to delete junk after 7 days and it appears to received emails even with with new force_select=false (and with max cached connections at 1). 

Looking on the win 10 box running "daily" with logging enabled, I see that SELECT of junk occurs about every 5 minutes with junk setting "Automatically delete junk mail older than X days" set. With it un-set, a SELECT of junk never occurs automatically, so with 1 connection, inbox never gets re-selected and new emails are not detected with force_select set false.

So an alternative to force_select=true is to set these account specific parameters:
a) max cached connection set to 1
b) move messages marked spam to a junk/spam folder of your choice
c) auto delete junk/spam older than X days
d) set get-mail timer to 5 minutes or more since junk/spam selected about every 5 minutes
or disable timer and just click "get new mail".

However, most charter users would not know these workarounds so I think the force_select (and probably defaulting it to true) is definitely a better solution and solves the receiving problem.

Note also that mail.imap.expunge_after_delete must also be true to MOVE emails between charter folders without problems unless max_cached_connection is 1, even with the new force_select set to true. This is probably because charter doesn't support the imap MOVE function but only COPY (pointed out by WADA). I can't really tell but this may not just be a charter specific problem since the other accounts I have tested all support MOVE: gmail and yahoo (and I suspect most modern IMAP servers have MOVE capability?). 

So, and I haven't researched this, a complete solution might be to force the expunge_after_delete actions if the server does not support MOVE (similar to my original proposed fix, attachment gds.diff).

Comment 48

[Jorg K (CEST = GMT+2)]

Attachment: 1231592-charter-imap.patch (v2).

I addressed the review issues and made the new preference 'true' by default.

Gene, thanks for testing.

You mention that for servers that don't support MOVE, it would be beneficial to Expunge() just as if mail.imap.expunge_after_delete were set.

So maybe set gExpungeAfterDelete also for servers which don't support MOVE.
https://dxr.mozilla.org/comm-central/search?q=gExpungeAfterDelete&redirect=false

It appears that the MOVE capability can be queried, see:
https://dxr.mozilla.org/comm-central/search?q=kHasMoveCapability&redirect=false

Would to like to enhance your patch to cover this case?

Comment 49

[gene smith]

(In reply to Jorg K (GMT+1) from comment #48)
> 
> Would to like to enhance your patch to cover this case?

Have something working but I need to test it some more...
Thanks for pointing out the dxr tool!

Comment 50

[gene smith]

Attachment: 1231592-charter-imap.patch(V3)

(In reply to Jorg K (GMT+1) from comment #48)
> 
> Would to like to enhance your patch to cover this case?

I think I found a way to fix the problems with moving emails between imap folders on the charter imap server. I had noticed that a capability of the charter server is UIDPLUS but didn't know what that meant. I was thinking that if there was a way to only expunge the messages "moved" (actually, copied) to another folder, that would solve the problem and would avoid having to expunge the whole source folder. I found the rfc for UIDPLUS and it defines a command UID EXPUNGE that is exactly what is needed since charter server doesn't support MOVE.

I think the root problem with the charter server is that when messages are "moved" to another folder they are still present in the source folder but marked as /deleted, which is expected. If a moved message is then copied or moved back to the original source folder it becomes gone forever in the source folder for unknown reasons. But, if the message in the source folder is copied and marked as /deleted and then the whole source folder or only the moved message is expunged, there is no problem moving or copying the message back to the source folder.

The pref expunge_after_delete set true will cause the source folder to be expunged and solves the problem. However, it is defaulted false to avoid having other clients that access the same folder having emails that they only marked as /deleted getting expunged and disappearing forever.

Servers that support MOVE have less problems with unexpected expunges since a true MOVE will copy the selected emails to another folder and then before the MOVE response occurs, the server itself will expunge the moved emails and reports them as untagged responses; and the server will not expunge any other emails in the folder, even ones marked as /deleted, so other clients are not affected as much.

Servers that don't support MOVE but support UIDPLUS (e.g., charter) can very closely simulate the MOVE behavior. "Moved" message are copied to the destination folder and are marked as /deleted in the source folder. Since UIDPLUS implies the UID EXPUNGE command is supported, the same UIDs that were just marked as /deleted can also be UID EXPUNGEd. 

The patch (V3) adds this functionality and calls UidExpunge(). 

The patch has no effect on servers that support true MOVE even if they also support UIDPLUS, e.g., yahoo and gmail. It also does not require any new or changed prefs (other than the force_select that addresses another problem). I have tested it with all the delete selections (move to trash, mark as deleted, delete immediately) and they all work as expected.

Comment 51

[Jorg K (CEST = GMT+2)]

Comment on attachment 8847436 [details] [diff] [review]
1231592-charter-imap.patch(V3)

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

Great analysis! All your fellow Charter clients will be so happy for getting this fixed. I have a question though.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +3019,5 @@
>              if (m_imapMailFolderSink)
>                m_imapMailFolderSink->OnlineCopyCompleted(this, copyState);
> +            // Don't mark msg 'Deleted' for aol servers or standard imap servers that support MOVE
> +            // since we already issued 'xaol-move' or 'move' cmd.
> +            bool imapMoveSupport = GetServerStateParser().GetCapabilityFlag() & kHasMoveCapability;

New variable here.

@@ +3024,3 @@
>              if (GetServerStateParser().LastCommandSuccessful() &&
>                (m_imapAction == nsIImapUrl::nsImapOnlineMove) &&
> +              !(GetServerStateParser().ServerIsAOLServer() || imapMoveSupport))

Used here. That's the only use, so why have the new variable?

Comment 52

[gene smith]

(In reply to Jorg K (GMT+1) from comment #51)
> Comment on attachment 8847436 [details] [diff] [review]
> 1231592-charter-imap.patch(V3)
> 
> Used here. That's the only use, so why have the new variable?

Good point. In an earlier iteration of my changes I had used that variable again below. I didn't notice I left it in until I made the patch after testing the code. But I thought, well, I'll leave it in since it will probably be optimized out by the compiler and it doesn't add any more lines to an already 10000 line file. But, I will attach a "V4" patch with it removed since it doesn't really add anything to the fix.

Comment 53

[gene smith]

Attachment: 1231592-charter-imap.patch(V4)

Fixed patch V3 to exclude unneeded new variable.

Comment 54

[Jorg K (CEST = GMT+2)]

Attachment: 1231592-charter-imap-v4.patch

Excellent, many thanks. Very helpful comments, too.

I'm know for being a terrible reviewer when it comes to comments. So I've taken the liberty to touch them up a bit ;-) We also have an antiquated line-length limit of 80 characters, so I reshuffled some comments.

Altogether, great research and a great patch.

Comment 55

[gene smith]

(In reply to Jorg K (GMT+1) from comment #54)
> Created attachment 8847809 [details] [diff] [review]
> 1231592-charter-imap-v4.patch
> 
> So I've
> taken the liberty to touch them up a bit ;-) We also have an antiquated
> line-length limit of 80 characters, so I reshuffled some comments.

Thanks! The adjusted patch looks good to me.  
-gene

Comment 56

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/56b629c7f032bf229a72c173c65cee31f48a8ddc

I think it would make sense to uplift this to previous versions of TB, otherwise we'll have it in TB 59 ESR in early 2018.

Comment 57

[Jorg K (CEST = GMT+2)]

Comment on attachment 8847809 [details] [diff] [review]
1231592-charter-imap-v4.patch

[Approval Request Comment]
Regression caused by (bug #): No regression, never worked correctly for certain IMAP servers.
User impact if declined: IMAP not working reliably for certain users.
Testing completed (on c-c, etc.): Manual.
Risk to taking this patch (and alternatives if risky):
Some risk, so let's not rush this to ESR.

Comment 58

[Wayne Mery (:wsmwk)]

I'd add, let's also not rush this to beta

Comment 59

[Jorg K (CEST = GMT+2)]

Backout:
https://hg.mozilla.org/comm-central/rev/0caf05e509724b99064ad9671aed95d382002f8a

Sadly this has caused test failures to be seen here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=84249183

TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_imapFilterActions.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_imapFilterActions.js | MoveToFolderBody - [MoveToFolderBody : 100] 3 == 4
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_imapFilterActionsPostplugin.js | xpcshell return code: 0 
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_imapFilterActionsPostplugin.js | Label - [Label : 143] 2 == 0

Gene, could you see what's going on here? You can run the tests locally with the patch applied:

mozilla/mach xpcshell-test mailnews/imap/test/unit/test_imapFilterActions.js
and
mozilla/mach xpcshell-test mailnews/imap/test/unit/test_imapFilterActionsPostplugin.js

Maybe due to the additional expunging some message counts are different now.

Comment 60

[gene smith]

(In reply to Jorg K (GMT+1) from comment #59)
> 
> mozilla/mach xpcshell-test mailnews/imap/test/unit/test_imapFilterActions.js
> and
> mozilla/mach xpcshell-test
> mailnews/imap/test/unit/test_imapFilterActionsPostplugin.js
> 
> Maybe due to the additional expunging some message counts are different now.

Both seem to pass for me. But I haven't "applied" the patch. I am just running with my changes that should pretty much match the patch. What is the official procedure for "applying" the patch (if you think that is necessary)?

[gene@wally comm-central]$ mozilla/mach xpcshell-test mailnews/imap/test/unit/test_imapFilterActionsPostplugin.js
:
:
Summary
=======

Ran 98 tests (2 parents, 96 subtests)
Expected results: 98
Unexpected results: 0

OK

[gene@wally comm-central]$ ./mozilla/mach xpcshell-test  mailnews/imap/test/unit/test_imapFilterActions.js
:
:
Summary
=======

Ran 318 tests (2 parents, 316 subtests)
Expected results: 318
Unexpected results: 0

OK

Comment 61

[Jorg K (CEST = GMT+2)]

Well, first you clean your local repository, so things like:
hg revert --all
hg up -C default
You can of course also refresh from C-C and M-C by doing hg pull -u in C-C and the mozilla subdirectory.

To import the patch you go
hg qimport bz:1231592

Or download the patch, put it into .hg/patches, include it into the series file. Then:
hg qpush
to apply the patch.

Then you build again and run the tests. I've run both and both fail. Sorry I can't be of more help, I'm on a flight in the morning, it's almost 22:00 here now and I haven't packed yet. All I can tell you right now is that the tests fail locally just as they fail on our servers when running the test suite. You can see that some counts don't match (3==4 and 2==0). You can set the new preference to false in the test to get the previous behaviour, but I think the problem arises from the expunge.

Comment 62

[gene smith]

(In reply to Jorg K (GMT+1) from comment #61)
> 
> To import the patch you go
> hg qimport bz:1231592
>
That didn't work.
Just for future reference, I did this:
add two lines to .hg/hgrc to enable an extension,  
[extensions]
mq =
then
hg qimport https://bug1231592.bmoattachments.org/attachment.cgi?id=8847809

After updating and queuing the patch but before applying the patch with hq qpush, I first rebuilt and ran the two tests and they passed. Then I applied the patch and built again and they fail.

The only difference between my personal version and the now patched version is comments and the default value for new pref "force select". I still had it defaulted to false in in the cpp file and in the mailnews.js. 

Changing the inital value for gForceSelect in the cpp file had no effect on the tests. But setting the default value of force_select to false in mailnews.js causes the tests to pass. So it doesn't seem to have anything to do with 'expunge' differences. I will peruse the tests and see if I can find why they are an flagging errors due to force_select default.

Comment 63

[Jorg K (CEST = GMT+2)]

Great investigation, I was wrong again ;-(
On one thing I'm right though, |hg qimport bz:1231592| works if properly configured.

Comment 64

[Jorg K (CEST = GMT+2)]

One other thing: Those Xpcshell tests are hard to understand and to debug (and hard to write). Even experienced developers despair. Usually you can add dump() statements to the JS code and printf to the C++ code. You should focus on the shorter test first, or strip down the test to the part that fails.

test_imapFilterActions.js fails at line 100 in MoveToFolderBody, so lose all the other sub-tests and just run with this one. I think they are independent.

Don't get frustrated, ask for help. The value for gForceSelect in the C++ is irrelevant since it's set from the preference as you found out. I'm just very curious to understand why a supposedly harmless extra SELECT causes the behaviour to change.

Comment 65

[Jorg K (CEST = GMT+2)]

Comment on attachment 8847809 [details] [diff] [review]
1231592-charter-imap-v4.patch

Clearing approval requests since we need to fix the test failures first.

Comment 66

[gene smith]

Attachment: 1231592-charter-imap.patch(V5)

I never could figure out exactly why the 2 tests were failing. However, I found that if I move the new "if (gForceSelect) {SelectMailbox(); selectIssued=true}" inside the "if (m_needNoop)", then the tests pass. This also continues to work correctly with charter since m_needNoop gets set true on click of get-new-mail and when the get mail timer fires so the new redundant select still occurs when needed.

Also, now only calling Noop() if gForceSelect is false. Noop() is only called at this point to obtain the EXISTS message count to see if mail has arrived or gone away. SELECT returns the same information so calling Noop() immediately after SelectMailbox() is redundant and just adds slightly to network traffic and server load.

I also discovered that the determination of when to call Check() contained some "dead" code. The variable gPromoteNoopToCheckCount is always 0. This is because it is obtained from pref mail.imap.noop_check_count that doesn't exist anywhere that I can find. I don't see it when I look at my running mail.imap.* parameters and it is not in mailnews.js. Also a recursive grep looking for it in comm-central finds it only in nsImapProtocol.cpp. Same with the DXR tool. The variable m_noopCount also is used only with these other variables so it is not needed either. So I removed these variables and where they are used to determine when to call Check(). So Check() is now only called when CheckNeeded() returns true (which seems to be hard coded at 10 minutes). 

I also saw that with the original code arrangement that CheckNeeded() is called when the get new mail timer fires. And when it returns true (on 10 minute intervals) it calls Check() and then does not call Noop(). This potentially causes check for new mail to be skipped since, at least for charter, imap check doesn't return anything at all (just like Noop). So a potential gap in the check for new mail timer occurs every 10 minutes. I am now calling "if (CheckNeeded) Check();" after SelectMailbox() or Noop() to avoid the gap.

I ran the two tests with gForceSelect true (default) and with it false and both now pass.

Comment 67

[Jorg K (CEST = GMT+2)]

Attachment: 1231592-charter-imap.patch (v5 - merged with v4).

Thanks for looking into it further. Here's the merged patch.

Let's agree on some things to make this easier:
- The patch file should be called 1231592-charter-imap.patch
- No version number in the file name, especially not the
  extension part (since that will upset me on Windows).
- One patch only.

Since it's Sunday, I won't look into it further today, but some remarks:

A DXR search
https://dxr.mozilla.org/comm-central/search?q=mail.imap.noop_check_count&redirect=false
is sufficient, no need to grep (although of course that's a quick operation in mailnews/).

That a preference isn't defined in mailnews.js or all-thunderbird.js doesn't mean that the preference can't be used. You can still add it manually. Most prominent example:
mail.ui.display.dateformat.*
http://kb.mozillazine.org/Date_display_format#Changing_the_information_displayed
(Maybe we should actually add these preferences to that they're clearly visible.)

The next thing, sadly, is to do a Google search on mail.imap.noop_check_count. There you'll find bug 15260, bug 28355 and bug 332347, especially bug 332347 comment #6. I'll leave it to you to study these in detail. So I think we need to leave this preference untouched, but instead add it to mailnews.js with some documentation to avoid further confusion.

Thanks again for your contribution, it's great that we'll get this sorted!

Comment 68

[gene smith]

Attachment: 1231592-charter-imap.patch (v6)

Sorry, didn't realize there were intentional doubly-hidden preferences.
This patch now addresses the test failure but none of the other minor side issues I brought up in my last patch and comments. It reflects all the changes  made to the two files.

Comment 69

[Jorg K (CEST = GMT+2)]

(In reply to gene smith from comment #68)
> Sorry, didn't realize there were intentional doubly-hidden preferences.
No problem. Sadly in TB we have a lot of legacy code and all the original authors have left.

> This patch now addresses the test failure but none of the other minor side
> issues I brought up in my last patch and comments.
Oh, can't we incorporate addressing the side issues?

If I understood the previous patch correctly, in the case of forcing the select, there was no Noop() required. Since we now maintaining the mystery preference that lets you promote any n-th Noop() to a Check(), wouldn't it be OK to make the Noop() conditional, as in:

        if (m_needNoop)
        {
          // For some misbehaving imap servers, we must send imap SELECT even when
          // already SELECTed on same mailbox.
          if (gForceSelect)
          {
            SelectMailbox(mailboxName.get());
            selectIssued = true;
          }

          m_noopCount++;
          if ((gPromoteNoopToCheckCount > 0 && (m_noopCount % gPromoteNoopToCheckCount) == 0) ||
            CheckNeeded())
            Check();
Change:   else if (!gForceSelect)
            Noop(); // I think this is needed when we're using a cached connection
          m_needNoop = false;
        }
What do you think?

Comment 70

[gene smith]

Attachment: 1231592-charter-imap.patch (with conditional Noop() call) (v7)

That seems fine to me and covers the previous issue with minimal code movement: Noop() not needed when SelectMailbox() is called.

Comment 71

[Jorg K (CEST = GMT+2)]

Comment on attachment 8848936 [details] [diff] [review]
1231592-charter-imap.patch (with conditional Noop() call) (v7)

OK, thanks. I'll get this landed then.

Comment 72

[WADA:World Anti-bad-Duping Agency]

> 1231592-charter-imap.patch
> +// Force send of extra/redundant imap SELECT before FETCH if needed by
> +// some misbehaving imap servers.
> +pref("mail.imap.force_select", true);

It's not "per server setting"?

Following is for current CONDSTORE support and this kind of setting is ordinal way.
  mail.server.default.use_condstore
  mail.server.server#.use_condstore
No need of option like next?
  mail.server.default.imap_force_select
  mail.server.server#.imap_force_select

FYI.
force_select option is also a solution of problem like bug 693204.
  Note: I still prefer force_deselect option by "select non-existent-mbox" after mbox use -:)
Workaround of bug 693204 is almost same as manual workaround of this bug.
I think comment of "by some misbevaing imap server" is not addequate as source comment.
"No flag change notification via IDLE" in bug 693204 is never "mis behaving".

If Gmail IMAP account and if [Gmail]/All Mail is pretty huge mbox, force_select=true may produce performance issue.
If such issue will arise, "per mbox force_select" may be needed.

Comment 73

[gene smith]

(In reply to WADA:World Anti-bad-Duping Agency from comment #72)
> > 1231592-charter-imap.patch
> > +// Force send of extra/redundant imap SELECT before FETCH if needed by
> > +// some misbehaving imap servers.
> > +pref("mail.imap.force_select", true);
> 
> It's not "per server setting"?
> 
> Following is for current CONDSTORE support and this kind of setting is
> ordinal way.
>   mail.server.default.use_condstore
>   mail.server.server#.use_condstore
> No need of option like next?
>   mail.server.default.imap_force_select
>   mail.server.server#.imap_force_select
> 

I think you are saying user_condstore is an example of how to create a server specific parameter?

> FYI.nsImapLiteSelectFolder
> force_select option is also a solution of problem like bug 693204.
>   Note: I still prefer force_deselect option by "select non-existent-mbox"
> after mbox use -:)
> Workaround of bug 693204 is almost same as manual workaround of this bug.

I read un-read emails on android device on charter and gmail. In tb, for charter have to wait for check for new email (10 min) and then mail becomes marked as read (not bold). On gmail (support IDLE) email becomes non-bold shortly after reading on android. So seems to be working but not sure if due to new select call.

> I think comment of "by some misbevaing imap server" is not addequate as
> source comment.
> "No flag change notification via IDLE" in bug 693204 is never "mis behaving".

Don't know what this means...

> 
> If Gmail IMAP account and if [Gmail]/All Mail is pretty huge mbox,
> force_select=true may produce performance issue.
> If such issue will arise, "per mbox force_select" may be needed.

Well, yes, there may be a problem here. I see that whenever a normal select or my new select occurs on charter and gmail, it triggers a read of ALL the flags in Inbox for charter and "All Mail" for gmail with "uid fetch 1:* (FLAGS)". This is a pretty long list (I only have 3-4K emails) and it occurs even when nothing has changed. I don't see or feel any performance problems but with a slow connection it might be a problem. Note: on gmail the new select doesn't occur as much since it uses IDLE and I have the poll timer for new mail disabled. I will look closer at this tomorrow.

Comment 74

[WADA:World Anti-bad-Duping Agency]

(In reply to gene smith from comment #73)
> I think you are saying user_condstore is an example of how to create a server specific parameter?

Yes.

> > I think comment of "by some misbevaing imap server" is not adequate as
> > source comment.
> > "No flag change notification via IDLE" in bug 693204 is never "mis behaving".
> Don't know what this means...

I wanted to say;

Although "force_select option/feature" was initially implemented to support IMAP server like Charter Net well,
the "force_select option/feature" is reasonable solution of problem of bug 693204.
- Mismatch between
  - IMAP client like Tb who expects "flag change notification via IDLE" because it's SHOULD.
  - IMAP server like Gmail IMAP, Dovecot, ... , who doesn't notify flag change via IDLE because it's never MUST.
- Even if max cached connection=1 is set and IDLE use is disabled by user,
  Inbox is always selected at a cached connection
  unless other than Inbox is explicitly selected by user with max cached connection=1.
  Then "uid fetch NextUID:* Flags" is issued upon next new mail check interval for the already selected Inbox,
  thus "flag change of already fetched mail at IMAP server" can not be detected by Thunderbird.
- "select Inbox, then uid fetch 1:* Flags" is a solution to know flag change of already fetched mails.
So, "force_select option/feature" is better called "new feature for Smart Phone like behavior in Thunderbird" instead of "quirks in Tb for misbehaving imap server".

Comment 75

[Jorg K (CEST = GMT+2)]

Wada, thanks for your comments, you've been working in this area for a long time.

Meanwhile I'd like to mention that the patch causes other test failures:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=063ae7917c4540d9a29a54e4c43dc81f080fef14

TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_dontStatNoSelect.js | xpcshell return code: 0

Comment 76

[gene smith]

(In reply to Jorg K (GMT+1) from comment #75)
> 
> TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_dontStatNoSelect.js |
> xpcshell return code: 0

This test times out. I can fix test_dontStatNoSelect.js by removing the condition on the Noop() that we added. Test needs that guy. Will look at other test failures next. More to come...

Comment 77

[Jorg K (CEST = GMT+2)]

(In reply to gene smith from comment #76)
> This test times out. I can fix test_dontStatNoSelect.js by removing the
> condition on the Noop() that we added. Test needs that guy.
Grrr, that was my request :-( - Ah, well. Thanks for your persistence.

Comment 78

[gene smith]

(In reply to Jorg K (GMT+1) from comment #77)
> (In reply to gene smith from comment #76)
> > This test times out. I can fix test_dontStatNoSelect.js by removing the
> > condition on the Noop() that we added. Test needs that guy.
> Grrr, that was my request :-( - Ah, well. Thanks for your persistence.
Well, it was my intention originally to make it conditional too.

The other test failures listed seem different. Are they related to mailnews? If so, how do I run them?

Comment 79

[Jorg K (CEST = GMT+2)]

Which other test failures? Sadly we rarely have a green run. Check: https://mzl.la/2lrQTHC. The failure in test_reload.js is expected. Patch to fix that is awaiting review.

So what are we going to do? Try the patch without the extra condition? What about Wada's suggestions re. changing comments and making the preference server specific?

Comment 80

[gene smith]

(In reply to Jorg K (GMT+1) from comment #79)
> 
> So what are we going to do? Try the patch without the extra condition? What
> about Wada's suggestions re. changing comments and making the preference
> server specific?

I can research how to make them server specific like CONDSTORE . If made server specific would they all default to "force_select=true"? Guess that and the comment wording can be decided after figuring out how to do it. I will give it a try.

Comment 81

[gene smith]

I may be wrong but condstore doesn't seem to have different values by server#. Instead, if you set it TRUE in about:config it is used by only servers that have CONDSTORE capability. Gmail has that cap and I have it in tb and when I set it true in about:config I only see the default value now true and no additional server specific value.
I see this: mail.server.default.use_condstore=true
but not this: mail.server.server#.use_condstore=true

Comment 82

[Jorg K (CEST = GMT+2)]

I think "use_condstore" was a bad example. Look for something like: mail.server.default.check_time (looking for check_time). That's server specific.

Comment 83

[gene smith]

Attachment: Advanced-account-settings.png

This shows a proposed new server specific parameter selection at the bottom of the "Advanced Account Setting" screen. This selects the value for mail.server.serverX.forceSelect. It currently defaults to false (not checked). See the following patch attachment for more discussion.

Comment 84

[gene smith]

Attachment: 1231592-charter-imap.patch (v8)

Here's a proposed patch for adding the server specific parameter mail.server.serverX.forceSelect. I think it is better to have it set in the "Advanced Account Settings" dialog since, when you have several accounts, it is a bit confusing and not very "user friendly" to have to go into about:config and find the correct server number and then find the appropriate pref and set it.

I have tested it with printf's in the code and it works as expected. With my two charter accounts I have "Force IMAP select when checking new mail" checked in the "advanced" screen. For my other accounts, yahoo and gmail it is not checked. I see when I click "get new mail" or when the get mail timer expires that the forced select occurs only for the charter accounts and not for the others.

The default value for forceSelect is false so now when a new account is set-up the new item in the advanced setting starts out not checked. 

Of course all is open to discussion and this is just a proposal.

Comment 85

[WADA:World Anti-bad-Duping Agency]

(In reply to gene smith from comment #84)
> I think it is better to have it set in the "Advanced Account Settings" dialog (snip)

Kind action for users. I didn't expect UI enhancement.
Because check box for "use_idle" is already moved to main Server Settings panel, "check box for ForceSelect in IMAP only Server Settings/Advanced panel" is good idea.

Comment 86

[Jorg K (CEST = GMT+2)]

Great job! Hopefully you'll join the development team when after this patch ;-) A good next bug would be bug 1348762. But no obligation ;-)

Some remarks:

Just for the record: "use_idle" is connected to "Allow immediate server notifications when new messages arrive".

The UI enhancement is nice, but I can't land that on TB 52 since I can't do new strings. So we'd need a patch without that if you want this to arrive in TB 52 one day during its lifetime of about one year.

Check your patch clicking onto the review button to see that you have some trailing spaces. Please remove them in the next round.

And finally: Wada suggested (comment #72) to change the wording of the comments and not to call those servers "misbehaving" (quote):
"No flag change notification via IDLE" ... is never "misbehaving".
Sadly I don't understand the details enough to come up with a better wording. Would it be this?
  For servers which don't notify flag change via IDLE, we must send imap SELECT
  even when already SELECTed on same mailbox to detect new email.

Comment 87

[gene smith]

Attachment: 1231592-charter-imap.patch (sets forceSelect only via config editor) (v9)

Well, this was harder than I expected! The problem I ran into was that the "mail.server.serverX.forceSelect" items don't automatically appear in the config editor, only the default item "mail.server.default.forceSelect". 

There may be a better way to fix this but I couldn't find anywhere this type of thing is done. All the mail.server.serverX items I looked at seemed to be gui controlled or set internally or via imap capabilities and not intended to be 
set by a user in the config editor.

What I did was make the default value, as set in mailnews.js, for mail.server.default.forceSelect -1 (was false). Then in my code I changed the type of forceSelect from bool to int. So where forceSelect is obtained in the main protocol file it is checked for the value -1. If it is -1, I then set it to the "effective" default of 0 (boolean false) using SetForceSelect(). This causes all the items mail.server.serverX.forceSelect to appear in bold as modified instead of being invisible. Without a change from the default value the server specific items don't appear.

The user can now see and set the mail.server.serverX.forceSelect to 0 (disable) or 1 (enable). Any non-zero value other than -1 should also enable. If set to -1 (or reset in the editor right-click menu) it will become to 0 (disabled) again.

Maybe there is an easy way to do this by marking or setting the hidden serverX items as "modified" and then they appear without having to do an actual modification? If so, I can't find it.

I left in the added gui code but commented out some in one file am-server-advanced.xul. With 5 lines uncommented you get the gui checkbox again. Not sure if this is OK. Probably not.

If and when the gui checkbox can be enabled, the type of forceSelect can be returned to bool and code that changes the default value so hidden items are un-hidden in config editor can be removed.

I think I got rid of any trailing blanks I may have added. I originally had my editor set to remove all trailing blanks but it removed the existing one and not just mine. So I had turned that off and didn't think about it since it appeared trailing blanks wasn't a problem.

I also tweaked the "misbehaving" comments. But maybe they are a bit too verbose?

Comment 88

[Jorg K (CEST = GMT+2)]

Thanks for the updated patch. I can't say that I like the trickery with -1/false, etc. A user can add any preference in the config editor (as we know), so if it ever ship this without GUI, the user will have to do that.

Oh, I'm sorry, I worded this badly:
> The UI enhancement is nice, but I can't land that on TB 52 since I can't do
> new strings. So we'd need a patch without that if you want this to arrive in
> TB 52 one day during its lifetime of about one year.

I think we should first land this on Trunk with all the GUI bits, and then think about the non-GUI version later.

So I think the previous patch was OK, it just needed some comments to be changed and the added trailing spaces removed again. Usually we only make sure that we don't at trailing spaced in new code, we don't go around removing them whole-sales since that gives us a lot of changes.

Let me do this for you.

Comment 89

[Jorg K (CEST = GMT+2)]

Attachment: 1231592-charter-imap.patch (v8b).

This is v8 (without the trailing spaces) and the comments from v9. Did I get that right? I'll do another try run once you confirm.

Comment 90

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9d73a32e10db64cd1d3ca7a743898c31a2d3c6cb

Comment 91

[Jorg K (CEST = GMT+2)]

Comment on attachment 8850036 [details] [diff] [review]
1231592-charter-imap.patch (v8b).

Richard, does the GUI look good to you?

Comment 92

[Jorg K (CEST = GMT+2)]

Actually, I should have tried this with the default set to true:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6853c638f852f7e73d6bae86c51e8582a13c9db9
to make sure that works as well.

Comment 93

[gene smith]

(In reply to Jorg K (GMT+1) from comment #89)
> Created attachment 8850036 [details] [diff] [review]
> 1231592-charter-imap.patch (v8b).
> 
> This is v8 (without the trailing spaces) and the comments from v9. Did I get
> that right? I'll do another try run once you confirm.

Looks ok to me.

Comment 94

[Jorg K (CEST = GMT+2)]

Comment on attachment 8850036 [details] [diff] [review]
1231592-charter-imap.patch (v8b).

Both try runs are "green" (if you subtract other failures which will be fixed in the next 12 hours). So I'll finally get that landed after I get Richard's OK.

Comment 95

[Richard Marti (:Paenglab)]

Comment on attachment 8850036 [details] [diff] [review]
1231592-charter-imap.patch (v8b).

I give ui-r+ for the position in Advanced Settings but ui-r- because I'm not happy with the wording. It describes the technical function but not when and why the user should enable it. Without deeper IMAP knowledge, or be CCed to this bug, now one knows for what it is. A better wording or at least a tooltip describing when it makes sense to use this should be added.

This is a mailnews patch. Shouldn't be at least the same text in suite's dtd file be too to not break SM?

Comment 96

[Jorg K (CEST = GMT+2)]

Thanks, let me handle this. Yes, adding it to the suite/ file as well, thanks for the reminder.

Comment 97

[Jorg K (CEST = GMT+2)]

Attachment: 1231592-charter-imap.patch (v8c).

Now with tooltip and the same text in the suite/ file as well.

Understanding the intricacies of the IMAP protocol is a difficult issue, hence the longish tooltip.

We can't dumb it down to:
[ ] Always get read messages in IMAP folder right ;-)

Gene, do you like my wording?

Comment 98

[Richard Marti (:Paenglab)]

Comment on attachment 8850140 [details] [diff] [review]
1231592-charter-imap.patch (v8c).

Better. I'm not native English, but would instead of "...or show email as read when read on another device", "...or show email as read when *set as* read on another device" be better?

Comment 99

[Wayne Mery (:wsmwk)]

I think yes. Like maybe...

Some servers need an additional IMAP select to reliably show new email, or to show email as read when it has been set as read on another device.

Comment 100

[Jorg K (CEST = GMT+2)]

Well, they *read* the e-mail on another device, like Wada said in comment #74 (smart phone behaviour). They don't *set [it] as read*.

Anyway, Gene as a Charter user is in the US, so he'll help with the final wording. Gene, how to improve:
Some servers need an additional IMAP select to reliably show new email or show email as read when read on another device

Comment 101

[Jorg K (CEST = GMT+2)]

Maybe:
Some servers need an additional IMAP select to reliably show new email, or to show email as read when it has been marked as read on another device.

Comment 102

[gene smith]

(In reply to Jorg K (GMT+1) from comment #101)
> Maybe:
> Some servers need an additional IMAP select to reliably show new email, or
> to show email as read when it has been marked as read on another device.

I see it as improving the receipt of email, not "showing" email. I would prefer a bit different description on the 2nd half about read emails. Also, (if this a tooltip popup) it might indicate a drawback of selecting this. So I might prefer:

Some servers need an additional IMAP select so new email is reliably received by Thunderbird. The select also helps 
Thunderbird shows an email as read after it has been read in another device or application. Selecting this will cause some increase in network activity.

Either yours or mine is OK with me. But not sure if above is a tooltip or a longer description after the checkbox? If the above is a tooltip then would the checkbox still read:

[ ] Force IMAP select when checking for new mail
?
or maybe this is better ("force" sounds a bit extreme):

[ ] Send IMAP select to server when checking for new mail

P/S: Not sure how to code a tooltip. Do you want me to do this?

Comment 103

[gene smith]

(In reply to gene smith from comment #102)
> (In reply to Jorg K (GMT+1) from comment #101)

> P/S: Not sure how to code a tooltip. Do you want me to do this?
Sorry, I see you already add the code

Comment 104

[Jorg K (CEST = GMT+2)]

I'll do it. The tooltip is already in the patch, I'll just change the wording. Stand by ;-)

Comment 105

[Jorg K (CEST = GMT+2)]

Let's do:

[ ] Send IMAP select to server when checking for new mail

Tooltip:

Some servers need an additional IMAP select so new email is reliably received, or is shown as read after it has been read on another device. Selecting this option will cause some increased network traffic.

OK?

Comment 106

[Jorg K (CEST = GMT+2)]

Attachment: 1231592-charter-imap.patch (v8d).

Improved wording.

Comment 107

[Richard Marti (:Paenglab)]

I'm okay with this.

Comment 108

[Jorg K (CEST = GMT+2)]

Attachment: Tooltip screenshot

OK like this?

Comment 109

[gene smith]

TT fine with me!

Comment 110

[Jorg K (CEST = GMT+2)]

Great. Frankly, I'd like to see this landed sooner than later, so I think this time has finally come ;-)

Comment 111

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/4fd458dcdd4a95a833e577ed0ef2e2b0d176b85e

Let's see how this goes and then we'll think about uplifting this without the string changes.

Comment 112

[Andrei Hajdukewycz [:sancus]]

This thread is very long and I didn't read every single comment, so I apologize if I'm covering ground that's already been covered, but why not remove mail.server.default.forceSelect from the UI as rkent suggested in Comment #45? Now that this is checked by default, it seems way too technical for any user to make a decision that they need to uncheck this, and it's even harder for localizers to convey what the pref even means.

Comment 113

[Jorg K (CEST = GMT+2)]

It's not checked by default, the opposite it true, it's unchecked by default.

Having it checked causes some unnecessary network traffic, so it should only be checked if necessary for a particular server. Gmail for example doesn't require it.

Kent's comment #45 predates all the "real" discussion we had after tests started to fail in about comment #59.

Comment 114

[Andrei Hajdukewycz [:sancus]]

OK, my bad. So shouldn't it be checked by default since all it does is cause additional network traffic, then? That seems like a pretty minor side effect for ensuring all IMAP servers worked. I dunno, I don't see a tiny amount of additional network traffic as significant compared to a user having to track down what this pref does and why they should check it if their IMAP server doesn't work.

Comment 115

[Jorg K (CEST = GMT+2)]

See last paragraph in comment #73. We didn't want to degrade performance for unaffected users.

Comment 116

[Andrei Hajdukewycz [:sancus]]

That's fair. Thanks for walking me through it. Is there any way to detect servers that require this pref, even to the point of having a list of them in Thunderbird? Seems pretty rare that it's needed. That approach is certainly more work, though.

The text is pretty much incomprehensible to non-technical users. This came up in #maildev because localizations of that text are really inconsistent and localizers have trouble understanding what it even means if they aren't already familiar with the IMAP protocol.

Comment 117

[Jorg K (CEST = GMT+2)]

Comment #31 says:
Charter/Spectrum IMAP servers which use InterMail or Openwave, these are not even listed here:
https://en.wikipedia.org/wiki/List_of_mail_server_software#POP.2FIMAP.

Gene, any way to detect those servers?

Of course translating is a difficult trade, you must actually understand what you're translating. If you're translating something related to the IMAP protocol, you need a basic understanding of the protocol. We wouldn't be here at comment #117 if the matter were dead easy.

Comment 118

[Andrei Hajdukewycz [:sancus]]

Yeah, I'm not at all suggesting there's an easy fix for this. We discussed it briefly in #maildev and Fallen/clokep noted it was strange to expose such a highly technical pref, after Tonnes said that localizers have a hard time writing the copy for this pref so that it makes any sense at all.

I'm not trying to meddle :) If you guys don't think it's worth changing anything here past the last patch(probably in a follow-up bug, not here) I'll leave it alone!

Comment 119

[Jorg K (CEST = GMT+2)]

(In reply to Andrei Hajdukewycz [:sancus] from comment #118)
> Fallen/clokep noted it was strange to expose such
> a highly technical pref, ...
Well, so we don't expose it and make it even harder for the users to find it?
Or we degrade performance for all Gmail users in order not to get translation problems?

Make a suggestion how to win this.

Comment 120

[Andrei Hajdukewycz [:sancus]]

Yeah, my best idea was 'keep an internal list of misbehaving servers, automatically check the pref for those, and add servers as necessary from filed bugs' which is a lot of additional work and may well not be worth it at all, depending on how common the broken servers are.

I'm obviously not the person who should be advocating for alternate fixes to this, given my poor knowledge of this problem. I was just trying to be helpful to the IRCers/localizers. I'll leave any further advocating to other people if they really care about this issue :)

Comment 121

[gene smith]

(In reply to Jorg K (GMT+2) from comment #117)
> Comment #31 says:
> Charter/Spectrum IMAP servers which use InterMail or Openwave, these are not
> even listed here:
> https://en.wikipedia.org/wiki/List_of_mail_server_software#POP.2FIMAP.
> 
> Gene, any way to detect those servers?

I've seen places in code where gmail and aol servers are given special treatment. Not sure how this works but I suspect it could also detect charter (or InterMail or Openwave). (Ideally, there would be some command you could send that detects the problem based on server reaction but I don't think there is one, but I haven't thought much about this approach.)

Comment 122

[Ben Bucksch (:BenB)]

Comment on attachment 8850178 [details]
Tooltip screenshot

> "Send IMAP select to server when checking for new mail" [in our prefs UI)


Guys, seriously? I've implemented both an IMAP client and an IMAP server, and even I have no idea what this means or - more importantly - when I should check this.

This sounds like this pref is a good idea to check whenever you have problems. Which is probably not what we want. This text is completely unhelpful. No user will know when to check this.

Don't put stuff on the user, if you can answer the question better than he can. How would a user even know he's using Charter IMAP? You, OTOH, know.

If this is about a specific IMAP server, then detect that server, e.g. using the server name string during the HELLO/CAPS phase, or detect it using its CAPABILITIES, or whatever, and enable it automatically for that server, and only for that server.

Please revert that UI (and the whole pref, in fact), and just detect the server.

Comment 123

[gene smith]

(In reply to Ben Bucksch (:BenB) from comment #122)
> 
> This sounds like this pref is a good idea to check whenever you have
> problems. Which is probably not what we want. This text is completely
> unhelpful. No user will know when to check this.

I don't totally disagree with your proposals, but did you notice there is a pop-up tool-tip that explains when someone might need this "advanced" option turned on that doesn't mention "select" or other technical details?

Comment 124

[Ben Bucksch (:BenB)]

Yes, I saw that tooltip. But you need to understand the user's perspective. Most users tell me "there was an error", and don't tell me which error, even if the error message was right in their face. Even if it's clear and understandable, they still don't bother. Do you think they'll understand this here?

This checkbox sounds like it's like a good idea to check whenever you have problems. We don't want that, esp. given that only 0.1% of our userbase is concerned. This UI is doing more harm than good, by confusing all the other users.

---

I see that detecting it is not trivial, because of the unhelpful CAPABILITIES response.
Please try to detect it anyway. Don't leave it to users, they have no way to know.

Comment 125

[Jorg K (CEST = GMT+2)]

New bug for any further work, please. A discussion like this is exactly wasting the resources we don't have, see bug 28211, 17 years old.

While you're at it, remove all the advanced IMAP preferences, since I never understood "IMAP server directory" or that namespace business either :-(

And hire someone to monitor the behaviour of InterMail and Openwave server software, so if it ever changes, we can follow.

Comment 127

[Jorg K (CEST = GMT+2)]

Comment on attachment 8850175 [details] [diff] [review]
1231592-charter-imap.patch (v8d).

Let's consider this for TB 52 ESR without the string changes. Just today another complaint arrived from a Charter user, bug 1359277.

The approval refers to a modified patch as landed in comment #127.

Comment 128

[Jorg K (CEST = GMT+2)]

Actually, as landed in comment #128:

Beta (TB 54):
https://hg.mozilla.org/releases/comm-beta/rev/aa6dfc810660c998859d9714a885d4c6a8dc75ee

Comment 129

[Ben Bucksch (:BenB)]

Comment on attachment 8850175 [details] [diff] [review]
1231592-charter-imap.patch (v8d).

Jörg, I've just said that this is not a good fix and should be backed out and re-done.

Why do you ask for backporting this to a stable branch?

This bug doesn't even meet the criteria for backporting in the first place, because it affects only few users, and it's always been there, it's not a regression. Plus, given that you change IMAP behavior, e.g. now a mail move works (a very basic operation), so this is dangerous and might theoretically cause regressions for other servers. So, even if the fix was good, it should not be backported.

ESR52-

Comment 130

[Jorg K (CEST = GMT+2)]

Comment on attachment 8850175 [details] [diff] [review]
1231592-charter-imap.patch (v8d).

Ben: I am the code manager for TB 52 ESR and I will decide what is included or not. In TB 52 ESR this would be a hidden feature anyway.

I have heard your opinion, please file another bug to discuss this issue further.

I general I *do not* appreciate you going around, complaining about bug fixes, re-opening bugs and demanding backouts, see for example bug 1342753.

You are in no position to do so. You are currently not a peer in any module of Thunderbird, and frankly, not even having rolled out your own IMAP software to 3 million users in an unnamed project gives you any rights here.

So please stick to your duties in the Thunderbird Council of which there are many and where you can employ your time better.

Also, we have discussed contributor motivation. Here we have the work of a new contributor which has been reviewed and approved. No one should come along who hasn't been part of the process and demand that work being reverted.

Comment 131

[Ben Bucksch (:BenB)]

Jörg, the TB council is running this project. If I see something going wrong, I have the obligation to speak out. This bug and bug 1342753 are both clear-cut examples of things going very wrong. If the result of the process is wrong, somebody needs to speak out. Who else, if not council members?

An option that not even an IMAP developer can understand what it means and what it does, is clearly not something that you can present to users in the UI. Sorry, this fails an objective minimum bar.

This has nothing to do with the persons involved, at all. I don't mean to say that I don't value your work. Surely, you're doing many good patches every day. In over 1 month, I've spoken out on only 2 bugs. You happen to be on both of them, because you do a lot of work, and that's appreciated. Can we please keep it on a rational level instead of getting personal?

Comment 132

[Jorg K (CEST = GMT+2)]

Take it up with the TB Council then, please. "Wrong" might be in the eye of the beholder.

Personally, I don't think there's anything particularly wrong with giving users of a large US ISP, Charter, the option to adjust the behaviour of TB by switching on a somewhat obscure option. If you have a better solution, like some sort of auto-detect, please present it in another bug. The UX change here was approved by TB's UX peer Richard.

As I said in comment #127: Just today we have a complaint from a Charter user in a new bug 1359277. I have to give them an even worse workaround since this bug isn't in TB 52 yet.

As for the date/time formatting issue, you're opposing the person hired by Mozilla to straighten out the localisation mess that existed. Again, file a new bug and see what Mozilla will do about it. Demanding a backout in bug 1342753 was not productive since it missed the bigger picture entirely.

Comment 133

[Ben Bucksch (:BenB)]

My problem is:
* This fix does not help Charter users, because they won't know the option exists, and the fix requires them to know. Even if they see it, they wouldn't understand it. You're helping only 10 users or so who contact us.
* This is so unclearly formulated that other users who see this option are inclined to check it, even though they don't need it. So, you're harming them.
* Even if you don't check the pref, the patch still changes how IMAP message move works, so there's an inherent code risk, as with every change. That's why we're very conservative about what to backport to stable, because it doesn't get the extensive testing of nightly, alpha, beta etc..
Keep in mind Charter is somewhere in the range of 0.5% of TB users (a lot less than 1%).

---

Suggestion: If this is mostly for Charter ISP users, just check the hostname of the IMAP server. You help 100 times more Charter users, and you don't risk to confuse other users. It's not nice, but better than this.

Comment 134

[Jorg K (CEST = GMT+2)]

(In reply to Ben Bucksch (:BenB) from comment #133)
> * This fix does not help Charter users, because they won't know the option
> exists, and the fix requires them to know. Even if they see it, they
> wouldn't understand it. You're helping only 10 users or so who contact us.
Not so. This would be published in support forums and maybe even the release notes.

> * This is so unclearly formulated that other users who see this option are
> inclined to check it, even though they don't need it. So, you're harming
> them.
No harm done. Performance will degrade a little. As I said, it's in the same category as the other hard to understand options (directory/namespace).

> * Even if you don't check the pref, the patch still changes how IMAP message
> move works, so there's an inherent code risk, as with every change. That's
> why we're very conservative about what to backport to stable, because it
> doesn't get the extensive testing of nightly, alpha, beta etc..
> Keep in mind Charter is somewhere in the range of 0.5% of TB users (a lot
> less than 1%).
Yes, the patch contains other code to fix move behaviour. Magnus complained in bug 1352859 and he's still considering his position. Personally I think that we improved the behaviour here.

I know that we are conservative, to that's why that got uplifted to TB 54 beta to give it a workout. Uplift to TB 52 is being considered.

> Suggestion: If this is mostly for Charter ISP users, just check the hostname
> of the IMAP server. You help 100 times more Charter users, and you don't
> risk to confuse other users. It's not nice, but better than this.
Looking forward to your patch in another bug ;-)

Comment 135

[Jorg K (CEST = GMT+2)]

BTW, the tooltip says:

Some servers need an additional IMAP select so new email is reliably received, or is shown as read after it has been read on another device.

So, people whose e-mail is not reliably received or not marked as read when they read it on their smartphone will be inclined to check this option. If it doesn't improve their situation, they should/will disable the option again.

You can't dumb down a complicated system so that everyone in the street will understand it. However, an autodetect of the situation would be great.

Comment 136

[Ben Bucksch (:BenB)]

> maybe even the release notes.

And does your mother read them?

---

Magnus, I'm referring this to you here.

My proposal:
* Remove UI option.
* Autodetect it somehow, either using CAPABILITY response, or some other hint in one of the responses, or worst case using the Charter IMAP hostname. Not nice, but still better than leaving it to end users.
* Don't backport. It's just a bugfix, changes IMAP behavior and helps less than 1% of users, at best.

Comment 137

[Jorg K (CEST = GMT+2)]

New bug , please. This one is closed.

I'm surprised you have telemetry information we don't have. 1% of users would still be 250.000 users.

Comment 138

[Wayne Mery (:wsmwk)]

> * Don't backport. It's just a bugfix, changes IMAP behavior and helps less than 1% of users, at best.

As I've said before, this isn't your call - it's release drivers (jorg) and release manager (me).  Please stop overstepping.

Comment 139

[Ben Bucksch (:BenB)]

Wayne, I referred to Magnus, and called it "My proposal".
Fact point: I just checked and Charter ISP actually has 0.1% of our users.
I hope that's my last comment here. I refer the rest to Magnus and leave the decision to him.

Comment 140

[Magnus Melin [:mkmelin]]

Well this is a bit of a mess...

My earlier comment was not to have a hidden pref, since it's not fixing thing for more than just a handful of users. Then Kent said to keep the pref and make it true by default. But there came performance concerns so it was left false. Which spiraled into it has to have UI then, because setting per server hidden prefs is quite hard. 

I'm not too crazy about autodetect for specific server's bugs either. When they fix the bug, then what? So, it appears to me the best way to resolve this would have been to contact Charter and have them fix their server bug instead of trying to paper over it on our end. I don't know if they would be responsive or not. Sometimes you're lucky.

Re backporting, we've backported pretty aggressively (for good or bad). This particular bug does change behavior for all users, so I'm note sure it's very good idea to backport to stable, but I'll leave that to Jörg+Wayne.


Now that we have the UI and all, maybe not worth removing it instantly. But if we can get Charter to fix the server it should obviously go. And really try to avoid per-server workaround going forwards.

Comment 141

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #140)
> This particular bug does change behavior for all users, ...
Right, in that is fixes the move behaviour for some servers.
We still have bug 1352859 on file where you wanted to tell us whether you can live with the new behaviour or not ;-)

Since we only do ESR, uplifts are the only way we get improvements to users without waiting one year. We're already limited by string changes.

As for contacting Charter: Surely they won't rip out the server software they're using; if anything, contact InterMail and Openwave.

I don't know where Ben has his data from, but 0.1% are still 25.000 users.

Comment 142

[Magnus Melin [:mkmelin]]

I don't know anything about their servers. Of course, they'd get their provider to fix it if it's not their own software. Even more reasons not to do per-server workarounds, if it's found elsewhere too.

Comment 143

[gene smith]

I and others have contacted charter but to no avail. Found a support email address for openwave. Sent them a question yesterday about the select problem but no response yet. Someone with more clout might be able to get a response: http://owmobility.com/contact/
I will make one last proposal to maybe resolve this without involving charter or openwave. I made a prototype that is working correctly that implements my proposal.
I found that tb does an Imap ID command (when listed in capabilities). Charter responds to ID with name="Email Mx" and vendor="Openwave Messaging". The exact response is this:
* ID ("name" "Email Mx" "version" "M.9.00.023.01 201-2473-194" "vendor" "Openwave Messaging" "support-url" "http://support.openwave.com" "date" "Mon Apr  4 21:15:22 PDT 2016")
The response to ID is already used to do special things for another server so adding something for openwave was not difficult.
The main idea of my proposal is to provide 3 values for the account/server specific preference "forceSelect":
0: Do the added SELECT only if ID response matches the ID "Email Mx/Openwave Messaging". This is the default value.
1: Always do the added SELECT. This can be used improve sync with other email clients as WADA pointed out.
2: Never do the added SELECT regardless of ID response.
The following attachments show the proposed selection of the 3 values (radio buttons) and the complete code changes to implements the proposal (sans tooltip text). Since this is just a prototype and not a formal patch submission, I have left in some printf's used for debug/testing. The diff is against my most recent accepted changes.

Comment 144

[gene smith]

Attachment: adaptive-select.png

Comment 145

[gene smith]

Attachment: adaptive-select.patch

Comment 146

[Jorg K (CEST = GMT+2)]

Thanks, Gene. I've moved this to bug 1360117.

Comment 148

[Jorg K (CEST = GMT+2)]

Comment on attachment 8850175 [details] [diff] [review]
1231592-charter-imap.patch (v8d).

As much as I'd like to ship this in TB 52 ESR, I've been criticised for uplifting too many functional changes to the "stable" branch. This bug causes a change in behaviour, see bug 1352859, so I'm afraid the critics have won and users need to wait for TB 59 ESR or use a beta.

Comment 149

[Jorg K (CEST = GMT+2)]

Comment on attachment 8850175 [details] [diff] [review]
1231592-charter-imap.patch (v8d).

By popular demand ;-)
Depends on bug 1352859.

Comment 150

[Ben Bucksch (:BenB)]

Reminder to fact from Ben Bucksch (:BenB) from comment #139:
> I just checked and Charter ISP actually has 0.1% of our users.

Comment 151

[gene smith]

(In reply to Ben Bucksch (:BenB) from comment #150)
> Reminder to fact from Ben Bucksch (:BenB) from comment #139:
> > I just checked and Charter ISP actually has 0.1% of our users.

Not sure how you know this unless tb has some spyware that is phoning home to TB/Moz-Hq as to what ISP I am using. :)
Anyhow, your figures might be right but you might not be aware that Charter (now called Spectrum) has merged with Time Warner Cable and Bright House. But I don't know if those additional customers have migrated to Charter's imap server.
Actually, you might instead need to check how many users are on the Openwave (aka, Email Mx) imap server since that seems to be the root of the problem, not specifically Charter. It is almost certain that other ISPs besides Charter use the Openwave server for imap based email.

Also, it is possible that most Charter (or Openwave) users don't have the loyalty I've had to tb. They try it and see they are not getting email (except when tb first starts up) and quickly and quietly move on to something that works.

Comment 152

[Matt]

(In reply to gene smith from comment #151)
> (In reply to Ben Bucksch (:BenB) from comment #150)
> > Reminder to fact from Ben Bucksch (:BenB) from comment #139:
> > > I just checked and Charter ISP actually has 0.1% of our users.
> 
> Not sure how you know this unless tb has some spyware that is phoning home
> to TB/Moz-Hq as to what ISP I am using. :)

The new account wizard queries the ISPDB for settings each time an account is added to Thunderbird.  Not hard to extrapolate relative user numbers from the level of queries to the server.  Having said that .1% is still some 10,000 users.  Mostly I would guess using POP.  Charter were very late to the IMAP game.

> Anyhow, your figures might be right but you might not be aware that Charter
> (now called Spectrum) has merged with Time Warner Cable and Bright House.
> But I don't know if those additional customers have migrated to Charter's
> imap server.

Brighthouse appear to be using RoadRunners network for their mail server.  As mail.brighthouse.com resolves to cdptpa-brighthouse-vip.email.rr.com

Comment 153

[Jorg K (CEST = GMT+2)]

I'd say that 0.1% are potentially 25,000 discontent users. Enough to care about.

Comment 154

[Jorg K (CEST = GMT+2)]

Comment on attachment 8850175 [details] [diff] [review]
1231592-charter-imap.patch (v8d).

Need to uplift the beta patches here which don't have string changes.

Comment 155

[Jorg K (CEST = GMT+2)]

TB 52.5 ESR (should be tracking 57+):
https://hg.mozilla.org/releases/comm-esr52/rev/2c0a5649e5d7
Uplifted the beta patch without the string changes.

Comment 156

[Magnus Melin [:mkmelin]]

In case it's not needed anymore, could be worth adding a code comment and remove it at some point in the future when all servers have the new version.