1417842 - non-ASCII content of xulstore.json corrupted after bug 1363482, was: Non-ASCII(?) chars in toolbar names are destroyed (and error is multiplied every TB start)

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Marcin]

Attachment: custom toolbars names changing

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171112125346

Steps to reproduce:

I have updated TB 56.0b4 -> 57.0b1 (pl_PL)


Actual results:

Toolbar name changed from "Pasek narzędzi poczty 2" to "Pasek narzÃÂÃÂÃÂÃÂdzi poczty 2". Then I made simple test:
1. created toolbar and named it "Narzędzia"
2. closed and started TB
3. and again...
4. and again
You may find that error in name is multiplied every time (see attachment: toolbar_names.jpg, corresponding parts)

In addition:
• Stable TB52 and previous betas (including TB56) are not affected.
• Default TB toolbars are not affected ("Pasek narzędzi poczty", "Pasek panelu folderów" - see toolbar_names.jpg, part 2, yellow boxes)


Expected results:

Toolbar name should not change ;-)

Comment 1

[Jorg K (CEST = GMT+2)]

I can confirm that. Add a toolbar with an accented character or some other non-ASCII character. Works. Restart TB and the name has mojibake in it.

Alice, this could be from bug 1363281. Could you please find the regression. I'm sure a Japanese toolbar name will also not work.

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5278dfcf5eb9f58eaf06ad1ce67e7fd4aba34772&tochange=a46a5879b8781ae9ea99f37b5d34a891f0f75047
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=f5394bcc7c1b41f1c8a2b1a8037a643a080b377f&tochange=f5394bcc7c1b41f1c8a2b1a8037a643a080b377f

Regressed by: Bug 1363482

Comment 3

[Jorg K (CEST = GMT+2)]

Thanks, Alice!!

Kris, please advise what we can do here. In TB we can create new named toolbars, and non-ASCII names get corrupted. I can't see that feature in FF.

Comment 4

[Alice0775 White]

xulstore.json file will be corrupted after restart and quit TB

When add custom toolbar and quit TB

"header-view-toolbox":{"labelalign":"end","iconsize":"small","mode":"icons"},"attachment-view-toolbar":{"iconsize":"small"},"msgHeaderViewDeck":{"selectedIndex":"0"},"statusbar-QLS":{"ccode":"ja-JP"},"folderPane-toolbar":{"currentset":"folderpane-mode-selector"},"customToolbars":{"toolbar1":"Narzędzia:button-goforward"}}


After restart and quit TB

"header-view-toolbox":{"labelalign":"end","iconsize":"small","mode":"icons"},"attachment-view-toolbar":{"iconsize":"small"},"msgHeaderViewDeck":{"selectedIndex":"0"},"statusbar-QLS":{"ccode":"ja-JP"},"folderPane-toolbar":{"currentset":"folderpane-mode-selector"},"customToolbars":{"toolbar1":"NarzÄdzia:button-goforward"}}

Comment 5

[Masatoshi Kimura [:emk]]

Is this still broken after bug 1420427 is fixed?

Comment 6

[Kris Maglione [:kmag]]

Yes. We need to change Cu.readFile to return an AString rather than an ACString. XPC apparently doesn't handle UTF-8 correctly when converting CStrings.

Comment 7

[Masatoshi Kimura [:emk]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #6)
> Yes. We need to change Cu.readFile to return an AString rather than an
> ACString. XPC apparently doesn't handle UTF-8 correctly when converting
> CStrings.

What encoding will be used to convert the file content to nsString (i.e. UTF-16)? What happens if the file is a binary?

Cu.readFile should return ArrayBuffer or it should be changed to Cu.readJSON.

Comment 8

[Masatoshi Kimura [:emk]]

Or
    AUTF8String readUTF8File(in nsIFile file);
.

Comment 9

[Masatoshi Kimura [:emk]]

Attachment: Bug 1417842 - Make Cu.readFile and Cu.readURI UTF-8 aware.

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

Comment 10

[Florian Quèze [:florian]]

[Tracking Requested - why for this release]: regression in 57 causing dataloss. The steps to reproduce given here only involve Thunderbird; if there's no way to reproduce on Firefox it probably doesn't matter much for 57/58, but we should really fix for 59 (the next esr branch that Thunderbird will be releasing from).

Comment 11

[Masatoshi Kimura [:emk]]

Comment on attachment 8933255 [details]
Bug 1417842 - Make Cu.readFile and Cu.readURI UTF-8 aware.

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

Comment 12

[Eric Rahm [:erahm]]

Comment on attachment 8933255 [details]
Bug 1417842 - Make Cu.readFile and Cu.readURI UTF-8 aware.

https://reviewboard.mozilla.org/r/204194/#review212598

Seems reasonable, r=me.

Comment 13

[Masatoshi Kimura [:emk]]

Comment on attachment 8933255 [details]
Bug 1417842 - Make Cu.readFile and Cu.readURI UTF-8 aware.

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6479b173009
Make Cu.readFile and Cu.readURI UTF-8 aware. r=erahm

Comment 15

[Andreea Pavel [:apavel]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 17

[Masatoshi Kimura [:emk]]

Comment on attachment 8933255 [details]
Bug 1417842 - Make Cu.readFile and Cu.readURI UTF-8 aware.

Approval Request Comment
[Feature/Bug causing the regression]: 1363482
[User impact if declined]: Potentially dataloss (especially in Thunderbird)
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No known regressions in Firefox. Thunderbird QE (if exists) should verify the fix following the step in comment #0.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Changed functions are used only for decoding JSON files which are encoded in UTF-8.
[String changes made/needed]: None

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8933255 [details]
Bug 1417842 - Make Cu.readFile and Cu.readURI UTF-8 aware.

From comment #10, the issue is not reproduced in FF and the next esr branch that Thunderbird will be releasing from is 59/60. Beta58-.