On IMAP, deleting attachment corrupts message by adding following messages, when using "mark as deleted" delete model
Categories
(MailNews Core :: Backend, defect, P1)
Tracking
(thunderbird_esr115+ fixed)
People
(Reporter: otsy, Assigned: gds)
References
Details
(Keywords: dataloss, Whiteboard: [TM: 115.2.1])
Attachments
(3 files, 3 obsolete files)
12.20 KB,
message/rfc822
|
Details | |
16.28 KB,
message/rfc822
|
Details | |
48 bytes,
text/x-phabricator-request
|
wsmwk
:
approval-comm-esr115+
|
Details | Review |
Comment 4•10 years ago
|
||
Comment 5•10 years ago
|
||
Reporter | ||
Comment 10•10 years ago
|
||
Comment 11•10 years ago
|
||
Comment 12•10 years ago
|
||
Updated•10 years ago
|
Comment 13•10 years ago
|
||
Reporter | ||
Comment 14•10 years ago
|
||
Comment 15•10 years ago
|
||
Comment 16•10 years ago
|
||
Comment 17•10 years ago
|
||
Comment 18•10 years ago
|
||
Updated•9 years ago
|
Comment 19•8 years ago
|
||
Updated•8 years ago
|
Comment 20•7 years ago
|
||
Updated•2 years ago
|
Assignee | ||
Comment 22•2 years ago
|
||
I can duplicate something similar with an mbox backed folder. It wasn't clear to me how to duplicate as described so here's what I did:
- Choose a fairly small folder and make sure it has mbox offline store and sort by "order received".
- Repair or Compact the folder to make sure it is initially OK.
- Copy into that folder a message with at least 2 attachments (msg A)
- Copy into that folder a fairly short message (msg B)
- Configure delete mode to "just mark as deleted" and restart tb for good measure
- Open msg A and delete (not detach) an attachment
- Msg A gets crossed out and modified message Msg A- is added to the message list at highest "order received" number
- Look at Msg A- and its source. It should be OK with the attachment removed.
- Look at mbox file for the folder with Msg A- last in the file. All looks OK at this point:
-- any leading messages in folder
-- Msg A
-- Msg B
-- Msg A- - Now undelete Msg A so the cross-out is removed (undelete may not be necessary, I always did undelete)
- In Msg A delete a different attachment
- Again Msg A gets crossed out and modified Msg A-- is added to the message list at highest "order received"
- Look at mbox file again and you will now see this:
-- any leading messages in folder
-- Msg A
-- Msg B
-- Msg A-
-- Msg A--
-- Msg B <-- wrong
-- Msg A- <-- wrong
Now if you forward Msg A-- as an attachment, the message will look correct at the destination but the message source will also contain Msg B and Msg A- content appended at the bottom.
Anyhow, this is what I see now when I tried this. It seems a bit different than described in comment 0 in that I have to do the attachment delete at least twice to trigger the bug.
Yes, if you delete even more attachments at Msg A you end up with even more baggage appended.
I haven't tried this with maildir so don't know if it has the problem too.
Assignee | ||
Comment 23•2 years ago
|
||
I was not able to consistently duplicate the problem with the step2 shown in comment 22. However, the comment 0 STR actually demonstrates the problem, restated like so:
- Choose a folder and make sure it has mbox offline store and sort by "order received".
- Repair and Compact the folder to make sure it is initially OK.
- Copy into that folder a message with attachments (msg A)
- Copy into that folder a couple fairly short messages (msg B1 msg B2)
- Look at folder's mbox file and make sure the order in the file is msgs A, B1, B2
- Configure delete mode to "just mark as deleted"
- Open msg A and delete an attachment
- Msg A gets crossed out and modified message Msg A- is added to the message list at highest "order received" number after B2
- Look at message source for Msg A, it will have msg B1 and B2 tacked on at the bottom.
- If you copy or move Msg A to another folder, the extra baggage (B2 and B2) will come along when you look at the destination source.
- Msg A always displays correctly even with the extra baggage.
The patch provided in comment 13 by Kent James does fix the problem. As he describes it, the most important part is to remove these lines:
https://searchfox.org/comm-central/rev/f8c1a9e5db5ea9efeb0ef6c1a996c25d039ae3d3/mailnews/imap/src/nsImapMailFolder.cpp#7487-7489
Setting the offline size to 0 when the messages is still present in offline store, as it would be with "just mark as deleted", causes usually the mbox file to be read from the start of Msg A to the end of file when only Msg A is wanted, so you get the extra "baggage".
As Kent James describes it, the other changes in the patch are error checking (some "overkill") that would have maybe caused the problem to be noticed without producing a possible corrupted message containing a lot of unintended content.
I've tested the changes with "delete to trash" and not setting the offline size to 0 cause no problems.
Also, my tests indicate this doesn't affect maildir, just mbox (although maildir seems to have other unrelated problems when "just mark deleted" is used).
The bug doesn't happen and the fix doesn't do anything bad when offline store is not used for the folder.
From comment 17:
If I apply this patch, I get a failure in mailnews/local/test/unit/test_streamHeaders.js (some method is throwing NS_ERROR_FAILURE)
I'm not seeing this failure.
Assignee | ||
Comment 24•2 years ago
|
||
This patch is the work of Kent James from 9 year ago. I rebased it and tested it extensively.
Updated•2 years ago
|
Assignee | ||
Comment 25•2 years ago
|
||
Wanted to run a try build on this to make sure it produces no test failure. However, I've been unsuccessful in getting a build to work, e.g., https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=6291ebdad6708bb0149a66c7e4a88d3072da45a4&selectedTaskRun=DEMx2OaDRpiVmz1nTmKjxw.0
So will go ahead and request checkin.
Comment 26•2 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/e55600ee3b94
With imap delete model, deleting attachment corrupts message by adding following messages. r=mkmelin
Assignee | ||
Comment 27•2 years ago
|
||
This fails when I try to "detach" an attachment. The attachment gets saved but it remains present in the message. Not sure it is working exactly right when deleting attachments either. Sometimes the attachment deleted completely disappears from mbox rather than leaving the "altered" and placeholder mime part with the message.
It also fails mailnews/imap/test/unit/test_imapAttachmentSaves.js when "AttachmentDetached" never gets written to mbox at line 149.
Re: comment 25 -- I should have manually run at least the imap unit tests.
So this needs to be backed out.
Assignee | ||
Comment 28•2 years ago
|
||
This fixes the problems described in the previous comment. I'll do some more checking tomorrow before submitted the changes formally.
Comment 29•2 years ago
|
||
Ok, we'll back it out.
Comment 30•2 years ago
|
||
Updated•2 years ago
|
Assignee | ||
Comment 31•2 years ago
|
||
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Updated•2 years ago
|
Comment 32•2 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/b1f3d40c9cef
With imap delete model, deleting attachment corrupts message by adding following messages. r=BenC
Comment 33•2 years ago
|
||
Lets land it and create a follow-up bug to think about how to tidy up the way attachments are removed from messages.
Is the follow-up bug filed?
Updated•2 years ago
|
Updated•2 years ago
|
Assignee | ||
Comment 34•2 years ago
|
||
(In reply to Wayne Mery (:wsmwk) from comment #33)
Lets land it and create a follow-up bug to think about how to tidy up the way attachments are removed from messages.
Is the follow-up bug filed?
That was Ben's recommendation here: https://phabricator.services.mozilla.com/D185617
I'm don't know what he has in mind for this new bug and he doesn't appear to be on the CC list for this bug at the BMO level so I'll CC/NI him.
Comment 35•2 years ago
|
||
Comment on attachment 9347780 [details]
Bug 1135434 - With imap delete model, deleting attachment corrupts message by adding following messages. r=benc,mkmelin
[Triage Comment]
Approved for esr115
Comment 36•2 years ago
|
||
bugherder uplift |
Thunderbird 115.2.3:
https://hg.mozilla.org/releases/comm-esr115/rev/3a1f9fba9487
Comment 37•1 years ago
|
||
(In reply to gene smith from comment #34)
That was Ben's recommendation here: https://phabricator.services.mozilla.com/D185617
I'm don't know what he has in mind for this new bug and he doesn't appear to be on the CC list for this bug at the BMO level so I'll CC/NI him.
I don't know I had anything specific in mind (other a general feeling of horror at the sheer obfuscated complexity in there).
So I've rolled it into Bug 1788159, which deals with general tidying-up of all the attachment-saving and detaching code.
Description
•