Copying a message to or from an IMAP folder copies the subject but loses the HasRe flag
Categories
(MailNews Core :: Networking: IMAP, defect)
Tracking
(thunderbird_esr68 fixed)
Tracking | Status | |
---|---|---|
thunderbird_esr68 | --- | fixed |
People
(Reporter: jorgk-bmo, Assigned: jorgk-bmo)
References
Details
Attachments
(3 files, 6 obsolete files)
1.25 KB,
patch
|
Details | Diff | Splinter Review | |
10.31 KB,
patch
|
Details | Diff | Splinter Review | |
4.56 KB,
patch
|
benc
:
review+
wsmwk
:
approval-comm-esr68+
|
Details | Diff | Splinter Review |
The current OpenPGP implementation in Thunderbird needs to restore a subject from a decrypted message. For treating subjects with Re: in them, it uses this hack:
https://searchfox.org/comm-central/rev/d9cc983e281ba4bfbb34722bb1f080124caf6263/mail/extensions/openpgp/content/ui/enigmailMsgHdrViewOverlay.js#1261
which then makes another hack necessary:
https://searchfox.org/comm-central/rev/51eaff8b48e029097e38475155cb43390a32405b/mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js#3936
Needless to say, that threading won't work that way.
The correct processing would be something like shown in the attached demo patch, that is stripping the Re. and instead setting the HasRe flag. You can find more details about the issue in bug 1629555 comment #13.
If you use the attached demo patch (it hacks into "Open message in a new tab") you can prepare a message in a local folder that originally didn't have a Re. to now have one. If you copy that message into an IMAP folder, the subject is carried over, the HasRe flag is lost.
The action happens in nsParseMailMessageState::FinalizeHeaders(). There in InternSubject(), NS_MsgStripRE() determines whether the header should have the HasRe flag set. This is done on the "raw" Subject: header of the message, but the subject is clobbered by calling m_mailDB->UpdatePendingAttributes()
. You can see it in the debug between [2] and [3].
The issue is that nsImapMailFolder::SetPendingAttributes() is lacking proper flag processing which I'll add in the next patch.
Assignee | ||
Comment 1•10 months ago
|
||
Assignee | ||
Comment 2•10 months ago
|
||
Assignee | ||
Comment 3•10 months ago
|
||
The patch is smaller than the entire introduction. I spent hours pinning down the spot. And it's only half the story since this only fixes the local to IMAP and IMAP to IMAP copy, the IMAP to local is still broken.
Another comment: I needed to interfere with the if (messageSize) {
code below, since that's typically not executed, so in the cases where it is executed, we need to set the flag again, but taking the previous result into account.
Assignee | ||
Comment 4•10 months ago
|
||
OK, the IMAP to local direction was a lot easier to find.
Assignee | ||
Updated•10 months ago
|
Assignee | ||
Comment 5•10 months ago
|
||
Tweaked comment.
Assignee | ||
Comment 6•10 months ago
|
||
Assignee | ||
Updated•10 months ago
|
Assignee | ||
Comment 7•10 months ago
|
||
Better commit message. Sorry 'bout the noise.
Assignee | ||
Comment 8•10 months ago
•
|
||
Hmm, the tree doesn't look so good, no idea whether this will show anything interesting:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=06c569eb46cc58ef83d23d1e6d3d51ebb43ea994
EDIT: That doesn't seem to have caused additional failures.
Assignee | ||
Updated•10 months ago
|
Assignee | ||
Comment 9•10 months ago
|
||
I'm really sorry about the noise. I looked at it again and saw that the variable name was no good now. So I changed it and also tweaked the comment some more.
Comment 10•10 months ago
|
||
I'm thinking this isn't the first time we've hit this general category of issue where the flags are not copied. Pretty sure I have experienced it.
Do we have tests for this?
Assignee | ||
Comment 11•10 months ago
|
||
Correct. I've heard that copying "ancillary" message information is quite bad, for example stars, tags, etc. since there are various storage locations, some stored in the message db (Mork, MSF), some in the raw message data, like the X-Mozilla-Status. The comments in the code are also not very nice, like:
https://searchfox.org/comm-central/rev/b00df3b6a9ff43fca2e3aa750a06109e6c32570e/mailnews/local/src/nsParseMailbox.cpp#1125
https://searchfox.org/comm-central/rev/b00df3b6a9ff43fca2e3aa750a06109e6c32570e/mailnews/local/src/nsLocalMailFolder.cpp#2250
Here I'm addressing the fact that the message subject is copied from the source message to the copy, but the HasRe flag is not. That is certainly wrong and shows in cases where the subject in the database doesn't match the subject in the raw message data since a PGP solution replaced it for viewing purposes.
I'm not aware of any tests for the Re: processing, and I also don't know of any tests for the other data I mentioned, but I believe there would be some for tags.
Comment 12•10 months ago
|
||
Comment on attachment 9143438 [details] [diff] [review] 1633205-HasRe.patch (v2c, complete) - for ESR68 Review of attachment 9143438 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Assignee | ||
Updated•10 months ago
|
Assignee | ||
Comment 14•10 months ago
|
||
Comment on attachment 9143438 [details] [diff] [review] 1633205-HasRe.patch (v2c, complete) - for ESR68 [Approval Request Comment] Regression caused by (bug #): No regression, has always been wrong. User impact if declined: OpenPGP solutions need ugly workarounds and threading not working. Should be good after some time on TB 77 beta.
Comment 15•10 months ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fd16d22d04fa
Pay attention to HasRe flag in nsImapMailFolder::SetPendingAttributes() and nsMsgLocalMailFolder::EndCopy(). r=benc DONTBUILD
Assignee | ||
Updated•10 months ago
|
Assignee | ||
Comment 16•10 months ago
|
||
Comment on attachment 9143438 [details] [diff] [review] 1633205-HasRe.patch (v2c, complete) - for ESR68 If you're doing another TB 76 beta, it could go there, too.
Comment 17•10 months ago
|
||
Comment on attachment 9143438 [details] [diff] [review] 1633205-HasRe.patch (v2c, complete) - for ESR68 Thanks for the patch. This just missed 76.0b3, but 77 goes beta next week.
Comment 18•9 months ago
|
||
Comment on attachment 9143438 [details] [diff] [review] 1633205-HasRe.patch (v2c, complete) - for ESR68 [Triage Comment] Approved for ESR
Comment 19•9 months ago
|
||
bugherderuplift |
Thunderbird 68.9.0:
https://hg.mozilla.org/releases/comm-esr68/rev/e07c55e15192
Assignee | ||
Updated•9 months ago
|
Description
•