Closed Bug 1420427 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Startup and Profile System, defect, major)

57 Branch
All
Windows 10
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 + verified
firefox58 + fixed
firefox59 + fixed

People

(Reporter: petruta.rasa, Assigned: emk)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dataloss, intl, regression)

Attachments

(1 file)

[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 'инсд@ēș#☈☎☢
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
Keywords: dataloss, intl
See Also: → 1392308
URLPreloader uses nsIFile::GetNativePath and NS_NewNativeLocalFile. Please do not. They are lossy on Windows.
Blocks: 1420827
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)
Flags: needinfo?(kmaglione+bmo)
Blocks: 685236
Kris may not respond on ni. Ping him on IRC.
See Also: → 1418325
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.
Attachment #8932243 - Flags: review?(kmaglione+bmo)
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 on attachment 8932243 [details]
Bug 1420427 - Stop using native charset in URLPreloader.

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

Thanks
Attachment #8932243 - Flags: review?(kmaglione+bmo) → review+
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/345fe119b8cf
Stop using native charset in URLPreloader. r=kmag
Hope you don't mind, I took the liberty to land it asap to have it bake a bi.
Assignee: nobody → VYV03354
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.
Flags: needinfo?(ryanvm)
Flags: needinfo?(VYV03354)
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
Flags: needinfo?(VYV03354)
Attachment #8932243 - Flags: approval-mozilla-release?
Attachment #8932243 - Flags: approval-mozilla-beta?
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+
Attachment #8932243 - Flags: approval-mozilla-release?
Attachment #8932243 - Flags: approval-mozilla-release+
Attachment #8932243 - Flags: approval-mozilla-beta?
Attachment #8932243 - Flags: approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/345fe119b8cf
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Verified as fixed using Firefox 57.0.1 build 2 under Win 10 64-bit and Win 7 64-bit.
Blocks: 1428258
You need to log in before you can comment on or make changes to this bug.