Unicode characters broken when __MSG_... in css file

VERIFIED FIXED in Firefox 62

Status

P5
normal
VERIFIED FIXED
2 years ago
9 months ago

People

(Reporter: yfdyh000, Assigned: robwu)

Tracking

({intl})

57 Branch
mozilla62

Firefox Tracking Flags

(firefox62 verified)

Details

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Posted file 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.

Updated

2 years ago
Flags: needinfo?(amckay)

Updated

2 years ago
Flags: needinfo?(amckay)
Priority: -- → P5
(Assignee)

Comment 1

11 months ago
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: 
Assignee: nobody → rob
Comment hidden (mozreview-request)

Comment 3

11 months ago
mozreview-review
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.

Updated

11 months ago
Attachment #8974162 - Flags: review?(aswan) → review?(kmaglione+bmo)

Comment 4

11 months ago
mozreview-review
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("xpcshell@tests.mozilla.org", "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.
Attachment #8974162 - Flags: review?(kmaglione+bmo) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 6

11 months ago
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: 
(Assignee)

Comment 7

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

Updated

11 months ago
Attachment #8974162 - Flags: review?(aswan)
Comment hidden (mozreview-request)

Comment 9

10 months ago
mozreview-review
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");
Attachment #8974162 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)

Comment 11

10 months ago
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 hidden (mozreview-request)

Comment 13

10 months ago
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/c4caca025498
Properly encode multi-byte translations in CSS r=kmag

Comment 15

10 months ago
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb806e16195a
Properly encode multi-byte translations in CSS r=kmag
(Assignee)

Updated

10 months ago
Flags: needinfo?(rob)

Comment 16

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fb806e16195a
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62

Comment 17

10 months ago
Posted image 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.

Updated

10 months ago
Status: RESOLVED → VERIFIED
status-firefox62: fixed → verified
status-firefox57: affected → ---

Updated

9 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.