If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Unicode characters are unavailable in autoconfig aka MCD (regression)

RESOLVED FIXED in Firefox 47

Status

()

Core
AutoConfig (Mission Control Desktop)
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: YUKI "Piro" Hiroshi, Assigned: emk)

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla49
x86
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed, firefox48 fixed, firefox49 fixed, firefox-esr4547+ fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
Created attachment 8745250 [details]
autoconfig.cfg actually tested

MCD(*1) provides ability to set/lock preferences in enterprise use, but now we cannot use Unicode characters for the value of any string preferences, on Firefox 45 ESR. On Firefox 38 ESR it works, so this seems a regression.

(*1) https://developer.mozilla.org/en-US/docs/MCD,_Mission_Control_Desktop_AKA_AutoConfig


Steps to reproduce:

1. Install Firefox 45.
2. Put "autoconfig.js" at "C:\Program Files (x86)\Mozilla Firefox\defaults\pref\autoconfig.js", with the contents:
--------------------------------------------
pref("general.config.filename", "autoconfig.cfg");
pref("general.config.vendor", "autoconfig");
pref("general.config.obscure_value", 0);
--------------------------------------------
3. Put "autoconfig.cfg" at "C:\Program Files (x86)\Mozilla Firefox\autoconfig.cfg", with the contents (UTF-8, no BOM):
--------------------------------------------
// # don't remove this comment! (the first line is ignored by Mozilla)
lockPref("_test.string.ASCII", "ASCII");
lockPref("_test.string.non-ASCII", "日本語");
--------------------------------------------
4. Start Firefox.
5. Go to "about:config" and find "_test".

Actual result:
"_test.string.ASCII" appears with the specified value but "_test.string.non-ASCII" is blank.

Expected result:
Both "_test.string.ASCII" and "_test.string.non-ASCII" appear with their specified value.


This problem doesn't happen on Firefox 38 ESR - I got the expected result with same steps.
(Reporter)

Comment 1

a year ago
The bug 545099 is similar to this, but clearly this is a regression verified on Firefox 38 and 45.
(Reporter)

Comment 2

a year ago
Created attachment 8745254 [details]
autoconfig.js actually tested
(Reporter)

Updated

a year ago
Keywords: regression
(Reporter)

Comment 3

a year ago
Confirmed on Nightly 49.0a1 (20160425102203) also.
Version: 45 Branch → Trunk
(Reporter)

Comment 4

a year ago
Created attachment 8745257 [details]
distribution.ini actually tested

FYI, localized preference values defined in distribution.ini still work as expected.
https://wiki.mozilla.org/Distribution_INI_File

Updated

a year ago
Component: Preferences: Backend → AutoConfig (Mission Control Desktop)
(Reporter)

Comment 5

a year ago
I realized that there is a workaround. The configuration file seems to be parsed as Unicode strings. On the other hand, `lockPref()` and other directives simply hands given value to internal nsIPrefBranch service. Thus it works when I convert the value from Unicode string to UTF-8 bytes array manually, like:

--------------------------------------------
var unicodeString = "日本語";
var utf8Bytes = unescape(encodeURIComponent(unicodeString));
lockPref("_test.string.non-ASCII", utf8Bytes);
--------------------------------------------
Blocks: 720362
AutoConfig used to be an ASCII File.

I fixed this in bug 1137799 so that AutoConfig files are now parsed as UTF-8, but if it it detects the file is ASCII, it should go back to the old way (See bug 1193625)

So if you do not save your AutoConfig as UTF-8, you should get the old behavior.

In general, string preferences do not support unicode strings. You've always had to do something special using complex preferences:

https://developer.mozilla.org/en-US/Add-ons/Code_snippets/Preferences#nsISupportsString

The fact that those strings used to work was probably just a side effect of using ASCII.

I could certainly investigate adding the complex preference conversion into the AutoConfig APIs.
(Reporter)

Comment 7

a year ago
My autoconfig.cfg is/was actually encoded in UTF-8. Anyway, the only one thing I'm interested in is: is this an intentional, incompatible spec change, or an unexpected regression?

> The fact that those strings used to work was probably just a side effect of using ASCII.

Then, this seems to be an intentional change and we should update our configuration files for the new spec.
(Reporter)

Comment 8

a year ago
One note: we cannot share same autoconfig.cfg/jsc for both old and new Firefox anymore, thus we have to something to avoid errors...
> My autoconfig.cfg is/was actually encoded in UTF-8. 

Was it UTF-8 in firefox 38 as well?

> One note: we cannot share same autoconfig.cfg/jsc for both old and new Firefox anymore, thus we have to something to avoid errors...

Firefox 38 will be out of support within a few weeks, so that should not be an issue.
And I would probably call this an unexpected regression, but I'm not clear why what you were doing was ever working properly.

AutoConfig files were never read as UTF8, so I can't see how this would have worked on Firefox 38

Plus, as I said, Firefox string preferences need to be complex preferences if they don't contain ASCII characters...

