1354822 - Thunderbird fails to display "undisclosed-recipients" in thread pane when message was sent to BCC recipient only

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1318964 +++

Bug 1318964 fixed the header pane but didn't fix the thread pane. Last working in TB 31.

Comment 1

[Charles]

As the one who confirmed this aspect of the bug in the old bug, I'm reposting the details here in the new bug...

To confirm, I sent a test email from one TB/account BCC only to another TB/account, and here is what I see in the different places:

Note: I use the CompactHeader Addon (and ToggleHeader for a keyboard shortcut for toggling).

1. In the Sent folder of the sending system I see:

  a) in the thread pane, the Recipient column is blank,
  b) in the message pane header with headers collapsed I see:
     'To [e-mail]',
  c) in the message pane header with headers expanded, I see:
     'Bcc [e-mail]'

2. In the recipient Inbox, I see:

  a) in the message pane header with headers collapsed, I see:
     'To' (it is blank)
  b) in the message pane header with headers expanded, I see:
     'To Undisclosed recipients;'

So, the ONLY place I ever see 'To Undisclosed recipients;' is in the receivers Thunderbird, and only when the headers are not collapsed.

Comment 2

[Jorg K (CEST = GMT+2)]

I don't know the CompactHeader add-on.

Since 1c) and 2b) are already correct, and 1b) and 2a) refer to an add-on, I'll be only fixing 1a) here.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot of sent message

(In reply to Charles from comment #1)
> So, the ONLY place I ever see 'To Undisclosed recipients;' is in the
> receivers Thunderbird, and only when the headers are not collapsed.
Not correct, in the headers of the sent mail you also see it together the BCCs.

Comment 4

[David E. Ross]

Windows 7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.0
Protocols:  POP3 and SMTP

I tested this in Safe Mode (i.e., without any extensions) and indeed see the problem.

Comment 5

[Charles]

It was very popular from the time it came into existence a mere weeks after the TB devs ripped the native compact header functionality out, and unless you weren't around in the TB world when that happened, I don't see how you could have missed the yelling.

It currently has over 100,000 users.

That said, 1)b and 2)a should probably be fixed by the Addon author, but sadly they haven't been heard from in the Mozillazine support forum for it in a while.

Comment 6

[Charles]

