mail.label_ascii_only_mail_as_us_ascii does not work with ISO-2022-JP

RESOLVED FIXED in Thunderbird 60.0

Status

defect
RESOLVED FIXED
Last year
Last year

People

(Reporter: rokuyama, Assigned: rokuyama)

Tracking

({regression})

Thunderbird 60.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [regression:TB45])

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

Last year
Posted patch thunderbird.patch (obsolete) — Splinter Review
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
Keywords: regression
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

Updated

Last year
Attachment #8957527 - Attachment is patch: true
Attachment #8957527 - Attachment mime type: text/x-patch → text/plain

Comment 2

Last year
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.
Attachment #8957527 - Flags: review?(jorgk)
Assignee

Comment 3

Last year
Thank you very much for your quick response!

Comment 4

Last year
Posted patch 1444371.patchSplinter Review
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
Attachment #8957527 - Attachment is obsolete: true
Attachment #8957527 - Flags: review?(jorgk)
Attachment #8957832 - Flags: review?(jorgk)
Blocks: 1202401
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [regression:TB45]

Comment 5

Last year
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+

Comment 6

Last year
Try also green.

Updated

Last year
Assignee: nobody → rokuyama

Comment 7

Last year
Pushed by mozilla@jorgk.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: Last year
Resolution: --- → FIXED

Updated

Last year
Target Milestone: --- → Thunderbird 60.0
Assignee

Comment 8

Last year
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.

Comment 9

Last year
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.
Assignee

Comment 10

Last year
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.