1747311 - Add support for imap extension QRESYNC and improvements for CONDSTORE (RFC 7162)

Status: NEW

(MailNews Core :: Networking: IMAP, enhancement)

Description

[gene smith]

Attachment: CS-fixes-add-QR---it-works.diff

CONDSTORE and QRESYNC are specified here: https://datatracker.ietf.org/doc/html/rfc7162.

Attached is improvements to the CONDSTORE feature that already exists in TB. It also includes the addition of "Quick Re-Sync" (QRESYNC).

I've tested this with commonly used imap servers that support both CONDSTORE and QRESYNC or just CONDSTORE. It seems to be working well but I need to do some more testing and maybe some more code changes before submitting a formal patch.

Both CONDSTORE and QRESYNC:
Dovecot
Cyrus
Zimbra
iCloud

Only CONDSTORE:
gmail

I may also test with Communigate-Pro which is in the process of adding CONDSTORE and possibly QRESYNC support. There are several other "enterprise" level servers, such as Oracle, that also reportedly support CONDSTORE and QRESYNC that I don't have access to for testing.

Note: RFC 7162 allows a server to support only CONDSTORE (e.g., gmail) but if QRESYNC is supported, CONDSTORE must also be supported.

Comment 1

[gene smith]

Attachment: CS-QR-Advanced-setting.diff

This diff is a proposed UI change to the "Advanced" server settings to allow CONDSTORE to be enabled on a per-server basis. It will appear at the bottom of the screen and default to disabled for each server (account). I've been running this with my many test accounts and it is working correctly.

See the next comment for screenshot.

If the new item is set to "enabled" (checked/ticked) it will create and set the pref mail.server.serverX.use_condstore to true where X is the server key assigned at account creation. Default mail.server.default.use_condstore remains set to false so this will require the user explicitly enable CONDSTORE using the new advanced setting.

This will allow a possible CONDSTORE problem with a single account to not require that it be disabled for all accounts. It also eliminates the need for the user to determine the number "X" for added pref mail.server.serverX.use_condstore and eliminates the need to manually create the added pref.

Note: This only has an effect if the server actually supports CONDSTORE. Also, if QRESYNC is supported by the server, setting this will enable QRESYNC. With my proposed implementation it's not possible to disable QRESYNC and enabled just CONDSTORE if the server supports QRESYNC. That would require more prefs and added program logic and is probably not that useful (since QRESYNC actually "fixes" issues with basic CONDSTORE).

Comment 2

[gene smith]

Attachment: CS-QR-Advanced.png

Comment 3

[gene smith]

Attachment: CS-fixes-add-QR-final.diff

This is an alternate and improved method to support CONDSTORE and QRESYNC compared to my original attached diff. With my original approach, mostly based on the current CONDSTORE implementation, reliably detecting message flag changes, additions and removals (expunges) by other clients when only CONDSTORE was in effect, without doing a full flag fetch, was often problematic. (The primary reason for supporting CONDSTORE and QRESYNC is to reduce the occurrence of full flag fetches [syncs] so a mailboxes with thousands of messages requires a shorter wait for sync to complete on the initial select.)

The issues with my original diff were more pronounced when the server only supported CONDSTORE, such as gmail. (Edit 8/11/23: The main issue is when a message is deleted by other client or another TB instance, this TB instance must be restarted for the message count to go down and for the message to disappear from the list.) This diff now builds a "backfill" array of UIDs, indexed by message sequence so that the effect of a message expunge is immediate. The backfill array is created from the UIDs already stored in the database on the first folder select during a session. But it can only be created this way when nothing was expunged by another client while the the folder was not selected or when tb was shutdown. Otherwise, if something was expunged, the array is not created by backfill but is created from scratch by a full flag fetch in the original way.

Edit 8/11/23: The backfill still does not eliminate the the need for full flag sync such as when the other client/TB instance deletes a message with this TB instance shutdown. QRESYNC is require to handle this but gmail still does not support QRESYNC. (From what I can tell, gmail is the only server type that supports CONDSTORE but not QRESYNC.)

The implementation of QRESYNC (supported by many popular non-gmail servers) is mostly the same as the original diff and, theoretically, should require only one full flag fetch (if there are no bugs in the server and the client).

