1358049 - Font name were garbled in Compose window if Font name is non-acsii character

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Alice0775 White]

Attachment: screenshot

Steps To Reproduce:
1. I am not sure but it is require Localized Windows.
   In my case, Windows10 Home 64 bit Japanese Edition
2. Start Thunderbird
   (not regardless of en-US build or ja build)
3. Open Compose window with HTML mode
4. Click message body pane to focus it
5. Select a font name from dropdown list
   In this case "MS ゴシック"

Actual Results:
Font name in the toolbar is garbled and indicated as not installed.
See attached screenshot.

Expected Results:
Font name in the toolbar should be "MS ゴシック"

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=139446daedab3d00b9f0faba25e1e82191c8c8d1&tochange=56f5ffd44b7f13d5db5bd497018f428dcbb28fc4
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=929b56b8c1ad&tochange=b7472e239aa2

Suspect:
	0b5b80bde5b9	aceman — Bug 1148790 - Add unknown fonts from message compose to the fonts menu. ui-r=bwinton, r=mkmelin, r=IanN

Comment 2

[Jorg K (CEST = GMT+2)]

Sure, the "(not installed)" got added, but I don't see why that should make if garbled.

Comment 3

[:aceman]

OK, can you determine if the garbled string is passed to us into the function onFontFaceChange in the file editor/ui/composer/content/editor.js ?

At the top of the function dump the value of editorFont whether the bad string is there:

