1727862 - getting multiple "Use Bcc instead" warnings when replying

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect, P1)

Description

[Red Adder]

Attachment: Bcc Warning.jpg

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Firefox/91.0

Steps to reproduce:

Reply to all with 20 recipients

Actual results:

Multiple Warnings popped up do you want to "Use Bcc instead" or "Keep Recipients public"
see attached photo
You can pick one of the warnings but no more

There were 20 recipients even though it shows Warnings for just 15 to 20

Expected results:

I'd expect just one warning where you pick what option you want to use

and maybe somewhere in Preferences to turn this off

Comment 1

[Magnus Melin [:mkmelin]]

I think this used to work, but proton made the notifications not stack on top of eachother...

Lasana, please have a look asap.

Comment 2

[Lasana Murray]

Seems to be caused by the changes in bug 1717127. The checkPublicRecipients() was made async (for localization!) but not awaited on in the mutation observer. However that will likely still cause this bug because checkPublicRecipients() being async means subsequent calls to this function can run before the prior finishes as it waits for the l10n value in the "next tick". Thus causing the notification to not be seen the second time around. I'm changing it back to sync for now.

PS: In my humble opinion, calling async functions from sync ones is bad practice, except in a few specific cases.

Comment 3

[Lasana Murray]

Attachment: Bug 1727862 - Make checkPublicRecipients() sync again. r=mkmelin

This function can get called multiple times, being async will allow
calls to preempt each other resulting in more than one notification.

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: Bug 1727862 - make sure to show only one bulk mail notification. r=lasana

The test started failing with "cardDetails", aDocumentNode is undefined. For what's done in testMailingListMembersCounted there won't be a document. I don't know why it seemed to work before... Stack like

updateExtraAddressProcessing@chrome://messenger/content/mailWidgets.js:1282:39
OnAddressBookDataChanged/<@chrome://messenger/content/msgHdrView.js:438:34
OnAddressBookDataChanged@chrome://messenger/content/msgHdrView.js:432:28
observe@chrome://messenger/content/msgHdrView.js:409:33
dropCard@resource:///modules/AddrBookDirectory.jsm:542:18
addCard@resource:///modules/AddrBookDirectory.jsm:423:17
addCard@resource:///modules/AddrBookMailingList.jsm:213:31

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/04fe84062d16
make sure to show only one bulk mail notification. r=lasana

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9238914 [details]
Bug 1727862 - make sure to show only one bulk mail notification. r=lasana

[Approval Request Comment]
Regression caused by (bug #): bug 1717127
User impact if declined: unreasonable amount of notification bars when replying to many
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): not risky

Comment 7

[Wayne Mery (:wsmwk)]

Comment on attachment 9238914 [details]
Bug 1727862 - make sure to show only one bulk mail notification. r=lasana

[Triage Comment]
Approved for beta

Comment 8

[Wayne Mery (:wsmwk)]

Let's schedule to get this for 92.1.1

Comment 9

[Rob Lemley [:rjl]]

Thunderbird 93.0b1:
https://hg.mozilla.org/releases/comm-beta/rev/b6845202357e

Comment 10

[Wayne Mery (:wsmwk)]

93.0b1 build2 worked for me. thanks for the fix.

Comment 11

[Red Adder]

Yes worked for me as well - thanks to all involved

Comment 12

[Wayne Mery (:wsmwk)]

Comment on attachment 9238914 [details]
Bug 1727862 - make sure to show only one bulk mail notification. r=lasana

[Triage Comment]
Approved for esr91

Comment 13

[Rob Lemley [:rjl]]

This cannot be uplifted to 91 with the D124197. It builds upon changes from bug 1707801, which included the "many-public-recipients-notice" that is used in this patch.
I think using "many-public-recipients-info" will work. Lasana can you confirm that and make a patch for c-esr91?

Comment 14

[Lasana Murray]

I didn't do the fix for this but maybe we should uplift bug 1707801 as well. It landed on 93, can we?

Comment 15

[Magnus Melin [:mkmelin]]

Attachment: Bug_1727862___minimal_esr91_patch_to_fix_getting_multiple__Use_Bcc_intead__notifications_.diff

I didn't build and test 91, but this should be the minimal fix there.

Comment 16

[Wayne Mery (:wsmwk)]

Rob, I can test the esr patch before shipping, once we have a candidate build

Comment 17

[Lasana Murray]

I think we should land the other patch so that if there are any more issues in this area we don't have to keep doing esr only versions.

Comment 18

[Rob Lemley [:rjl]]

Thunderbird 91.1.1:
https://hg.mozilla.org/releases/comm-esr91/rev/74ecf9f3aa67