1782374 - IMAP folder Compact/Expunge doesn't correctly manage listener notification - ensure expunge completes before compact begins

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Ben Campbell]

The IMAP folder compaction functions are a little odd in that they attempt to perform an IMAP Expunge as well as a compact.

The compact() and compactAll() functions can be passed an nsIUrlListener to signal when they complete. There is an attempt in the code to make sure this is only called after both the expunge and compaction are completed, but this is incomplete.
There's a lot of opportunity this to get out of sync, and for listeners to be called more than once.

For compaction, the listener is passed directly to the foldercompaction code, which calls it when done (without any consideration of an ongoing expunge).
For the expunge, the listener is stashed on the folder m_UrlListener member, and the folder itself is passed as a listener to the Expunge operation. The folders own OnStopRunningUrl() is then called, the url checked, and if it was an expunge operation, there's a hack in there to call m_UrlListener, but only if there's no ongoing compaction still running. Basically, it's a bit of a mess - even more so for the multi-folder CompactAll().

I think there are three alternative approaches that could be taken:

1. route both the compact operation and the expunge through the folder's own OnStopRunningUrl(), and decide there when to call the listener passed in to Compact() (i.e. once both compaction and expunge are complete). This is tricky, because the compaction is not an IMAP protocol operation, and there's no URL associated with it, so the folders OnStopRunningUrl() can't easily identify the compaction completion.
2. Use an separate nsIUrlListener which synchronises it all (probably using C++ closures to keep the completion logic near to the site of the initiation - a C++ equivalent of PromiseTestUtils.PromiseUrlListener).
3. punt on this and turn Compact and Expunge into separate operations (see Bug 1130277). Probably letting the GUI code handle waiting for them both to finish (using PromiseTestUtils.PromiseUrlListener objects).

I lean toward 3, with a dash of 2 if this actually shows up as a problem in the meantime.

Comment 1

[Ben Campbell]

Possible manifestation as Bug 1766101?

Comment 2

[Ben Campbell]

Attachment: Bug 1782374 - Add UrlListener to serialise IMAP folder expunge and compact operations. r=mkmelin

On IMAP folders, the user-facing concept of "compact" also implies an expunge.
This patch ensures that the expunge operation completes before the compact is started.
This means that expunged messages will be compacted immediately rather than being missed and left for the next compaction.
It also makes sure that any provided listener is properly informed when operation as a whole is finished (or has failed).

Comment 3

[Ben Campbell]

try run here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4e5de27adf749f7ecab8cc7be33da7e28ea75902

Comment 4

[Ben Campbell]

Left open for now - I'm still looking at what to do with the CompactAll() methods.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/392004364d88
Add UrlListener to serialise IMAP folder expunge and compact operations. r=mkmelin

Comment 6

[Ben Campbell]

Attachment: Bug 1782374 - Ensure correct listener notification in nsImapMailFolder::CompactAll(). r=mkmelin

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/02130e0b2a4f
Ensure correct listener notification in nsImapMailFolder::CompactAll(). r=mkmelin

Comment 8

[Wayne Mery (:wsmwk)]

from bug 1766101 - that this would have had no impact on Bug 1766101 - Startup Crash in [@ std::_Func_impl_no_alloc<T>::_Do_call | nsFolderCompactState::OnStopRunningUrl] at least not on version 102.