1610023 - Thunderbird erroneously displays incorrect additional email address in To list.

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect, P1)

Description

[Bob Clary [:bc] (inactive)]

I just noticed a problem with the display of messages in Thunderbird 74.0a1 20200117095219 on Fedora 31 x86_64.

It appeared I had sent a personal email and accidentally included [e-mail], [e-mail] and [e-mail] in the To: list when I sent it. I was mortified but on closer investigation the message did not appear in the news groups or mailing lists and it was a red-herring. On closer inspection of my emails, I found every email showed these extra email addresses in the To: list even though the view source for the email did not show them. Quitting Thunderbird and restarting resolved the issue.

I did see a lot of "XUL box for mail-messageid element contained an inline #text child, forcing all its children to be wrapped in a block." messages in the error console.

I can't for certain blame it on today's build since it is possible I missed it since I don't normally read mail I've already sent.

I don't have steps to reproduce but I'll comment here if I do.

Comment 1

[Magnus Melin [:mkmelin]]

This seems to be reproducible: have two messages in a folder: one with many recipients and one with one. Switching back to the one with just one will not update it properly.

Recent regression, but hard to see exactly when since there was bug 1609420 and before that (2020-01-07) doesn't show the bug. Maybe need to set a userChrome.css with

.emailDisplayButton { display: -moz-inline-box !important }

Comment 2

[Calum Mackay]

In my case, it happens when I reply to a message with several folk on the original's To list, but not on my reply.

Even though I reply only to the Sender, cc me, my copy appears to show it being sent to the original To list.

However, it's only wrong in the front-end: if I look at the source with Ctrl-U, it's correct.

The problem survives a TB restart, too. If I restart, and look at the same messages again, the error returns, even though I've sent no email in this instance of TB.

MacOS Daily 2020-01-20. Has been there for a few days, at least.

Comment 3

[Magnus Melin [:mkmelin]]

I'm working on this.

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: bug1610023_multimail_fix.patch

I ended up reworking this code, which did things in a quite strange way. The old code was needlessly creating children it wouldn't ever show under normal circumstances, and then re-use the old children later if need be. This caching wasn't a very good idea. For the normal case to just add the addresses we'll show will be much faster if the email has many recipients. But looks like I get a speedup of 10-30% for the case when clicking open the "xxx more" too.

Sent to try now: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0a253583662a693604e4801bec6ce15c2465a27b

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9122291 [details] [diff] [review]
bug1610023_multimail_fix.patch

Maybe Khushil can review this so we can get it into nightly ASAP. It's a big problem using nightly with it.

Comment 6

[Khushil Mistry [:khushil324]]

Comment on attachment 9122291 [details] [diff] [review]
bug1610023_multimail_fix.patch

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

Looks good. Working fine on Mac.  r=khushil.
I encountered this code during the task of de-grid. Good to have this code cleaned now.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5982cbb01992
rework mail-multi-emailheaderfield to always just create the children it needs, and not cache+update old ones. r=khushil DONTBUILD

Comment 8

[Paul Morris [:pmorris]]

Comment on attachment 9122291 [details] [diff] [review]
bug1610023_multimail_fix.patch

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

Thanks for the review Khushil.  I took a quick look, and happened to notice a couple things.

::: mail/test/browser/message-header/browser_messageHeader.js
@@ +905,5 @@
>    // get maxline pref
>    let maxLines = Services.prefs.getIntPref(
>      "mailnews.headers.show_n_lines_before_more"
>    );
> +  dump("xxxmagnus test maxLines=" + maxLines + "\n");

Looks like some dev debugging code that should be deleted?

::: mailnews/test/resources/folderEventLogHelper.js
@@ +58,5 @@
>      let args = [
>        aJunkProcessed ? "junk processed" : "did not junk process",
>        aTraitProcessed ? "trait processed" : "did not trait process",
>      ];
> +    dump("xxxmagnus aMsgs=" + aMsgs + "\n");

Here too.

Comment 9

[Jorg K (CEST = GMT+2)]

Well, sadly the debugging code was landed :-(

Comment 10

[Calum Mackay]

Looks good to me, in Daily 20200122; thanks very much indeed for the quick response.

Comment 11

[Magnus Melin [:mkmelin]]

Removed the debugging that slipped in with https://hg.mozilla.org/comm-central/rev/5469eaff743f77211543cd2efca845054855b026

Comment 12

[Jorg K (CEST = GMT+2)]

Since when do we merge unrelated changes into other bugs? :-( - There would have been no additional cost in doing a separate changeset.

Comment 13

[Magnus Melin [:mkmelin]]

It was just a one line debug remove, so I didn't see the need.

Comment 14

[Jorg K (CEST = GMT+2)]

There's always a need to follow the rules, even for the technical manager. A patch carries the bug number where the changes are tracked. Please refer to https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Creating_a_patch.

Comment 15

[Jorg K (CEST = GMT+2)]

There rules are there to avoid creating a dependency with another bug. Say someone wanted to back out this bug for some reason. They would then create a dependency with bug 1594000. In case of an uplift, we'd also have pull in some bits of another bug. Sure, we're not backing it out and we're not uplifting it, but there really is no good reason to not adhere to proper process. Especially a senior developer should set a good example.