Closed Bug 1629555 Opened 4 years ago Closed 4 years ago

Setting msgHdr.flags |= Ci.nsMsgMessageFlags.HasRe has no effect on repainting, only on rebuilding the thread pane (by switching folders)

Categories

(Thunderbird :: Folder and Message Lists, defect)

Product:
Thunderbird
Component:
Folder and Message Lists
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

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

Details

Attachments

(1 file, 2 obsolete files)

demo.patch
1.25 KB, patch
Details | Diff | Splinter Review

Encryption add-ons typically hide the message subject and need to set it again after decryption of a message.

For a message with a "Re: " prefix, the un-prefixed subject should be stored, together with
msgHdr.flags |= Ci.nsMsgMessageFlags.HasRe.
Sadly that has no effect when calling .invalidate() in the tree. It looks like the HasRe flag is only taken into account when completely rebuilding the tree by switching to another folder and returning.

Enigmail currently does something like
https://searchfox.org/comm-central/search?q=(Re%3A+)%2B&case=false&regexp=false&path=
and expecially
https://searchfox.org/comm-central/rev/51eaff8b48e029097e38475155cb43390a32405b/mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js#3936
is a pure workaround for the fact that the subject was incorrectly stored with the "Re: " prefix.

Further in Enigmail, the "Re: " processing is of course defective since it should take mailnews.localizedRe into account.

Kai, this bug will impact you.

Flags: needinfo?(kaie)

Here's my workaround for this bug:
https://pep-security.lu/dev/repos/pEp_for_Thunderbird/file/136420ae4d47/chrome/content/TbMessageView.js#l19

Magnus, could this be fixed for the sake of OpenPGP in Thunderbird? Once the HasRe flag is set, the next repaint must take this into account. Otherwise you'll have to maintain the incorrect and incomplete Enigmail workarounds forever.

Flags: needinfo?(mkmelin+mozilla)

I guess the fix would be:
https://searchfox.org/comm-central/rev/cf3e85f8f805f6ac3887f93d2737937038ba9c7a/mailnews/base/src/nsMsgDBView.cpp#541