(In reply to Jorg K (GMT+2) from comment #3)
> Created attachment 8856128 [details]
> Screenshot of sent message
> 
> (In reply to Charles from comment #1)
>> So, the ONLY place I ever see 'To Undisclosed recipients;' is in the
>> receivers Thunderbird, and only when the headers are not collapsed.

> Not correct, in the headers of the sent mail you also see it together the
> BCCs.

If you are goint to correct someone, it is best to make sure you are correct yourself.

I said the only place I see 'To Undisclosed recipients'.

So, you are wrong, and I was right.

:)

Comment 7

[Jorg K (CEST = GMT+2)]

(In reply to Charles from comment #1)
> So, the ONLY place I ever see 'To Undisclosed recipients;' is in the
> receivers Thunderbird, and only when the headers are not collapsed.
I also see it in the sender's Sent mailbox, see screenshot.
That also shows the BCCs, so it's *not only* in the receiver's but also the sender's.
What am I missing?

> it is best to make sure you are correct yourself.
Indeed, that's why I attached a screenshot showing the sender's Sent mailbox.

Comment 8

[David E. Ross]

In any case, the Compact Headers extension only affects the message pane and the separate message window.  It does not affect the thread pane.  

With regard to comment #1 --

Case 1.a:  This is indeed an error.  

Case 1.b:  I see "To undisclosed-recipients: ;".  While the punctuation (: ;) might be considered an error, it is at most very minor.  

Case 1.c:  I see "To undisclosed-recipients: ;" and the bcc list along with its E-mail addresses exposed.  This is not an error.  The SENDER should see this.  

Case 2.a:  I see "To undisclosed-recipients: ;".  I do not see the error.  

Case 2.b:  I see "To undisclosed-recipients: ;".  This is not an error.  

I got these results running Thunderbird both in Safe Mode (all extensions disabled) and in normal mode (all extensions -- including Compact Headers -- enabled).  Thus, to me, only Case 1.a indicates an error that has "Normal" severity.

Comment 9

[Jorg K (CEST = GMT+2)]

Thanks, David. 1a) is the error I'll be addressing here.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: test undisclosed.eml - Test message

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: 1354822-undisclosed-recipients.patch (v1).

Very hard to debug what's happening in the tread pane. It's all buried in C++ code and gets its values by calling nsMsgHdr::[Get|Set]StringProperty() and doing its own raw header parsing and address book lookups.

Anyway, here's the C++ equivalent of what was implemented for the header pane here in bug 1318964:
https://hg.mozilla.org/comm-central/rev/3f40fe5e5b2799f9273293f1777f86121b97f027#l3.12

Comment 12

[Jorg K (CEST = GMT+2)]

Just for the record: Why is this change necessary? It's necessary because the address parser has changed from TB 31 where this worked to TB 38 where it didn't work.

In TB 31 this code
  ExtractAllAddresses(EncodedHeader(unparsedRecipients, headerCharset.get()),
    names, UTF16ArrayAdapter<>(emails));
returned one address for "undisclosed-recipients:;"

Technically this is an empty group named "undisclosed-recipients", empty, since there's nothing after the colon, so the modern JS Mime returns zero addresses.

When we detect that case, we use the raw header. The good thing is that the string "undisclosed-recipients" is even localised, in German I've seen: Verborgene_Empfaenger:;
https://dxr.mozilla.org/comm-central/rev/b08dc613a3d275711f76b42cf0ceba1119d82f02/mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties#125

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1354822-undisclosed-recipients.patch (v1b).

Comment should be in the block.

Comment 14

[:aceman]

Comment on attachment 8856142 [details] [diff] [review]
1354822-undisclosed-recipients.patch (v1).

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

The test message doesn't display when just put into the mbox file of a test folder. It seems it needs at least some body.

The patch seems to work for the Recipient column, thanks.

Viewing the message shows this in the console:
23:42:25.757 ReferenceError: reference to undefined property "fullAddress" 1 mailWidgets.xml:629:1

But that may be for another bug.

Comment 15

[Jorg K (CEST = GMT+2)]

Very strange. A message doesn't need a body, the message is plain text and has some empty lines as body. It displays fine for me.

ReferenceError: reference to undefined property "fullAddress" 1 mailWidgets.xml:629:1

is a little hard to believe given this code:

            aEmailNode.setAttribute("fullAddress", aAddress.fullAddress || ""); <=== 616
            aEmailNode.setAttribute("displayName", aAddress.displayName || "");
(snip)
            if ("labelElement" in this) {
                let ariaLabel = this.labelElement.value + ": " +
                                aAddress.fullAddress || aAddress.displayName; <=== 629
                aEmailNode.setAttribute("aria-label", ariaLabel);
            }

So why doesn't it barf just above?

Or maybe that should read:
                let ariaLabel = this.labelElement.value + ": " +
                                (aAddress.fullAddress || aAddress.displayName || "");

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1354822-undisclosed-recipients.patch (v2).

OK, fixed anther JS error that was caused by making an illegal/pseudo address semi-legal ;-)

Comment 17

[:aceman]

(In reply to Jorg K (GMT+2) from comment #15)
>             if ("labelElement" in this) {
>                 let ariaLabel = this.labelElement.value + ": " +
>                                 aAddress.fullAddress ||
> aAddress.displayName; <=== 629
>                 aEmailNode.setAttribute("aria-label", ariaLabel);
>             }
> 
> So why doesn't it barf just above?

Yes, the line may be the one above it, which has the aAddress.fullAddress.
 
> Or maybe that should read:
>                 let ariaLabel = this.labelElement.value + ": " +
>                                 (aAddress.fullAddress ||
> aAddress.displayName || "");

Yes, adding the brackets helped, even without the last '|| ""'. But you can add that for safety as the other lines do it too. You may then also do it in the 'aEmailNode.setAttribute("label", ...' line.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 1354822-undisclosed-recipients.patch (v3).

Fixing more trouble ;-)

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1354822-undisclosed-recipients.patch (v4).

More trouble.

Comment 20

