Closed Bug 1444371 Opened 3 years ago Closed 3 years ago
.label _ascii _only _mail _as _us _ascii does not work with ISO-2022-JP
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:58.0) Gecko/20100101 Firefox/58.0 Build ID: 20180206200532 Steps to reproduce: Set mail.label_ascii_only_mail_as_us_ascii to true, and outgoing encoding to ISO-2022-JP. Then, send a mail which does not contain any non-ASCII characters in its message body. Actual results: The message is sent with charset=iso-2022-jp in Content-Type header. Expected results: It should be charset=us-ascii, and it was before 2015. This bug seems to be introduced by following sequence of commits: https://github.com/mozilla/releases-comm-central/commit/e33e768ba65bf30ea376e742f7839489b1103873 https://github.com/mozilla/releases-comm-central/commit/3bd1578dad0b01cfa3ed33771c9fb216af731705 After these commits, any messages are determined as "not ASCII only" with stateful outgoing encoding (i.e., ISO-2022-JP). If I understand correctly, non-ASCII characters can more easily determined with UTF-8, rather than with outgoing encoding. The attached patch works fine at least for me. It is for 52.6.0, but it can be applied to the master branch.
So that is Bug 1202401 - Prepare for m-c removal of nsISaveAsCharset
Component: Message Compose Window → Composition
Product: Thunderbird → MailNews Core
Summary: [patch] mail.label_ascii_only_mail_as_us_ascii does not work with ISO-2022-JP → mail.label_ascii_only_mail_as_us_ascii does not work with ISO-2022-JP
Version: 52 Branch → 52
Comment on attachment 8957527 [details] [diff] [review] thunderbird.patch Thanks for the patch. The next step is to get it reviewed. I'll do this in the next few days.
Thank you very much for your quick response!
A few remarks before I start the review: - Please give patches a meaningful name, I deal with dozens of "Thunderbird patches" every day. - We accept patches only if the apply to current trunk. If approved, we can back-port to prior versions. I've rebased the patch to current trunk and also changed this: bool isAsciiOnly = NS_IsAscii(static_cast<const char16_t*>(msgBody.get())); The cast is necessary, see: https://hg.mozilla.org/comm-central/rev/d0eae7f808b91b9383c4fca2106e772405cdac91#l1.12 Now, the patch basically changes the "is ASCII" check to run on the UFT-16 original string instead of the string that was encoded into the target charset. Note that nsMsgI18NConvertFromUnicode() encodes from the internal UTF-16 format to a given charset, for example ISO-2022-JP. It also reverts this change https://hg.mozilla.org/comm-central/rev/e90ac2142811#l1.12 that was done to get some tests to pass. That change wasn't awfully well motivated. I can't see that this change was correct, since when ISO-2022-JP was requested, isAsciiOnly was never true. So in other words, whatever you write in ISO-2022-JP would always go out in ISO-2022-JP, even if it was plain ASCII. Personally I also don't understand why an "is ASCII" test makes any sense after the conversion. Surely ISO-2022-JP is a 7bit charset, and anything in the target string will *always* be ASCII. So checking before the conversion makes more sense, as you pointed out. Let me do some testing here and also ship this off to a try run to see whether it causes any problems. Maybe we have to fix any test failure in a different way. Try here: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e927bbd9eb0419fb115536ec293c78129a63b5b8
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 8957832 [details] [diff] [review] 1444371.patch The try run isn't finished, but I ran the test that failed in bug 1202401, test-charset-edit.js, locally and it passed. I don't expect further trouble, but I'll check the try run again later. Thanks again for the contribution.
Attachment #8957832 - Flags: review?(jorgk) → review+
Try also green.
Pushed by email@example.com: https://hg.mozilla.org/comm-central/rev/57432f5a60e2 check "is ASCII" before encoding to target charset and remove ISO-2022-JP special case. r=jorgk
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Thank you very much for your kind response, and I'm so sorry for bothering you with the patch for wrong trunk and with inappropriate file name. I tried a nightly build of 60.0a1 (2018-03-11), which already contains the fix (https://hg.mozilla.org/comm-central/rev/57432f5a60e2), then everything works just fine for me. Let me thank you again for kind cooperation.
Thanks for fixing the bug! I can't include the fix in TB 52.7 due out next week or so, but it could go into TB 52.8. But at that time, TB 60 ESR will be released.
Thank you for the information. I look forward to TB 60 ESR :-).
You need to log in before you can comment on or make changes to this bug.