1771409 - Detecting the need for and sending of imap SELECT to detect new mail is no longer needed by Charter/Spectrum

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[gene smith]

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

This removes the code to automatically detect servers that require an extra imap SELECT to successfully get new mail during runtime. It was added to support the imap server used by ISP Charter/Spectrum. It was made general so that other servers, depending on imap ID response, could also be supported via a matching preference text string. However, the server used by Charter/Spectrum has been updated and now does new mail detection properly with just imap NOOP as required by RFC 3501. (Charter/Spectrum server also no longer responds with an ID string.) This problem has never been seen with other servers so most of the code that handles this can be removed. I did leave in a single boolean pref "force_select" (defaulted to false) that can be manually set true just in case some non conformant server still needs this.

Comment 1

[gene smith]

Attachment: 1771409-remove-force-select-detection.patch

This acts upon bug 1231592 comment 156.
The Charter/Spectrum server no longer has the issue and I haven't heard any reports of other servers needing this feature so I think it's ok to remove this somewhat obscure feature from future releases.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9278413 [details] [diff] [review]
1771409-remove-force-select-detection.patch

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

Thanks! r=mkmelin

Comment 3

[Ben Bucksch (:BenB)]

You wrote:

I did leave in a single boolean pref "force_select" (defaulted to false) that can be manually set true just in case some non conformant server still needs this.

In the patch:

-NS_IMPL_SERVERPREF_STR(nsImapIncomingServer, ForceSelect, "force_select")
+NS_IMPL_SERVERPREF_BOOL(nsImapIncomingServer, ForceSelect, "force_select")

The fact that you use the exact same pref name as before concerns me, because it was string before and now it's boolean. Some users will still have values in their prefs.js . I don't know how the preferences API will react to that? Will it throw an exception on read? Will it delete the string preference? Did you test this?

Comment 4

[gene smith]

Did you test this?

I did test it to make sure mail.server.default.force_select controlled whether the extra select is sent and it didn't crash. But I didn't try setting a specific mail.server.serverX.force_select to make sure serverX responds as expected. I have lots of accounts (servers) setup for testing and for each one I see in prefs.js and "Advanced Preferences" items like this (filtering on force_select) with the patch in effect:

mail.server.default.force_select false
mail.server.server1.force_select no-auto
mail.server.server12.force_select no-auto
mail.server.server21.force_select no-auto

The no-auto values are the string values from the release code. If I want to make server21 do the forced select with the patched code I have to "delete" the item in "Advanced Preferences" with the trash can icon, then change it from string to bool and add it back with + icon and set it to true and then restart TB. The old non-default string values don't automatically change to bool or automatically just get deleted. (The default item with the same name automatically changed from string to boolean: mail.server.default.force_select.)

Another thing to do might be to just change pref name from force_select to something else like force_select_imap. When I do this to override the default I have to create a new item such as mail.server.server21.force_select_imap as a boolean and set it true. However the old non-default strings like mail.server.server21.force_select no-auto are still present but cause no harm and can be manually deleted.

Anyhow, not sure which is best or if there is another way. Or maybe just remove the "force select" feature completely?

Comment 5

[Ben Bucksch (:BenB)]

just change pref name from force_select to something else like force_select_imap

Yes, that's the safe way to do.

maybe just remove the "force select" feature completely?

Your call.

Comment 6

[gene smith]

Attachment: 1771409-remove-force-select-detection-v2.patch

Your call

I just went with changing the name from "force_select" to "force_select_imap"

Comment 7

[Ben Bucksch (:BenB)]

Comment on attachment 9278584 [details] [diff] [review]
1771409-remove-force-select-detection-v2.patch

That's better, thanks Gene!

I'll let Magnus set his review flag again, because he did the initial review.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9278584 [details] [diff] [review]
1771409-remove-force-select-detection-v2.patch

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

Thanks, indeed that pref name rename was needed. r=mkmelin

Comment 9

[gene smith]

I re-tested to make double sure the patch builds and works and it does.

I mentioned this above that it still leaves in the old mail.server.serverX.force_select string values that are now ignored in TB. These can optionally be deleted using "Advance Preferences/Config Editor".

In case someone wants to still do the force select on all imap servers, just set mail.server.default.force_select_imap in Advanced Preferences to true and restart TB.

If force select is only wanted for a particular account/server you have to determine the server/account number X by looking with Advance Preferences and enter mail.server.server and find the account number based on other info shown such as server name. Then in Advance Preference enter mail.server.serverX.force_select_imap (where X is the corresponding server/account number such as server12) and add it with the + icon and select it as boolean and set it true . Then restart tb and the imap select will occur on each check for new mail for server/account X only.

Comment 10

[Ben Bucksch (:BenB)]

it still leaves in the old mail.server.serverX.force_select string values that are now ignored

That's fine, we have been doing that since 20 years. They don't cause any speed issue, and it might be useful for diagnosing in case something does go wrong (which we don't hope).

In case someone wants to still do the force select on all imap servers, just set mail.server.default.force_select_imap in Advanced Preferences to true and restart TB.

I think it's a safe approach, just in the case it might still be needed. If nobody needs this, we can take it out completely in the next release.

Well done, thank you, Gene!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5711dbdd2c78
Remove detection of need for forced imap select. r=BenB,mkmelin

Comment 12

[Geoff Lankow (:darktrojan)]

The test mailnews/imap/test/unit/test_imapID.js is now failing. Here's an sample log.

I'll likely back this patch out in a few hours unless there's a quick fix.

Comment 13

[gene smith]

Attachment: 1771409-remove-force-select-detection-v3.patch

I took out the place where the ID response string got stored. It is now only used to check for a very old Zimbra server put in by Bienvenu that is broken for condstore so I didn't see the problem just running normally. Sorry, should have did a try or run the xpcshell-test. It now passes ./mach xpcshell-test comm/mailnews/imap/test/unit/test_imapID.js

The change in the patch is here in nsImapProtocol.cpp (marked below with <----):

@@ -8196,78 +8148,23 @@ void nsImapProtocol::ProcessAfterAuthent
   // backend.  If we enable compression early the proxy
   // will be confused.
   if (UseCompressDeflate()) StartCompressDeflate();
 
   if ((GetServerStateParser().GetCapabilityFlag() & kHasEnableCapability) &&
       UseCondStore())
     EnableCondStore();
 
-  bool haveIdResponse = false;
   if ((GetServerStateParser().GetCapabilityFlag() & kHasIDCapability) &&
       m_sendID) {
     ID();
-    if (m_imapServerSink && !GetServerStateParser().GetServerID().IsEmpty()) {
-      haveIdResponse = true;
-      // Determine value for m_forceSelect based on config editor
-      // entries and comparison to imap ID string returned by the server.
+    if (m_imapServerSink && !GetServerStateParser().GetServerID().IsEmpty())    <-----
       m_imapServerSink->SetServerID(GetServerStateParser().GetServerID());      <-----

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9278865 [details] [diff] [review]
1771409-remove-force-select-detection-v3.patch

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

Thanks, looks ok try as well!

Gene: please consider switching to using moz-phab. Setting up and learning it is 10min very well spent - https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d290d9886eb6
Backed out changeset 5711dbdd2c78 for test failures. r=backout
https://hg.mozilla.org/comm-central/rev/09164b65684e
Remove detection of need for forced imap select. r=BenB,mkmelin