527679 - gloda indexing does not properly handle undone message deletions

Status: RESOLVED FIXED

(MailNews Core :: Database, defect, P1)

Description

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

I delete a message from an IMAP folder using the default move-to-trash deletion model.  Gloda does some stuff.  I undo it.  Gloda reports msgsDeleted and msgsClassified notifications, but apparently ignores the message.  Looking into it.

Comment 1

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

The current state of things is that I don't understand what kind of events are getting generated or should be getting generated.  I am no longer seeing a msgsClassified event which could very likely be the result of my changes, but if my changes made it go away I rather suspect it was not a useful notification to me in the first place.

I am definitely not seeing an OnItemPropertyFlagChanged for IMAPDeleted.  I thought that might be happening and my code was filtering it out (because they normally are just noise), but it was not the case.

I rather suspect undo represents a hole in nsIMsgFolderListener's coverage; there are no unit tests in test_nsIMsgFolderListenerIMAP.js and I have no reason to expect it should work.  This means Vista/OS X indexing probably screws up too.

I think the likely proper solution is to make the undo of a non-imap-deletion model move-to-trash deletion manifest as a msgsMoveCopyCompleted in all cases.  It sounds like that might theoretically be the case in the cases where the deleted flag can't just be subtracted off, but it's unclear to me without further investigation whether that operation happens at a high enough level to result in said notification.

bienvenu, in the increasingly likely chance you are around before me tomorrow and assuming you don't have other blockers on your plate, I think it would be ideal if you could pursue making sure said event gets generated and updating test_nsIMsgFolderListenerIMAP.js to verify this.  I think that test is of a similar architecture to the existing IMAP undo test you already implemented so it should hopefully fit in well.

Ugh, and I just tried to undo deletion in a local folder and that also fell down.  So, uh, maybe the local listener test could use a boost too...

Comment 2

[David :Bienvenu]

I agree that sending a msgsMoveCopyCompleted notification after undo (and redo) sounds like the right thing to do. I'll have a whack at it.

Comment 3

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

Attachment: add test that adds an already \Seen message in IMAP online which hangs (and will eventually timeout fail) v1

Comment 4

[David :Bienvenu]

Attachment: this notifies when read imap headers are downloaded

I'll try the new test case with this patch.

Sadly, for the imap undo case, we don't actually download the header until the user reselects the folder, if the server supports COPYUID. Which may make it hard to write a unit test, though in TB, gloda will eventually find out about the new header.

Comment 5

[David :Bienvenu]

Attachment: v2 - handle the early return case

this is a bit ugly, and definitely needs some comments. The new test fails, but I am sending the notification.

Comment 6

[David :Bienvenu]

Attachment: latest version for asuth to tweak

Comment 7

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

Attachment: updated bienvenu's patch, fuses test in. tests pass for me

I moved the async_trash_message and async_delete_message test helper logic from asyncTestUtils.js to messageInjection.js so that I could make async_trash_message work like async_move_messages in terms of forcing updateFolders to occur and generate the event that helps glodaTestHelper wait for events it should really wait for.

Comment 8

[David :Bienvenu]

Comment on attachment 411869 [details] [diff] [review]
updated bienvenu's patch, fuses test in.  tests pass for me

tests pass for me too!

Comment 9

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

Attachment: cleanup a few deletion issues v1

This patch looks larger than it really is.  Everything in here is well unit tested.

Changes:

- getMessagesByMessageID and helper class moved from gloda.js to index_msg.js.  This was previously moved by because of dependency issues but because of an oversight got moved to a public location instead of a private location.  Because the function performs privileged queries the function absolutely should not be public.  Generally ignore this in the patch; it's just a move and it's an exceedingly well unit tested function.

- Added some documentation and better function names to deletion-related functions for GlodaMessage in datamodel.js

- Along with the previous, I fixed a typo bug related to the previous that was resulting in the 'deleted' SQL column for messages getting set to an internal SpiderMonkey value rather than the actual correct value.  I added a soft schema change that runs an UPDATE at upgrade to fix the ramifications of this.

- Improved/clarified the logic with the deletetion-related helper functions in the core message indexing logic.

- Same deal for the actual deletion logic.  Added a test check for the minor change in logic.

Comment 10

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

The combination of the two preceding patches make undo work properly for IMAP.

If your IMAP server is only so-so, then it may take some time for gloda to actually hear that the message is back.  Per my understanding, we only get the notification once the IMAP server tells us about the updates.  It may take someone forcing an update of the folder for this to happen.

But it will happen, eventually.  You can make it happen sooner if you hit get new messages in the inbox or click away and come back for other folders, etc.

Comment 11

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

Attachment: generate move notification for local-to-local undo v1

This makes local undo work when the move is from local to local.  A move from IMAP to local also undoes correctly just because of how IMAP undo gets handled by us.

A move from local to IMAP does not undo correctly and I have no immediate concept of how to resolve that.  While this is a bug, this blocker is about undoing deletions being problematic.  People moving who regularly move messages from local folders to IMAP folders and then undo them are far enough out of the norm that this seems acceptable.

Comment 12

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

Attachment: mark source headers as dirty and the folder dirty when experiencing the local-to-imap move case that does the wrong thing v1

Figured out how to workaround the bad situation.  This should give us full coverage of undo cases(with the other patches).  But to really emphasize, this is eventual correctness; we depend on an indexing sweep to happen in this specific case and for folders to be selected in the IMAP case.  But the difference versus without these patches is that without them gloda would never index the messages again unless the user did something to dirty them.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 411931 [details] [diff] [review]
mark source headers as dirty and the folder dirty when experiencing the local-to-imap move case that does the wrong thing v1

This seems better than nothing :-)

Comment 14

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

pushed to trunk:
http://hg.mozilla.org/comm-central/rev/6165f866e8c0
http://hg.mozilla.org/comm-central/rev/ba58af600269
http://hg.mozilla.org/comm-central/rev/97bdd32faaca
http://hg.mozilla.org/comm-central/rev/d60c6eb9a7b3

pushed to comm-1.9.1:
http://hg.mozilla.org/releases/comm-1.9.1/rev/afd6f399628c
http://hg.mozilla.org/releases/comm-1.9.1/rev/0a15d6468a7f
http://hg.mozilla.org/releases/comm-1.9.1/rev/ac68b14a0874
http://hg.mozilla.org/releases/comm-1.9.1/rev/3f1a55a4a6ca

Although the gloda deletion-related test cases have been improved, we do not have test cases for the undo cases, and we would benefit from having them.  Setting the flag to be questionny.