1420427 - prefs.js changes are not kept after restarting Firefox with non-ascii characters in profile path

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Petruta Horea [:phorea], Desktop Test Engineering]

[Tracking Requested - why for this release]:
Major regression affecting prefs.js for profiles with non-ASCII characters under Windows

[Note]:
- Other areas may be affected too, not only prefs.js; I will continue to investigate

[Affected versions]:
- Firefox Quantum 57
- Firefox 58 beta 6
- latest Nightly 59.0a1

[Affected platforms]:
- Win 10 64-bit

[Steps to reproduce]:
1. Open Firefox and create a new profile containing non-ASCII characters (eg 'инсд@ēș#☈☎☢

Comment 1

[Petruta Horea [:phorea], Desktop Test Engineering]

2. Change a few default preferences in about:preferences and close the session
3. Restart the browser and check that the previous changed preferences are still in place

[Expected result]:
- All the modified preferences are kept after restart

[Actual result]:
- The browser restarts as if the session is new
- The modified preferences are set back to default
- Selecting "Restore Previous Session" brings back the previously opened tabs, but has no effect on preferences (this results in /firstrun/ and /privacy/firefox/ tabs to be opened several times - as many times as the previous session is restored)

[Regression range]:
- I will check to see if this is caused by bug 1363482 or other bug

[Additional notes]:
- Mac OS X 10.10 and Ubuntu 16.04 x86 are not affected, so I'm assuming only Windows has difficulties in retrieving the path to Firefox profile if it contains non-ASCII characters.
- prefs.js is updated with the changes when closing Firefox. After reopening Firefox, prefs.js is set back to default

Comment 2

[Masatoshi Kimura [:emk]]

URLPreloader uses nsIFile::GetNativePath and NS_NewNativeLocalFile. Please do not. They are lossy on Windows.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Kris, could you please take care of that asap ? I could be part of a 57 dot release (probably will gtb on Tuesday or Wednesday)

Comment 4

[Masatoshi Kimura [:emk]]

Kris may not respond on ni. Ping him on IRC.

Comment 5

[Petruta Horea [:phorea], Desktop Test Engineering]

Regressed by bug 1363482:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d69a6595950ce55e48c3f072081984818703803a&tochange=a9a24a28013bb346dc75044d02d4266842349e6c

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: Bug 1420427 - Stop using native charset in URLPreloader.

I removed the nsACString variant of ReadFile because the only caller nsINIFile::Init(const char*) is not used anywhere (including c-c).

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

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8932243 [details]
Bug 1420427 - Stop using native charset in URLPreloader.

https://reviewboard.mozilla.org/r/203284/#review208670

::: js/xpconnect/loader/URLPreloader.cpp:301
(Diff revision 1)
> +    bool isUTF8 = true;
>      if (memcmp(URL_MAGIC, data.get(), sizeof(URL_MAGIC))) {
> +        if (memcmp(URL_V1_MAGIC, data.get(), sizeof(URL_V1_MAGIC))) {

No need for this. The cache file gets blown away on update. Just bail out if the magic number doesn't match the current version.

::: js/xpconnect/loader/URLPreloader.cpp:704
(Diff revision 1)
> -    Code(buffer);
> +    buffer.codeUint8(*reinterpret_cast<uint8_t*>(&mType));
> +    if (isUTF8 || mType != TypeFile) {
> +        buffer.codeString(mPath);
> +    } else {
> +        nsCString path;
> +        buffer.codeString(path);
> +        nsString path16;
> +        NS_CopyNativeToUnicode(path, path16);
> +        CopyUTF16toUTF8(path16, mPath);
> +    }

No need to duplicate all of this. No need for the non-UTF-8 case, either.

Comment 8

[Masatoshi Kimura [:emk]]

Comment on attachment 8932243 [details]
Bug 1420427 - Stop using native charset in URLPreloader.

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

Comment 9

[Masatoshi Kimura [:emk]]

Comment on attachment 8932243 [details]
Bug 1420427 - Stop using native charset in URLPreloader.

https://reviewboard.mozilla.org/r/203284/#review208670

> No need for this. The cache file gets blown away on update. Just bail out if the magic number doesn't match the current version.

Removed.

> No need to duplicate all of this. No need for the non-UTF-8 case, either.

Removed. Now the patch is much simpler.

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8932243 [details]
Bug 1420427 - Stop using native charset in URLPreloader.

https://reviewboard.mozilla.org/r/203284/#review209058

Thanks

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/345fe119b8cf
Stop using native charset in URLPreloader. r=kmag

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Hope you don't mind, I took the liberty to land it asap to have it bake a bi.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Masatoshi, could you please nominate this bug for uplift to m-r? Ryan, fyi, we will most likely want to land this on m-r before we gtb 57.0.1.

Comment 14

[Masatoshi Kimura [:emk]]

Comment on attachment 8932243 [details]
Bug 1420427 - Stop using native charset in URLPreloader.

Approval Request Comment
[Feature/Bug causing the regression]: 1363482
[User impact if declined]: Prefs not save if the Windows user name a contains unicode character
[Is this code covered by automated tests?]: No, manually tested
[Has the fix been verified in Nightly?]: not yet, currently in autoland
[Needs manual test from QE? If yes, steps to reproduce]: Yes, see comment #0 in this bug.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only change the encoding of file paths. Old cache will be just thrown out.
[String changes made/needed]: none

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8932243 [details]
Bug 1420427 - Stop using native charset in URLPreloader.

We've reviewed the user impact here and believe this is a good low-risk/high-reward ride-along for 57.0.1. Release57+, Beta58+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c9aaf387d44f25ebaf6e4e3486b1a1bb77e8d5cc
https://hg.mozilla.org/releases/mozilla-release/rev/d2e449c73daca64d8c8185590750873b1cfcd412

Comment 17

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/345fe119b8cf

Comment 18

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified as fixed using Firefox 57.0.1 build 2 under Win 10 64-bit and Win 7 64-bit.