Closed Bug 1782374 Opened 4 months ago Closed 4 months ago

IMAP folder Compact/Expunge doesn't correctly manage listener notification.

Categories

(MailNews Core :: Networking: IMAP, defect)

defect

Tracking

(thunderbird_esr102 wontfix)

RESOLVED FIXED
105 Branch
Tracking Status
thunderbird_esr102 --- wontfix

People

(Reporter: benc, Assigned: benc)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.

Component: General → Networking: IMAP
Product: Thunderbird → MailNews Core
See Also: → 1130277
See Also: → 1766101

Possible manifestation as Bug 1766101?

Assignee: nobody → benc

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).

Keywords: leave-open

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

Status: NEW → ASSIGNED
Target Milestone: --- → 105 Branch

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

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/02130e0b2a4f
Ensure correct listener notification in nsImapMailFolder::CompactAll(). r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Regressions: 1785003
Regressions: 1787650
Blocks: 1766101
See Also: 1766101
You need to log in before you can comment on or make changes to this bug.