Closed
Bug 527679
Opened 15 years ago
Closed 15 years ago
gloda indexing does not properly handle undone message deletions
Categories
(MailNews Core :: Database, defect, P1)
MailNews Core
Database
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0rc1
People
(Reporter: asuth, Assigned: asuth)
Details
(Whiteboard: [no l10n impact][bienvenu, asuth working on patches])
Attachments
(4 files, 4 obsolete files)
35.95 KB,
patch
|
asuth
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
33.58 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
2.96 KB,
patch
|
standard8
:
review+
standard8
:
superreview+
|
Details | Diff | Splinter Review |
2.70 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
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.
Flags: blocking-thunderbird3?
Updated•15 years ago
|
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Priority: -- → P1
Updated•15 years ago
|
Target Milestone: --- → Thunderbird 3.0rc1
Updated•15 years ago
|
Assignee: nobody → bugmail
Assignee | ||
Comment 1•15 years ago
|
||
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...
Updated•15 years ago
|
Whiteboard: [no l10n impact]
Comment 2•15 years ago
|
||
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.
Updated•15 years ago
|
Whiteboard: [no l10n impact] → [no l10n impact][bienvenu, asuth working on patches]
Assignee | ||
Comment 3•15 years ago
|
||
Attachment #411728 -
Flags: review?(bienvenu)
Comment 4•15 years ago
|
||
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.
Updated•15 years ago
|
Attachment #411770 -
Flags: review?(bugmail)
Updated•15 years ago
|
Attachment #411728 -
Flags: review?(bienvenu) → review+
Comment 5•15 years ago
|
||
this is a bit ugly, and definitely needs some comments. The new test fails, but I am sending the notification.
Attachment #411770 -
Attachment is obsolete: true
Attachment #411770 -
Flags: review?(bugmail)
Assignee | ||
Updated•15 years ago
|
Attachment #411813 -
Flags: review?(bugmail)
Comment 6•15 years ago
|
||
Attachment #411813 -
Attachment is obsolete: true
Attachment #411813 -
Flags: review?(bugmail)
Assignee | ||
Comment 7•15 years ago
|
||
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.
Attachment #411728 -
Attachment is obsolete: true
Attachment #411833 -
Attachment is obsolete: true
Comment 8•15 years ago
|
||
Comment on attachment 411869 [details] [diff] [review]
updated bienvenu's patch, fuses test in. tests pass for me
tests pass for me too!
Attachment #411869 -
Flags: superreview+
Attachment #411869 -
Flags: review?
Assignee | ||
Comment 9•15 years ago
|
||
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.
Attachment #411919 -
Flags: review?(bugzilla)
Assignee | ||
Updated•15 years ago
|
Attachment #411869 -
Flags: review? → review+
Assignee | ||
Comment 10•15 years ago
|
||
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.
Assignee | ||
Comment 11•15 years ago
|
||
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.
Attachment #411927 -
Flags: superreview?(bugzilla)
Attachment #411927 -
Flags: review?(bugzilla)
Assignee | ||
Comment 12•15 years ago
|
||
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.
Attachment #411931 -
Flags: review?(bugzilla)
Updated•15 years ago
|
Attachment #411919 -
Flags: review?(bugzilla) → review+
Updated•15 years ago
|
Attachment #411927 -
Flags: superreview?(bugzilla)
Attachment #411927 -
Flags: superreview+
Attachment #411927 -
Flags: review?(bugzilla)
Attachment #411927 -
Flags: review+
Comment 13•15 years ago
|
||
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 :-)
Attachment #411931 -
Flags: review?(bugzilla) → review+
Assignee | ||
Comment 14•15 years ago
|
||
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.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Updated•15 years ago
|
Keywords: fixed-seamonkey2.0.1
Updated•15 years ago
|
Keywords: fixed-seamonkey2.0.1
Assignee | ||
Updated•12 years ago
|
Flags: in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•