function onFontFaceChange(fontFaceMenuList, commandID)
{
  var commandNode = document.getElementById(commandID);
  var editorFont = commandNode.getAttribute("state");

Comment 4

[Jorg K (CEST = GMT+2)]

I guess Alice builds FF and TB, right? If not, you can unpack omni.ja and add one line in there. Let me know if that's unclear. Or use remote debugging?

What I don't understand is comment #0:
5. Select a font name from dropdown list
   In this case "MS ゴシック"
So the drop down looks OK, I assume. It shows available fonts. So after it's applied, it shows "#$@#(garbled)##$% (not installed)". That's weird, but yes, "#$@#(garbled)##$%" is not installed, the installed font is "MS ゴシック".

Comment 5

[Alice0775 White]

I add logStringMessage as follows: 

function onFontFaceChange(fontFaceMenuList, commandID)
{
  var commandNode = document.getElementById(commandID);
  var editorFont = commandNode.getAttribute("state");
+      Components.classes["@mozilla.org/consoleservice;1"]
+        .getService(Components.interfaces.nsIConsoleService)
+        .logStringMessage("onFontFaceChange: " + editorFont);
  // Strip quotes in font names. Experiments have shown that we only
  // ever get double quotes around the font name, never single quotes,
  // even if they were in the HTML source. Also single or double



When I select "MS ゴシック" once, Error console shows the following two results.

onFontFaceChange: ＭＳ ゴシック
onFontFaceChange: ï¼­ï¼³ ã´ã·ãã¯

Comment 6

[Alice0775 White]

FYI,

The problem seems to be fixed if I change the editor.js as following().

 function onFontFaceChange(fontFaceMenuList, commandID)
 {
   var commandNode = document.getElementById(commandID);
   var editorFont = commandNode.getAttribute("state");
   // Strip quotes in font names. Experiments have shown that we only
   // ever get double quotes around the font name, never single quotes,
   // even if they were in the HTML source. Also single or double
   // quotes within the font name are never returned.
   editorFont = editorFont.replace(/"/g, "");
+  var UI = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"].
+                createInstance(Components.interfaces.nsIScriptableUnicodeConverter);
+  UI.charset = "UTF-8";
+  editorFont = UI.ConvertToUnicode(editorFont);
   switch (editorFont) {
   case "mixed":
     // Selection is the "mixed" ( > 1 style) state.

Comment 7

[Jorg K (CEST = GMT+2)]

Thanks for the hint. nsIScriptableUnicodeConverter is deprecated, bug 1347877, but we can use something else.

Comment 8

[:aceman]

So you choose the font from the menulist and get 2 message output, the second time the it is garbled.

OK, I can reproduce this on Linux using a crafted message containing <font face="MS ゴシック">...</font> .
I can also see some problem with a font having apostrophe (') in the name. Strangely, a font having name in Greek appears fine.

Comment 9

[:aceman]

For some reason in ComposerCommands.js::pokeMultiStateUI() the font name goes through the desiredAttrib = cmdParams.getCStringValue("state_attribute"); path (notice CString)...
Doing the conversion to UTF-8 in this function also fixes it for me. This needs deeper analysis why this attribute is encoded as CString, not UTF-8 String already. I don't know yet which code sets these 'state' attributes and what the cmdParams are.

Comment 10

[Jorg K (CEST = GMT+2)]

Nice analysis so far.

I think the states are the states of the editor here, so in which font, size, bold, underline, paragraph style, etc. are we currently. Can you please try to remove the |if (valuetype == Components.interfaces.nsICommandParams.eStringType)| and always use |desiredAttrib = cmdParams.getStringValue("state_attribute");| and see what happens at
https://dxr.mozilla.org/comm-central/rev/c1f32169afa72cbb606cb135d978b29e131ec169/editor/ui/composer/content/ComposerCommands.js#368

Comment 11

[:aceman]

Yes I tried that and it makes TB half-hanged. I'm not sure if it totally crashed, but as if no further JS gets executed and some windows couldn't be closed. In one case I had to kill it from the command line.

Comment 12

[Jorg K (CEST = GMT+2)]

Huh? Doesn't crash/hang for me but it throws and I get this in the catch:

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsICommandParams.getStringValue]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://editor/content/ComposerCommands.js :: pokeMultiStateUI :: line 369"  data: no]

I changed getCStringValue() to getStringValue() at line 369. So I think we leave the former and do the UTF-8 trick, right?

Comment 13

[:aceman]

(In reply to Jorg K (GMT+2) from comment #12)
> I changed getCStringValue() to getStringValue() at line 369. So I think we
> leave the former and do the UTF-8 trick, right?

I don't know yet. What if also other attributes (not just font name) are passed via this path and are not just CString encoded UTF-8 strings? What if our added UTF-8 decoding them garbles those? We should see if this isn't a gecko bug or if it is intentional and if we do not have to adapt in some way.

Alice, what is the TB version you are trying it on? The "culprit" bug was landed in TB43. Are you seeing this on the official TB45 or is it a nightly build (55). I can see it on nightly.

Comment 14

[Alice0775 White]

(In reply to :aceman from comment #13)
> Alice, what is the TB version you are trying it on? 

45.8.0

> The "culprit" bug was landed in TB43. 
> Are you seeing this on the official TB45 or is it a nightly
> build (55). I can see it on nightly.

Yes, I can reproduce on 45.8.0 and Daily55.0a1

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #13)
> I don't know yet. What if also other attributes (not just font name) are
> passed via this path and are not just CString encoded UTF-8 strings? What if
> our added UTF-8 decoding them garbles those? We should see if this isn't a
> gecko bug or if it is intentional and if we do not have to adapt in some way.
If you encode ASCII as UTF-8 you get the same result, so there is no harm done at all.

The Gecko bug might be that we need to use getCStringValue() and getStringValue() fails as per comment #12.

I'd add something like this |let encodedText = new TextEncoder("UTF-8").encode(text);|
https://dxr.mozilla.org/comm-central/rev/fcf3534bdd64fcf673c9a230a660468680da4de6/mailnews/mime/jsmime/jsmime.js#2904
after the getCStringValue(). I'll try it myself once my build finishes.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1358049-getCStringValue-decode-utf8.patch (v1).

This works. If ASCII is returned in the CString, the decode does nothing. If UTF-8 is returned, we decode it properly to a JS string, I believe UTF-16.

Tested with a message as per comment #8.

Code inspired by:
https://dxr.mozilla.org/comm-central/rev/5d39d63d0bf223541c5515ab2691736f5a638ded/mailnews/compose/test/unit/test_longLines.js#46

Comment 17

[:aceman]

Alice's code was shorter. Is this awful code the replacement for nsIScriptableUnicodeConverter ?

Comment 18

[Jorg K (CEST = GMT+2)]

Awful code?? I feel insulted now ;-) The replacement depends on the case. If you read bug 1347877, changes are the we're moving this to C-C anyway.

Comment 19

[Jorg K (CEST = GMT+2)]

Grammar failed: If you read bug 1347877, chances are that we're moving this to C-C anyway.

Comment 20

[Alice0775 White]

(In reply to :aceman from comment #17)
> Alice's code was shorter. Is this awful code the replacement for
> nsIScriptableUnicodeConverter ?

Code of comment#6 outputs several error in Error console when editorFont fails to convert to unicode.

Comment 21

[:aceman]

No offence, you just dared to copy awful code :)

Comment 22

[Jorg K (CEST = GMT+2)]

No, I *wrote* test_longLines.js. It took me quite some time to understand all the intricacies and to put it all together.

Comment 23

[:aceman]

Comment on attachment 8867839 [details] [diff] [review]
1358049-getCStringValue-decode-utf8.patch (v1).

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

This works to fix the bug, I just hope it doesn't break something else.

Comment 24

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/249bb3317336f0856202b29fd6eb0f28f1a62030

(In reply to :aceman from comment #23)
If ASCII weren't a subset of UTF-8, then the world would come to a stand-still. So I don't see what could possibly break. But then, I've been wrong before ;-(

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 8867839 [details] [diff] [review]
1358049-getCStringValue-decode-utf8.patch (v1).

Long-standing bug, let's get it fixed one day on the branches ;-)

Comment 26

[:aceman]

(In reply to Jorg K (GMT+2) from comment #24)
> If ASCII weren't a subset of UTF-8, then the world would come to a
> stand-still.

I don't think this is exact. Maybe the first 128 chars (7bit) are a subset (represent the same chars) of UTF-8. There must be some characters that indicate you need to read more (e.g. 2 bytes) to compose a letter. Thus that leading character no longer is the same meaning as in ASCII.

Comment 27

[Jorg K (CEST = GMT+2)]

Sure, 7bit ASCII is a subset of UTF-8. The "read more" bytes all have the high-bit set:
https://en.wikipedia.org/wiki/UTF-8#Description
Backward compatibility: One-byte codes are used for the ASCII values 0 through 127, so ASCII text is valid UTF-8.

Comment 28

[Jorg K (CEST = GMT+2)]

Beta (TB 54):
https://hg.mozilla.org/releases/comm-beta/rev/25595541cc39b940d289254bb140cf9fd04d9b15

Comment 29

[Jorg K (CEST = GMT+2)]

TB 52 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/2dee4d0030050694a7a1c14ea0eab0cd83f8e2d7