The reason I made the change that I made was because other international characters were not working (because the file had to be ASCII).
Sorry, one more thing.

Can you give me an example of an actual Firefox pref that needs to be in Japanese so I can test this in firefox? Versus just a test string?
For Firefox 38, this must be something that only works on Japanese systems.

If I take your autoconfig file from the attachments and use it on Firefox 38 on a US system, I get an error.
(Assignee)

Comment 13

a year ago
TL;DR - Firefox 45+ will convert AutoConfig pref strings to UTF-8 twice.

Firefox used to abuse the pref API to store Unicode strings to the pref.
It will read "ASCII" (actually ISO-8859-1) strings from the pref and parse them as if it is a UTF-8 string:
https://mxr.mozilla.org/mozilla-central/source/modules/libpref/Preferences.cpp?rev=fcc0936b576d&mark=1432-1432#1423
So, Firefox 38 is expected to treat
  lockPref("_test.string.non-ASCII", "é");
(in ISO-8859-1) as _test.string.non-ASCII=é.
Firefox 45 will read the above line as
  lockPref("_test.string.non-ASCII", "é");
(in UTF-8) due to the change from bug 1137799. Then Preferences::GetString will try to convert the value to a Unicode String, but it will fail because 0xe9 is not a valid sequence as a UTF-8 string.

lockPref should take UTF-8 strings when the AutoConfig is read as UTF-8 for compatibility.

(In reply to Mike Kaply [:mkaply] from comment #12)
> For Firefox 38, this must be something that only works on Japanese systems.
> 
> If I take your autoconfig file from the attachments and use it on Firefox 38
> on a US system, I get an error.

I have no idea why it fails on US system. The above logic does not depend on the locale at all. What error message did you see?
> I have no idea why it fails on US system. The above logic does not depend on the locale at all. What error message did you see?

It was a testing problem. Sorry about that. It does work.

> lockPref should take UTF-8 strings when the AutoConfig is read as UTF-8 for compatibility.

I don't know that I can detect UTF-8 at that point. The file has already been converted.

I could switch lockPref to use nsISupportsStrings, but I think we would still be broken when the AutoConfig file is read as ASCII.

I'm not sure what the right thing is here.

What really frustrates me is that we don't have any tests for any of the AutoConfig stuff and no one has figured out how to create them in our existing testing framework. It's frustrating.
(Assignee)

Comment 15

a year ago
Created attachment 8748921 [details] [diff] [review]
Treat string prefs as Unicode when the script encoding is UTF-8

I added a global variable to determine the script encoding.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #8748921 - Flags: review?(mrbkap)
(Assignee)

Comment 16

a year ago
Created attachment 8748922 [details] [diff] [review]
unit test

Try runs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecef8c99879b31751b64776dea104c6d517d824f
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c08ff3b305e6e0817353c55314cfa1dc2f60e43a
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a04baf7882bfa8e4f6af2e8a9e69f51675738c9
Attachment #8748922 - Flags: review?(mrbkap)
Comment on attachment 8748921 [details] [diff] [review]
Treat string prefs as Unicode when the script encoding is UTF-8

Review of attachment 8748921 [details] [diff] [review]:
-----------------------------------------------------------------

The C++ bits look good to me. mkaply should sign off on the API change, though.

::: extensions/pref/autoconfig/src/nsJSConfigTriggers.cpp
@@ +125,5 @@
>        /* If the length is 0, the conversion failed. Fallback to ASCII */
>        convertedScript = NS_ConvertASCIItoUTF16(script);
>      }
> +    JS::Rooted<JS::Value> value(cx, JS::BooleanValue(isUTF8));
> +    JS_DefineProperty(cx, autoconfigSb, "gIsUTF8", value, JSPROP_ENUMERATE);

This should check the return value for failure.
Attachment #8748921 - Flags: review?(mrbkap)
Attachment #8748921 - Flags: review?(mozilla)
Attachment #8748921 - Flags: feedback+
Attachment #8748922 - Flags: review?(mrbkap) → review?(mozilla)
(Assignee)

Comment 18

a year ago
Created attachment 8748961 [details] [diff] [review]
Treat string prefs as Unicode when the script encoding is UTF-8, v2

* Added an error check to JS_DefineProperty.
* Changed the tab-width to 4 for consistency with the rest of this file.
Attachment #8748921 - Attachment is obsolete: true
Attachment #8748921 - Flags: review?(mozilla)
Attachment #8748961 - Flags: review?(mozilla)
Comment on attachment 8748961 [details] [diff] [review]
Treat string prefs as Unicode when the script encoding is UTF-8, v2

Looks great to me.

Once this has landed, I think we should request esr approval. I'll justify if necessary
Attachment #8748961 - Flags: review?(mozilla) → review+
[Tracking Requested - why for this release]:

AutoConfig is an enterprise only feature and this is a regression between ESR38 and ESR45
tracking-firefox-esr45: --- → ?
Comment on attachment 8748922 [details] [diff] [review]
unit test

I'm shocked that you were able to figure out how to do AutoConfig unit tests.