This is a large and complicated change (I've been working on it since September) and requires a lot of detailed and often tedious testing to ensure everything is working as expected with the various imap server types, using multiple clients or tb instances while recording and examining the corresponding imap logs. I've done a lot of testing and code fixing but I'm sure more would be needed.

Since, as I was just recently informed, the c++ code this is based on is scheduled for complete replacement, I'm not submitting this as a formal patch for review. Hopefully, it might be useful to someone in the future. I had intended to write a updated detail "theory of operation" like this but since it's probably doubtful that this patch would ever be used in its current form, I'll let the code and comments (in the diff) be the documentation.

Comment 4

[gene smith]

Attachment: CS-fixes-add-QR-final-2.diff

I tested the code again with Zimbra server which I hadn't tried in a couple months and discovered a problem. After doing the initial CHANGEDSINCE fetch I was checking to make sure that any expunges were accounted for. The other servers I had tested (dovecot, cyrus and icloud) report the number of expunges by other clients with the untagged "* VANISHED (EARLIER) <UID-list>" response within SELECT and reported the actual number that were expunged (vanished) while tb was shutdown. But, due to this zimbra issue Zimbra Bug 109436 the count of the number of vanished (expunged) messages reported by zimbra can be much larger than the correct value. The incorrect value was causing me to do unnecessary full flag fetches after the initial select.
This made me realize that with QRESYNC in effect (as it is for zimbra) that there is no need for the check at all after doing the CHANGEDSINCE fetch. That check is only needed when only CONDSTORE is in effect as with gmail and vanished is never reported.

With this fix and with the zimbra bug still present it is working OK again. Currently the zimbra issue just causes an excessively long list of supposedly expunged UIDs to be reported. However, this list is mostly just "gaps" in the UIDs so they are ignored and cause no problem (other than unnecessary processing). Any truly expunged (vanished) UIDs are also present in and detected in the list and are correctly removed from the database.

Note: The "gaps" in zimbra's UID numbering is mostly because zimbra seems to use a single counter per account to assigned the ascending UID number to messages. Other servers seem to use a per folder (mailbox) counter so within a folder the UIDs are always consecutive. However, TB sending the "From " header line also causes gaps in zimbra UID numbers as described here: Bug 1741748.

Comment 5

[gene smith]

Attachment: CS-fixes-add-QR-with-Advanced-UI.diff

I merged my previous work, including the "Advanced" server setting, with daily 102.0a1 as updated on 5-28-2022. Only a few minor conflicts had to be resolved. I've been running with these changes all along with no problems with daily builds.

Comment 7

[gene smith]

Attachment: Bug 1747311 - Add support for QRESYNC and improve CONDSTORE (RFC 7162) r=mkmelin

Includes "advanced" imap server setting to enable condstore/qresync which is
disabled by default.
Qresync, if supported by the server, always supercedes condstore. Currently the only
server type supporting only condstore is gmail. Others, such as dovecot, cyrus (used
by fastmail), icloud and zimbra support condstore and qresync.
This is a rebase, clean up and retest of work originally done about 1.5 years ago.

Comment 8

[gene smith]

As requested by Magnus in phab, I started researching how to add unit tests for this change. I found a file https://searchfox.org/comm-central/rev/6f657d1fc22e66b7106e23e402884b44ca550a99/mailnews/imap/test/TestImapFlagAndUidState.cpp#1 that appears to do some testing for CONDSTORE that was added a while back here: https://bugzilla.mozilla.org/show_bug.cgi?id=537551#c4 or maybe here: https://bugzilla.mozilla.org/show_bug.cgi?id=540554#c12. However, from what I can tell, this file is not used and does not run when unit tests occur. It was verified to be "removed" here: https://bugzilla.mozilla.org/show_bug.cgi?id=632429#c9

Anyhow, not sure how useful this c++ based unit test file would be, but I'd never seen it before and thought maybe it could be a possible starting point. At one time that file was listed in a now removed Makefile.in and got built into an executable, but it appears to now be unused but still present (along with another c++ test file TestImapHdrXferInfo.cpp), unlike the Makefile.in which is now gone.