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
Categories
(MailNews Core :: Composition, defect, P1)
Tracking
(thunderbird_esr91+ fixed, thunderbird93 unaffected)
Tracking | Status | |
---|---|---|
thunderbird_esr91 | + | fixed |
thunderbird93 | --- | unaffected |
People
(Reporter: rachel, Assigned: rnons)
References
(Regression)
Details
(Keywords: regression)
Attachments
(1 file)
48 bytes,
text/x-phabricator-request
|
wsmwk
:
approval-comm-esr91+
|
Details | Review |
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
Reporter | ||
Updated•3 years ago
|
Reporter | ||
Comment 1•3 years ago
|
||
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.
Reporter | ||
Updated•3 years ago
|
Assignee | ||
Comment 2•3 years ago
|
||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Comment 3•3 years ago
|
||
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.
Reporter | ||
Comment 4•3 years ago
|
||
Patch looks good, better to go forward than regressing the dataloss bug 1730738.
Reporter | ||
Comment 5•3 years ago
|
||
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•3 years ago
•
|
||
(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.
Comment 7•3 years ago
|
||
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.
Reporter | ||
Comment 8•3 years ago
|
||
Thanks for confirming. For testing we created a new message every time and filled in the To/Bcc, Subject fields and body.
Updated•3 years ago
|
Assignee | ||
Comment 9•3 years ago
|
||
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 hidden (Intermittent Failures Robot) |
Reporter | ||
Comment 11•3 years ago
|
||
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.
Assignee | ||
Comment 12•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
bugherder uplift |
Thunderbird 91.2.1:
https://hg.mozilla.org/releases/comm-esr91/rev/9139895b79a0
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Reporter | ||
Comment 16•3 years ago
|
||
(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" ?
Description
•