nsresult nsMsgDBView::FetchSubject(nsIMsgDBHdr *aMsgHdr, uint32_t aFlags,
                                   nsAString &aValue) {
  uint32_t flags;
  // Check whether someone has set the HasRe flag in the meantime.
  aMsgHdr->GetFlags(&flags);
  if ((aFlags | flags) & nsMsgMessageFlags::HasRe) {
Attached patch demo.patch (obsolete) — — Splinter Review

Patch to demo the issue. Select a message, then 'Open in new tab'. You won't see the Re. prefix, but you will see it after switching folders.

Attached patch 1629555-hasRe.patch (obsolete) — — Splinter Review

Alta88, you know this code best, what do you think of this?

This fix would allow to remove various incorrect workarounds in add-ons and make the system work the way it was intended, IMHO.

Assignee: nobody → jorgk-bmo
Flags: needinfo?(mkmelin+mozilla)
Attachment #9140378 - Flags: review?(alta88)

Alta88, there's another option to not only pay attention to the flag if it has been switched on, but also if it has been switched off:
So just: if (flags & nsMsgMessageFlags::HasRe) {.
Or maybe the caller should refresh its m_flags[]. Open to suggestions ;-)

Comment on attachment 9140378 [details] [diff] [review]
1629555-hasRe.patch

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

No, this isn't the way to do it. I have a feeling you spent no more than an hour and a half on it ;)
But first, what is your expected process? The extension cleans a subject of Re:, stores both the cleaned subject and sets the HasRe flag in the db? Or is this intended to only affect the view/display of the subject by setting a cleaned value in msgHdr cached object and not affect the raw received subject value in the db?
If the first, then you realize that losing the HasRe flag in a .msf loss will forever lose that Re data. That would affect threading for those that use Re to thread.
If the second, then you might be toggling HasRe in the db to a state that isn't accurate for the underlying db subject, just for transient display purposes. And what happens when .msf is rebuilt and and HasRe is set per db subject?
Attachment #9140378 - Flags: review?(alta88) → review-

Exactly, I spent more time compiling than finding the spot and writing this hack ;-)

Basically we're talking about encryption extensions here, or even the future OpenPGP implementation in TB. What happens is that the message was transmitted with an obfuscated subject and now we want to restore that subject, and for things to work correctly, we also want to set the HasRe flag. Preferably we don't want to store "Re: original subject" without the HasRe flag, since that causes additional chagrin when you answer, you get double Re: which you need to clean out with another hack, as the Enigmail code does here:
https://searchfox.org/comm-central/rev/51eaff8b48e029097e38475155cb43390a32405b/mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js#3936

My observed issue is that setting HasRe doesn't fix the display immediately, but it does fix it if you leave the folder and come back. That does sound like a bug, don't you agree? Hence the tweak in the view code where the subject display is retrieved, that runs when the tree is refreshed without leaving the folder.

My current hack, which has evolved, is this:
https://pep-security.lu/dev/repos/pEp_for_Thunderbird/file/52a656aaef70/chrome/content/TbMessageView.js#l36

To answer your questions, I'd first have to understand them. What are you referring to when you say "db". To me, the message database is the Mork database in the MSF file, not the raw data stored in an mbox or maildir file, or in case of IMAP, maybe not stored at all.

In my understanding, msgHdr.subject = subject; would store the new subject in the message db/Mork/MSF. And that's what we want. In case the MSF is lost, the message will need to be decrypted again, and the decryption process will do the same as it did the first time around.

I hope that answers the question(s).

That's makes it clearer, yes. That the original subject is not changed in the raw file. But you do know that X-Mozilla-Status, in the raw file for local messages, does contain flag information as well as it being in the db (.msf/mork/msgDb) a cached to disk type system, sometimes backed by panacea.dat.

There isn't an api for HasRe, meaning one that broadcasts on header flags changed type notification. To do it properly, experiment with nsMsgDatabase::MarkHdrMarked() or such.

It turns out that although there isn't a direct HasRe api, updating both the flag and the view can be accomplished in js with one line of a generic api.

Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(kaie)
Resolution: --- → WONTFIX

Magnus, the line you quoted is a follow-on hack to remove the double Re: when replying to a message incorrectly treated here:
https://searchfox.org/comm-central/rev/d9cc983e281ba4bfbb34722bb1f080124caf6263/mail/extensions/openpgp/content/ui/enigmailMsgHdrViewOverlay.js#1261
There, all "Re:" prefixes should be removed and instead Ci.nsMsgMessageFlags.HasRe should be set on the header. But that doesn't work, hence this bug. I don't know what line Alta88 had in mind, I didn't find an easy solution, and apparently Patrick didn't have one either.

Attached patch demo.patch — — Splinter Review

TL;DR: The Enigmail solution can't be improved right now :-(

Long version:
As Alta88 pointed out, the one line can be found following nsMsgDatabase::MarkHdrMarked() into nsMsgDatabase::SetMsgHdrFlag() where you find NotifyHdrChangeAll(msgHdr, oldFlags, flags, instigator). So the bug was closed WONTFIX correctly.

But even if you call this after setting the HasRe flag, which does solve the immediate issue of the thread pane update, you run into the next issue when copying the message to a different folder. Copying between IMAP and local folders apparently re-derives the HasRe flag again from the raw subject stored in the message. If there is no "Re: ", just some obfuscation, then the HasRe flag isn't set on the copy target although the copy's subject is not taken from the raw subject but the message header/database. Confusing, huh? This sounds like another bug. When copying between local folders, the HasRe flags appears to be carried over and not re-derived. The Enigmail approach of storing the subject including "Re: " at the front works since a) it is of course displayed and b) it survives a copy. Sadly that doesn't work for threading.

Attachment #9140374 - Attachment is obsolete: true
Attachment #9140378 - Attachment is obsolete: true

Continuation in bug 1633205.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: