527687 - msgsClassified event may fire multiple times for a single message

Status: RESOLVED FIXED

(MailNews Core :: Database, defect, P2)

Description

[Andrew Sutherland [:asuth] (he/him)]

It sounds like the problem where msgsClassified tells us about messages we already know about is worse than hoped for.

If a user receives a new messages in a folder, a msgsClassified event is generated for _all_ the messages marked as new in that folder.  Because we limit the number of event-driven messages to be indexed to 20 and the folder indexing sweep logic is pretty good, in the pathological worst-case we will:
- redundantly index 20 messages that do not need to be indexed
- undertake a filtered enumeration of the messages in the folder (C++ enumeration of all headers)
- index the messages that actually are new and have not been indexed

This problem likely interacts with fast-pathing IMAP moves and the un-delete case.  Unfortunately, there may be no zero-cost solution since we may be unable to easily disambiguate amongst all the possible cases and so always have to perform a db lookup to check the message's location to see if we need to update it.  We might be able to mitigate by marking messages using processing flags during the msgAdded event assuming those get cleaned up if the message ends up getting moved by the spam detection.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Attachment: make msgsClassified only ever generate a single msgsClassified notification for a message v1

I should have just done this originally instead of bothering with the workaround.

This patch introduces a new processing flag that gets set on message headers just after we generate the msgAdded nsIMsgFolderListener notification.  We only generate a msgsClassified notification for messages with the processing flag and we clear it when we generate it.  This allowed me to remove the workaround from messageInjection.js which should improve our mozmill troubles.

I also tried to improve logic related to the processing flags.  Namely, I cause us to remove the processing flags for a header when it is removed from the database.  Additionally, I cause us to clear all processing flags when the folder gets compacted.  I opted not to clear the processing flags in the reparse case since one could make the case that in that situation either 1) there are no processing flags because this is a first-open-this-session case or 2) there should theoretically be no message key changes for a 'rebuild index' triggered by the user.

I think clearing the processing flags is probably important for the TraitsDone and FiltersDone which are arguably very close to being memory leaks and bad for correctness in the face of compaction.  (Although there is some mitigation in the IMAP case where UIDs have a high probability of playing right into the message key set's run-length encoding mechanism.  Local folders will not have consecutive message keys and will suck.)

Comment 2

[David :Bienvenu]

Comment on attachment 411518 [details] [diff] [review]
make msgsClassified only ever generate a single msgsClassified notification for a message v1

r/sr=me

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

pushed to trunk:
http://hg.mozilla.org/comm-central/rev/12ceaa786d23

pushed to trunk:
http://hg.mozilla.org/releases/comm-1.9.1/rev/f34b497649e0