1389099 - Unicode characters broken when __MSG_... in css file

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P5)

Description

[YF (Yang)]

Attachment: testcase.xpi

I noticed the problem from https://github.com/Robbendebiene/Gesturefy/pull/18#issuecomment-321553312.


STR:
1. Load the testcase.xpi in about:debugging#addons.
2. Click the added toolbar-button, check the popup, the second line is garbled.


Actual results:
Unicode characters broken, regardless of the css or html encoding.


Expected results:
Correctly handle characters.

Comment 1

[Rob Wu [:robwu]]

While auditing the code, I found it peculiar that the stream converter for CSS converted the input (stream) to a binary string, without any regard for character encoding, localized the string, and then generated a new stream based on it.

I created a test case where I embedded multi-byte UTF-8, like this:

content style:
body::before {
    content: 'Here is some poop: 

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1389099 - Properly encode multi-byte translations in CSS

The localization filter was not unicode-aware because convertToStream
assigns the output to a nsIStringInputStream (which takes 8-bit chars).
The input was read as a 8-bit string, but after localization it can
contain wide strings if a translation has a multi-byte character.

To fix this, the input stream is now first read as a UTF-8 string, then
localized, and finally converted via nsIScriptableUnicodeConverter.

Review commit: https://reviewboard.mozilla.org/r/242436/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242436/

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8974162 [details]
Bug 1389099 - Properly encode multi-byte translations in CSS

https://reviewboard.mozilla.org/r/242436/#review248388

I think there's probably a simpler way to do this, but Kris knows better than I do, redirect to him.

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8974162 [details]
Bug 1389099 - Properly encode multi-byte translations in CSS

https://reviewboard.mozilla.org/r/242436/#review248412

::: toolkit/components/extensions/test/xpcshell/test_ext_i18n_css.js:23
(Diff revision 1)
>  
>  AddonTestUtils.init(this);
>  
>  createAppInfo("[e-mail]", "XPCShell", "1", "42");
>  
> +const PILE_OF_POO = "\uD83D\uDCA9"; // Pile of Poo (some multi-byte unicode character).

No. Nothing of the sort in any code in any code I have to deal with in my daily life. Pick a reasonable multi-byte character.

::: toolkit/components/utils/simpleServices.js:67
(Diff revision 1)
> -      .createInstance(Ci.nsIStringInputStream);
> -
> -    stream.data = aAddon.localize(aString);
> -    return stream;
> +    let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
> +      .createInstance(Ci.nsIScriptableUnicodeConverter);
> +    converter.charset = "UTF-8";
> +    return converter.convertToInputStream(aString);

nsIScriptableUnicodeConverter is deprecated. Please use TextEncoder instead.

Comment 5

[Rob Wu [:robwu]]

Comment on attachment 8974162 [details]
Bug 1389099 - Properly encode multi-byte translations in CSS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242436/diff/1-2/

Comment 6

[Rob Wu [:robwu]]

I have replaced the Pile of Poo character with some bytes from an existing web platform test:
https://searchfox.org/mozilla-central/rev/5ff2d7683078c96e4b11b8a13674daded935aa44/testing/web-platform/tests/encoding/api-basics.html#28


(ignore me below - I am testing Bugzilla because comment 1 is cut off)
Test, expect some text after this: 

Comment 7

[Rob Wu [:robwu]]

Apparently it's known that comments are sometimes truncated on BMO: bug 1253535

Comment 8

[Rob Wu [:robwu]]

Comment on attachment 8974162 [details]
Bug 1389099 - Properly encode multi-byte translations in CSS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242436/diff/2-3/

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8974162 [details]
Bug 1389099 - Properly encode multi-byte translations in CSS

https://reviewboard.mozilla.org/r/242436/#review250506

Thanks.

Sorry for the harsh response in the last review, but in the future, please do not put poop emojis in front of me. I (and many other people) find them disgusting, and shouldn't have to worry about stumbling across them in entirely fecal-unrelated source code.

::: toolkit/components/extensions/test/xpcshell/test_ext_i18n_css.js:121
(Diff revision 3)
>    cssURL = cssURL.replace(/foo.css$/, "locale.css");
>  
>    css = await contentPage.fetch(cssURL);
>    equal(css, '* { content: "en-US ltr rtl left right" }', "CSS file localized in mochitest scope");
>  
> +  cssURL = cssURL.replace(/locale\.css$/, "multibyte.css");

This is getting gross. Let's just have the extension send the base URL and append the various CSS filenames to it when we need them.

::: toolkit/components/extensions/test/xpcshell/test_ext_i18n_css.js:123
(Diff revision 3)
>    css = await contentPage.fetch(cssURL);
>    equal(css, '* { content: "en-US ltr rtl left right" }', "CSS file localized in mochitest scope");
>  
> +  cssURL = cssURL.replace(/locale\.css$/, "multibyte.css");
> +  css = await contentPage.fetch(cssURL);
> +  equal(css, `body::before { content: '${MULTIBYTE_STRING}'; } body::after { content: '${MULTIBYTE_STRING}'; }`, "CSS file contains multibyte string");

Can you move this to a helper function something like:

    getCSS(a, b) {
      return `a { content: '${a}' } b { content: '${b}' }`
    }

and then do:

    getCSS(MULTIBYTE_STRING, "__MSG_multibyteKey__");
    ...
    getCSS(MULTIBYTE_STRING, MULTIBYTE_STRING);

?

::: toolkit/components/utils/simpleServices.js:68
(Diff revision 3)
> -
> -    stream.data = aAddon.localize(aString);
> +    let stream = Cc["@mozilla.org/io/arraybuffer-input-stream;1"]
> +      .createInstance(Ci.nsIArrayBufferInputStream);
> +    stream.setData(bytes, 0, bytes.byteLength);

Please add a Components.Constructor for this above:

    const ArrayBufferStream = Components.Constructor("@mozilla.org/io/arraybuffer-input-stream;1", "nsIArrayBufferInputStream", "setData");

Comment 10

[Rob Wu [:robwu]]

Comment on attachment 8974162 [details]
Bug 1389099 - Properly encode multi-byte translations in CSS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242436/diff/3-4/

Comment 11

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 834eb86cdb11be66bf5e84be0180c8d2fad94b2e -d a00ab75aea2b: rebasing 464023:834eb86cdb11 "Bug 1389099 - Properly encode multi-byte translations in CSS r=kmag" (tip)
merging toolkit/components/utils/simpleServices.js
warning: conflicts while merging toolkit/components/utils/simpleServices.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 12

[Rob Wu [:robwu]]

Comment on attachment 8974162 [details]
Bug 1389099 - Properly encode multi-byte translations in CSS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242436/diff/4-5/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4caca025498
Properly encode multi-byte translations in CSS r=kmag

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset c4caca025498 (bug 1389099) for failing toolkit/components/extensions/test/xpcshell/test_locale_converter.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/672e64a2425343f6098aeaef0b14444ee6133138 

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c4caca02549844815d894a60acf340cade739cc1

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=179025175&repo=autoland&lineNumber=2331

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb806e16195a
Properly encode multi-byte translations in CSS r=kmag

Comment 16

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/fb806e16195a

Comment 17

[CosminB]

Attachment: Bug1389099.gif

I can reproduce this issue on Firefox 60.0.1 (20180516032328) under Win 7 64-bit and Mac OS X 10.13.3.

The second line is broken, as presented in the step 2 from the description.

This issue is verified as fixed on Firefox 62.0a1(20180525005138) under Win 7 64-bit and Mac OS X 10.13.3.