1733651 - test_attachment.js and test_bcc.js fail on all platfroms on TB 91 ESR - BCC shipped out with old C++ sending module when "send later" is used

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect, P1)

Description

[Rachel Martin]

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

Steps to reproduce:

TEST-UNEXPECTED-FAIL | xpcshell-cpp.ini:comm/mailnews/compose/test/unit/test_attachment.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | xpcshell-cpp.ini:comm/mailnews/compose/test/unit/test_attachment.js | testBinaryAfterPlainTextAttachment - [testBinaryAfterPlainTextAttachment : 166] false == true
TEST-UNEXPECTED-FAIL | xpcshell-cpp.ini:comm/mailnews/compose/test/unit/test_bcc.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | xpcshell-cpp.ini:comm/mailnews/compose/test/unit/test_bcc.js | testBccWithSendLater - [testBccWithSendLater : 218] false == true

Started with this push:
https://treeherder.mozilla.org/jobs?repo=comm-esr91&selectedTaskRun=Yon6aq5HRBi9ws1nuId5UQ.0&revision=f6bebff25dd4b19be993e286d9594fde1b451516

Comment 1

[Rachel Martin]

The problem is that the new test is incompatible with the old C++ sending module. Likely it will only fail there. Sadly, as a result of backporting bug 1730738 to TB 91, for people still on the old C++ sending module the effect is that BCC and "send later" results in the BCC shipped out. That is rather unfortunate, so say the very least, especially for people sending to BCC-only (undisclosed recipients) and checking the message in the Outbox before it goes out. They've just published the BCC list to everyone. One solution could be to back out bug 1730738 from the ESR until an ESR-compatible solution is found.

Comment 2

[Ping Chen [:rnons] (away)]

Attachment: Bug 1733651 - (esr91 only) Prevent sending out bcc header when mailnews.send.jsmodule=false. r=mkmelin

Comment 3

[Rob Lemley [:rjl]]

The attached patch does indeed make the two tests pass on comm-esr91. I'm not qualified to review however.

There's time to get this in for 92.2.0. Otherwise I am inclined to agree with Rachel that a backout is warranted given the privacy implications.

Comment 4

[Rachel Martin]

Patch looks good, better to go forward than regressing the dataloss bug 1730738.

Comment 5

[Rachel Martin]

We tested this a bit: With the patch, the BCC is no longer sent out. However, in 91 regardless of whether the old or new sending module is used, the BCC doesn't end up in the message stored in the Sent folder, so for 91 at least, bug 1730738 isn't fixed. That's surprising since the test is meant to make sure that the BCC is stored.

Comment 6

[Rob Lemley [:rjl]]

(In reply to Rachel Martin from comment #5)

We tested this a bit: With the patch, the BCC is no longer sent out. However, in 91 regardless of whether the old or new sending module is used, the BCC doesn't end up in the message stored in the Sent folder, so for 91 at least, bug 1730738 isn't fixed. That's surprising since the test is meant to make sure that the BCC is stored.

My testing produced different results.
With the patch and with both the old and new sending module, the BCC header is not sent. It is stored in the copy in the Sent folder however.

My test account is a POP account, with the Sent folder redirected to "Local Folders/Sent". The SMTP server is a dummy server that just prints out the message headers and body to the terminal. I set both mailnews.send.jsmodule and mailnews.smtp.jsmodule to false for the c++ testing. Restarted Thunderbird in between to make sure.

Try build: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=83489fcc8b2021c9613b040af1db0ff38779aed7

Comment 7

[Rob Lemley [:rjl]]

More testing...

I was using the same message over and over, just using "Edit as New Message" out of the Sent folder. It works in that case (using Send Later, Send Unsent).

If I changed the message though, (in my case, I changed the To header, but left the BCCs) then there is no BCC header in the Sent folder copy.

Comment 8

[Rachel Martin]

Thanks for confirming. For testing we created a new message every time and filled in the To/Bcc, Subject fields and body.

Comment 9

[Ping Chen [:rnons] (away)]

Interesting, I can't reproduce it. I tested with creating a new message and editing a message from the Sent folder, the bcc header exists in both cases.

Can you try reverting it to the version before bug 1730738, and maybe print out mJsSendModule?

          // MessageSend.jsm ensures bcc header is not sent.
-         prune_p = !mJsSendModule;
+         prune_p = true;

Comment 10

[Intermittent Failures Robot]

4 failures in 3376 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• comm-esr91: 4

Platform and build breakdown:

• linux64-shippable: 1
  • opt: 1
• macosx1015-64-shippable: 1
  • opt: 1
• windows10-64-shippable-qr: 1
  • opt: 1
• windows7-32-shippable-qr: 1
  • opt: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1733651&startday=2021-09-27&endday=2021-10-03&tree=all

Comment 11

[Rachel Martin]

The patch regresses bug 1730738 when the JS module is used, simply because in nsMsgSendLater::Init() you get the pref too late, after checking mailnews.sendInBackground which is false by default. So as an effect, the BCC is always pruned. If you get the pref right at the top of the function, everything works. Sorry about the confusing comment #5, we confused ourselves or forgot to restart :-( - It would be good to get this corrected and shipped in 91.2.0.

Comment 12

[Ping Chen [:rnons] (away)]

Good catch, fixed now. I also fixed the test by cleaning up the Sent folder first. Will fix the test for trunk tomorrow

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9244040 [details]
Bug 1733651 - (esr91 only) Prevent sending out bcc header when mailnews.send.jsmodule=false. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #): bug 1730738
User impact if declined: when using the c++ sending backend, and send later, bcc sent out
Testing completed (on c-c, etc.): this is 91 only. Code removed on trunk/beta.
Risk to taking this patch (and alternatives if risky): needs to be tested, but it's an edge case configuration + use case, so should not be too risky

Comment 14

[Wayne Mery (:wsmwk)]

Comment on attachment 9244040 [details]
Bug 1733651 - (esr91 only) Prevent sending out bcc header when mailnews.send.jsmodule=false. r=mkmelin

[Triage Comment]
Approved for esr91

Comment 15

[Rob Lemley [:rjl]]

Thunderbird 91.2.1:
https://hg.mozilla.org/releases/comm-esr91/rev/9139895b79a0

Comment 16

[Rachel Martin]

(In reply to Ping Chen (:rnons) from comment #12)

Good catch, fixed now. I also fixed the test by cleaning up the Sent folder first. Will fix the test for trunk tomorrow

"Will fix the test for trunk tomorrow" ?

Comment 17

[Ping Chen [:rnons] (away)]

Fixed in bug 1734070.