Closed Bug 1633205 Opened 4 months ago Closed 4 months ago

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

Categories

(MailNews Core :: Networking: IMAP, defect)

defect

Tracking

(thunderbird_esr68 fixed)

RESOLVED FIXED
Thunderbird 77.0
Tracking Status
thunderbird_esr68 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

Details

Attachments

(3 files, 6 obsolete files)

Attached patch demo.patchSplinter 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.

Attached patch debug.patch (obsolete) — Splinter Review
Attached patch debug.patch (obsolete) — Splinter Review
Attachment #9143400 - Attachment is obsolete: true
Attached patch 1633205-HasRe.patch (obsolete) — Splinter Review

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: nobody → jorgk-bmo
Status: NEW → ASSIGNED
Attachment #9143403 - Flags: review?(benc)

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

Attachment #9143403 - Attachment is obsolete: true
Attachment #9143403 - Flags: review?(benc)
Attachment #9143405 - Flags: review?(benc)
Attachment #9143405 - Attachment is patch: true

Tweaked comment.

Attachment #9143405 - Attachment is obsolete: true
Attachment #9143405 - Flags: review?(benc)
Attachment #9143406 - Flags: review?(benc)
Attached patch debug.patchSplinter Review
Attachment #9143402 - Attachment is obsolete: true
Attachment #9143406 - Attachment is patch: true

Better commit message. Sorry 'bout the noise.

Attachment #9143406 - Attachment is obsolete: true
Attachment #9143406 - Flags: review?(benc)
Attachment #9143408 - Flags: review?(benc)

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.

Attachment #9143408 - Attachment is patch: true

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.

Attachment #9143408 - Attachment is obsolete: true
Attachment #9143408 - Flags: review?(benc)
Attachment #9143438 - Flags: review?(benc)
Blocks: 1633251

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?

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 on attachment 9143438 [details] [diff] [review]
1633205-HasRe.patch (v2c, complete) - for ESR68

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

LGTM
Attachment #9143438 - Flags: review?(benc) → review+

Needs a rebase.

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.
Attachment #9143438 - Attachment description: 1633205-HasRe.patch (v2c, complete) → 1633205-HasRe.patch (v2c, complete) - for ESR68
Attachment #9143438 - Flags: approval-comm-esr68?

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

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 77.0
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.
Attachment #9143438 - Flags: approval-comm-beta?
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.
Attachment #9143438 - Flags: approval-comm-beta? → approval-comm-beta-
Comment on attachment 9143438 [details] [diff] [review]
1633205-HasRe.patch (v2c, complete) - for ESR68

[Triage Comment]
Approved for ESR
Attachment #9143438 - Flags: approval-comm-esr68? → approval-comm-esr68+
Attachment #9143438 - Flags: approval-comm-beta-
You need to log in before you can comment on or make changes to this bug.