[:aceman]

Comment on attachment 8856177 [details] [diff] [review]
1354822-undisclosed-recipients.patch (v4).

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

Thanks for polishing it.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 1354822-undisclosed-recipients.patch (v5).

More trouble fixed.

Comment 22

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/459a6096cd2fd7a38ed37ae14f624bdf00abd261
Typo in the commit message, grrr, which I copied from bug 1318964.

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8856178 [details] [diff] [review]
1354822-undisclosed-recipients.patch (v5).

[Approval Request Comment]
Regression caused by (bug #): JS Mime and friends, incomplete fix in bug 1318964.
User impact if declined: completely blank Recipients field in UI, not nice.
Testing completed (on c-c, etc.): Manual.
Risk to taking this patch (and alternatives if risky):
Low, only affects code path when message sent to BCCs only.

Comment 24

[Jorg K (CEST = GMT+2)]

Backout:
https://hg.mozilla.org/comm-central/rev/1dfb8d6fa77488b153d3173b241ec78f35a1d4f6

By the looks of it, that caused Mozmill failures:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a1d37b5d035aa13a471b17f6acedc6397c76a44c

From the log:
08:04:41     INFO -  SUMMARY-UNEXPECTED-FAIL | test-display-name.js | test-display-name.js::test_to_display_name_multiple
08:04:41     INFO -    EXCEPTION: recipients: 'Carter Burke, Carter Burke' != 'Carter Burke, Dwayne Hicks'.
08:04:41     INFO -      at: test-folder-display-helpers.js line 2890
08:04:41     INFO -         assert_true test-folder-display-helpers.js:2890 11
08:04:41     INFO -         assert_equals test-folder-display-helpers.js:2877 3
08:04:41     INFO -         check_display_name test-display-name.js:185 3
08:04:41     INFO -         Runner.prototype.wrapper frame.js:585 9
08:04:41     INFO -         Runner.prototype._runTestModule frame.js:655 9
08:04:41     INFO -         Runner.prototype.runTestModule frame.js:701 3
08:04:41     INFO -         Runner.prototype.runTestDirectory frame.js:525 7
08:04:41     INFO -         runTestDirectory frame.js:707 3
08:04:41     INFO -         Bridge.prototype._execFunction server.js:179 10
08:04:41     INFO -         Bridge.prototype.execFunction server.js:183 16
08:04:41     INFO -         Session.prototype.receive server.js:283 3
08:04:41     INFO -         AsyncRead.prototype.onDataAvailable server.js:88 3
08:04:41     INFO -  SUMMARY-PASS | test-display-name.js::test_from_in_abook_pdn
08:04:41     INFO -  SUMMARY-PASS | test-display-name.js::test_from_in_abook_no_pdn
08:04:41     INFO -  SUMMARY-PASS | test-display-name.js::test_to_in_abook_pdn
08:04:41     INFO -  SUMMARY-PASS | test-display-name.js::test_to_in_abook_no_pdn
08:04:41     INFO -  SUMMARY-UNEXPECTED-FAIL | test-display-name.js | test-display-name.js::test_to_in_abook_multiple_mixed_pdn
08:04:41     INFO -    EXCEPTION: recipients: 'Sarge, Sarge' != 'Sarge, Rebeccah Jorden'.
08:04:41     INFO -      at: test-folder-display-helpers.js line 2890
08:04:41     INFO -         assert_true test-folder-display-helpers.js:2890 11
08:04:41     INFO -         assert_equals test-folder-display-helpers.js:2877 3
08:04:41     INFO -         check_display_name test-display-name.js:185 3
08:04:41     INFO -         Runner.prototype.wrapper frame.js:585 9
08:04:41     INFO -         Runner.prototype._runTestModule frame.js:655 9
08:04:41     INFO -         Runner.prototype.runTestModule frame.js:701 3
08:04:41     INFO -         Runner.prototype.runTestDirectory frame.js:525 7
08:04:41     INFO -         runTestDirectory frame.js:707 3
08:04:41     INFO -         Bridge.prototype._execFunction server.js:179 10
08:04:41     INFO -         Bridge.prototype.execFunction server.js:183 16
08:04:41     INFO -         Session.prototype.receive server.js:283 3
08:04:41     INFO -         AsyncRead.prototype.onDataAvailable server.js:88 3

I'll run another try to see whether that was really the problem.

Comment 25

[Jorg K (CEST = GMT+2)]

Here's the try after the backout, let's see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c3a52d1057af64426773ced94ef0aa9b014a0da9

Comment 26

[Jorg K (CEST = GMT+2)]

OK, that try is green, so this bug must have been the culprit.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: 1354822-undisclosed-recipients.patch (v6).

Something very fishy going on here. Moving the variable 'recipient' from inside the loop to outside caused the test failure.

So some junk must have been left behind in the variable for the next iteration.

Now my new code block uses its own variable and everything is cool ;-)