This doesn't interfere with any of the other tests? (I.e. it cleans up properly?).

This is going to be a game changer if we can start creating tests...
Attachment #8748922 - Flags: review?(mozilla) → review+
Can you do a try run with tests on Windows and Mac as well? I'm nervous about the test. Thanks!
(Assignee)

Comment 23

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a7442a5e365
(Assignee)

Comment 24

a year ago
(In reply to Mike Kaply [:mkaply] from comment #21)
> Comment on attachment 8748922 [details] [diff] [review]
> This doesn't interfere with any of the other tests? (I.e. it cleans up
> properly?).

Good point. I added a resetPrefs() call after autoconfig.js is removed to ensure the proper cleanup.
nsReadConfig will stay loaded, but it should not interfere other tests.
(Assignee)

Comment 25

a year ago
Created attachment 8749426 [details] [diff] [review]
Unit test for non-ASCII AutoConfig files
Attachment #8748922 - Attachment is obsolete: true
Attachment #8749426 - Flags: review+
(Assignee)

Comment 26

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0080c16e94dbd0a3488ded2bf04403324dc747e2
Bug 1267567 - Treat string prefs as Unicode when the script encoding is UTF-8. r=mkaply

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8b3673f1332549228d077ac87bb1d4dfde36a9a
Bug 1267567 - Unit test for non-ASCII AutoConfig files. r=mkaply

Comment 27

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0080c16e94db
https://hg.mozilla.org/mozilla-central/rev/d8b3673f1332
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Comment 28

a year ago
Comment on attachment 8748961 [details] [diff] [review]
Treat string prefs as Unicode when the script encoding is UTF-8, v2

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
AutoConfig is an enterprise only feature and this is a regression between ESR38 and ESR45.
User impact if declined: Enterprise admins will be suffered from incompatibility of AutoConfig scripts until the next ESR(52).
Fix Landed on Version: 49 (47 if aurora/beta uplift is approved)
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None

Approval Request Comment
[Feature/regressing bug #]: 1137799
[User impact if declined]: See above. Some enterprise use non-ESR release.
[Describe test coverage new/current, TreeHerder]: A basic unit test landed.
[Risks and why]: See above
[String/UUID change made/needed]: None
Attachment #8748961 - Flags: approval-mozilla-esr45?
Attachment #8748961 - Flags: approval-mozilla-beta?
Attachment #8748961 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 29

a year ago
Comment on attachment 8749426 [details] [diff] [review]
Unit test for non-ASCII AutoConfig files

[Approval Request Comment]
See the above comment.
Attachment #8749426 - Flags: approval-mozilla-esr45?
Attachment #8749426 - Flags: approval-mozilla-beta?
Attachment #8749426 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

a year ago
Depends on: 1271032

Updated

a year ago
status-firefox47: --- → affected
status-firefox48: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox-esr45: ? → 47+
Hello Mike, if AutoConfig is an enterprise only feature, why does this need to be uplifted to Beta? Please let me know.
Flags: needinfo?(mozilla)
We have some enterprises that are on the rapid release model. Since this is very low risk (and a regression), it would be useful to have on the beta.

But ESR is the most important.
Flags: needinfo?(mozilla)
(In reply to Mike Kaply [:mkaply] from comment #31)
> We have some enterprises that are on the rapid release model. Since this is
> very low risk (and a regression), it would be useful to have on the beta.
> 
> But ESR is the most important.

Got it. Thanks. In that case, I'll take it on all nom'd branches.
Comment on attachment 8748961 [details] [diff] [review]
Treat string prefs as Unicode when the script encoding is UTF-8, v2

Low risk fix, Aurora48+, Beta47+, ESR45+
Attachment #8748961 - Flags: approval-mozilla-esr45?
Attachment #8748961 - Flags: approval-mozilla-esr45+
Attachment #8748961 - Flags: approval-mozilla-beta?
Attachment #8748961 - Flags: approval-mozilla-beta+
Attachment #8748961 - Flags: approval-mozilla-aurora?
Attachment #8748961 - Flags: approval-mozilla-aurora+

Updated

a year ago
Attachment #8749426 - Flags: approval-mozilla-esr45?
Attachment #8749426 - Flags: approval-mozilla-esr45+
Attachment #8749426 - Flags: approval-mozilla-beta?
Attachment #8749426 - Flags: approval-mozilla-beta+
Attachment #8749426 - Flags: approval-mozilla-aurora?
Attachment #8749426 - Flags: approval-mozilla-aurora+

Comment 34

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/d2397dcceff4
https://hg.mozilla.org/releases/mozilla-aurora/rev/95eba1c7da97
status-firefox48: affected → fixed

Comment 35

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr45/rev/1041e712cc44
https://hg.mozilla.org/releases/mozilla-esr45/rev/629824ddac49
status-firefox-esr45: affected → fixed

Comment 36

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/845ee0a38186
https://hg.mozilla.org/releases/mozilla-beta/rev/38353172c113
status-firefox47: affected → fixed
You need to log in before you can comment on or make changes to this bug.