1633205 - Copying a message to or from an IMAP folder copies the subject but loses the HasRe flag

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Jorg K (CEST = GMT+2)]

Attachment: demo.patch

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.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: debug.patch

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: debug.patch

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1633205-HasRe.patch

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.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1633205-HasRe.patch (v2, complete)

OK, the IMAP to local direction was a lot easier to find.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1633205-HasRe.patch (v2b, complete)

Tweaked comment.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: debug.patch

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1633205-HasRe.patch (v2b, complete)

Better commit message. Sorry 'bout the noise.

Comment 8

[Jorg K (CEST = GMT+2)]

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.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1633205-HasRe.patch (v2c, complete) - for ESR68

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

[Wayne Mery (:wsmwk)]

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?

Comment 11

[Jorg K (CEST = GMT+2)]

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

[Ben Campbell]

Comment on attachment 9143438 [details] [diff] [review]
1633205-HasRe.patch (v2c, complete) - for ESR68

Review of attachment 9143438 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 13

[Jorg K (CEST = GMT+2)]

Needs a rebase.

Comment 14

[Jorg K (CEST = GMT+2)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fd16d22d04fa
Pay attention to HasRe flag in nsImapMailFolder::SetPendingAttributes() and nsMsgLocalMailFolder::EndCopy(). r=benc DONTBUILD

Comment 16

[Jorg K (CEST = GMT+2)]

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

[Wayne Mery (:wsmwk)]

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

[Wayne Mery (:wsmwk)]

Comment on attachment 9143438 [details] [diff] [review]
1633205-HasRe.patch (v2c, complete) - for ESR68

[Triage Comment]
Approved for ESR

Comment 19

[Rob Lemley [:rjl]]

Thunderbird 68.9.0:
https://hg.mozilla.org/releases/comm-esr68/rev/e07c55e15192