Took a while to find this, since for the test, the new code doesn't even run, so I was staring at it until it occurred to me that just shifting the variable declaration caused all the trouble.

Comment 28

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/713c26f47c6a5434a96589c1ade2b07ae158c4ce

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 8856297 [details] [diff] [review]
1354822-undisclosed-recipients.patch (v6).

Second time lucky ;-)

[Approval Request Comment]
Regression caused by (bug #): JS Mime and friends, incomplete fix in bug 1318964.
User impact if declined: completely blank Recipients field in UI, not nice.
Testing completed (on c-c, etc.): Manual.
Risk to taking this patch (and alternatives if risky):
Low, only affects code path when message sent to BCCs only.

That last comment is particularly funny since even a moved variable caused unexpected trouble :-(

Comment 30

[:aceman]

Good that we have tests :)

Comment 31

[Jorg K (CEST = GMT+2)]

That test is so bogus, I couldn't believe my eyes. It tests for
"Carter Burke, Dwayne Hicks" and
"Sarge, Rebeccah Jorden"
but those recipients are nowhere to be seen in the UI. Plus, we now have a Correspondents column and the test was written for a Recipients column. Someone ought to look at that test and decide whether it's still useful.

Comment 32

[Charles]

(In reply to David E. Ross from comment #8)
> In any case, the Compact Headers extension only affects the message pane and
> the separate message window.  It does not affect the thread pane.  
> 
> With regard to comment #1 --
> 
> Case 1.a:  This is indeed an error.  
> 
> Case 1.b:  I see "To undisclosed-recipients: ;".  While the punctuation (:
> ;) might be considered an error, it is at most very minor.  
> 
> Case 1.c:  I see "To undisclosed-recipients: ;" and the bcc list along with
> its E-mail addresses exposed.  This is not an error.  The SENDER should see
> this.  
> 
> Case 2.a:  I see "To undisclosed-recipients: ;".  I do not see the error.  
> 
> Case 2.b:  I see "To undisclosed-recipients: ;".  This is not an error.  
> 
> I got these results running Thunderbird both in Safe Mode (all extensions
> disabled) and in normal mode (all extensions -- including Compact Headers --
> enabled).  Thus, to me, only Case 1.a indicates an error that has "Normal"
> severity.

What version of Compact Header are you using? Because with headers compacted I'm definitely not seeing what you are.

I'll have to find the time to go back and test again, but probably won't get to it until tomorrow morning.

Comment 33

[Jorg K (CEST = GMT+2)]

Aurora (TB 54):
https://hg.mozilla.org/releases/comm-aurora/rev/3ba871050cc772c5dd35bc024407177300f29395

Comment 34

[David E. Ross]

(In reply to Charles from comment #32)
> (In reply to David E. Ross from comment #8)

         [snipped]
 
> What version of Compact Header are you using? Because with headers compacted
> I'm definitely not seeing what you are.
> 
> I'll have to find the time to go back and test again, but probably won't get
> to it until tomorrow morning.

I am using Compact Header 2.1.0, which was last released two years ago.

Comment 35

[Jorg K (CEST = GMT+2)]

Beta (TB 53) - landed for SM:
https://hg.mozilla.org/releases/comm-beta/rev/32a50d6921787a9827a70f15deeeed252bcb302a

Comment 36

[Jorg K (CEST = GMT+2)]

TB 52 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/6530cb2f7b23e25e04c895e5f14b3304d1263ec5